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

[Harmandeep Kaur <write.harmandeep@xxxxxxxxx>]

On Thu, Feb 11, 2016 at 3:18 PM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> 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?

I think NULL may not be handled properly in auto_autoballoon() in
tools/libxl/xl.c Other instances seems okay to me.

Should I fix the caller, too?

>> 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)
>

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