libxl: fix handling returns in libxl_get_version_info()

Avoid handling issue of the return value of xc_version() in many cases.

Coverity ID 1351217

Signed-off-by Harmandeep Kaur <[email protected]>

---
 tools/libxl/libxl.c | 39 ++++++++++++++++++++++++++++++---------

 1 file changed, 30 insertions(+), 9 deletions(-)

tools/libxl/libxl.c +0 -0

index 2d18b8d..a939e51 100644

tools/libxl/libxl.c +0 -0

      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;

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

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

Hi Harmandeep,

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

Quoted 3 lines · Harmandeep Kaur

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?

Quoted 12 lines

Coverity ID 1351217

Signed-off-by Harmandeep Kaur <[email protected]>

---
 tools/libxl/libxl.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

tools/libxl/libxl.c +0 -0

index 2d18b8d..a939e51 100644

tools/libxl/libxl.c const libxl_version_info* · line 5272

Quoted 6 lines

tools/libxl/libxl.c +1 -0

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

Quoted 4 lines

tools/libxl/libxl.c +0 -0

     long xen_version;
     libxl_version_info *info = &ctx->version_info;
 
     if (info->xen_version_extra != NULL)

tools/libxl/libxl.c const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx) · line 5286

Quoted 10 lines

tools/libxl/libxl.c +4 -1

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

Quoted 7 lines

tools/libxl/libxl.c +4 -1

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

tools/libxl/libxl.c const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx) · line 5299

Quoted 10 lines

tools/libxl/libxl.c +2 -1

+    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.

tools/libxl/libxl.c const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx) · line 5328

Quoted 8 lines

tools/libxl/libxl.c +4 -1

  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)

Quoted 10 lines · Dario Faggioli

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.

Quoted 9 lines

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?

Quoted 22 lines · Harmandeep Kaur

Quoted 10 lines · Dario Faggioli

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.

Quoted 9 lines

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?

Quoted 3 lines

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

So, here:

    info = libxl_get_version_info(ctx);
    if (!info)
        return 1; /* default to on */

Why do you think this is a problem? It looks like this call site is actually prepared to see and handle a NULL return value...

Quoted 2 lines

Should I fix the caller, too?

If there are issues with them, yes. If there aren't just put something like "xxx callers are already fine xxx" in the changelog.

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)

Quoted 1 line · Dario Faggioli

Sorry I actually meant,
tools/libxl/xl_cmdimpl.c:    vinfo = libxl_get_version_info(ctx);

vinfo = libxl_get_version_info(ctx);
    if (vinfo) {

        i = (1 << 20) / vinfo->pagesize; printf("total_memory           : %"PRIu64"\n", info.total_pages / i);
        printf("free_memory            : %"PRIu64"\n",

(info.free_pages - info.outstanding_pages) / i); printf("sharing_freed_memory   : %"PRIu64"\n", info.sharing_freed_pages / i); printf("sharing_used_memory    : %"PRIu64"\n", info.sharing_used_frames / i); printf("outstanding_claims     : %"PRIu64"\n", info.outstanding_pages / i);
    }