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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

Hi Harmandeep,

So, I think the code in this patch is ok. Still, a few comments...

On Thu, 2016-02-11 at 14:00 +0530, Harmandeep Kaur wrote:
> Avoid handling issue of the return value of xc_version() in many
> cases.
> 
This can be rephrased to be easier to understand (I'm not sure I can
tell what you mean with "Avoid handling issue of the return value").

Something like "Check the return value of xc_version() and return NULL
if it fails."

In fact, I think the fact that the function can now actually return
NULL should be mentioned here in the changelog.

Another thing that you should check, and probably quickly mention too,
is whether or not the callers of these functions --the ones inside
xen.git of course-- are prepared to deal with this. I mean, returning
NULL on failure of xc_version() is, IMO, the right thing to do here
anyway, but it is something important to know whether more work is
needed to fix the callers as well, or if we're just fine.

To do so, search (e.g., with grep or cscope) for uses of
libxl_get_version_info inside the tools/ dir of xen.git. For instance,
there is one such use in xl_cmdimpl.c, in the output_xeninfo()
function, which looks like it would interact well with this patch...
Can you confirm this is the case also for all the other instances and
note it down here?

> Coverity ID 1351217
> 
> Signed-off-by: Harmandeep Kaur <write.harmandeep@xxxxxxxxx>
> ---
> Âtools/libxl/libxl.c | 39 ++++++++++++++++++++++++++++++---------
> Â1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2d18b8d..a939e51 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5267,42 +5267,63 @@ const libxl_version_info*
> libxl_get_version_info(libxl_ctx *ctx)
> ÂÂÂÂÂÂÂÂÂxen_platform_parameters_t p_parms;
> ÂÂÂÂÂÂÂÂÂxen_commandline_t xen_commandline;
> ÂÂÂÂÂ} u;
> +ÂÂÂÂint rc;
>
So, according toÂtools/libxl/CODING_STYLE:

The following local variable names should be used where applicable:

 int rc;ÂÂÂÂ/* a libxl error code - and not anything else */
 int r;ÂÂÂÂÂ/* the return value from a system call (or libxc call) */

Given how it's used, this variable you're introducing falls into the
latter category.

And I also think you need to initialize it.

> ÂÂÂÂÂlong xen_version;
> ÂÂÂÂÂlibxl_version_info *info = &ctx->version_info;
> Â
> ÂÂÂÂÂif (info->xen_version_extra != NULL)
> ÂÂÂÂÂÂÂÂÂgoto out;
> Â
> -ÂÂÂÂxen_version = xc_version(ctx->xch, XENVER_version, NULL);
> +ÂÂÂÂrc = xen_version = xc_version(ctx->xch, XENVER_version, NULL);
> +ÂÂÂÂif ( rc < 0 )
> +ÂÂÂÂÂÂÂÂgoto out;
> +
> ÂÂÂÂÂinfo->xen_version_major = xen_version >> 16;
> ÂÂÂÂÂinfo->xen_version_minor = xen_version & 0xFF;
>
I think you can just get rid of the xen_version variable and, if
xc_version() returns > 0, do the necessary manipulations on r itself
(this, for instance, matches what happens in
xc_core.c:elfnote_fill_xen_version, and is IMO ok to be done here as
well).

> +ÂÂÂÂrc = xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
> +ÂÂÂÂif ( rc < 0 )
> +ÂÂÂÂÂÂÂÂgoto out;
> Â
> -ÂÂÂÂxc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
> ÂÂÂÂÂinfo->xen_version_extra = libxl__strdup(NOGC, u.xen_extra);
> +ÂÂÂÂrc = xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
> +ÂÂÂÂif ( rc < 0 )
> +ÂÂÂÂÂÂÂÂgoto out;
> Â
> -ÂÂÂÂxc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
> ÂÂÂÂÂinfo->compiler = libxl__strdup(NOGC, u.xen_cc.compiler);
> ÂÂÂÂÂinfo->compile_by = libxl__strdup(NOGC, u.xen_cc.compile_by);
> ÂÂÂÂÂinfo->compile_domain = libxl__strdup(NOGC,
> u.xen_cc.compile_domain);
> ÂÂÂÂÂinfo->compile_date = libxl__strdup(NOGC, u.xen_cc.compile_date);
>
Although functionally correct, the final look of all these "blocks"
would result rather hard to read.

I think it would be better if you make the patch in such a way that the
final code would look like this:

  r = xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
  if ( r < 0 )
    goto out;
  info->xen_version_extra = libxl__strdup(NOGC, u.xen_extra);

  r = xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
  if ( r < 0 )
    goto out; Â
  info->compiler = libxl__strdup(NOGC, u.xen_cc.compiler);
  info->compile_by = libxl__strdup(NOGC, u.xen_cc.compile_by);
  info->compile_domain = libxl__strdup(NOGC,
u.xen_cc.compile_domain);
  info->compile_date = libxl__strdup(NOGC, u.xen_cc.compile_date);

  r = xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
  if ( r < 0 )
    goto out;
  info->capabilities = libxl__strdup(NOGC, u.xen_caps);

etc.

> Â out:
> ÂÂÂÂÂGC_FREE;
> -ÂÂÂÂreturn info;
> +ÂÂÂÂif ( rc < 0 )
> +     return NULL;
> +ÂÂÂÂelse
> +     return info;
>
This can become "return r < 0 ? NULL : info;"

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