| Message ID | 1455179459-3392-1-git-send-email-write.harmandeep@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@gmail.com> > --- > 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
On Thu, 2016-02-11 at 10:48 +0100, Dario Faggioli wrote: > 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); That is, each "paragraph" consists of: call a variant of xc_version check for errors in that call propagate results of that call to info->... as necessary and I agree. FWIW CODING_STYLE also allows (but doesn't mandate) for if ( r < 0 ) goto out; i.e. on a single line, which Harmandeep might prefer here for brevity. Ian
On Thu, Feb 11, 2016 at 3:18 PM, Dario Faggioli <dario.faggioli@citrix.com> 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@gmail.com> >> --- >> 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) >
On Fri, 2016-02-12 at 16:22 +0530, Harmandeep Kaur wrote: > On Thu, Feb 11, 2016 at 3:18 PM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: > > > > 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. > 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... > 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
On Fri, Feb 12, 2016 at 4:30 PM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Fri, 2016-02-12 at 16:22 +0530, Harmandeep Kaur wrote: >> On Thu, Feb 11, 2016 at 3:18 PM, Dario Faggioli >> <dario.faggioli@citrix.com> wrote: >> > >> > 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. >> > 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... > 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); } Regards. >> 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) >
On Fri, 2016-02-12 at 16:34 +0530, Harmandeep Kaur wrote: > On Fri, Feb 12, 2016 at 4:30 PM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: > > > Sorry I actually meant, > tools/libxl/xl_cmdimpl.c: vinfo = libxl_get_version_info(ctx); > > vinfo = libxl_get_version_info(ctx); > if (vinfo) { > Which again checks for vinfo to not be NULL before using it, so this also seems ok to me. > 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); > } > Dario
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; 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); + rc = xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps); + if ( rc < 0 ) + goto out; - xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps); info->capabilities = libxl__strdup(NOGC, u.xen_caps); + rc = xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset); + if ( rc < 0 ) + goto out; - xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset); info->changeset = libxl__strdup(NOGC, u.xen_chgset); + rc = xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms); + if ( rc < 0 ) + goto out; - xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms); info->virt_start = u.p_parms.virt_start; + rc = info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL); + if ( rc < 0 ) + goto out; - info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL); + rc = xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline); + if ( rc < 0 ) + goto out; - xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline); info->commandline = libxl__strdup(NOGC, u.xen_commandline); out: GC_FREE; - return info; + if ( rc < 0 ) + return NULL; + else + return info; } libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
Avoid handling issue of the return value of xc_version() in many cases. Coverity ID 1351217 Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> --- tools/libxl/libxl.c | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-)