Re: [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE

From: Ian Campbell <ian.campbell@citrix.com>
To: Joshua Otto <jtotto@uwaterloo.ca>,
	xen-devel@lists.xen.org, Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com,
	george.dunlap@eu.citrix.com, dario.faggioli@citrix.com,
	ian.jackson@eu.citrix.com, czylin@uwaterloo.ca,
	hjarmstr@uwaterloo.ca
Subject: Re: [PATCH 1/5] libxl: tidy libxl_get_scheduler() according to CODING_STYLE
Date: Mon, 4 Jan 2016 16:23:47 +0000	[thread overview]
Message-ID: <1451924627.13361.178.camel@citrix.com> (raw)
In-Reply-To: <1451279808-25264-2-git-send-email-jtotto@uwaterloo.ca>

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;
>      }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel