[PATCH v2 for-4.5 0/2] xl/libxl: fix API and two memory leaks

[PATCH v2 for-4.5 0/2] xl/libxl: fix API and two memory leaks

[Wei Liu]

Return value of libxl_basename was erroneously marked as "const". This
series removes that "const" and fixes two memory leaks in xl.

I think these fixes should be included in 4.5, given that they fix real
issues and are very straight foward to reason about.

Wei.

Wei Liu (2):
  libxl: un-constify return value of libxl_basename
  xl: fix two memory leaks

 tools/libxl/libxl.h       |   10 ++++++++++
 tools/libxl/libxl_utils.c |    5 ++++-
 tools/libxl/libxl_utils.h |    6 +++++-
 tools/libxl/xl_cmdimpl.c  |    8 ++++++--
 4 files changed, 25 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[PATCH v2 for-4.5 1/2] libxl: un-constify return value of libxl_basename

[Wei Liu]

The string returned is malloc'ed but marked as "const".

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.h       |   10 ++++++++++
 tools/libxl/libxl_utils.c |    5 ++++-
 tools/libxl/libxl_utils.h |    6 +++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 41d6e8d..291c190 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -478,6 +478,16 @@ typedef struct libxl__ctx libxl_ctx;
 #endif
 
 /*
+ * LIBXL_HAVE_CONST_LIBXL_BASENAME_RETURN_VALUE
+ *
+ * The return value of libxl_basename is malloc'ed but the erroneously
+ * marked as "const" in releases before 4.5.
+ */
+#if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION < 0x040500
+#define LIBXL_HAVE_CONST_LIBXL_BASENAME_RETURN_VALUE 1
+#endif
+
+/*
  * LIBXL_HAVE_PHYSINFO_OUTSTANDING_PAGES
  *
  * If this is defined, libxl_physinfo structure will contain an uint64 field
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 3e1ba17..22119fc 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -19,7 +19,10 @@
 
 #include "libxl_internal.h"
 
-const char *libxl_basename(const char *name)
+#ifdef LIBXL_HAVE_CONST_LIBXL_BASENAME_RETURN_VALUE
+const
+#endif
+char *libxl_basename(const char *name)
 {
     const char *filename;
     if (name == NULL)
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 117b229..8277eb9 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -18,7 +18,11 @@
 
 #include "libxl.h"
 
-const char *libxl_basename(const char *name); /* returns string from strdup */
+#ifdef LIBXL_HAVE_CONST_LIBXL_BASENAME_RETURN_VALUE
+const
+#endif
+char *libxl_basename(const char *name); /* returns string from strdup */
+
 unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned int smp_cpus);
 int libxl_name_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
 int libxl_domain_qualifier_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
-- 
1.7.10.4

[PATCH v2 for-4.5 2/2] xl: fix two memory leaks

[Wei Liu]

Free strings returned by libxl_basename after used.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0e754e7..fe3034f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -920,6 +920,7 @@ static void parse_config_data(const char *config_source,
     int pci_permissive = 0;
     int pci_seize = 0;
     int i, e;
+    char *basename;
 
     libxl_domain_create_info *c_info = &d_config->c_info;
     libxl_domain_build_info *b_info = &d_config->b_info;
@@ -1116,13 +1117,15 @@ static void parse_config_data(const char *config_source,
 
     switch(b_info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
-        if (!strcmp(libxl_basename(b_info->kernel), "hvmloader")) {
+        basename = libxl_basename(b_info->kernel);
+        if (!strcmp(basename, "hvmloader")) {
             fprintf(stderr, "WARNING: you seem to be using \"kernel\" "
                     "directive to override HVM guest firmware. Ignore "
                     "that. Use \"firmware_override\" instead if you "
                     "really want a non-default firmware\n");
             b_info->kernel = NULL;
         }
+        free(basename);
 
         xlu_cfg_replace_string (config, "firmware_override",
                                 &b_info->u.hvm.firmware, 0);
@@ -7023,7 +7026,7 @@ int main_cpupoolcreate(int argc, char **argv)
     int config_len = 0;
     XLU_Config *config;
     const char *buf;
-    const char *name;
+    char *name = NULL;
     uint32_t poolid;
     libxl_scheduler sched = 0;
     XLU_ConfigList *cpus;
@@ -7197,6 +7200,7 @@ int main_cpupoolcreate(int argc, char **argv)
 out_cfg:
     xlu_cfg_destroy(config);
 out:
+    free(name);
     free(config_data);
     return rc;
 }
-- 
1.7.10.4

Re: [PATCH v2 for-4.5 0/2] xl/libxl: fix API and two memory leaks

[Ian Campbell]

On Mon, 2014-12-01 at 11:31 +0000, Wei Liu wrote:
> Return value of libxl_basename was erroneously marked as "const". This
> series removes that "const" and fixes two memory leaks in xl.
> 
> I think these fixes should be included in 4.5, given that they fix real
> issues and are very straight foward to reason about.

Agreed. Both patches:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

I've added the CCs to this intro mail...

Did you by any chance look for other const char * return values?

Ian.

Re: [PATCH v2 for-4.5 0/2] xl/libxl: fix API and two memory leaks

[Wei Liu]

On Mon, Dec 01, 2014 at 11:40:09AM +0000, Ian Campbell wrote:
> On Mon, 2014-12-01 at 11:31 +0000, Wei Liu wrote:
> > Return value of libxl_basename was erroneously marked as "const". This
> > series removes that "const" and fixes two memory leaks in xl.
> > 
> > I think these fixes should be included in 4.5, given that they fix real
> > issues and are very straight foward to reason about.
> 
> Agreed. Both patches:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I've added the CCs to this intro mail...
> 
> Did you by any chance look for other const char * return values?
> 

I've looked at other public API headers. I didn't spot other obvious
problems with regard to const char * return values.

Wei.

> Ian.

Re: [PATCH v2 for-4.5 0/2] xl/libxl: fix API and two memory leaks

[Ian Campbell]

On Mon, 2014-12-01 at 11:50 +0000, Wei Liu wrote:
> On Mon, Dec 01, 2014 at 11:40:09AM +0000, Ian Campbell wrote:
> > On Mon, 2014-12-01 at 11:31 +0000, Wei Liu wrote:
> > > Return value of libxl_basename was erroneously marked as "const". This
> > > series removes that "const" and fixes two memory leaks in xl.
> > > 
> > > I think these fixes should be included in 4.5, given that they fix real
> > > issues and are very straight foward to reason about.
> > 
> > Agreed. Both patches:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > I've added the CCs to this intro mail...
> > 
> > Did you by any chance look for other const char * return values?
> > 
> 
> I've looked at other public API headers. I didn't spot other obvious
> problems with regard to const char * return values.

Super, thanks!

Re: [PATCH v2 for-4.5 0/2] xl/libxl: fix API and two memory leaks

[Konrad Rzeszutek Wilk]

On Mon, Dec 01, 2014 at 11:31:11AM +0000, Wei Liu wrote:
> Return value of libxl_basename was erroneously marked as "const". This
> series removes that "const" and fixes two memory leaks in xl.
> 
> I think these fixes should be included in 4.5, given that they fix real
> issues and are very straight foward to reason about.

Thank you for catching those!

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Wei.
> 
> Wei Liu (2):
>   libxl: un-constify return value of libxl_basename
>   xl: fix two memory leaks
> 
>  tools/libxl/libxl.h       |   10 ++++++++++
>  tools/libxl/libxl_utils.c |    5 ++++-
>  tools/libxl/libxl_utils.h |    6 +++++-
>  tools/libxl/xl_cmdimpl.c  |    8 ++++++--
>  4 files changed, 25 insertions(+), 4 deletions(-)
> 
> -- 
> 1.7.10.4
> 

Re: [PATCH v2 for-4.5 2/2] xl: fix two memory leaks

[Ian Campbell]

On Mon, 2014-12-01 at 11:31 +0000, Wei Liu wrote:
> Free strings returned by libxl_basename after used.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 0e754e7..fe3034f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -920,6 +920,7 @@ static void parse_config_data(const char *config_source,
>      int pci_permissive = 0;
>      int pci_seize = 0;
>      int i, e;
> +    char *basename;

For some reason on arm32 (only) this causes:
xl_cmdimpl.c: In function ‘parse_config_data’:
xl_cmdimpl.c:929:11: error: declaration of ‘basename’ shadows a global declaration [-Werror=shadow]

basename(3) is defined by libgen.h which we don't include, so I suspect
this is a libc issue on armhf (unless Ian has any other ideas?).

How about I s/basename/kernel_basename/ or some such on commit?

Ian.


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

Re: [PATCH v2 for-4.5 2/2] xl: fix two memory leaks

[Wei Liu]

On Tue, Dec 02, 2014 at 02:19:49PM +0000, Ian Campbell wrote:
> On Mon, 2014-12-01 at 11:31 +0000, Wei Liu wrote:
> > Free strings returned by libxl_basename after used.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 0e754e7..fe3034f 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -920,6 +920,7 @@ static void parse_config_data(const char *config_source,
> >      int pci_permissive = 0;
> >      int pci_seize = 0;
> >      int i, e;
> > +    char *basename;
> 
> For some reason on arm32 (only) this causes:
> xl_cmdimpl.c: In function ‘parse_config_data’:
> xl_cmdimpl.c:929:11: error: declaration of ‘basename’ shadows a global declaration [-Werror=shadow]
> 
> basename(3) is defined by libgen.h which we don't include, so I suspect
> this is a libc issue on armhf (unless Ian has any other ideas?).
> 
> How about I s/basename/kernel_basename/ or some such on commit?
> 

That's OK.

Wei.

> Ian.

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

Re: [PATCH v2 for-4.5 0/2] xl/libxl: fix API and two memory leaks

[Ian Campbell]

On Mon, 2014-12-01 at 16:54 -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 01, 2014 at 11:31:11AM +0000, Wei Liu wrote:
> > Return value of libxl_basename was erroneously marked as "const". This
> > series removes that "const" and fixes two memory leaks in xl.
> > 
> > I think these fixes should be included in 4.5, given that they fix real
> > issues and are very straight foward to reason about.
> 
> Thank you for catching those!
> 
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Applied both, including the modification discussed in my reply to the
second.

> > 
> > Wei.
> > 
> > Wei Liu (2):
> >   libxl: un-constify return value of libxl_basename
> >   xl: fix two memory leaks
> > 
> >  tools/libxl/libxl.h       |   10 ++++++++++
> >  tools/libxl/libxl_utils.c |    5 ++++-
> >  tools/libxl/libxl_utils.h |    6 +++++-
> >  tools/libxl/xl_cmdimpl.c  |    8 ++++++--
> >  4 files changed, 25 insertions(+), 4 deletions(-)
> > 
> > -- 
> > 1.7.10.4
> > 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v2 for-4.5 1/2] libxl: un-constify return value of libxl_basename

[Andrew Cooper]

On 01/12/14 11:31, Wei Liu wrote:
> The string returned is malloc'ed but marked as "const".
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl.h       |   10 ++++++++++
>  tools/libxl/libxl_utils.c |    5 ++++-
>  tools/libxl/libxl_utils.h |    6 +++++-
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 41d6e8d..291c190 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -478,6 +478,16 @@ typedef struct libxl__ctx libxl_ctx;
>  #endif
>  
>  /*
> + * LIBXL_HAVE_CONST_LIBXL_BASENAME_RETURN_VALUE
> + *
> + * The return value of libxl_basename is malloc'ed but the erroneously
> + * marked as "const" in releases before 4.5.
> + */
> +#if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION < 0x040500
> +#define LIBXL_HAVE_CONST_LIBXL_BASENAME_RETURN_VALUE 1
> +#endif

This define is currently useless.  Only newer code is capable of making
use of newly introduced LIBXL_HAVE_$FOO flags, and with its current
arrangement, this flag is only exposed to code requesting an older API
version.

This instead needs to be LIBXL_HAVE_NONCONST_LIBXL_BASENAME_RETURN_VALUE
which should be 1 for any API version >= 4.5

~Andrew

Re: [PATCH v2 for-4.5 1/2] libxl: un-constify return value of libxl_basename

[Ian Campbell]

On Tue, 2014-12-02 at 18:44 +0000, Andrew Cooper wrote:
> On 01/12/14 11:31, Wei Liu wrote:
> > The string returned is malloc'ed but marked as "const".
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > ---
> >  tools/libxl/libxl.h       |   10 ++++++++++
> >  tools/libxl/libxl_utils.c |    5 ++++-
> >  tools/libxl/libxl_utils.h |    6 +++++-
> >  3 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 41d6e8d..291c190 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -478,6 +478,16 @@ typedef struct libxl__ctx libxl_ctx;
> >  #endif
> >  
> >  /*
> > + * LIBXL_HAVE_CONST_LIBXL_BASENAME_RETURN_VALUE
> > + *
> > + * The return value of libxl_basename is malloc'ed but the erroneously
> > + * marked as "const" in releases before 4.5.
> > + */
> > +#if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION < 0x040500
> > +#define LIBXL_HAVE_CONST_LIBXL_BASENAME_RETURN_VALUE 1
> > +#endif
> 
> This define is currently useless.  Only newer code is capable of making
> use of newly introduced LIBXL_HAVE_$FOO flags, and with its current
> arrangement, this flag is only exposed to code requesting an older API
> version.
> 
> This instead needs to be LIBXL_HAVE_NONCONST_LIBXL_BASENAME_RETURN_VALUE
> which should be 1 for any API version >= 4.5

Oops, yes. Wei, can you send an incremental fixup please?

Ian.