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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxx>


> ---
> Â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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel