| Message ID | 1451279808-25264-2-git-send-email-jtotto@uwaterloo.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote: > To more closely follow the guidelines in CODING_STYLE, store the result > of the libxc call in the local variable r, and the check the result of > the call in a separate statement. I think a far more important aspect of this change is: don't store the int return value of xc_sched_id into a variable of type libxl_scheduler libxl_scheduler is an enum, and hence "int-like", but... still. I think this is worth mentioning in the commit message, mainly because I'm only 99% confident this is just a benign oddity rather than an actual latent bug. > Additionally, change the error log statement to more accurately reflect > the failure. This is the only functional change introduced by this > patch. Right. > Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca> > --- > tools/libxl/libxl.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 9207621..ca4679b 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5585,10 +5585,11 @@ out: > > libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx) > { > - libxl_scheduler sched, ret; > GC_INIT(ctx); > - if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) { > - LOGE(ERROR, "getting domain info list"); > + libxl_scheduler sched; > + int r = xc_sched_id(ctx->xch, (int *)&sched); If you were minded to make a further cleanup (i.e. this is totally optional) then I'm not convinced this case is actually safe, since libxl_scheduler could potentially be smaller than sizeof(int), or at least IIRC the C standard give the compiler that option, although I don't know if gcc will make use of it or if something else (e.g. OS calling convention on Linux) would make it a non-issue (or I might be totally wrong...). Safer (and cleaner looking even if I'm wrong) would be to use a temporary int for the function call and turn it into an enum implicitly in the return. > + if (r != 0) { > + LOGE(ERROR, "getting current scheduler id"); > return ERROR_FAIL; > GC_FREE; > }
On Mon, 2016-01-04 at 16:23 +0000, Ian Campbell wrote: > On Mon, 2015-12-28 at 00:16 -0500, Joshua Otto wrote: > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > > @@ -5585,10 +5585,11 @@ out: > > > > libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx) > > { > > - libxl_scheduler sched, ret; > > GC_INIT(ctx); > > - if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) { > > - LOGE(ERROR, "getting domain info list"); > > + libxl_scheduler sched; > > + int r = xc_sched_id(ctx->xch, (int *)&sched); > > Safer (and cleaner looking even if I'm wrong) would be to use a > temporary > int for the function call and turn it into an enum implicitly in the > return. > FWIW, +1 to this Regadrs, Dario
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 9207621..ca4679b 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5585,10 +5585,11 @@ out: libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx) { - libxl_scheduler sched, ret; GC_INIT(ctx); - if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) { - LOGE(ERROR, "getting domain info list"); + libxl_scheduler sched; + int r = xc_sched_id(ctx->xch, (int *)&sched); + if (r != 0) { + LOGE(ERROR, "getting current scheduler id"); return ERROR_FAIL; GC_FREE; }
To more closely follow the guidelines in CODING_STYLE, store the result of the libxc call in the local variable r, and the check the result of the call in a separate statement. Additionally, change the error log statement to more accurately reflect the failure. This is the only functional change introduced by this patch. Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca> --- tools/libxl/libxl.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)