| Message ID | 1452825229-15881-1-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
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.
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; }
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(-)