libxl: fix handling of returns in libxl_get_version_info()
PATCH v3 1/1 libxl: fix handling of returns in libxl_get_version_info()
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
---
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(-)
tools/libxl/libxl.c +21 -19
@@ -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,
Signature · 1 line
2.5.0
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...
Quoted 3 lines · Harmandeep Kaur
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.
Quoted 3 lines
Callers of the function
libxl_get_version_info()are already prepared to deal with returning NULL on failure ofxc_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"
Quoted 3 lines
Group all calls to
xc_version(), so that data copies in various info fields only if all calls toxc_versiongo 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.
Quoted 35 lines
tools/libxl/libxl.c +1
-1@@ -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,
Quoted 9 lines
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).
Quoted 5 lines
out:
GC_FREE;
- return info;
+ return r < 0 ? NULL:info;
NULL : info; (spaces around ':').
Thanks and Regards,
Dario
Signature · 4 lines
<<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
[email protected]
http://lists.xen.org/xen-devel