[PATCH for-4.6 v2 0/8] tools: fixes inspired by Coverity scan

[PATCH for-4.6 v2 0/8] tools: fixes inspired by Coverity scan

[Wei Liu]

Wei Liu (8):
  libxl: properly clean up array in libxl_list_cpupool failure path
  xl: lockdir should be lockfile in error message
  xl: call libxl_dominfo_init in main_list
  xl: valid fd can be 0 in main_loadpolicy
  xl: call libxl_dominfo_{init,dispose} in main_cpupoolnumasplit
  libxlu: free buffer in failure path for PCI related functions
  python/xc: reinstate original implementation of next_bdf
  libxl: remove dead code libxl__domain_shutdown_reason

 tools/libxl/libxl.c               | 31 +++++++++++++++++++++----------
 tools/libxl/libxl_dom.c           | 17 -----------------
 tools/libxl/libxl_internal.h      |  1 -
 tools/libxl/libxlu_pci.c          |  2 ++
 tools/libxl/xl.c                  |  2 +-
 tools/libxl/xl_cmdimpl.c          | 23 +++++++++++++++++------
 tools/python/xen/lowlevel/xc/xc.c | 30 ++++++++++--------------------
 7 files changed, 51 insertions(+), 55 deletions(-)

-- 
2.1.4

[PATCH for-4.6 v2 1/8] libxl: properly clean up array in libxl_list_cpupool failure path

[Wei Liu]

Document how cpupool_info works.  Distinguish success (ERROR_FAIL +
ENOENT) vs failure in libxl_list_cpupool and properly clean up the array
in failure path.

Also switch to libxl__realloc and call libxl_cpupool_{init,dispose}
where appropriate.

There is change of behaviour. Previously if memory allocation fails the
said function returns NULL. Now memory allocation failure is fatal. This
is in line with how we deal with memory allocation failure in other
places in libxl though.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Dario Faggioli <dario.faggioli@citrix.com>
---
 tools/libxl/libxl.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ff0d616..c3a13f6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -682,6 +682,11 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
     return 0;
 }
 
+/* Returns:
+ *   0 - success
+ *   ERROR_FAIL + errno == ENOENT - no entry found
+ *   ERROR_$FOO + errno != ENOENT - other failure
+ */
 static int cpupool_info(libxl__gc *gc,
                         libxl_cpupoolinfo *info,
                         uint32_t poolid,
@@ -737,7 +742,8 @@ int libxl_cpupool_info(libxl_ctx *ctx,
 libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
 {
     GC_INIT(ctx);
-    libxl_cpupoolinfo info, *ptr, *tmp;
+    libxl_cpupoolinfo info, *ptr;
+
     int i;
     uint32_t poolid;
 
@@ -745,24 +751,29 @@ 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);
+            if (errno != ENOENT) goto out;
             break;
-        tmp = realloc(ptr, (i + 1) * sizeof(libxl_cpupoolinfo));
-        if (!tmp) {
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
-            libxl_cpupoolinfo_list_free(ptr, i);
-            ptr = NULL;
-            goto out;
         }
-        ptr = tmp;
+
+        ptr = libxl__realloc(NOGC, ptr, (i+1) * sizeof(libxl_cpupoolinfo));
         ptr[i] = info;
         poolid = info.poolid + 1;
+        /* Don't dispose of info because it will be returned to caller */
     }
 
     *nb_pool_out = i;
-out:
+
     GC_FREE;
     return ptr;
+
+out:
+    libxl_cpupoolinfo_list_free(ptr, i);
+    *nb_pool_out = 0;
+    GC_FREE;
+    return NULL;
 }
 
 /* this API call only list VM running on this host. A VM can
-- 
2.1.4

Re: [PATCH for-4.6 v2 1/8] libxl: properly clean up array in libxl_list_cpupool failure path

[Dario Faggioli]

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

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> Document how cpupool_info works.  Distinguish success (ERROR_FAIL +
> ENOENT) vs failure in libxl_list_cpupool and properly clean up the array
> in failure path.
> 
> Also switch to libxl__realloc and call libxl_cpupool_{init,dispose}
> where appropriate.
> 
> There is change of behaviour. Previously if memory allocation fails the
> said function returns NULL. Now memory allocation failure is fatal. This
> is in line with how we deal with memory allocation failure in other
> places in libxl though.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

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 for-4.6 v2 1/8] libxl: properly clean up array in libxl_list_cpupool failure path

[Ian Campbell]

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> Document how cpupool_info works.  Distinguish success (ERROR_FAIL +
> ENOENT) vs failure in libxl_list_cpupool and properly clean up the array
> in failure path.
> 
> Also switch to libxl__realloc and call libxl_cpupool_{init,dispose}
> where appropriate.
> 
> There is change of behaviour. Previously if memory allocation fails the
> said function returns NULL. Now memory allocation failure is fatal. This
> is in line with how we deal with memory allocation failure in other
> places in libxl though.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH for-4.6 v2 2/8] xl: lockdir should be lockfile in error message

[Wei Liu]

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

diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index f014306..5316ad9 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -118,7 +118,7 @@ static void parse_global_config(const char *configfile,
     }
 
     if (!lockfile) {
-        fprintf(stderr, "failed to allocate lockdir\n");
+        fprintf(stderr, "failed to allocate lockfile\n");
         exit(1);
     }
 
-- 
2.1.4

Re: [PATCH for-4.6 v2 2/8] xl: lockdir should be lockfile in error message

[Ian Campbell]

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH for-4.6 v2 3/8] xl: call libxl_dominfo_init in main_list

[Wei Liu]

Always call init and dispose function on info_buf though it's not
always used.

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d0bf0cb..63f80de 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4860,6 +4860,8 @@ int main_list(int argc, char **argv)
         break;
     }
 
+    libxl_dominfo_init(&info_buf);
+
     if (optind >= argc) {
         info = libxl_list_domain(ctx, &nb_domain);
         if (!info) {
@@ -4894,8 +4896,8 @@ int main_list(int argc, char **argv)
 
     if (info_free)
         libxl_dominfo_list_free(info, nb_domain);
-    else
-        libxl_dominfo_dispose(info);
+
+    libxl_dominfo_dispose(&info_buf);
 
     return 0;
 }
-- 
2.1.4

Re: [PATCH for-4.6 v2 3/8] xl: call libxl_dominfo_init in main_list

[Ian Campbell]

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> Always call init and dispose function on info_buf though it's not
> always used.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH for-4.6 v2 4/8] xl: valid fd can be 0 in main_loadpolicy

[Wei Liu]

Initialise polFd to -1 before hand to avoid closing 0 by accident.

Also fixed some style problems while I was there.

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 63f80de..77cf603 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7859,7 +7859,7 @@ int main_setenforce(int argc, char **argv)
 int main_loadpolicy(int argc, char **argv)
 {
     const char *polFName;
-    int polFd = 0;
+    int polFd = -1;
     void *polMemCp = NULL;
     struct stat info;
     int ret;
@@ -7871,7 +7871,7 @@ int main_loadpolicy(int argc, char **argv)
 
     polFName = argv[optind];
     polFd = open(polFName, O_RDONLY);
-    if ( polFd < 0 ) {
+    if (polFd < 0) {
         fprintf(stderr, "Error occurred opening policy file '%s': %s\n",
                 polFName, strerror(errno));
         ret = -1;
@@ -7879,7 +7879,7 @@ int main_loadpolicy(int argc, char **argv)
     }
 
     ret = stat(polFName, &info);
-    if ( ret < 0 ) {
+    if (ret < 0) {
         fprintf(stderr, "Error occurred retrieving information about"
                 "policy file '%s': %s\n", polFName, strerror(errno));
         goto done;
@@ -7911,7 +7911,7 @@ int main_loadpolicy(int argc, char **argv)
 
 done:
     free(polMemCp);
-    if ( polFd > 0 )
+    if (polFd >= 0)
         close(polFd);
 
     return ret;
-- 
2.1.4

Re: [PATCH for-4.6 v2 4/8] xl: valid fd can be 0 in main_loadpolicy

[Ian Campbell]

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> Initialise polFd to -1 before hand to avoid closing 0 by accident.
> 
> Also fixed some style problems while I was there.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH for-4.6 v2 5/8] xl: call libxl_dominfo_{init, dispose} in main_cpupoolnumasplit

[Wei Liu]

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 77cf603..70ed675 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7685,6 +7685,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
         /* No options */
     }
 
+    libxl_dominfo_init(&info);
+
     rc = 1;
 
     libxl_bitmap_init(&cpumap);
@@ -7741,6 +7743,12 @@ int main_cpupoolnumasplit(int argc, char **argv)
         goto out;
     }
     for (c = 0; c < 10; c++) {
+        /* We've called libxl_dominfo_init before the loop and will
+         * call libxl_dominfo_dispose after the loop when we're done
+         * with info.
+         */
+        libxl_dominfo_dispose(&info);
+        libxl_dominfo_init(&info);
         if (libxl_domain_info(ctx, &info, 0)) {
             fprintf(stderr, "error on getting info for Domain-0\n");
             goto out;
@@ -7793,6 +7801,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
 out:
     libxl_cputopology_list_free(topology, n_cpus);
     libxl_bitmap_dispose(&cpumap);
+    libxl_dominfo_dispose(&info);
     free(name);
 
     return rc;
-- 
2.1.4

Re: [PATCH for-4.6 v2 5/8] xl: call libxl_dominfo_{init, dispose} in main_cpupoolnumasplit

[Dario Faggioli]

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

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

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 for-4.6 v2 5/8] xl: call libxl_dominfo_{init, dispose} in main_cpupoolnumasplit

[Ian Campbell]

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

> ---
>  tools/libxl/xl_cmdimpl.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 77cf603..70ed675 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7685,6 +7685,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
>          /* No options */
>      }
>  
> +    libxl_dominfo_init(&info);
> +
>      rc = 1;
>  
>      libxl_bitmap_init(&cpumap);

I'd have been inclined to init everything in one block, but nevermind.

[PATCH for-4.6 v2 6/8] libxlu: free buffer in failure path for PCI related functions

[Wei Liu]

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

diff --git a/tools/libxl/libxlu_pci.c b/tools/libxl/libxlu_pci.c
index 026413b..16ddaf7 100644
--- a/tools/libxl/libxlu_pci.c
+++ b/tools/libxl/libxlu_pci.c
@@ -178,6 +178,7 @@ int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str
     return 0;
 
 parse_error:
+    free(buf2);
     return ERROR_INVAL;
 }
 
@@ -254,6 +255,7 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str)
     return 0;
 
 parse_error:
+    free(buf2);
     return ERROR_INVAL;
 }
 
-- 
2.1.4

Re: [PATCH for-4.6 v2 6/8] libxlu: free buffer in failure path for PCI related functions

[Ian Campbell]

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxlu_pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/libxl/libxlu_pci.c b/tools/libxl/libxlu_pci.c
> index 026413b..16ddaf7 100644
> --- a/tools/libxl/libxlu_pci.c
> +++ b/tools/libxl/libxlu_pci.c
> @@ -178,6 +178,7 @@ int xlu_pci_parse_bdf(XLU_Config *cfg, 
> libxl_device_pci *pcidev, const char *str
>      return 0;
>  
>  parse_error:
> +    free(buf2);

There is a path (half a dozen lines above) which can do free(buf2) followed
by goto parse_error. That free ought to set buf2=NULL, or the free needs to
move after that check on the success path.


>      return ERROR_INVAL;
>  }
>  
> @@ -254,6 +255,7 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve 
> *rdm, const char *str)
>      return 0;
>  
>  parse_error:
> +    free(buf2);

Same here.

>      return ERROR_INVAL;
>  }
>  

[PATCH for-4.6 v2 7/8] python/xc: reinstate original implementation of next_bdf

[Wei Liu]

I missed the fact that next_bdf is used to parsed user supplied
strings when reviewing. The user supplied string is a NULL-terminated
string separated by comma. User can supply several PCI devices in that
string. There is, however, no delimiter for different devices, hence
we can't change the syntax of that string.

This patch reinstate the original implementation of next_bdf to
preserve the original syntax. The last argument for xc_assign_device
is always 0.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Tiejun Chen <tiejun.chen@intel.com>

Tiejun, are you actually using this python binding? I don't think we
have in tree user.

If nobody is using it, I propose we remove this binding in next
release.

I don't have live example of that string. My analysis is based on
reverse-engineering of original code.
---
 tools/python/xen/lowlevel/xc/xc.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index c8380d1..2c36eb2 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -592,8 +592,7 @@ static int token_value(char *token)
     return strtol(token, NULL, 16);
 }
 
-static int next_bdf(char **str, int *seg, int *bus, int *dev, int *func,
-                    int *flag)
+static int next_bdf(char **str, int *seg, int *bus, int *dev, int *func)
 {
     char *token;
 
@@ -608,17 +607,8 @@ static int next_bdf(char **str, int *seg, int *bus, int *dev, int *func,
     *dev  = token_value(token);
     token = strchr(token, ',') + 1;
     *func  = token_value(token);
-    token = strchr(token, ',') + 1;
-    if ( token ) {
-        *flag = token_value(token);
-        *str = token + 1;
-    }
-    else
-    {
-        /* O means we take "strict" as our default policy. */
-        *flag = 0;
-        *str = NULL;
-    }
+    token = strchr(token, ',');
+    *str = token ? token + 1 : NULL;
 
     return 1;
 }
@@ -630,14 +620,14 @@ static PyObject *pyxc_test_assign_device(XcObject *self,
     uint32_t dom;
     char *pci_str;
     int32_t sbdf = 0;
-    int seg, bus, dev, func, flag;
+    int seg, bus, dev, func;
 
     static char *kwd_list[] = { "domid", "pci", NULL };
     if ( !PyArg_ParseTupleAndKeywords(args, kwds, "is", kwd_list,
                                       &dom, &pci_str) )
         return NULL;
 
-    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func, &flag) )
+    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func) )
     {
         sbdf = seg << 16;
         sbdf |= (bus & 0xff) << 8;
@@ -663,21 +653,21 @@ static PyObject *pyxc_assign_device(XcObject *self,
     uint32_t dom;
     char *pci_str;
     int32_t sbdf = 0;
-    int seg, bus, dev, func, flag;
+    int seg, bus, dev, func;
 
     static char *kwd_list[] = { "domid", "pci", NULL };
     if ( !PyArg_ParseTupleAndKeywords(args, kwds, "is", kwd_list,
                                       &dom, &pci_str) )
         return NULL;
 
-    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func, &flag) )
+    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func) )
     {
         sbdf = seg << 16;
         sbdf |= (bus & 0xff) << 8;
         sbdf |= (dev & 0x1f) << 3;
         sbdf |= (func & 0x7);
 
-        if ( xc_assign_device(self->xc_handle, dom, sbdf, flag) != 0 )
+        if ( xc_assign_device(self->xc_handle, dom, sbdf, 0) != 0 )
         {
             if (errno == ENOSYS)
                 sbdf = -1;
@@ -696,14 +686,14 @@ static PyObject *pyxc_deassign_device(XcObject *self,
     uint32_t dom;
     char *pci_str;
     int32_t sbdf = 0;
-    int seg, bus, dev, func, flag;
+    int seg, bus, dev, func;
 
     static char *kwd_list[] = { "domid", "pci", NULL };
     if ( !PyArg_ParseTupleAndKeywords(args, kwds, "is", kwd_list,
                                       &dom, &pci_str) )
         return NULL;
 
-    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func, &flag) )
+    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func) )
     {
         sbdf = seg << 16;
         sbdf |= (bus & 0xff) << 8;
-- 
2.1.4

Re: [PATCH for-4.6 v2 7/8] python/xc: reinstate original implementation of next_bdf

[Andrew Cooper]

On 27/07/2015 18:45, Wei Liu wrote:
> I missed the fact that next_bdf is used to parsed user supplied
> strings when reviewing. The user supplied string is a NULL-terminated
> string separated by comma. User can supply several PCI devices in that
> string. There is, however, no delimiter for different devices, hence
> we can't change the syntax of that string.
>
> This patch reinstate the original implementation of next_bdf to
> preserve the original syntax. The last argument for xc_assign_device
> is always 0.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Tiejun Chen <tiejun.chen@intel.com>
>
> Tiejun, are you actually using this python binding? I don't think we
> have in tree user.
>
> If nobody is using it, I propose we remove this binding in next
> release.
>
> I don't have live example of that string. My analysis is based on
> reverse-engineering of original code.

XenServer still uses a few methods from the lowlevel xc bindings, but
this only extends to debug situations (our version of xen-bugtool) and I
am not aware of us using this particular bit of the bindings.

~Andrew

Re: [PATCH for-4.6 v2 7/8] python/xc: reinstate original implementation of next_bdf

[Chen, Tiejun]

On 2015/7/28 1:45, Wei Liu wrote:
> I missed the fact that next_bdf is used to parsed user supplied
> strings when reviewing. The user supplied string is a NULL-terminated
> string separated by comma. User can supply several PCI devices in that
> string. There is, however, no delimiter for different devices, hence
> we can't change the syntax of that string.
>
> This patch reinstate the original implementation of next_bdf to
> preserve the original syntax. The last argument for xc_assign_device
> is always 0.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Tiejun Chen <tiejun.chen@intel.com>
>
> Tiejun, are you actually using this python binding? I don't think we

This change is just following to RMRR series but currently we don't use 
this. So its fine if you think this don't break any other usages.

Thanks
Tiejun

> have in tree user.
>
> If nobody is using it, I propose we remove this binding in next
> release.
>
> I don't have live example of that string. My analysis is based on
> reverse-engineering of original code.
> ---
>   tools/python/xen/lowlevel/xc/xc.c | 30 ++++++++++--------------------
>   1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index c8380d1..2c36eb2 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -592,8 +592,7 @@ static int token_value(char *token)
>       return strtol(token, NULL, 16);
>   }
>
> -static int next_bdf(char **str, int *seg, int *bus, int *dev, int *func,
> -                    int *flag)
> +static int next_bdf(char **str, int *seg, int *bus, int *dev, int *func)
>   {
>       char *token;
>
> @@ -608,17 +607,8 @@ static int next_bdf(char **str, int *seg, int *bus, int *dev, int *func,
>       *dev  = token_value(token);
>       token = strchr(token, ',') + 1;
>       *func  = token_value(token);
> -    token = strchr(token, ',') + 1;
> -    if ( token ) {
> -        *flag = token_value(token);
> -        *str = token + 1;
> -    }
> -    else
> -    {
> -        /* O means we take "strict" as our default policy. */
> -        *flag = 0;
> -        *str = NULL;
> -    }
> +    token = strchr(token, ',');
> +    *str = token ? token + 1 : NULL;
>
>       return 1;
>   }
> @@ -630,14 +620,14 @@ static PyObject *pyxc_test_assign_device(XcObject *self,
>       uint32_t dom;
>       char *pci_str;
>       int32_t sbdf = 0;
> -    int seg, bus, dev, func, flag;
> +    int seg, bus, dev, func;
>
>       static char *kwd_list[] = { "domid", "pci", NULL };
>       if ( !PyArg_ParseTupleAndKeywords(args, kwds, "is", kwd_list,
>                                         &dom, &pci_str) )
>           return NULL;
>
> -    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func, &flag) )
> +    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func) )
>       {
>           sbdf = seg << 16;
>           sbdf |= (bus & 0xff) << 8;
> @@ -663,21 +653,21 @@ static PyObject *pyxc_assign_device(XcObject *self,
>       uint32_t dom;
>       char *pci_str;
>       int32_t sbdf = 0;
> -    int seg, bus, dev, func, flag;
> +    int seg, bus, dev, func;
>
>       static char *kwd_list[] = { "domid", "pci", NULL };
>       if ( !PyArg_ParseTupleAndKeywords(args, kwds, "is", kwd_list,
>                                         &dom, &pci_str) )
>           return NULL;
>
> -    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func, &flag) )
> +    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func) )
>       {
>           sbdf = seg << 16;
>           sbdf |= (bus & 0xff) << 8;
>           sbdf |= (dev & 0x1f) << 3;
>           sbdf |= (func & 0x7);
>
> -        if ( xc_assign_device(self->xc_handle, dom, sbdf, flag) != 0 )
> +        if ( xc_assign_device(self->xc_handle, dom, sbdf, 0) != 0 )
>           {
>               if (errno == ENOSYS)
>                   sbdf = -1;
> @@ -696,14 +686,14 @@ static PyObject *pyxc_deassign_device(XcObject *self,
>       uint32_t dom;
>       char *pci_str;
>       int32_t sbdf = 0;
> -    int seg, bus, dev, func, flag;
> +    int seg, bus, dev, func;
>
>       static char *kwd_list[] = { "domid", "pci", NULL };
>       if ( !PyArg_ParseTupleAndKeywords(args, kwds, "is", kwd_list,
>                                         &dom, &pci_str) )
>           return NULL;
>
> -    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func, &flag) )
> +    while ( next_bdf(&pci_str, &seg, &bus, &dev, &func) )
>       {
>           sbdf = seg << 16;
>           sbdf |= (bus & 0xff) << 8;
>

Re: [PATCH for-4.6 v2 7/8] python/xc: reinstate original implementation of next_bdf

[Ian Campbell]

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> I missed the fact that next_bdf is used to parsed user supplied
> strings when reviewing. The user supplied string is a NULL-terminated
> string separated by comma. User can supply several PCI devices in that
> string. There is, however, no delimiter for different devices, hence
> we can't change the syntax of that string.
> 
> This patch reinstate the original implementation of next_bdf to
> preserve the original syntax. The last argument for xc_assign_device
> is always 0.

Specifically it returns us to exactly the state in 9b34056cb4ca~1, I
believe? Plus an extra 0 flags parameter?

> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Tiejun Chen <tiejun.chen@intel.com>
> 
> Tiejun, are you actually using this python binding? I don't think we
> have in tree user.
> 
> If nobody is using it, I propose we remove this binding in next
> release.
> 
> I don't have live example of that string. My analysis is based on
> reverse-engineering of original code.

FWIW I've said several times that it is not necessary to plumb new options
such as this through the Python bindings, it is sufficient to pass in
whatever value means "do as you did before". If a user of the Python
bindings wants to then plumb in the ability to actually set the option
(i.e. there is a use case for it) then that can be done later.

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

Re: [PATCH for-4.6 v2 7/8] python/xc: reinstate original implementation of next_bdf

[Wei Liu]

On Tue, Jul 28, 2015 at 11:38:29AM +0100, Ian Campbell wrote:
> On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> > I missed the fact that next_bdf is used to parsed user supplied
> > strings when reviewing. The user supplied string is a NULL-terminated
> > string separated by comma. User can supply several PCI devices in that
> > string. There is, however, no delimiter for different devices, hence
> > we can't change the syntax of that string.
> > 
> > This patch reinstate the original implementation of next_bdf to
> > preserve the original syntax. The last argument for xc_assign_device
> > is always 0.
> 
> Specifically it returns us to exactly the state in 9b34056cb4ca~1, I
> believe? Plus an extra 0 flags parameter?
> 

Yes to both questions.

> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: Tiejun Chen <tiejun.chen@intel.com>
> > 
> > Tiejun, are you actually using this python binding? I don't think we
> > have in tree user.
> > 
> > If nobody is using it, I propose we remove this binding in next
> > release.
> > 
> > I don't have live example of that string. My analysis is based on
> > reverse-engineering of original code.
> 
> FWIW I've said several times that it is not necessary to plumb new options
> such as this through the Python bindings, it is sufficient to pass in
> whatever value means "do as you did before". If a user of the Python
> bindings wants to then plumb in the ability to actually set the option
> (i.e. there is a use case for it) then that can be done later.
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH for-4.6 v2 8/8] libxl: remove dead code libxl__domain_shutdown_reason

[Wei Liu]

There is no user in tree.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dom.c      | 17 -----------------
 tools/libxl/libxl_internal.h |  1 -
 2 files changed, 18 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index b3ae5b5..5555fea 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -42,23 +42,6 @@ libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
         return LIBXL_DOMAIN_TYPE_PV;
 }
 
-int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid)
-{
-    libxl_ctx *ctx = libxl__gc_owner(gc);
-    xc_domaininfo_t info;
-    int ret;
-
-    ret = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
-    if (ret != 1)
-        return -1;
-    if (info.domain != domid)
-        return -1;
-    if (!(info.flags & XEN_DOMINF_shutdown))
-        return -1;
-
-    return (info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask;
-}
-
 int libxl__domain_cpupool(libxl__gc *gc, uint32_t domid)
 {
     xc_domaininfo_t info;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f2466dc..c7e4709 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1042,7 +1042,6 @@ _hidden int libxl__file_reference_unmap(libxl__file_reference *f);
 
 /* from xl_dom */
 _hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid);
-_hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
 _hidden int libxl__domain_cpupool(libxl__gc *gc, uint32_t domid);
 _hidden libxl_scheduler libxl__domain_scheduler(libxl__gc *gc, uint32_t domid);
 _hidden int libxl__sched_set_params(libxl__gc *gc, uint32_t domid,
-- 
2.1.4

Re: [PATCH for-4.6 v2 8/8] libxl: remove dead code libxl__domain_shutdown_reason

[Ian Campbell]

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> There is no user in tree.

git log -S libxl__domain_shutdown_reason
git log -S get_shutdown_reason

(the later being a rename that the former picks up on) suggests it has
_never_ been used!

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

> ---
>  tools/libxl/libxl_dom.c      | 17 -----------------
>  tools/libxl/libxl_internal.h |  1 -
>  2 files changed, 18 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index b3ae5b5..5555fea 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -42,23 +42,6 @@ libxl_domain_type libxl__domain_type(libxl__gc *gc, 
> uint32_t domid)
>          return LIBXL_DOMAIN_TYPE_PV;
>  }
>  
> -int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid)
> -{
> -    libxl_ctx *ctx = libxl__gc_owner(gc);
> -    xc_domaininfo_t info;
> -    int ret;
> -
> -    ret = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
> -    if (ret != 1)
> -        return -1;
> -    if (info.domain != domid)
> -        return -1;
> -    if (!(info.flags & XEN_DOMINF_shutdown))
> -        return -1;
> -
> -    return (info.flags >> XEN_DOMINF_shutdownshift) & 
> XEN_DOMINF_shutdownmask;
> -}
> -
>  int libxl__domain_cpupool(libxl__gc *gc, uint32_t domid)
>  {
>      xc_domaininfo_t info;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index f2466dc..c7e4709 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1042,7 +1042,6 @@ _hidden int 
> libxl__file_reference_unmap(libxl__file_reference *f);
>  
>  /* from xl_dom */
>  _hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t 
> domid);
> -_hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t 
> domid);
>  _hidden int libxl__domain_cpupool(libxl__gc *gc, uint32_t domid);
>  _hidden libxl_scheduler libxl__domain_scheduler(libxl__gc *gc, uint32_t 
> domid);
>  _hidden int libxl__sched_set_params(libxl__gc *gc, uint32_t domid,

Re: [PATCH for-4.6 v2 0/8] tools: fixes inspired by Coverity scan

[Ian Campbell]

On Mon, 2015-07-27 at 18:45 +0100, Wei Liu wrote:
> Wei Liu (8):

Applied 7 of these, but not:

>   libxlu: free buffer in failure path for PCI related functions

which I have commented on.

Ian.