[PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands

[PATCH 00/10] xl: improve coding style and return more failure on failure for more xl commands

[Paulina Szubarczyk]

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

[PATCH 01/10] libxl_pci: improve return codes for more xl commands

[Paulina Szubarczyk]

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

[PATCH 02/10] libxl-pci: clean unused return variable

[Paulina Szubarczyk]

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

[PATCH 03/10] libxl_pci: Return error code for more pci-* functions

[Paulina Szubarczyk]

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

[PATCH 04/10] xl: improve return code for freemem function

[Paulina Szubarczyk]

- 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

[PATCH 05/10] xl_cmdimpl: improve return codes for memset commands

[Paulina Szubarczyk]

 - 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

[PATCH 06/10] xl_cmdimpl: improve return codes for cd-insert commands

[Paulina Szubarczyk]

 - 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

[PATCH 07/10] Add return codes for pci-detach, pci-attach, pci-asssignable-add, and pci-assignable-remove.

[Paulina Szubarczyk]

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

[PATCH 08/10] libxl: improve main_tmem_* return codes

[Paulina Szubarczyk]

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

[PATCH 09/10] libxl: Fix libxl_set_memory_target return value

[Paulina Szubarczyk]

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

[PATCH 10/10] libxl: libxl_tmem functions improving coding style

[Paulina Szubarczyk]

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

Re: [PATCH 01/10] libxl_pci: improve return codes for more xl commands

[Roger Pau Monné]

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

Re: [PATCH 02/10] libxl-pci: clean unused return variable

[Roger Pau Monné]

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

Re: [PATCH 03/10] libxl_pci: Return error code for more pci-* functions

[Roger Pau Monné]

[-- 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

Re: [PATCH 04/10] xl: improve return code for freemem function

[Roger Pau Monné]

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

Re: [PATCH 05/10] xl_cmdimpl: improve return codes for memset commands

[Roger Pau Monné]

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

Re: [PATCH 06/10] xl_cmdimpl: improve return codes for cd-insert commands

[Roger Pau Monné]

[-- 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

Re: [PATCH 08/10] libxl: improve main_tmem_* return codes

[Roger Pau Monné]

[-- 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

Re: [PATCH 09/10] libxl: Fix libxl_set_memory_target return value

[Roger Pau Monné]

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

Re: [PATCH 08/10] libxl: improve main_tmem_* return codes

[Konrad Rzeszutek Wilk]

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

Re: [PATCH 10/10] libxl: libxl_tmem functions improving coding style

[Konrad Rzeszutek Wilk]

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

Re: [PATCH 10/10] libxl: libxl_tmem functions improving coding style

[Roger Pau Monné]

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

Re: [PATCH 05/10] xl_cmdimpl: improve return codes for memset commands

[George Dunlap]

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

Re: [PATCH 09/10] libxl: Fix libxl_set_memory_target return value

[George Dunlap]

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

Re: [PATCH 09/10] libxl: Fix libxl_set_memory_target return value

[Paulina Szubarczyk]

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