[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/3] libxl: check for underlying xenstore operation failure...
(Added CC to Ian C and Wei.) Paul Durrant writes ("[Xen-devel] [PATCH v2 2/3] libxl: check for underlying xenstore operation failure..."): > ...in libxl__xs_writev_perms() and libxl__xs_printf() Thanks for looking at this. > ERROR_FAIL is returned when any underlying operation fails. This is a > semantic change in the case of a vasprintf() failure in libxl__xs_printf(), > but appears to be better than returning a hardcoded -1. This is what libxl__alloc_failed is for. But, surely it would be better to replace the call to vasprintf with one to libxl__vsprintf ? > path = libxl__sprintf(gc, "%s/%s", dir, kvs[i]); > if (path && kvs[i + 1]) { > int length = strlen(kvs[i + 1]); > - xs_write(ctx->xsh, t, path, kvs[i + 1], length); > - if (perms) > - xs_set_permissions(ctx->xsh, t, path, perms, num_perms); > + if (!xs_write(ctx->xsh, t, path, kvs[i + 1], length)) > + return ERROR_FAIL; This is a good change semantically but I'm not keen on this error handling style. CODING_STYLE says: * Function calls which might fail (ie most function calls) are handled by putting the return/status value into a variable, and then checking it in a separate statement: char *dompath = libxl__xs_get_dompath(gc, bl->domid); if (!dompath) { rc = ERROR_FAIL; goto out; } In this case the libxenstore return value variable should be called `r', I think. CODING_STYLE says: int r; /* the return value from a system call (or libxc call) */ > + if (perms && > + !xs_set_permissions(ctx->xsh, t, path, perms, num_perms)) > + return ERROR_FAIL; And this is worse, I'm afraid. Nested ifs are not expensive and there is no risk of overloading the compiler. So I would like to see this: if (perms) { r = xs_set ... if (r) ... } I won't insist on you changing the error exits to use `goto out', although I tend to think that would be better. Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |