Re: [Xen-devel] [PATCH v3] libxl: fix handling of returns in libxl_get_version_info()

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

Hey, Harmandeep,

We're almost there, I would say.

The subject is still suboptimal, IMO. I would go for something like

"libxl: handle failure of xc_version() in libxl_get_version_info()

The changelog...

On Sat, 2016-02-13 at 02:05 +0530, Harmandeep Kaur wrote:
> Check the return value of xc_version() and return NULL if it
> fails. libxl_get_version_info() can also return NULL now.
>
Put a blank line here.

> Callers of the function libxl_get_version_info() are already
> prepared to deal with returning NULL on failure of xc_version().
>
Callers don't know what actually failed inside the function, and that
is perfectly ok. I'd just say "are already prepared to deal with a NULL
return value"

> Group all calls to xc_version() , so that data copies in various
> info fields only if all calls to xc_version go error-free.
>
This is not super-important to have here in the changelog. I would have
put it in the "v2:" section, below the "---".

I don't mind too much if you want to leave it here, though.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2d18b8d..f660280 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5267,42 +5267,44 @@ const libxl_version_info*
> libxl_get_version_info(libxl_ctx *ctx)
> ÂÂÂÂÂÂÂÂÂxen_platform_parameters_t p_parms;
> ÂÂÂÂÂÂÂÂÂxen_commandline_t xen_commandline;
> ÂÂÂÂÂ} u;
> -ÂÂÂÂlong xen_version;
> +ÂÂÂÂlong r = 0;
> ÂÂÂÂÂlibxl_version_info *info = &ctx->version_info;
> Â
> ÂÂÂÂÂif (info->xen_version_extra != NULL)
> ÂÂÂÂÂÂÂÂÂgoto out;
> Â
> -ÂÂÂÂxen_version = xc_version(ctx->xch, XENVER_version, NULL);
> -ÂÂÂÂinfo->xen_version_major = xen_version >> 16;
> -ÂÂÂÂinfo->xen_version_minor = xen_version & 0xFF;
> -
> -ÂÂÂÂxc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
> +ÂÂÂÂr = xc_version(ctx->xch, XENVER_version, NULL);
> +ÂÂÂÂif (r < 0) goto out;
> +ÂÂÂÂr = xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
> +ÂÂÂÂif (r < 0) goto out;
> +ÂÂÂÂr = xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
> +ÂÂÂÂif (r < 0) goto out;
> +ÂÂÂÂr = xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
> +ÂÂÂÂif (r < 0) goto out;
> +ÂÂÂÂr = xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
> +ÂÂÂÂif (r < 0) goto out;
> +ÂÂÂÂr = xc_version(ctx->xch, XENVER_platform_parameters,
> &u.p_parms);
> +ÂÂÂÂif (r < 0) goto out;
> +ÂÂÂÂr = info->pagesize = xc_version(ctx->xch, XENVER_pagesize,
> NULL);
> +ÂÂÂÂif (r < 0) goto out;
> +ÂÂÂÂr = xc_version(ctx->xch, XENVER_commandline,
> &u.xen_commandline);
> +ÂÂÂÂif (r < 0) goto out;
> +
> +ÂÂÂÂinfo->xen_version_major = r >> 16;
> +ÂÂÂÂinfo->xen_version_minor = r & 0xFF;
>
But now you are using the value of 'r' returned by
xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline),
which is not what you want!

You either need to make the call to
xc_version(ctx->xch, XENVER_version, NULL) the last one, or avoid
getting rid of the xen_version local variable.

I'd do the latter. I know it was me that suggested you did not need it,
but that was with the previous structure of the function. With this re-
arrangement, I think it's more than fine to keep it.

But that's mostly a matter of taste (yours, and tools' maintainers' one
:-D).

> Â out:
> ÂÂÂÂÂGC_FREE;
> -ÂÂÂÂreturn info;
> +ÂÂÂÂreturn r < 0 ? NULL:info;
>
ÂNULL : info; Â(spaces around ':').

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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