| Message ID | 1455309313-5149-1-git-send-email-write.harmandeep@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; info->xen_version_extra = libxl__strdup(NOGC, u.xen_extra); - - 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); - - xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps); info->capabilities = libxl__strdup(NOGC, u.xen_caps); - - xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset); info->changeset = libxl__strdup(NOGC, u.xen_chgset); - - xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms); info->virt_start = u.p_parms.virt_start; - - info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL); - - xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline); info->commandline = libxl__strdup(NOGC, u.xen_commandline); out: GC_FREE; - return info; + return r < 0 ? NULL:info; } libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
Check the return value of xc_version() and return NULL if it fails. libxl_get_version_info() can also return NULL now. Callers of the function libxl_get_version_info() are already prepared to deal with returning NULL on failure of xc_version(). Group all calls to xc_version() , so that data copies in various info fields only if all calls to xc_version go error-free. Coverity ID 1351217 Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> --- v2: Change local variable rc to r. Remove xen_version. Better readiblity of blocks of code. v3: Group all calls to xc_version() , so that data copies in various info fields only if all calls to xc_version work. --- tools/libxl/libxl.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-)