xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands
@ 2016-04-01 12:40 Paulina Szubarczyk
  2016-04-01 12:40 ` [PATCH 01/10] libxl_pci: improve return codes " Paulina Szubarczyk
                   ` (9 more replies)
  0 siblings, 10 replies; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

This is my bite-sized outreachy project [1][2]. I resend it due to the mistake I made in the first try.

The patch aims to improve coding_style and return failure for more xl commands:
- pci-*
-- tmem-*

After rebase to staging it seems that the patch {09} cleaning libxl_set_memory_target() 
to return useful error codes from [0] is not applied I resent it here witouth changes.

In pci-* functions libxl__create_pci_backend(), libxl__device_pci_destroy_all()
error codes are returned instead of always 0.

Int the changes I follow CODING_STYLE as in the coresponding patches:
 - In general, don't initialize rc / r, but use set-and-goto
 - Use 'r' for non-libxl error codes
 - Use EXIT_FAILURE and EXIT_SUCCESS in main_foo()
 - Use 1 and 0 in internal functions of xl

[0] http://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02246.html
[1] http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg03031.html
[2] https://www.mail-archive.com/xen-devel@lists.xen.org/msg62055.html

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 01/10] libxl_pci: improve return codes for more xl commands
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 14:00   ` Roger Pau Monné
  2016-04-01 12:40 ` [PATCH 02/10] libxl-pci: clean unused return variable Paulina Szubarczyk
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

Retrun error code instead of allways 0. Remove assigned-only
ret variable.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/libxl_pci.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index dc10cb7..3435ce2 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -84,13 +84,11 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
     flexarray_t *front = NULL;
     flexarray_t *back = NULL;
     libxl__device device;
-    int ret = ERROR_NOMEM, i;
+    int i;
 
     front = flexarray_make(gc, 16, 1);
     back = flexarray_make(gc, 16, 1);
 
-    ret = 0;
-
     LOG(DEBUG, "Creating pci backend");
 
     /* add pci device */
@@ -108,12 +106,10 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
     flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", 0));
     flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising));
 
-    libxl__device_generic_add(gc, XBT_NULL, &device,
+    return libxl__device_generic_add(gc, XBT_NULL, &device,
                               libxl__xs_kvs_of_flexarray(gc, back, back->count),
                               libxl__xs_kvs_of_flexarray(gc, front, front->count),
                               NULL);
-
-    return 0;
 }
 
 static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting)
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 02/10] libxl-pci: clean unused return variable
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
  2016-04-01 12:40 ` [PATCH 01/10] libxl_pci: improve return codes " Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 14:13   ` Roger Pau Monné
  2016-04-01 12:40 ` [PATCH 03/10] libxl_pci: Return error code for more pci-* functions Paulina Szubarczyk
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

libxl_device_pci_assignable_list returns:
- list of the libxl_device_pci
- NULL in case of error

the rc variable is unused as return code.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/libxl_pci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 3435ce2..6051ee4 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -397,12 +397,11 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
     libxl_device_pci *pcidevs = NULL, *new, *assigned;
     struct dirent *de;
     DIR *dir;
-    int rc, num_assigned;
+    int num_assigned;
 
     *num = 0;
 
-    rc = get_all_assigned_devices(gc, &assigned, &num_assigned);
-    if ( rc )
+    if ( get_all_assigned_devices(gc, &assigned, &num_assigned) )
         goto out;
 
     dir = opendir(SYSFS_PCIBACK_DRIVER);
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 03/10] libxl_pci: Return error code for more pci-* functions
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
  2016-04-01 12:40 ` [PATCH 01/10] libxl_pci: improve return codes " Paulina Szubarczyk
  2016-04-01 12:40 ` [PATCH 02/10] libxl-pci: clean unused return variable Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 14:20   ` Roger Pau Monné
  2016-04-01 12:40 ` [PATCH 04/10] xl: improve return code for freemem function Paulina Szubarczyk
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

Return rc value instead of allways 0.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/libxl_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 6051ee4..e4a2c2c 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1604,7 +1604,7 @@ int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid)
     }
 
     free(pcidevs);
-    return 0;
+    return rc;
 }
 
 int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid,
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 04/10] xl: improve return code for freemem function
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
                   ` (2 preceding siblings ...)
  2016-04-01 12:40 ` [PATCH 03/10] libxl_pci: Return error code for more pci-* functions Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 14:24   ` Roger Pau Monné
  2016-04-01 12:40 ` [PATCH 05/10] xl_cmdimpl: improve return codes for memset commands Paulina Szubarczyk
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

- Return 0 or 1 for freemem function
- Correct the condition of checking return values of freemem

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7750995..7ee6953 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2678,38 +2678,34 @@ static int preserve_domain(uint32_t *r_domid, libxl_event *event,
 
 static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
 {
-    int rc, retries = 3;
+    int retries = 3;
     uint32_t need_memkb, free_memkb;
 
     if (!autoballoon)
         return 0;
 
-    rc = libxl_domain_need_memory(ctx, b_info, &need_memkb);
-    if (rc < 0)
-        return rc;
+    if (libxl_domain_need_memory(ctx, b_info, &need_memkb) < 0)
+        return 1;
 
     do {
-        rc = libxl_get_free_memory(ctx, &free_memkb);
-        if (rc < 0)
-            return rc;
+        if (libxl_get_free_memory(ctx, &free_memkb) < 0)
+            return 1;
 
         if (free_memkb >= need_memkb)
             return 0;
 
-        rc = libxl_set_memory_target(ctx, 0, free_memkb - need_memkb, 1, 0);
-        if (rc < 0)
-            return rc;
+        if (libxl_set_memory_target(ctx, 0, free_memkb - need_memkb, 1, 0) < 0)
+            return 1;
 
         /* wait until dom0 reaches its target, as long as we are making
          * progress */
-        rc = libxl_wait_for_memory_target(ctx, 0, 10);
-        if (rc < 0)
-            return rc;
+        if (libxl_wait_for_memory_target(ctx, 0, 10) < 0)
+            return 1;
 
         retries--;
     } while (retries > 0);
 
-    return ERROR_NOMEM;
+    return 1;
 }
 
 static void autoconnect_console(libxl_ctx *ctx_ignored,
@@ -2975,7 +2971,7 @@ start:
 
     if (domid_soft_reset == INVALID_DOMID) {
         ret = freemem(domid, &d_config.b_info);
-        if (ret < 0) {
+        if (ret) {
             fprintf(stderr, "failed to free memory for the domain\n");
             ret = ERROR_FAIL;
             goto error_out;
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 05/10] xl_cmdimpl: improve return codes for memset commands
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
                   ` (3 preceding siblings ...)
  2016-04-01 12:40 ` [PATCH 04/10] xl: improve return code for freemem function Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 14:33   ` Roger Pau Monné
  2016-04-01 12:40 ` [PATCH 06/10] xl_cmdimpl: improve return codes for cd-insert commands Paulina Szubarczyk
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

 - Use EXIT_{SUCCESS,FAILURE} for main_mem*() function
 - Use 0/1 as return values of set_memory_{max,target}

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7ee6953..31f037f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3380,15 +3380,15 @@ static int set_memory_max(uint32_t domid, const char *mem)
     memorykb = parse_mem_size_kb(mem);
     if (memorykb == -1) {
         fprintf(stderr, "invalid memory size: %s\n", mem);
-        exit(3);
+        exit(EXIT_FAILURE);
     }
 
     if (libxl_domain_setmaxmem(ctx, domid, memorykb)) {
         fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem);
-        return EXIT_FAILURE;
+        return 1;
     }
 
-    return EXIT_SUCCESS;
+    return 0;
 }
 
 int main_memmax(int argc, char **argv)
@@ -3404,7 +3404,11 @@ int main_memmax(int argc, char **argv)
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
 
-    return set_memory_max(domid, mem);
+    if (set_memory_max(domid, mem)) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 static int set_memory_target(uint32_t domid, const char *mem)
@@ -3414,15 +3418,15 @@ static int set_memory_target(uint32_t domid, const char *mem)
     memorykb = parse_mem_size_kb(mem);
     if (memorykb == -1)  {
         fprintf(stderr, "invalid memory size: %s\n", mem);
-        exit(3);
+        exit(EXIT_FAILURE);
     }
 
     if (libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1)) {
         fprintf(stderr, "cannot set domid %d dynamic max memory to : %s\n", domid, mem);
-        return EXIT_FAILURE;
+        return 1;
     }
 
-    return EXIT_SUCCESS;
+    return 0;
 }
 
 int main_memset(int argc, char **argv)
@@ -3438,7 +3442,11 @@ int main_memset(int argc, char **argv)
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
 
-    return set_memory_target(domid, mem);
+    if (set_memory_target(domid, mem)) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 06/10] xl_cmdimpl: improve return codes for cd-insert commands
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
                   ` (4 preceding siblings ...)
  2016-04-01 12:40 ` [PATCH 05/10] xl_cmdimpl: improve return codes for memset commands Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 14:41   ` Roger Pau Monné
  2016-04-01 12:40 ` [PATCH 07/10] Add return codes for pci-detach, pci-attach, pci-asssignable-add, and pci-assignable-remove Paulina Szubarczyk
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

 - Use EXIT_{SUCCESS,FAILURE} for main_cd*() function
 - Use 0/1 as return values of cd_insert function

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 31f037f..2232a1e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3471,16 +3471,16 @@ static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
         && stat(disk.pdev_path, &b)) {
         fprintf(stderr, "Cannot stat file: %s\n",
                 disk.pdev_path);
-        r = EXIT_FAILURE;
+        r = 1;
         goto out;
     }
 
     if (libxl_cdrom_insert(ctx, domid, &disk, NULL)) {
-        r = EXIT_FAILURE;
+        r = 1;
         goto out;
     }
 
-    r = EXIT_SUCCESS;
+    r = 0;
 
 out:
     libxl_device_disk_dispose(&disk);
@@ -3502,7 +3502,11 @@ int main_cd_eject(int argc, char **argv)
     domid = find_domain(argv[optind]);
     virtdev = argv[optind + 1];
 
-    return cd_insert(domid, virtdev, NULL);
+    if (cd_insert(domid, virtdev, NULL)) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 int main_cd_insert(int argc, char **argv)
@@ -3520,7 +3524,11 @@ int main_cd_insert(int argc, char **argv)
     virtdev = argv[optind + 1];
     file = argv[optind + 2];
 
-    return cd_insert(domid, virtdev, file);
+    if (cd_insert(domid, virtdev, file)) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 int main_usbctrl_attach(int argc, char **argv)
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 07/10] Add return codes for pci-detach, pci-attach, pci-asssignable-add, and pci-assignable-remove.
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
                   ` (5 preceding siblings ...)
  2016-04-01 12:40 ` [PATCH 06/10] xl_cmdimpl: improve return codes for cd-insert commands Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 12:40 ` [PATCH 08/10] libxl: improve main_tmem_* return codes Paulina Szubarczyk
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

Returning error codes makes it easier for shell scripts to tell if a
command has failed or succeeded.

NB this violates the CODING_STYLE preference for not initializing the
return-value variable at declaration; but in these cases, having a
"goto out" that jumped over nothing but an "r = 0" seemed
a bit pointless.

Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 78 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 22 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2232a1e..38129f4 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3808,10 +3808,11 @@ int main_pcilist(int argc, char **argv)
     return 0;
 }
 
-static void pcidetach(uint32_t domid, const char *bdf, int force)
+static int pcidetach(uint32_t domid, const char *bdf, int force)
 {
     libxl_device_pci pcidev;
     XLU_Config *config;
+    int r = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3820,15 +3821,20 @@ static void pcidetach(uint32_t domid, const char *bdf, int force)
 
     if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
         fprintf(stderr, "pci-detach: malformed BDF specification \"%s\"\n", bdf);
-        exit(2);
+        exit(EXIT_FAILURE);
+    }
+    if (force) {
+        if (libxl_device_pci_destroy(ctx, domid, &pcidev, 0))
+            r = 1;
+    } else {
+        if (libxl_device_pci_remove(ctx, domid, &pcidev, 0))
+            r = 1;
     }
-    if (force)
-        libxl_device_pci_destroy(ctx, domid, &pcidev, 0);
-    else
-        libxl_device_pci_remove(ctx, domid, &pcidev, 0);
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pcidetach(int argc, char **argv)
@@ -3847,13 +3853,18 @@ int main_pcidetach(int argc, char **argv)
     domid = find_domain(argv[optind]);
     bdf = argv[optind + 1];
 
-    pcidetach(domid, bdf, force);
-    return 0;
+    if (pcidetach(domid, bdf, force)) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
-static void pciattach(uint32_t domid, const char *bdf, const char *vs)
+
+static int pciattach(uint32_t domid, const char *bdf, const char *vs)
 {
     libxl_device_pci pcidev;
     XLU_Config *config;
+    int r = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3862,12 +3873,16 @@ static void pciattach(uint32_t domid, const char *bdf, const char *vs)
 
     if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
         fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", bdf);
-        exit(2);
+        exit(EXIT_FAILURE);
     }
-    libxl_device_pci_add(ctx, domid, &pcidev, 0);
+    
+    if (libxl_device_pci_add(ctx, domid, &pcidev, 0)) 
+        r = 1;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return 1;
 }
 
 int main_pciattach(int argc, char **argv)
@@ -3886,8 +3901,11 @@ int main_pciattach(int argc, char **argv)
     if (optind + 1 < argc)
         vs = argv[optind + 2];
 
-    pciattach(domid, bdf, vs);
-    return 0;
+    if (pciattach(domid, bdf, vs)) {
+        return EXIT_FAILURE;
+    }
+    
+    return EXIT_SUCCESS;
 }
 
 static void pciassignable_list(void)
@@ -3919,10 +3937,11 @@ int main_pciassignable_list(int argc, char **argv)
     return 0;
 }
 
-static void pciassignable_add(const char *bdf, int rebind)
+static int pciassignable_add(const char *bdf, int rebind)
 {
     libxl_device_pci pcidev;
     XLU_Config *config;
+    int r = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3931,12 +3950,16 @@ static void pciassignable_add(const char *bdf, int rebind)
 
     if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
         fprintf(stderr, "pci-assignable-add: malformed BDF specification \"%s\"\n", bdf);
-        exit(2);
+        exit(EXIT_FAILURE);
     }
-    libxl_device_pci_assignable_add(ctx, &pcidev, rebind);
+    
+    if (libxl_device_pci_assignable_add(ctx, &pcidev, rebind))
+        r = 1;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciassignable_add(int argc, char **argv)
@@ -3950,14 +3973,18 @@ int main_pciassignable_add(int argc, char **argv)
 
     bdf = argv[optind];
 
-    pciassignable_add(bdf, 1);
-    return 0;
+    if (pciassignable_add(bdf, 1)) {
+        return EXIT_FAILURE;
+    }
+    
+    return EXIT_SUCCESS;
 }
 
-static void pciassignable_remove(const char *bdf, int rebind)
+static int pciassignable_remove(const char *bdf, int rebind)
 {
     libxl_device_pci pcidev;
     XLU_Config *config;
+    int r = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3968,10 +3995,14 @@ static void pciassignable_remove(const char *bdf, int rebind)
         fprintf(stderr, "pci-assignable-remove: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
-    libxl_device_pci_assignable_remove(ctx, &pcidev, rebind);
+    
+    if (libxl_device_pci_assignable_remove(ctx, &pcidev, rebind))
+        r = 1;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciassignable_remove(int argc, char **argv)
@@ -3988,8 +4019,11 @@ int main_pciassignable_remove(int argc, char **argv)
 
     bdf = argv[optind];
 
-    pciassignable_remove(bdf, rebind);
-    return 0;
+    if (pciassignable_remove(bdf, rebind)) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 static void pause_domain(uint32_t domid)
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 08/10] libxl: improve main_tmem_* return codes
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
                   ` (6 preceding siblings ...)
  2016-04-01 12:40 ` [PATCH 07/10] Add return codes for pci-detach, pci-attach, pci-asssignable-add, and pci-assignable-remove Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 14:50   ` Roger Pau Monné
  2016-04-01 15:09   ` Konrad Rzeszutek Wilk
  2016-04-01 12:40 ` [PATCH 09/10] libxl: Fix libxl_set_memory_target return value Paulina Szubarczyk
  2016-04-01 12:40 ` [PATCH 10/10] libxl: libxl_tmem functions improving coding style Paulina Szubarczyk
  9 siblings, 2 replies; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

Functions libxl_tmem_freeze(), libxl_tmem_thaw(), libxl_tmem_set() and
libxl_tmem_shared_auth() located in libxl.c file return ERROR_FAIL/ERROR_INVAL 
or internal error codes from libxc library.

Improve main_tmem_* return codes by returning EXIT_{SUCCESS/FAILURE}
accordingly to return codes of those functions.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 51 ++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 38129f4..52c3b9b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7519,7 +7519,7 @@ int main_tmem_freeze(int argc, char **argv)
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-freeze");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (all)
@@ -7527,8 +7527,11 @@ int main_tmem_freeze(int argc, char **argv)
     else
         domid = find_domain(dom);
 
-    libxl_tmem_freeze(ctx, domid);
-    return 0;
+    if (libxl_tmem_freeze(ctx, domid) < 0) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_thaw(int argc, char **argv)
@@ -7548,7 +7551,7 @@ int main_tmem_thaw(int argc, char **argv)
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-thaw");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (all)
@@ -7556,8 +7559,11 @@ int main_tmem_thaw(int argc, char **argv)
     else
         domid = find_domain(dom);
 
-    libxl_tmem_thaw(ctx, domid);
-    return 0;
+    if (libxl_tmem_thaw(ctx, domid) < 0) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_set(int argc, char **argv)
@@ -7568,6 +7574,7 @@ int main_tmem_set(int argc, char **argv)
     int opt_w = 0, opt_c = 0, opt_p = 0;
     int all = 0;
     int opt;
+    int r = 0;
 
     SWITCH_FOREACH_OPT(opt, "aw:c:p:", NULL, "tmem-set", 0) {
     case 'a':
@@ -7591,7 +7598,7 @@ int main_tmem_set(int argc, char **argv)
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-set");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (all)
@@ -7602,17 +7609,21 @@ int main_tmem_set(int argc, char **argv)
     if (!opt_w && !opt_c && !opt_p) {
         fprintf(stderr, "No set value specified.\n\n");
         help("tmem-set");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (opt_w)
-        libxl_tmem_set(ctx, domid, "weight", weight);
+        r = libxl_tmem_set(ctx, domid, "weight", weight);
     if (opt_c)
-        libxl_tmem_set(ctx, domid, "cap", cap);
+        r = libxl_tmem_set(ctx, domid, "cap", cap);
     if (opt_p)
-        libxl_tmem_set(ctx, domid, "compress", compress);
+        r = libxl_tmem_set(ctx, domid, "compress", compress);
 
-    return 0;
+    if (r < 0) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_shared_auth(int argc, char **argv)
@@ -7642,7 +7653,7 @@ int main_tmem_shared_auth(int argc, char **argv)
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-shared-auth");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (all)
@@ -7653,18 +7664,20 @@ int main_tmem_shared_auth(int argc, char **argv)
     if (uuid == NULL || autharg == NULL) {
         fprintf(stderr, "No uuid or auth specified.\n\n");
         help("tmem-shared-auth");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     auth = strtol(autharg, &endptr, 10);
     if (*endptr != '\0') {
         fprintf(stderr, "Invalid auth, valid auth are <0|1>.\n\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
-    libxl_tmem_shared_auth(ctx, domid, uuid, auth);
+    if (libxl_tmem_shared_auth(ctx, domid, uuid, auth) < 0) {
+        return EXIT_FAILURE;
+    }
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_freeable(int argc, char **argv)
@@ -7678,10 +7691,10 @@ int main_tmem_freeable(int argc, char **argv)
 
     mb = libxl_tmem_freeable(ctx);
     if (mb == -1)
-        return -1;
+        return EXIT_FAILURE;
 
     printf("%d\n", mb);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_cpupoolcreate(int argc, char **argv)
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 09/10] libxl: Fix libxl_set_memory_target return value
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
                   ` (7 preceding siblings ...)
  2016-04-01 12:40 ` [PATCH 08/10] libxl: improve main_tmem_* return codes Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 14:55   ` Roger Pau Monné
  2016-04-01 12:40 ` [PATCH 10/10] libxl: libxl_tmem functions improving coding style Paulina Szubarczyk
  9 siblings, 1 reply; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

libxl_set_memory_target seems to have the following return values:

* 1 on failure, if the failure happens because of a xenstore error *or*
* invalid
target

* -1 if the setmaxmem hypercall

* -errno if the set_pod_target hypercall target fails

* 0 on success

Make it consistently return ERROR_FAIL on failure, unless the
parameters were invalid, in which case return ERROR_INVAL.

In accordance with CODING_SYTLE:

1. Leave rc uninitialized, and set when an error is detected

2. Use 'r' for return values to functions whose return values are a
different error space (like xc_domain_setmaxmem and
xc_domain_set_pod_target), or where a failure means retry, rather than
fail the whole function (libxl__fill_dom0_memory_info), to reduce the
risk that

Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
---
 tools/libxl/libxl.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ac4d1f6..6bb2f82 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4693,7 +4693,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
         int32_t target_memkb, int relative, int enforce)
 {
     GC_INIT(ctx);
-    int rc = 1, abort_transaction = 0;
+    int rc, r, abort_transaction = 0;
     uint64_t memorykb;
     uint32_t videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
@@ -4721,15 +4721,15 @@ retry_transaction:
     if (!target && !domid) {
         if (!xs_transaction_end(ctx->xsh, t, 1))
             goto out_no_transaction;
-        rc = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
+        r = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
                                           &current_max_memkb);
-        if (rc < 0)
-            goto out_no_transaction;
+        if (r < 0) { rc = ERROR_FAIL; goto out_no_transaction; }
         goto retry_transaction;
     } else if (!target) {
         LOGE(ERROR, "cannot get target memory info from %s/memory/target",
              dompath);
         abort_transaction = 1;
+        rc = ERROR_FAIL;
         goto out;
     } else {
         current_target_memkb = strtoul(target, &endptr, 10);
@@ -4737,6 +4737,7 @@ retry_transaction:
             LOGE(ERROR, "invalid memory target %s from %s/memory/target\n",
                  target, dompath);
             abort_transaction = 1;
+            rc = ERROR_FAIL;
             goto out;
         }
     }
@@ -4745,6 +4746,7 @@ retry_transaction:
         LOGE(ERROR, "cannot get memory info from %s/memory/static-max",
              dompath);
         abort_transaction = 1;
+        rc = ERROR_FAIL;
         goto out;
     }
     memorykb = strtoul(memmax, &endptr, 10);
@@ -4752,6 +4754,7 @@ retry_transaction:
         LOGE(ERROR, "invalid max memory %s from %s/memory/static-max\n",
              memmax, dompath);
         abort_transaction = 1;
+        rc = ERROR_FAIL;
         goto out;
     }
 
@@ -4771,6 +4774,7 @@ retry_transaction:
             "memory_dynamic_max must be less than or equal to"
             " memory_static_max\n");
         abort_transaction = 1;
+        rc = ERROR_INVAL;
         goto out;
     }
 
@@ -4778,33 +4782,36 @@ retry_transaction:
         LOG(ERROR, "new target %d for dom0 is below the minimum threshold",
             new_target_memkb);
         abort_transaction = 1;
+        rc = ERROR_INVAL;
         goto out;
     }
 
     if (enforce) {
         memorykb = new_target_memkb + videoram;
-        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
+        r = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
                 LIBXL_MAXMEM_CONSTANT);
-        if (rc != 0) {
+        if (r != 0) {
             LOGE(ERROR,
                  "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed ""rc=%d\n",
                  domid,
                  memorykb + LIBXL_MAXMEM_CONSTANT,
-                 rc);
+                 r);
             abort_transaction = 1;
+            rc = ERROR_FAIL;
             goto out;
         }
     }
 
-    rc = xc_domain_set_pod_target(ctx->xch, domid,
+    r = xc_domain_set_pod_target(ctx->xch, domid,
             (new_target_memkb + LIBXL_MAXMEM_CONSTANT) / 4, NULL, NULL, NULL);
-    if (rc != 0) {
+    if (r != 0) {
         LOGE(ERROR,
              "xc_domain_set_pod_target domid=%d, memkb=%d ""failed rc=%d\n",
              domid,
              new_target_memkb / 4,
-             rc);
+             r);
         abort_transaction = 1;
+        rc = ERROR_FAIL;
         goto out;
     }
 
@@ -4818,6 +4825,7 @@ retry_transaction:
                      "%"PRIu32, new_target_memkb / 1024);
     libxl_dominfo_dispose(&ptr);
 
+    rc = 0;
 out:
     if (!xs_transaction_end(ctx->xsh, t, abort_transaction)
         && !abort_transaction)
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 10/10] libxl: libxl_tmem functions improving coding style
  2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
                   ` (8 preceding siblings ...)
  2016-04-01 12:40 ` [PATCH 09/10] libxl: Fix libxl_set_memory_target return value Paulina Szubarczyk
@ 2016-04-01 12:40 ` Paulina Szubarczyk
  2016-04-01 15:14   ` Konrad Rzeszutek Wilk
  2016-04-01 15:39   ` Roger Pau Monné
  9 siblings, 2 replies; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-01 12:40 UTC (permalink / raw)
  To: xen-devel, roger.pau, George.Dunlap
  Cc: Paulina Szubarczyk, dario.faggioli, wei.liu2, ian.campbell

In accordance with CODING_SYTLE:
 - Use 'r' for return values to functions whose return values are a
   different error space (like xc_tmem_control, xc_tmem_auth)

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/libxl.c | 58 ++++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 6bb2f82..5794ade 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6107,14 +6107,14 @@ uint32_t libxl_vm_get_start_time(libxl_ctx *ctx, uint32_t domid)
 
 char *libxl_tmem_list(libxl_ctx *ctx, uint32_t domid, int use_long)
 {
-    int rc;
+    int r;
     char _buf[32768];
     GC_INIT(ctx);
 
-    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_LIST, domid, 32768, use_long,
+    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_LIST, domid, 32768, use_long,
                          _buf);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not get tmem list");
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not get tmem list");
         GC_FREE;
         return NULL;
     }
@@ -6125,36 +6125,36 @@ char *libxl_tmem_list(libxl_ctx *ctx, uint32_t domid, int use_long)
 
 int libxl_tmem_freeze(libxl_ctx *ctx, uint32_t domid)
 {
-    int rc;
+    int r;
     GC_INIT(ctx);
 
-    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_FREEZE, domid, 0, 0,
+    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_FREEZE, domid, 0, 0,
                          NULL);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not freeze tmem pools");
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not freeze tmem pools");
         GC_FREE;
         return ERROR_FAIL;
     }
 
     GC_FREE;
-    return rc;
+    return r;
 }
 
 int libxl_tmem_thaw(libxl_ctx *ctx, uint32_t domid)
 {
-    int rc;
+    int r;
     GC_INIT(ctx);
 
-    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_THAW, domid, 0, 0,
+    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_THAW, domid, 0, 0,
                          NULL);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not thaw tmem pools");
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not thaw tmem pools");
         GC_FREE;
         return ERROR_FAIL;
     }
 
     GC_FREE;
-    return rc;
+    return r;
 }
 
 static int32_t tmem_setop_from_string(char *set_name)
@@ -6171,7 +6171,7 @@ static int32_t tmem_setop_from_string(char *set_name)
 
 int libxl_tmem_set(libxl_ctx *ctx, uint32_t domid, char* name, uint32_t set)
 {
-    int rc;
+    int r;
     int32_t subop = tmem_setop_from_string(name);
     GC_INIT(ctx);
 
@@ -6180,48 +6180,48 @@ int libxl_tmem_set(libxl_ctx *ctx, uint32_t domid, char* name, uint32_t set)
         GC_FREE;
         return ERROR_INVAL;
     }
-    rc = xc_tmem_control(ctx->xch, -1, subop, domid, set, 0, NULL);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not set tmem %s", name);
+    r = xc_tmem_control(ctx->xch, -1, subop, domid, set, 0, NULL);
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not set tmem %s", name);
         GC_FREE;
         return ERROR_FAIL;
     }
 
     GC_FREE;
-    return rc;
+    return r;
 }
 
 int libxl_tmem_shared_auth(libxl_ctx *ctx, uint32_t domid,
                            char* uuid, int auth)
 {
-    int rc;
+    int r;
     GC_INIT(ctx);
 
-    rc = xc_tmem_auth(ctx->xch, domid, uuid, auth);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not set tmem shared auth");
+    r = xc_tmem_auth(ctx->xch, domid, uuid, auth);
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not set tmem shared auth");
         GC_FREE;
         return ERROR_FAIL;
     }
 
     GC_FREE;
-    return rc;
+    return r;
 }
 
 int libxl_tmem_freeable(libxl_ctx *ctx)
 {
-    int rc;
+    int r;
     GC_INIT(ctx);
 
-    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_QUERY_FREEABLE_MB, -1, 0, 0, 0);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not get tmem freeable memory");
+    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_QUERY_FREEABLE_MB, -1, 0, 0, 0);
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not get tmem freeable memory");
         GC_FREE;
         return ERROR_FAIL;
     }
 
     GC_FREE;
-    return rc;
+    return r;
 }
 
 int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap)
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 01/10] libxl_pci: improve return codes for more xl commands
  2016-04-01 12:40 ` [PATCH 01/10] libxl_pci: improve return codes " Paulina Szubarczyk
@ 2016-04-01 14:00   ` Roger Pau Monné
  0 siblings, 0 replies; 25+ messages in thread
From: Roger Pau Monné @ 2016-04-01 14:00 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:
> Retrun error code instead of allways 0. Remove assigned-only
     ^ ur                       ^ extra "l"
> ret variable.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

You have missed to Cc one of the maintainers of the code, Ian Jackson. 
Please either check the top-level MAINTAINERS file manually, or run 
scripts/get_maintainer.pl against the patches.

In general the patch LGTM, just a couple of comments.

> ---
>  tools/libxl/libxl_pci.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index dc10cb7..3435ce2 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -84,13 +84,11 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
>      flexarray_t *front = NULL;
>      flexarray_t *back = NULL;
>      libxl__device device;
> -    int ret = ERROR_NOMEM, i;
> +    int i;
>  
>      front = flexarray_make(gc, 16, 1);
>      back = flexarray_make(gc, 16, 1);
>  
> -    ret = 0;
> -
>      LOG(DEBUG, "Creating pci backend");
>
>      /* add pci device */

There's a call to libxl__device_from_pcidev here that returns a value 
(although it's always 0 right now). Either make that function return void, 
or check the return value for errors.

> @@ -108,12 +106,10 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
>      flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", 0));
>      flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising));
>  
> -    libxl__device_generic_add(gc, XBT_NULL, &device,
> +    return libxl__device_generic_add(gc, XBT_NULL, &device,
>                                libxl__xs_kvs_of_flexarray(gc, back, back->count),
>                                libxl__xs_kvs_of_flexarray(gc, front, front->count),
>                                NULL);

You should indent those line to match the start of the parentheses.

Roger.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 02/10] libxl-pci: clean unused return variable
  2016-04-01 12:40 ` [PATCH 02/10] libxl-pci: clean unused return variable Paulina Szubarczyk
@ 2016-04-01 14:13   ` Roger Pau Monné
  0 siblings, 0 replies; 25+ messages in thread
From: Roger Pau Monné @ 2016-04-01 14:13 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:
> libxl_device_pci_assignable_list returns:
> - list of the libxl_device_pci
> - NULL in case of error
> 
> the rc variable is unused as return code.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
>  tools/libxl/libxl_pci.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 3435ce2..6051ee4 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -397,12 +397,11 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
>      libxl_device_pci *pcidevs = NULL, *new, *assigned;
>      struct dirent *de;
>      DIR *dir;
> -    int rc, num_assigned;
> +    int num_assigned;
>  
>      *num = 0;
>  
> -    rc = get_all_assigned_devices(gc, &assigned, &num_assigned);
> -    if ( rc )
> +    if ( get_all_assigned_devices(gc, &assigned, &num_assigned) )
>          goto out;

This is against libxl coding style, see the file in 
tools/libxl/CODING_STYLE:

  * Function calls which might fail (ie most function calls) are
    handled by putting the return/status value into a variable, and
    then checking it in a separate statement:
            char *dompath = libxl__xs_get_dompath(gc, bl->domid);
            if (!dompath) { rc = ERROR_FAIL; goto out; }

The only issues with this piece of code is the extra spaces inside the 
parentheses AFAICT.

There are other things that you can fix in this function, like removing 
the extra "out_closedir" label. There are cleaner ways to achieve the same 
using a single "out" label.

Roger.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 03/10] libxl_pci: Return error code for more pci-* functions
  2016-04-01 12:40 ` [PATCH 03/10] libxl_pci: Return error code for more pci-* functions Paulina Szubarczyk
@ 2016-04-01 14:20   ` Roger Pau Monné
  0 siblings, 0 replies; 25+ messages in thread
From: Roger Pau Monné @ 2016-04-01 14:20 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:
> Return rc value instead of allways 0.
                              ^ extra "l"

The subject on this patch is too vague, please use something more 
descriptive, like:

libxl: fix return value of libxl__device_pci_destroy_all

> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

LGTM, so provided that you fix the subject and commit message:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

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

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 04/10] xl: improve return code for freemem function
  2016-04-01 12:40 ` [PATCH 04/10] xl: improve return code for freemem function Paulina Szubarczyk
@ 2016-04-01 14:24   ` Roger Pau Monné
  0 siblings, 0 replies; 25+ messages in thread
From: Roger Pau Monné @ 2016-04-01 14:24 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:
> - Return 0 or 1 for freemem function
> - Correct the condition of checking return values of freemem
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7750995..7ee6953 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2678,38 +2678,34 @@ static int preserve_domain(uint32_t *r_domid, libxl_event *event,
>  
>  static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
>  {
> -    int rc, retries = 3;
> +    int retries = 3;
>      uint32_t need_memkb, free_memkb;
>  
>      if (!autoballoon)
>          return 0;
>  
> -    rc = libxl_domain_need_memory(ctx, b_info, &need_memkb);
> -    if (rc < 0)
> -        return rc;
> +    if (libxl_domain_need_memory(ctx, b_info, &need_memkb) < 0)
> +        return 1;

As noted on patch 02/10, this is against libxl coding style.

Roger.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 05/10] xl_cmdimpl: improve return codes for memset commands
  2016-04-01 12:40 ` [PATCH 05/10] xl_cmdimpl: improve return codes for memset commands Paulina Szubarczyk
@ 2016-04-01 14:33   ` Roger Pau Monné
  2016-04-04 11:38     ` George Dunlap
  0 siblings, 1 reply; 25+ messages in thread
From: Roger Pau Monné @ 2016-04-01 14:33 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:

>  - Use EXIT_{SUCCESS,FAILURE} for main_mem*() function
>  - Use 0/1 as return values of set_memory_{max,target}
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7ee6953..31f037f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3380,15 +3380,15 @@ static int set_memory_max(uint32_t domid, const char *mem)
>      memorykb = parse_mem_size_kb(mem);
>      if (memorykb == -1) {
>          fprintf(stderr, "invalid memory size: %s\n", mem);
> -        exit(3);
> +        exit(EXIT_FAILURE);

IMHO (I would like to hear others' opinion) but I think you can just 
return 1 here instead of exiting.

>      }
>  
>      if (libxl_domain_setmaxmem(ctx, domid, memorykb)) {
>          fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem);
> -        return EXIT_FAILURE;
> +        return 1;
>      }
>  
> -    return EXIT_SUCCESS;
> +    return 0;
>  }
>  
>  int main_memmax(int argc, char **argv)
> @@ -3404,7 +3404,11 @@ int main_memmax(int argc, char **argv)
>      domid = find_domain(argv[optind]);
>      mem = argv[optind + 1];
>  
> -    return set_memory_max(domid, mem);
> +    if (set_memory_max(domid, mem)) {
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
>  }
>  
>  static int set_memory_target(uint32_t domid, const char *mem)
> @@ -3414,15 +3418,15 @@ static int set_memory_target(uint32_t domid, const char *mem)
>      memorykb = parse_mem_size_kb(mem);
>      if (memorykb == -1)  {
>          fprintf(stderr, "invalid memory size: %s\n", mem);
> -        exit(3);
> +        exit(EXIT_FAILURE);

Same here.

>      }
>  
>      if (libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1)) {
>          fprintf(stderr, "cannot set domid %d dynamic max memory to : %s\n", domid, mem);
> -        return EXIT_FAILURE;
> +        return 1;
>      }
>  
> -    return EXIT_SUCCESS;
> +    return 0;
>  }
>  
>  int main_memset(int argc, char **argv)
> @@ -3438,7 +3442,11 @@ int main_memset(int argc, char **argv)
>      domid = find_domain(argv[optind]);
>      mem = argv[optind + 1];
>  
> -    return set_memory_target(domid, mem);
> +    if (set_memory_target(domid, mem)) {
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
>  }
>  
>  static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
> -- 
> 1.9.1
> 

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 06/10] xl_cmdimpl: improve return codes for cd-insert commands
  2016-04-01 12:40 ` [PATCH 06/10] xl_cmdimpl: improve return codes for cd-insert commands Paulina Szubarczyk
@ 2016-04-01 14:41   ` Roger Pau Monné
  0 siblings, 0 replies; 25+ messages in thread
From: Roger Pau Monné @ 2016-04-01 14:41 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:

>  - Use EXIT_{SUCCESS,FAILURE} for main_cd*() function
>  - Use 0/1 as return values of cd_insert function
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

LGTM, although I would prefer that the return value of cd_insert is stored 
in a local variable and then checked. I don't think this should block the 
patch, since it's an improvement overall:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Roger.

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

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 08/10] libxl: improve main_tmem_* return codes
  2016-04-01 12:40 ` [PATCH 08/10] libxl: improve main_tmem_* return codes Paulina Szubarczyk
@ 2016-04-01 14:50   ` Roger Pau Monné
  2016-04-01 15:09   ` Konrad Rzeszutek Wilk
  1 sibling, 0 replies; 25+ messages in thread
From: Roger Pau Monné @ 2016-04-01 14:50 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:

> Functions libxl_tmem_freeze(), libxl_tmem_thaw(), libxl_tmem_set() and
> libxl_tmem_shared_auth() located in libxl.c file return ERROR_FAIL/ERROR_INVAL 
> or internal error codes from libxc library.
> 
> Improve main_tmem_* return codes by returning EXIT_{SUCCESS/FAILURE}
> accordingly to return codes of those functions.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

LGTM:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Roger.

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

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 09/10] libxl: Fix libxl_set_memory_target return value
  2016-04-01 12:40 ` [PATCH 09/10] libxl: Fix libxl_set_memory_target return value Paulina Szubarczyk
@ 2016-04-01 14:55   ` Roger Pau Monné
  2016-04-04 11:40     ` George Dunlap
  0 siblings, 1 reply; 25+ messages in thread
From: Roger Pau Monné @ 2016-04-01 14:55 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:
> libxl_set_memory_target seems to have the following return values:
> 
> * 1 on failure, if the failure happens because of a xenstore error *or*
> * invalid
> target

AFAICT it seems like you haven't touched this patch at all, but the commit 
message looks messed up (at least there's an extra "\n" here).

Also, I'm not a git expert, but I think if the patch is from George, and 
you haven't touched it, it should contain an extra "From:" field in 
the message body with George's address.

Roger.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 08/10] libxl: improve main_tmem_* return codes
  2016-04-01 12:40 ` [PATCH 08/10] libxl: improve main_tmem_* return codes Paulina Szubarczyk
  2016-04-01 14:50   ` Roger Pau Monné
@ 2016-04-01 15:09   ` Konrad Rzeszutek Wilk
  1 sibling, 0 replies; 25+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-04-01 15:09 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

On Fri, Apr 01, 2016 at 02:40:11PM +0200, Paulina Szubarczyk wrote:
> Functions libxl_tmem_freeze(), libxl_tmem_thaw(), libxl_tmem_set() and
> libxl_tmem_shared_auth() located in libxl.c file return ERROR_FAIL/ERROR_INVAL 
> or internal error codes from libxc library.
> 
> Improve main_tmem_* return codes by returning EXIT_{SUCCESS/FAILURE}
> accordingly to return codes of those functions.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, albeit

The CODING_STYLE says:
Every indented statement is braced apart from blocks that contain just
one statement."

Which is not very clear. I think it is missing an ',', which would make it

Every indented statement is braced, apart from blocks that contain just one statement.

Which means that:

> ---
>  tools/libxl/xl_cmdimpl.c | 51 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 38129f4..52c3b9b 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7519,7 +7519,7 @@ int main_tmem_freeze(int argc, char **argv)
>      if (!dom && all == 0) {
>          fprintf(stderr, "You must specify -a or a domain id.\n\n");
>          help("tmem-freeze");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (all)
> @@ -7527,8 +7527,11 @@ int main_tmem_freeze(int argc, char **argv)
>      else
>          domid = find_domain(dom);
>  
> -    libxl_tmem_freeze(ctx, domid);
> -    return 0;
> +    if (libxl_tmem_freeze(ctx, domid) < 0) {
> +        return EXIT_FAILURE;
> +    }

This and the other three can drop the braces.

> +
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_tmem_thaw(int argc, char **argv)
> @@ -7548,7 +7551,7 @@ int main_tmem_thaw(int argc, char **argv)
>      if (!dom && all == 0) {
>          fprintf(stderr, "You must specify -a or a domain id.\n\n");
>          help("tmem-thaw");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (all)
> @@ -7556,8 +7559,11 @@ int main_tmem_thaw(int argc, char **argv)
>      else
>          domid = find_domain(dom);
>  
> -    libxl_tmem_thaw(ctx, domid);
> -    return 0;
> +    if (libxl_tmem_thaw(ctx, domid) < 0) {
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_tmem_set(int argc, char **argv)
> @@ -7568,6 +7574,7 @@ int main_tmem_set(int argc, char **argv)
>      int opt_w = 0, opt_c = 0, opt_p = 0;
>      int all = 0;
>      int opt;
> +    int r = 0;
>  
>      SWITCH_FOREACH_OPT(opt, "aw:c:p:", NULL, "tmem-set", 0) {
>      case 'a':
> @@ -7591,7 +7598,7 @@ int main_tmem_set(int argc, char **argv)
>      if (!dom && all == 0) {
>          fprintf(stderr, "You must specify -a or a domain id.\n\n");
>          help("tmem-set");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (all)
> @@ -7602,17 +7609,21 @@ int main_tmem_set(int argc, char **argv)
>      if (!opt_w && !opt_c && !opt_p) {
>          fprintf(stderr, "No set value specified.\n\n");
>          help("tmem-set");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (opt_w)
> -        libxl_tmem_set(ctx, domid, "weight", weight);
> +        r = libxl_tmem_set(ctx, domid, "weight", weight);
>      if (opt_c)
> -        libxl_tmem_set(ctx, domid, "cap", cap);
> +        r = libxl_tmem_set(ctx, domid, "cap", cap);
>      if (opt_p)
> -        libxl_tmem_set(ctx, domid, "compress", compress);
> +        r = libxl_tmem_set(ctx, domid, "compress", compress);
>  
> -    return 0;
> +    if (r < 0) {
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_tmem_shared_auth(int argc, char **argv)
> @@ -7642,7 +7653,7 @@ int main_tmem_shared_auth(int argc, char **argv)
>      if (!dom && all == 0) {
>          fprintf(stderr, "You must specify -a or a domain id.\n\n");
>          help("tmem-shared-auth");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (all)
> @@ -7653,18 +7664,20 @@ int main_tmem_shared_auth(int argc, char **argv)
>      if (uuid == NULL || autharg == NULL) {
>          fprintf(stderr, "No uuid or auth specified.\n\n");
>          help("tmem-shared-auth");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      auth = strtol(autharg, &endptr, 10);
>      if (*endptr != '\0') {
>          fprintf(stderr, "Invalid auth, valid auth are <0|1>.\n\n");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
> -    libxl_tmem_shared_auth(ctx, domid, uuid, auth);
> +    if (libxl_tmem_shared_auth(ctx, domid, uuid, auth) < 0) {
> +        return EXIT_FAILURE;
> +    }
>  
> -    return 0;
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_tmem_freeable(int argc, char **argv)
> @@ -7678,10 +7691,10 @@ int main_tmem_freeable(int argc, char **argv)
>  
>      mb = libxl_tmem_freeable(ctx);
>      if (mb == -1)
> -        return -1;
> +        return EXIT_FAILURE;
>  
>      printf("%d\n", mb);
> -    return 0;
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_cpupoolcreate(int argc, char **argv)
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 10/10] libxl: libxl_tmem functions improving coding style
  2016-04-01 12:40 ` [PATCH 10/10] libxl: libxl_tmem functions improving coding style Paulina Szubarczyk
@ 2016-04-01 15:14   ` Konrad Rzeszutek Wilk
  2016-04-01 15:39   ` Roger Pau Monné
  1 sibling, 0 replies; 25+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-04-01 15:14 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

On Fri, Apr 01, 2016 at 02:40:13PM +0200, Paulina Szubarczyk wrote:
> In accordance with CODING_SYTLE:
>  - Use 'r' for return values to functions whose return values are a
>    different error space (like xc_tmem_control, xc_tmem_auth)
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 10/10] libxl: libxl_tmem functions improving coding style
  2016-04-01 12:40 ` [PATCH 10/10] libxl: libxl_tmem functions improving coding style Paulina Szubarczyk
  2016-04-01 15:14   ` Konrad Rzeszutek Wilk
@ 2016-04-01 15:39   ` Roger Pau Monné
  1 sibling, 0 replies; 25+ messages in thread
From: Roger Pau Monné @ 2016-04-01 15:39 UTC (permalink / raw)
  To: Paulina Szubarczyk
  Cc: wei.liu2, ian.campbell, George.Dunlap, dario.faggioli, xen-devel,
	roger.pau

On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:

> In accordance with CODING_SYTLE:
>  - Use 'r' for return values to functions whose return values are a
>    different error space (like xc_tmem_control, xc_tmem_auth)
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
>  tools/libxl/libxl.c | 58 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 6bb2f82..5794ade 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -6107,14 +6107,14 @@ uint32_t libxl_vm_get_start_time(libxl_ctx *ctx, uint32_t domid)
>  
>  char *libxl_tmem_list(libxl_ctx *ctx, uint32_t domid, int use_long)
>  {
> -    int rc;
> +    int r;
>      char _buf[32768];
>      GC_INIT(ctx);
>  
> -    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_LIST, domid, 32768, use_long,
> +    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_LIST, domid, 32768, use_long,
>                           _buf);
> -    if (rc < 0) {
> -        LOGEV(ERROR, rc, "Can not get tmem list");
> +    if (r < 0) {
> +        LOGEV(ERROR, r, "Can not get tmem list");
>          GC_FREE;
>          return NULL;
>      }
> @@ -6125,36 +6125,36 @@ char *libxl_tmem_list(libxl_ctx *ctx, uint32_t domid, int use_long)
>  
>  int libxl_tmem_freeze(libxl_ctx *ctx, uint32_t domid)
>  {
> -    int rc;
> +    int r;
>      GC_INIT(ctx);
>  
> -    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_FREEZE, domid, 0, 0,
> +    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_FREEZE, domid, 0, 0,
>                           NULL);
> -    if (rc < 0) {
> -        LOGEV(ERROR, rc, "Can not freeze tmem pools");
> +    if (r < 0) {
> +        LOGEV(ERROR, r, "Can not freeze tmem pools");
>          GC_FREE;
>          return ERROR_FAIL;
>      }
>  
>      GC_FREE;
> -    return rc;
> +    return r;

This is conceptually wrong IMHO, you are returning an error code that's 
not in libxl space from a libxl function (although due to the "return" 
above this is always going to be 0). I would just return 0 here, or add an 
"out" label, keep rc and set it in the error case above replacing the 
return with a goto out.

Roger.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 05/10] xl_cmdimpl: improve return codes for memset commands
  2016-04-01 14:33   ` Roger Pau Monné
@ 2016-04-04 11:38     ` George Dunlap
  0 siblings, 0 replies; 25+ messages in thread
From: George Dunlap @ 2016-04-04 11:38 UTC (permalink / raw)
  To: Roger Pau Monné, Paulina Szubarczyk
  Cc: George.Dunlap, xen-devel, dario.faggioli, wei.liu2, ian.campbell

On 01/04/16 15:33, Roger Pau Monné wrote:
> On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:
> 
>>  - Use EXIT_{SUCCESS,FAILURE} for main_mem*() function
>>  - Use 0/1 as return values of set_memory_{max,target}
>>
>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>> ---
>>  tools/libxl/xl_cmdimpl.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 7ee6953..31f037f 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -3380,15 +3380,15 @@ static int set_memory_max(uint32_t domid, const char *mem)
>>      memorykb = parse_mem_size_kb(mem);
>>      if (memorykb == -1) {
>>          fprintf(stderr, "invalid memory size: %s\n", mem);
>> -        exit(3);
>> +        exit(EXIT_FAILURE);
> 
> IMHO (I would like to hear others' opinion) but I think you can just 
> return 1 here instead of exiting.

I'm generally not in favor of non-top-level functions calling exit()
unless returning an error is really impossible (as libxl does with
memory allocation failures).

IOW, I tend to agree with Roger.

 -George


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 09/10] libxl: Fix libxl_set_memory_target return value
  2016-04-01 14:55   ` Roger Pau Monné
@ 2016-04-04 11:40     ` George Dunlap
  2016-04-06 10:33       ` Paulina Szubarczyk
  0 siblings, 1 reply; 25+ messages in thread
From: George Dunlap @ 2016-04-04 11:40 UTC (permalink / raw)
  To: Roger Pau Monné, Paulina Szubarczyk
  Cc: George.Dunlap, xen-devel, dario.faggioli, wei.liu2, ian.campbell

On 01/04/16 15:55, Roger Pau Monné wrote:
> On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:
>> libxl_set_memory_target seems to have the following return values:
>>
>> * 1 on failure, if the failure happens because of a xenstore error *or*
>> * invalid
>> target
> 
> AFAICT it seems like you haven't touched this patch at all, but the commit 
> message looks messed up (at least there's an extra "\n" here).
> 
> Also, I'm not a git expert, but I think if the patch is from George, and 
> you haven't touched it, it should contain an extra "From:" field in 
> the message body with George's address.

I'm not sure the "From" thing is really worth a re-send, but since she's
re-sending it anyway, you might as well. :-)

 -George

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 09/10] libxl: Fix libxl_set_memory_target return value
  2016-04-04 11:40     ` George Dunlap
@ 2016-04-06 10:33       ` Paulina Szubarczyk
  0 siblings, 0 replies; 25+ messages in thread
From: Paulina Szubarczyk @ 2016-04-06 10:33 UTC (permalink / raw)
  To: George Dunlap
  Cc: Wei Liu, Ian Campbell, George Dunlap, Dario Faggioli,
	Ian Jackson, xen-devel, Roger Pau Monné

On 4 April 2016 at 12:40, George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 01/04/16 15:55, Roger Pau Monné wrote:
> > On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:
> >> libxl_set_memory_target seems to have the following return values:
> >>
> >> * 1 on failure, if the failure happens because of a xenstore error *or*
> >> * invalid
> >> target
> >
> > AFAICT it seems like you haven't touched this patch at all, but the commit
> > message looks messed up (at least there's an extra "\n" here).
> >
> > Also, I'm not a git expert, but I think if the patch is from George, and
> > you haven't touched it, it should contain an extra "From:" field in
> > the message body with George's address.
>
> I'm not sure the "From" thing is really worth a re-send, but since she's
> re-sending it anyway, you might as well. :-)
>
>  -George

I have found the previous thread concerning this patch
(http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg00333.html)
where Ian asked for a change from 'r' to 'lrc' as a return value of
libxl__fill_dom0_memory_info, so I will modify it before resend.

There is also proposition of improvement of the modified function but maybe
it will be better to make the changes in different path.

Paulina

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2016-04-06 10:33 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-01 12:40 [PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands Paulina Szubarczyk
2016-04-01 12:40 ` [PATCH 01/10] libxl_pci: improve return codes " Paulina Szubarczyk
2016-04-01 14:00   ` Roger Pau Monné
2016-04-01 12:40 ` [PATCH 02/10] libxl-pci: clean unused return variable Paulina Szubarczyk
2016-04-01 14:13   ` Roger Pau Monné
2016-04-01 12:40 ` [PATCH 03/10] libxl_pci: Return error code for more pci-* functions Paulina Szubarczyk
2016-04-01 14:20   ` Roger Pau Monné
2016-04-01 12:40 ` [PATCH 04/10] xl: improve return code for freemem function Paulina Szubarczyk
2016-04-01 14:24   ` Roger Pau Monné
2016-04-01 12:40 ` [PATCH 05/10] xl_cmdimpl: improve return codes for memset commands Paulina Szubarczyk
2016-04-01 14:33   ` Roger Pau Monné
2016-04-04 11:38     ` George Dunlap
2016-04-01 12:40 ` [PATCH 06/10] xl_cmdimpl: improve return codes for cd-insert commands Paulina Szubarczyk
2016-04-01 14:41   ` Roger Pau Monné
2016-04-01 12:40 ` [PATCH 07/10] Add return codes for pci-detach, pci-attach, pci-asssignable-add, and pci-assignable-remove Paulina Szubarczyk
2016-04-01 12:40 ` [PATCH 08/10] libxl: improve main_tmem_* return codes Paulina Szubarczyk
2016-04-01 14:50   ` Roger Pau Monné
2016-04-01 15:09   ` Konrad Rzeszutek Wilk
2016-04-01 12:40 ` [PATCH 09/10] libxl: Fix libxl_set_memory_target return value Paulina Szubarczyk
2016-04-01 14:55   ` Roger Pau Monné
2016-04-04 11:40     ` George Dunlap
2016-04-06 10:33       ` Paulina Szubarczyk
2016-04-01 12:40 ` [PATCH 10/10] libxl: libxl_tmem functions improving coding style Paulina Szubarczyk
2016-04-01 15:14   ` Konrad Rzeszutek Wilk
2016-04-01 15:39   ` Roger Pau Monné

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).