[PATCH v3 00/10] xl/libxl: fix issues discovered by coverity

[PATCH v3 00/10] xl/libxl: fix issues discovered by coverity

[Wei Liu]

Wei Liu (10):
  libxl: don't check s!=NULL in libxl__abs_path
  libxl: turn two malloc's to libxl__malloc
  libxl: make libxl__strdup handle NULL
  libxl: avoid leaking in libxl__initia_device_remove
  libxl: localtime(3) can return NULL
  libxl: qmp_init_handler can return NULL
  xl: correct handling of extra_config in main_cpupoolcreate
  xl: correctly handle null extra config in main_config_update
  xl: fix a typo in error string in create_domain
  libxl: fix caller of libxl_cpupool functions

 tools/libxl/libxl.c          |  6 +++++-
 tools/libxl/libxl_aoutils.c  |  3 +--
 tools/libxl/libxl_device.c   |  3 ++-
 tools/libxl/libxl_dm.c       |  3 +--
 tools/libxl/libxl_dom.c      |  5 +++--
 tools/libxl/libxl_internal.c |  9 ++++++---
 tools/libxl/libxl_qmp.c      |  1 +
 tools/libxl/libxl_x86.c      |  6 ++++++
 tools/libxl/xl_cmdimpl.c     | 11 ++++++-----
 9 files changed, 31 insertions(+), 16 deletions(-)

-- 
1.9.1

[PATCH v3 01/10] libxl: don't check s!=NULL in libxl__abs_path

[Wei Liu]

That argument should not be NULL. Let subsequent dereferencing crashes
the process.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_internal.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 42d548e..24a0901 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -233,8 +233,7 @@ void libxl__log(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
 
 char *libxl__abs_path(libxl__gc *gc, const char *s, const char *path)
 {
-    if (!s || s[0] == '/')
-        return libxl__strdup(gc, s);
+    if (s[0] == '/') return libxl__strdup(gc, s);
     return libxl__sprintf(gc, "%s/%s", path, s);
 }
 
-- 
1.9.1

[PATCH v3 02/10] libxl: turn two malloc's to libxl__malloc

[Wei Liu]

One is to combine malloc + libxl__alloc_failed. The other is to avoid
dereferencing NULL pointer in case of malloc failure. Also use gc for
memory allocation and remove free() in second case.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_aoutils.c | 3 +--
 tools/libxl/libxl_dm.c      | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 0931eee..0300396 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -245,8 +245,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
 
             buf = LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
             if (!buf || buf->used >= sizeof(buf->buf)) {
-                buf = malloc(sizeof(*buf));
-                if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
+                buf = libxl__malloc(NOGC, sizeof(*buf));
                 buf->used = 0;
                 LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
             }
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ad434f0..8f3c7a5 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1010,7 +1010,7 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
         i++;
     }
     dmargs_size++;
-    dmargs = (char *) malloc(dmargs_size);
+    dmargs = (char *) libxl__malloc(gc, dmargs_size);
     i = 1;
     dmargs[0] = '\0';
     while (args[i] != NULL) {
@@ -1030,7 +1030,6 @@ retry_transaction:
     if (!xs_transaction_end(ctx->xsh, t, 0))
         if (errno == EAGAIN)
             goto retry_transaction;
-    free(dmargs);
     return 0;
 }
 
-- 
1.9.1

[PATCH v3 03/10] libxl: make libxl__strdup handle NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_internal.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 24a0901..48d58f2 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -161,7 +161,11 @@ char *libxl__sprintf(libxl__gc *gc, const char *fmt, ...)
 
 char *libxl__strdup(libxl__gc *gc, const char *c)
 {
-    char *s = strdup(c);
+    char *s;
+
+    if (!c) return NULL;
+
+    s = strdup(c);
 
     if (!s) libxl__alloc_failed(CTX, __func__, strlen(c), 1);
 
-- 
1.9.1

[PATCH v3 04/10] libxl: avoid leaking in libxl__initia_device_remove

[Wei Liu]

Change "return" to "goto out_success" to correctly dispose of the
structure.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 2493972..bee5ed5 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -816,7 +816,7 @@ void libxl__initiate_device_remove(libxl__egc *egc,
                            be_path);
                 goto out;
             }
-            return;
+            goto out_success;
         }
     }
 
@@ -868,6 +868,7 @@ void libxl__initiate_device_remove(libxl__egc *egc,
         goto out;
     }
 
+out_success:
     libxl_dominfo_dispose(&info);
     return;
 
-- 
1.9.1

[PATCH v3 05/10] libxl: localtime(3) can return NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_x86.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 8cd15ca..6be9b1b 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -311,6 +311,11 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
         t = time(NULL);
         tm = localtime(&t);
 
+        if (!tm) {
+            ret = ERROR_FAIL;
+            goto out;
+        }
+
         rtc_timeoffset += tm->tm_gmtoff;
     }
 
@@ -335,6 +340,7 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
         }
     }
 
+out:
     return ret;
 }
 
-- 
1.9.1

[PATCH v3 06/10] libxl: qmp_init_handler can return NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_qmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 6484f5e..965c507 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -694,6 +694,7 @@ libxl__qmp_handler *libxl__qmp_initialize(libxl__gc *gc, uint32_t domid)
     char *qmp_socket;
 
     qmp = qmp_init_handler(gc, domid);
+    if (!qmp) return NULL;
 
     qmp_socket = GCSPRINTF("%s/qmp-libxl-%d", libxl__run_dir_path(), domid);
     if ((ret = qmp_open(qmp, qmp_socket, QMP_SOCKET_CONNECT_TIMEOUT)) < 0) {
-- 
1.9.1

[PATCH v3 07/10] xl: correct handling of extra_config in main_cpupoolcreate

[Wei Liu]

Don't dereference extra_config if it's NULL. Don't leak extra_config in
the end.

Also fixed a typo in error string while I was there.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 971209c..607db4e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7228,9 +7228,9 @@ int main_cpupoolcreate(int argc, char **argv)
     else
         config_src="command line";
 
-    if (strlen(extra_config)) {
+    if (extra_config && strlen(extra_config)) {
         if (config_len > INT_MAX - (strlen(extra_config) + 2)) {
-            fprintf(stderr, "Failed to attach extra configration\n");
+            fprintf(stderr, "Failed to attach extra configuration\n");
             goto out;
         }
         config_data = xrealloc(config_data,
@@ -7365,6 +7365,7 @@ out_cfg:
 out:
     free(name);
     free(config_data);
+    free(extra_config);
     return rc;
 }
 
-- 
1.9.1

[PATCH v3 08/10] xl: correctly handle null extra config in main_config_update

[Wei Liu]

Don't dereference NULL.

Also fixed a typo in error string while I was there.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 607db4e..888f729 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5010,9 +5010,9 @@ int main_config_update(int argc, char **argv)
         if (rc) { fprintf(stderr, "Failed to read config file: %s: %s\n",
                            filename, strerror(errno));
                   free(extra_config); return ERROR_FAIL; }
-        if (strlen(extra_config)) {
+        if (extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
-                fprintf(stderr, "Failed to attach extra configration\n");
+                fprintf(stderr, "Failed to attach extra configuration\n");
                 exit(1);
             }
             /* allocate space for the extra config plus two EOLs plus \0 */
-- 
1.9.1

[PATCH v3 09/10] xl: fix a typo in error string in create_domain

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 888f729..8b4be72 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2658,7 +2658,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
         }
         if (!restoring && extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
-                fprintf(stderr, "Failed to attach extra configration\n");
+                fprintf(stderr, "Failed to attach extra configuration\n");
                 return ERROR_FAIL;
             }
             /* allocate space for the extra config plus two EOLs plus \0 */
-- 
1.9.1

[PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions

[Wei Liu]

Coverity complains cpupool_info leaks a string in failure path. Instead
of fixing that path, we rely on the callers (two public APIs at the
moment) of cpupool_info correctly call libxl_cpupoolinfo_dispose in
their failure path to dispose of any resources.

That involves:
1. Call _init and _dispose in libxl_list_cpupool
2. Call _init in numa_place_domain

Fix numa_place_domain to use goto style to make things more clearer.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
---
 tools/libxl/libxl.c     | 6 +++++-
 tools/libxl/libxl_dom.c | 5 +++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 38aff8d..02b4a26 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -771,8 +771,11 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
 
     poolid = 0;
     for (i = 0;; i++) {
-        if (cpupool_info(gc, &info, poolid, false))
+        libxl_cpupoolinfo_init(&info);
+        if (cpupool_info(gc, &info, poolid, false)) {
+            libxl_cpupoolinfo_dispose(&info);
             break;
+        }
         tmp = realloc(ptr, (i + 1) * sizeof(libxl_cpupoolinfo));
         if (!tmp) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
@@ -783,6 +786,7 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
         ptr = tmp;
         ptr[i] = info;
         poolid = info.poolid + 1;
+        /* Don't dispose of info. It needs to be returned to caller. */
     }
 
     *nb_pool_out = i;
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 8642192..26e7c67 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -142,6 +142,7 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
 
     libxl__numa_candidate_init(&candidate);
     libxl_bitmap_init(&cpupool_nodemap);
+    libxl_cpupoolinfo_init(&cpupool_info);
 
     /*
      * Extract the cpumap from the cpupool the domain belong to. In fact,
@@ -150,10 +151,10 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
      */
     rc = cpupool = libxl__domain_cpupool(gc, domid);
     if (rc < 0)
-        return rc;
+        goto out;
     rc = libxl_cpupool_info(CTX, &cpupool_info, cpupool);
     if (rc)
-        return rc;
+        goto out;
 
     rc = libxl_domain_need_memory(CTX, info, &memkb);
     if (rc)
-- 
1.9.1

Re: [PATCH v3 01/10] libxl: don't check s!=NULL in libxl__abs_path

[Ian Jackson]

Wei Liu writes ("[PATCH v3 01/10] libxl: don't check s!=NULL in libxl__abs_path"):
> That argument should not be NULL. Let subsequent dereferencing crashes
> the process.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v3 02/10] libxl: turn two malloc's to libxl__malloc

[Ian Jackson]

Wei Liu writes ("[PATCH v3 02/10] libxl: turn two malloc's to libxl__malloc"):
> One is to combine malloc + libxl__alloc_failed. The other is to avoid
> dereferencing NULL pointer in case of malloc failure. Also use gc for
> memory allocation and remove free() in second case.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v3 03/10] libxl: make libxl__strdup handle NULL

[Ian Jackson]

Wei Liu writes ("[PATCH v3 03/10] libxl: make libxl__strdup handle NULL"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Doc comment too please.  /* NULL OK */ would do, or some such.  (I
haven't looked at libxl_internal.h to see what precisely to suggest.)

Ian.

Re: [PATCH v3 04/10] libxl: avoid leaking in libxl__initia_device_remove

[Ian Jackson]

Wei Liu writes ("[PATCH v3 04/10] libxl: avoid leaking in libxl__initia_device_remove"):
> Change "return" to "goto out_success" to correctly dispose of the
> structure.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v3 05/10] libxl: localtime(3) can return NULL

[Ian Jackson]

Wei Liu writes ("[PATCH v3 05/10] libxl: localtime(3) can return NULL"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
...
>          t = time(NULL);
>          tm = localtime(&t);
>  
> +        if (!tm) {
> +            ret = ERROR_FAIL;

You must log something here I think.  Otherwise this function
generates ERROR_FAIL entirely mysteriously (in this quite unlikely
error path).

Ian.

Re: [PATCH v3 06/10] libxl: qmp_init_handler can return NULL

[Ian Jackson]

Wei Liu writes ("[PATCH v3 06/10] libxl: qmp_init_handler can return NULL"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v3 07/10] xl: correct handling of extra_config in main_cpupoolcreate

[Ian Jackson]

Wei Liu writes ("[PATCH v3 07/10] xl: correct handling of extra_config in main_cpupoolcreate"):
> Don't dereference extra_config if it's NULL. Don't leak extra_config in
> the end.
> 
> Also fixed a typo in error string while I was there.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v3 08/10] xl: correctly handle null extra config in main_config_update

[Ian Jackson]

Wei Liu writes ("[PATCH v3 08/10] xl: correctly handle null extra config in main_config_update"):
> Don't dereference NULL.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v3 09/10] xl: fix a typo in error string in create_domain

[Ian Jackson]

Wei Liu writes ("[PATCH v3 09/10] xl: fix a typo in error string in create_domain"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Next time, it would be better to have collected all these string typo
fixes into one patch, rather than mixing them up with the functional
fixes.

Ian.

Re: [PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions

[Ian Jackson]

Wei Liu writes ("[PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions"):
> Coverity complains cpupool_info leaks a string in failure path. Instead
> of fixing that path, we rely on the callers (two public APIs at the
> moment) of cpupool_info correctly call libxl_cpupoolinfo_dispose in
> their failure path to dispose of any resources.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 38aff8d..02b4a26 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -771,8 +771,11 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
>  
>      poolid = 0;
>      for (i = 0;; i++) {
> -        if (cpupool_info(gc, &info, poolid, false))
> +        libxl_cpupoolinfo_init(&info);
> +        if (cpupool_info(gc, &info, poolid, false)) {
> +            libxl_cpupoolinfo_dispose(&info);
>              break;
> +        }

I'm not convinced by this change.

I think that this function has broken error handling: if cpupool_info
fails, it simply ignores the error.

To fix this, it would have to clean up the array properly.

Furthermore, I don't understand why it uses the temporary variable
`info'.  Surely it could have cpupool_info write directly into the
right array slot.  If it did that, then the error handling path would
work to free up any failure.


By putting both these fixes in the same patch, you have denied me the
opportunity to ack the other change, which is fine ...

Ian.

Re: [PATCH v3 00/10] xl/libxl: fix issues discovered by coverity

[Ian Campbell]

On Tue, 2015-07-14 at 17:41 +0100, Wei Liu wrote:
> Wei Liu (10):
>   libxl: don't check s!=NULL in libxl__abs_path
>   libxl: turn two malloc's to libxl__malloc

2 applied w/ Ian's ack.

>   libxl: make libxl__strdup handle NULL

Ian had a comment.

>   libxl: avoid leaking in libxl__initia_device_remove

1 applied (correcting the typo).

>   libxl: localtime(3) can return NULL

Ian had a comment.

>   libxl: qmp_init_handler can return NULL
>   xl: correct handling of extra_config in main_cpupoolcreate
>   xl: correctly handle null extra config in main_config_update
>   xl: fix a typo in error string in create_domain

4 applied.

>   libxl: fix caller of libxl_cpupool functions

Ian had comments.

Re: [PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions

[Wei Liu]

On Tue, Jul 14, 2015 at 06:33:22PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions"):
> > Coverity complains cpupool_info leaks a string in failure path. Instead
> > of fixing that path, we rely on the callers (two public APIs at the
> > moment) of cpupool_info correctly call libxl_cpupoolinfo_dispose in
> > their failure path to dispose of any resources.
> 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 38aff8d..02b4a26 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -771,8 +771,11 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
> >  
> >      poolid = 0;
> >      for (i = 0;; i++) {
> > -        if (cpupool_info(gc, &info, poolid, false))
> > +        libxl_cpupoolinfo_init(&info);
> > +        if (cpupool_info(gc, &info, poolid, false)) {
> > +            libxl_cpupoolinfo_dispose(&info);
> >              break;
> > +        }
> 
> I'm not convinced by this change.
> 
> I think that this function has broken error handling: if cpupool_info
> fails, it simply ignores the error.
> 

I think the semantics of this function is to get back as many cpupool
info as it can.

Unfortunately the failing of cpupool_info can either mean the cpupool id
does not exist or other internal errors.  So not cleaning up is the
right thing to do (speaking from semantics point of view).

I think I need to overhaul cpupool_info a bit if we want to make this
API better.

I'm not sure if it is possible to interleave pool ids. If so, we need a
way to get back the exact number of cpupools. Dario?

Wei.

> To fix this, it would have to clean up the array properly.
> 
> Furthermore, I don't understand why it uses the temporary variable
> `info'.  Surely it could have cpupool_info write directly into the
> right array slot.  If it did that, then the error handling path would
> work to free up any failure.
> 
> 
> By putting both these fixes in the same patch, you have denied me the
> opportunity to ack the other change, which is fine ...
> 
> Ian.

Re: [PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2109 bytes --]

[Adding Juergen]		

On Wed, 2015-07-15 at 18:16 +0100, Wei Liu wrote:
> On Tue, Jul 14, 2015 at 06:33:22PM +0100, Ian Jackson wrote:
> > Wei Liu writes ("[PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions"):

> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -771,8 +771,11 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
> > >  
> > >      poolid = 0;
> > >      for (i = 0;; i++) {
> > > -        if (cpupool_info(gc, &info, poolid, false))
> > > +        libxl_cpupoolinfo_init(&info);
> > > +        if (cpupool_info(gc, &info, poolid, false)) {
> > > +            libxl_cpupoolinfo_dispose(&info);
> > >              break;
> > > +        }
> > 
> > I'm not convinced by this change.
> > 
> > I think that this function has broken error handling: if cpupool_info
> > fails, it simply ignores the error.
> > 
> 
> I think the semantics of this function is to get back as many cpupool
> info as it can.
> 
Yes, I also think this is the case.

> Unfortunately the failing of cpupool_info can either mean the cpupool id
> does not exist or other internal errors.  So not cleaning up is the
> right thing to do (speaking from semantics point of view).
> 
Indeed.

> I think I need to overhaul cpupool_info a bit if we want to make this
> API better.
> 
Well, perhaps having cpupool_info() treating specially the situation
where the pool ID is not there may help... However, what would you do
once you have this additional piece of information available?

Maybe, depending on the error, we can cleanup the whole array? Is it
this that we are after?

> I'm not sure if it is possible to interleave pool ids. If so, we need a
> way to get back the exact number of cpupools. Dario?
> 
Sorry, what you mean by 'interleave pool ids'?

Dario
-- 
<<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)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions

[Wei Liu]

On Thu, Jul 16, 2015 at 10:57:24AM +0200, Dario Faggioli wrote:
> [Adding Juergen]		
> 
> On Wed, 2015-07-15 at 18:16 +0100, Wei Liu wrote:
> > On Tue, Jul 14, 2015 at 06:33:22PM +0100, Ian Jackson wrote:
> > > Wei Liu writes ("[PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions"):
> 
> > > > --- a/tools/libxl/libxl.c
> > > > +++ b/tools/libxl/libxl.c
> > > > @@ -771,8 +771,11 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
> > > >  
> > > >      poolid = 0;
> > > >      for (i = 0;; i++) {
> > > > -        if (cpupool_info(gc, &info, poolid, false))
> > > > +        libxl_cpupoolinfo_init(&info);
> > > > +        if (cpupool_info(gc, &info, poolid, false)) {
> > > > +            libxl_cpupoolinfo_dispose(&info);
> > > >              break;
> > > > +        }
> > > 
> > > I'm not convinced by this change.
> > > 
> > > I think that this function has broken error handling: if cpupool_info
> > > fails, it simply ignores the error.
> > > 
> > 
> > I think the semantics of this function is to get back as many cpupool
> > info as it can.
> > 
> Yes, I also think this is the case.
> 
> > Unfortunately the failing of cpupool_info can either mean the cpupool id
> > does not exist or other internal errors.  So not cleaning up is the
> > right thing to do (speaking from semantics point of view).
> > 
> Indeed.
> 
> > I think I need to overhaul cpupool_info a bit if we want to make this
> > API better.
> > 
> Well, perhaps having cpupool_info() treating specially the situation
> where the pool ID is not there may help... However, what would you do
> once you have this additional piece of information available?
> 

See below.

> Maybe, depending on the error, we can cleanup the whole array? Is it
> this that we are after?
> 
> > I'm not sure if it is possible to interleave pool ids. If so, we need a
> > way to get back the exact number of cpupools. Dario?
> > 
> Sorry, what you mean by 'interleave pool ids'?
> 

For example, pools have ids 0 1 4 6. In this case when should we stop
querying hypervisor about cpu pools? Current model will stop at id 2.

So my first question is if the example I mention is valid. If not, we
can use the same trick as we do now, otherwise we need some way to get
the number of cpu pools and the upper bound of ids.

Wei.

> Dario
> -- 
> <<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)

Re: [PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions

[Juergen Gross]

On 07/16/2015 10:57 AM, Dario Faggioli wrote:
> [Adding Juergen]

Near miss. You took my old mail address. :-)

>
> On Wed, 2015-07-15 at 18:16 +0100, Wei Liu wrote:
>> On Tue, Jul 14, 2015 at 06:33:22PM +0100, Ian Jackson wrote:
>>> Wei Liu writes ("[PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions"):
>
>>>> --- a/tools/libxl/libxl.c
>>>> +++ b/tools/libxl/libxl.c
>>>> @@ -771,8 +771,11 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
>>>>
>>>>       poolid = 0;
>>>>       for (i = 0;; i++) {
>>>> -        if (cpupool_info(gc, &info, poolid, false))
>>>> +        libxl_cpupoolinfo_init(&info);
>>>> +        if (cpupool_info(gc, &info, poolid, false)) {
>>>> +            libxl_cpupoolinfo_dispose(&info);
>>>>               break;
>>>> +        }
>>>
>>> I'm not convinced by this change.
>>>
>>> I think that this function has broken error handling: if cpupool_info
>>> fails, it simply ignores the error.
>>>
>>
>> I think the semantics of this function is to get back as many cpupool
>> info as it can.
>>
> Yes, I also think this is the case.

Indeed.

>> Unfortunately the failing of cpupool_info can either mean the cpupool id
>> does not exist or other internal errors.  So not cleaning up is the
>> right thing to do (speaking from semantics point of view).
>>
> Indeed.
>
>> I think I need to overhaul cpupool_info a bit if we want to make this
>> API better.
>>
> Well, perhaps having cpupool_info() treating specially the situation
> where the pool ID is not there may help... However, what would you do
> once you have this additional piece of information available?
>
> Maybe, depending on the error, we can cleanup the whole array? Is it
> this that we are after?

I think the best would be:

Modify cpupool_info() to return either success, internal error, or no
cpupool found.

In case of internal error libxl_list_cpupool() should clean up and
return NULL.

>
>> I'm not sure if it is possible to interleave pool ids. If so, we need a
>> way to get back the exact number of cpupools. Dario?

libxl_list_cpupool() does that (2nd parameter).

>>
> Sorry, what you mean by 'interleave pool ids'?

Not sure if this is an answer to interleaving of pool ids, but it is
possible to specify the pool id when creating a new cpupool at the
libxc interface. Even in case this is not used, pool ids can easily
be sparse after having deleted a cpupool.


Juergen

Re: [PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions

[Juergen Gross]

On 07/16/2015 11:07 AM, Wei Liu wrote:
> On Thu, Jul 16, 2015 at 10:57:24AM +0200, Dario Faggioli wrote:
>> [Adding Juergen]		
>>
>> On Wed, 2015-07-15 at 18:16 +0100, Wei Liu wrote:
>>> On Tue, Jul 14, 2015 at 06:33:22PM +0100, Ian Jackson wrote:
>>>> Wei Liu writes ("[PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions"):
>>
>>>>> --- a/tools/libxl/libxl.c
>>>>> +++ b/tools/libxl/libxl.c
>>>>> @@ -771,8 +771,11 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
>>>>>
>>>>>       poolid = 0;
>>>>>       for (i = 0;; i++) {
>>>>> -        if (cpupool_info(gc, &info, poolid, false))
>>>>> +        libxl_cpupoolinfo_init(&info);
>>>>> +        if (cpupool_info(gc, &info, poolid, false)) {
>>>>> +            libxl_cpupoolinfo_dispose(&info);
>>>>>               break;
>>>>> +        }
>>>>
>>>> I'm not convinced by this change.
>>>>
>>>> I think that this function has broken error handling: if cpupool_info
>>>> fails, it simply ignores the error.
>>>>
>>>
>>> I think the semantics of this function is to get back as many cpupool
>>> info as it can.
>>>
>> Yes, I also think this is the case.
>>
>>> Unfortunately the failing of cpupool_info can either mean the cpupool id
>>> does not exist or other internal errors.  So not cleaning up is the
>>> right thing to do (speaking from semantics point of view).
>>>
>> Indeed.
>>
>>> I think I need to overhaul cpupool_info a bit if we want to make this
>>> API better.
>>>
>> Well, perhaps having cpupool_info() treating specially the situation
>> where the pool ID is not there may help... However, what would you do
>> once you have this additional piece of information available?
>>
>
> See below.
>
>> Maybe, depending on the error, we can cleanup the whole array? Is it
>> this that we are after?
>>
>>> I'm not sure if it is possible to interleave pool ids. If so, we need a
>>> way to get back the exact number of cpupools. Dario?
>>>
>> Sorry, what you mean by 'interleave pool ids'?
>>
>
> For example, pools have ids 0 1 4 6. In this case when should we stop
> querying hypervisor about cpu pools? Current model will stop at id 2.

No, it won't. cpupool_info(..., poolid, false) will return the next
cpupool with an id >= poolid. Only with the last parameter being "true"
an exact match is required.


Juergen

Re: [PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1687 bytes --]

On Thu, 2015-07-16 at 11:30 +0200, Juergen Gross wrote:
> On 07/16/2015 10:57 AM, Dario Faggioli wrote:
> > [Adding Juergen]
> 
> Near miss. You took my old mail address. :-)
> 
Sorry for this! That's what I have in the MUA... Apparently it's never
got updated as the most of the mail I sent to you go via `stg mail'! :-P

> > On Wed, 2015-07-15 at 18:16 +0100, Wei Liu wrote:

> >> I think I need to overhaul cpupool_info a bit if we want to make this
> >> API better.
> >>
> > Well, perhaps having cpupool_info() treating specially the situation
> > where the pool ID is not there may help... However, what would you do
> > once you have this additional piece of information available?
> >
> > Maybe, depending on the error, we can cleanup the whole array? Is it
> > this that we are after?
> 
> I think the best would be:
> 
> Modify cpupool_info() to return either success, internal error, or no
> cpupool found.
> 
> In case of internal error libxl_list_cpupool() should clean up and
> return NULL.
>
Agreed.

> > Sorry, what you mean by 'interleave pool ids'?
> 
> Not sure if this is an answer to interleaving of pool ids, but it is
> possible to specify the pool id when creating a new cpupool at the
> libxc interface. Even in case this is not used, pool ids can easily
> be sparse after having deleted a cpupool.
> 
Exactly, but that should not be a problem.

Regards,
Dario
-- 
<<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)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v3 07/10] xl: correct handling of extra_config in main_cpupoolcreate

[Ian Jackson]

Wei Liu writes ("[PATCH v3 07/10] xl: correct handling of extra_config in main_cpupoolcreate"):
> Don't dereference extra_config if it's NULL. Don't leak extra_config in
> the end.

I have cherry picked onto all stable trees (back to 4.2).  4.3 and
earlier were also missing 8d394a43 aka 7737ecb2 "libxl: fix leak of
config_data in main_cpupoolcreate".

Ian.