[1/4] libxl: Use libxl_strdup instead of strdup on libxl_version_info
diff mbox

Message ID 1453843860-29591-2-git-send-email-konrad.wilk@oracle.com
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk Jan. 26, 2016, 9:30 p.m. UTC
The change is simple replace of raw strdup with a libxl variant.
The benefit of that is the libxl variant has the extra
behaviour of abort-on-alloc-fail - and will improve error handling.

libxl_version_info is a bit odd - it is a public function and as libxl.h
mentions - the callers of libxl_ public function needs to call the appropiate
_dispose() function.

"However libxl_get_version_info() is special and returns a cached
result from the ctx which cannot and should not be freed (as evidenced
by it returning a const struct). This data is freed in libxl_ctx_free()
by calling libxl_version_info_dispose(). This is why none of the callers
remember to free -- they shouldn't be doing so." (Ian Campbell)

So the patch makes sure to use the NOGC.

Suggested-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Comments

Wei Liu Feb. 1, 2016, 12:13 p.m. UTC | #1
On Tue, Jan 26, 2016 at 04:30:57PM -0500, Konrad Rzeszutek Wilk wrote:
> The change is simple replace of raw strdup with a libxl variant.
> The benefit of that is the libxl variant has the extra
> behaviour of abort-on-alloc-fail - and will improve error handling.
> 
> libxl_version_info is a bit odd - it is a public function and as libxl.h
> mentions - the callers of libxl_ public function needs to call the appropiate
> _dispose() function.
> 
> "However libxl_get_version_info() is special and returns a cached
> result from the ctx which cannot and should not be freed (as evidenced
> by it returning a const struct). This data is freed in libxl_ctx_free()
> by calling libxl_version_info_dispose(). This is why none of the callers
> remember to free -- they shouldn't be doing so." (Ian Campbell)
> 
> So the patch makes sure to use the NOGC.
> 
> Suggested-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>
Ian Campbell Feb. 3, 2016, 11:46 a.m. UTC | #2
On Mon, 2016-02-01 at 12:13 +0000, Wei Liu wrote:
> On Tue, Jan 26, 2016 at 04:30:57PM -0500, Konrad Rzeszutek Wilk wrote:
> > The change is simple replace of raw strdup with a libxl variant.
> > The benefit of that is the libxl variant has the extra
> > behaviour of abort-on-alloc-fail - and will improve error handling.
> > 
> > libxl_version_info is a bit odd - it is a public function and as
> > libxl.h
> > mentions - the callers of libxl_ public function needs to call the
> > appropiate
> > _dispose() function.
> > 
> > "However libxl_get_version_info() is special and returns a cached
> > result from the ctx which cannot and should not be freed (as evidenced
> > by it returning a const struct). This data is freed in libxl_ctx_free()
> > by calling libxl_version_info_dispose(). This is why none of the
> > callers
> > remember to free -- they shouldn't be doing so." (Ian Campbell)
> > 
> > So the patch makes sure to use the NOGC.
> > 
> > Suggested-by: Wei Liu <wei.liu2@citrix.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Applied.

Patch
diff mbox

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2bde0f5..548e2e2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5258,6 +5258,7 @@  libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
 
 const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
 {
+    GC_INIT(ctx);
     union {
         xen_extraversion_t xen_extra;
         xen_compile_info_t xen_cc;
@@ -5270,26 +5271,26 @@  const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
     libxl_version_info *info = &ctx->version_info;
 
     if (info->xen_version_extra != NULL)
-        return info;
+        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);
-    info->xen_version_extra = strdup(u.xen_extra);
+    info->xen_version_extra = libxl__strdup(NOGC, u.xen_extra);
 
     xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
-    info->compiler = strdup(u.xen_cc.compiler);
-    info->compile_by = strdup(u.xen_cc.compile_by);
-    info->compile_domain = strdup(u.xen_cc.compile_domain);
-    info->compile_date = strdup(u.xen_cc.compile_date);
+    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 = strdup(u.xen_caps);
+    info->capabilities = libxl__strdup(NOGC, u.xen_caps);
 
     xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
-    info->changeset = strdup(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;
@@ -5297,8 +5298,10 @@  const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
     info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
 
     xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
-    info->commandline = strdup(u.xen_commandline);
+    info->commandline = libxl__strdup(NOGC, u.xen_commandline);
 
+ out:
+    GC_FREE;
     return info;
 }