[PATCH] libxl: Use libxl_strdup instead of strdup on libxl_version_info

[PATCH] libxl: Use libxl_strdup instead of strdup on libxl_version_info

[Konrad Rzeszutek Wilk]

As the libxl_strdup allows us to unwind and free all
of the allocations, while strdup requires the callers
to remember to free (which they didn't seem too).

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

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9207621..03505ee 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5282,19 +5282,19 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
     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;
@@ -5302,7 +5302,7 @@ 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);
 
     return info;
 }
-- 
2.4.3

Re: [PATCH] libxl: Use libxl_strdup instead of strdup on libxl_version_info

[Konrad Rzeszutek Wilk]

On January 14, 2016 9:33:49 PM EST, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>As the libxl_strdup allows us to unwind and free all
>of the allocations, while strdup requires the callers
>to remember to free (which they didn't seem too).
>


Grrrrr.. Ignore it pls. I cherry picked it and had a preceding patch that added the GC_INIT which this patch missed.

(And also an GC_FREE).

>Suggested-by: Wei Liu <wei.liu2@citrix.com>
>Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>---
> tools/libxl/libxl.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
>diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>index 9207621..03505ee 100644
>--- a/tools/libxl/libxl.c
>+++ b/tools/libxl/libxl.c
>@@ -5282,19 +5282,19 @@ const libxl_version_info*
>libxl_get_version_info(libxl_ctx *ctx)
>     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;
>@@ -5302,7 +5302,7 @@ 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);
> 
>     return info;
> }

Re: [PATCH] libxl: Use libxl_strdup instead of strdup on libxl_version_info

[Ian Campbell]

On Thu, 2016-01-14 at 22:03 -0500, Konrad Rzeszutek Wilk wrote:
> On January 14, 2016 9:33:49 PM EST, Konrad Rzeszutek Wilk <konrad.wilk@or
> acle.com> wrote:
> > As the libxl_strdup allows us to unwind and free all
> > of the allocations, while strdup requires the callers
> > to remember to free (which they didn't seem too).
> > 
> 
> 
> Grrrrr.. Ignore it pls. I cherry picked it and had a preceding patch that
> added the GC_INIT which this patch missed.
> 
> (And also an GC_FREE).

Allocating these returned values via a gc would be wrong per the comment
about memory management at the top of libxl.h, where these fall into
category (b) and require the caller to free. They should normally do so by
calling libxl_version_info_dispose(), not manually (which would be quite
tedious and error prone).

Returning GC'd values to the application would be wrong since they would be
freed by the GC_FREE before return upon exit from libxl.

However you've used NOGC, which compared to raw strdup etc only has the
extra behaviour of abort-on-alloc-fail and not any "unwind and free all"
behaviour which the commit message mentions. So this would be a good
change, in that it improves error handling, rather than for any of the
reasons mentioned in the commit message.

WRT freeing the results, normally the caller would need to do so by calling
the appropriate _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.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel