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

[PATCH v2 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]. 

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.

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

Changed since v1:
 * Make  libxl__device_from_pcidev() return void instead 0.
 * Modify the libxl_device_pci_assignable_list() function to use
   only one 'out' cleaning path.
 * Changed exit() calls to 'return 1;' for set_memory_* and pci_* functions
   in xl_cmdimpl 
 * Added the error cleanup path 'out' in libxl_tmem_* functions and replaced 
   return with set-and-goto statements.
 * Use 'lrc' for return values to local functions libxl__*
 

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

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

[Paulina Szubarczyk]

Return error code instead of always 0. Remove assigned-only ret variable.

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

---
Changed since v1:
 * The function libxl__device_from_pcidev() initialize the values
   of libxl__device and does not return any error code.
   Make it return void instead 0.
---
 tools/libxl/libxl_pci.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index dc10cb7..9c9cd04 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -64,7 +64,7 @@ static void libxl_create_pci_backend_device(libxl__gc *gc, flexarray_t *back, in
     flexarray_append_pair(back, GCSPRINTF("state-%d", num), GCSPRINTF("%d", XenbusStateInitialising));
 }
 
-static int libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
+static void libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
                                      libxl_device_pci *pcidev,
                                      libxl__device *device)
 {
@@ -74,8 +74,6 @@ static int libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
     device->devid = 0;
     device->domid = domid;
     device->kind = LIBXL__DEVICE_KIND_PCI;
-
-    return 0;
 }
 
 int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
@@ -84,13 +82,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 +104,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,
-                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                              libxl__xs_kvs_of_flexarray(gc, front, front->count),
-                              NULL);
-
-    return 0;
+    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);
 }
 
 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 v2 02/10] libxl-pci: removing the extra 'out_closedir' cleaning path

[Paulina Szubarczyk]

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

---
Changed since v1:
 * Modify the libxl_device_pci_assignable_list() function to use
   only one 'out' cleaning path.
---
 tools/libxl/libxl_pci.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 9c9cd04..8549378 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -395,34 +395,35 @@ 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 r, num_assigned;
 
     *num = 0;
 
-    rc = get_all_assigned_devices(gc, &assigned, &num_assigned);
-    if ( rc )
+    r = get_all_assigned_devices(gc, &assigned, &num_assigned);
+    if (r)
         goto out;
 
     dir = opendir(SYSFS_PCIBACK_DRIVER);
-    if ( NULL == dir ) {
-        if ( errno == ENOENT ) {
+    if (NULL == dir) {
+        if (errno == ENOENT) {
             LOG(ERROR, "Looks like pciback driver not loaded");
-        }else{
+        } else {
             LOGE(ERROR, "Couldn't open %s", SYSFS_PCIBACK_DRIVER);
         }
-        goto out_closedir;
+        closedir(dir);
+        goto out;
     }
 
-    while( (de = readdir(dir)) ) {
+    while((de = readdir(dir))) {
         unsigned dom, bus, dev, func;
-        if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+        if (sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4)
             continue;
 
-        if ( is_pcidev_in_array(assigned, num_assigned, dom, bus, dev, func) )
+        if (is_pcidev_in_array(assigned, num_assigned, dom, bus, dev, func))
             continue;
 
         new = realloc(pcidevs, ((*num) + 1) * sizeof(*new));
-        if ( NULL == new )
+        if (NULL == new)
             continue;
 
         pcidevs = new;
@@ -433,8 +434,6 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
         (*num)++;
     }
 
-out_closedir:
-    closedir(dir);
 out:
     GC_FREE;
     return pcidevs;
-- 
1.9.1


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

[PATCH v2 03/10] libxl: fix return value of libxl__device_pci_destroy_all

[Paulina Szubarczyk]

Return rc value instead of always 0.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.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 8549378..b667dba 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1602,7 +1602,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 v2 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 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 80908ee..fe27f35 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2690,30 +2690,30 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
 
     rc = libxl_domain_need_memory(ctx, b_info, &need_memkb);
     if (rc < 0)
-        return rc;
+        return 1;
 
     do {
         rc = libxl_get_free_memory(ctx, &free_memkb);
         if (rc < 0)
-            return rc;
+            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;
+            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;
+            return 1;
 
         retries--;
     } while (retries > 0);
 
-    return ERROR_NOMEM;
+    return 1;
 }
 
 static void autoconnect_console(libxl_ctx *ctx_ignored,
@@ -2981,7 +2981,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 v2 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>

---
Changed since v1
* Changed exit() calls to 'return 1;'
---
 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 fe27f35..ad88b4b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3391,15 +3391,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);
+        return 1;
     }
 
     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)
@@ -3415,7 +3415,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)
@@ -3425,15 +3429,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);
+        return 1;
     }
 
     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)
@@ -3449,7 +3453,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 v2 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>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ad88b4b..fd23f23 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3482,16 +3482,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);
@@ -3513,7 +3513,10 @@ 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)
@@ -3531,7 +3534,10 @@ 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 v2 07/10] xl_cmdimpl - Add return codes for pci-detach, pci-attach, pci-asssignable-add, and pci-assignable-remove.

[Paulina Szubarczyk]

From: George Dunlap <George.Dunlap@eu.citrix.com>

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>

---
Changed since v1:
* Changed exit() calls to 'return 1;'

---
 tools/libxl/xl_cmdimpl.c | 80 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 23 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index fd23f23..0bc7ad9 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3817,10 +3817,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);
 
@@ -3829,15 +3830,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);
+        return 1;
+    }
+    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)
@@ -3856,13 +3862,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);
 
@@ -3871,12 +3882,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);
+        return 1;
     }
-    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)
@@ -3895,8 +3910,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)
@@ -3928,10 +3946,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);
 
@@ -3940,12 +3959,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);
+        return 1;
     }
-    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)
@@ -3959,14 +3982,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);
 
@@ -3975,12 +4002,16 @@ static void pciassignable_remove(const char *bdf, int rebind)
 
     if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
         fprintf(stderr, "pci-assignable-remove: malformed BDF specification \"%s\"\n", bdf);
-        exit(2);
+        return 1;
     }
-    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)
@@ -3997,8 +4028,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 v2 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>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 51 ++++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0bc7ad9..4b22548 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7529,11 +7529,11 @@ int main_tmem_list(int argc, char **argv)
 
     buf = libxl_tmem_list(ctx, domid, use_long);
     if (buf == NULL)
-        return -1;
+        return EXIT_FAILURE;
 
     printf("%s\n", buf);
     free(buf);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_freeze(int argc, char **argv)
@@ -7553,7 +7553,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)
@@ -7561,8 +7561,10 @@ 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)
@@ -7582,7 +7584,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)
@@ -7590,8 +7592,10 @@ 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)
@@ -7602,6 +7606,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 rc = 0;
 
     SWITCH_FOREACH_OPT(opt, "aw:c:p:", NULL, "tmem-set", 0) {
     case 'a':
@@ -7625,7 +7630,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)
@@ -7636,17 +7641,20 @@ 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);
+        rc = libxl_tmem_set(ctx, domid, "weight", weight);
     if (opt_c)
-        libxl_tmem_set(ctx, domid, "cap", cap);
+        rc = libxl_tmem_set(ctx, domid, "cap", cap);
     if (opt_p)
-        libxl_tmem_set(ctx, domid, "compress", compress);
+        rc = libxl_tmem_set(ctx, domid, "compress", compress);
 
-    return 0;
+    if (rc < 0)
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_shared_auth(int argc, char **argv)
@@ -7676,7 +7684,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)
@@ -7687,18 +7695,19 @@ 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)
@@ -7712,10 +7721,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 v2 09/10] libxl: Fix libxl_set_memory_target return value

[Paulina Szubarczyk]

From: George Dunlap <George.Dunlap@eu.citrix.com>

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)

---
Changed since v1:

3. Use 'lrc' for return values to local functions libxl__*
where a failure means retry, rather than fail the whole function
(libxl__fill_dom0_memory_info), to reduce the risk of that.

Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.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 75f00be..057366e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4824,7 +4824,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, lrc, abort_transaction = 0;
     uint64_t memorykb;
     uint32_t videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
@@ -4852,15 +4852,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,
+        lrc = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
                                           &current_max_memkb);
-        if (rc < 0)
-            goto out_no_transaction;
+        if (lrc < 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);
@@ -4868,6 +4868,7 @@ retry_transaction:
             LOGE(ERROR, "invalid memory target %s from %s/memory/target\n",
                  target, dompath);
             abort_transaction = 1;
+            rc = ERROR_FAIL;
             goto out;
         }
     }
@@ -4876,6 +4877,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);
@@ -4883,6 +4885,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;
     }
 
@@ -4902,6 +4905,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;
     }
 
@@ -4909,33 +4913,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;
     }
 
@@ -4949,6 +4956,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 v2 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>
---
Changed since v1:
* Added the error cleanup path 'out'.
* Replaced return with set-and-goto statements.
---
 tools/libxl/libxl.c | 82 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 36 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 057366e..bf0c3c3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6238,14 +6238,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;
     }
@@ -6256,34 +6256,38 @@ 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, rc;
     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");
-        GC_FREE;
-        return ERROR_FAIL;
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not freeze tmem pools");
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
 
 int libxl_tmem_thaw(libxl_ctx *ctx, uint32_t domid)
 {
-    int rc;
+    int r, rc;
     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");
-        GC_FREE;
-        return ERROR_FAIL;
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not thaw tmem pools");
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
@@ -6302,22 +6306,24 @@ 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, rc;
     int32_t subop = tmem_setop_from_string(name);
     GC_INIT(ctx);
 
     if (subop == -1) {
         LOGEV(ERROR, -1, "Invalid set, valid sets are <weight|cap|compress>");
-        GC_FREE;
-        return ERROR_INVAL;
+        rc = ERROR_INVAL;
+        goto out;
     }
-    rc = xc_tmem_control(ctx->xch, -1, subop, domid, set, 0, NULL);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not set tmem %s", name);
-        GC_FREE;
-        return ERROR_FAIL;
+    r = xc_tmem_control(ctx->xch, -1, subop, domid, set, 0, NULL);
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not set tmem %s", name);
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
@@ -6325,32 +6331,36 @@ int libxl_tmem_set(libxl_ctx *ctx, uint32_t domid, char* name, uint32_t set)
 int libxl_tmem_shared_auth(libxl_ctx *ctx, uint32_t domid,
                            char* uuid, int auth)
 {
-    int rc;
+    int r, rc;
     GC_INIT(ctx);
 
-    rc = xc_tmem_auth(ctx->xch, domid, uuid, auth);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not set tmem shared auth");
-        GC_FREE;
-        return ERROR_FAIL;
+    r = xc_tmem_auth(ctx->xch, domid, uuid, auth);
+    if (r < 0) {
+        LOGEV(ERROR, r, "Can not set tmem shared auth");
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
 
 int libxl_tmem_freeable(libxl_ctx *ctx)
 {
-    int rc;
+    int r, rc;
     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");
-        GC_FREE;
-        return ERROR_FAIL;
+    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");
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
-- 
1.9.1


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

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

[George Dunlap]

On Wed, Apr 6, 2016 at 12:46 PM, Paulina Szubarczyk
<paulinaszubarczyk@gmail.com> wrote:
> From: George Dunlap <George.Dunlap@eu.citrix.com>
>
> 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)
>
> ---
> Changed since v1:
>
> 3. Use 'lrc' for return values to local functions libxl__*
> where a failure means retry, rather than fail the whole function
> (libxl__fill_dom0_memory_info), to reduce the risk of that.
>
> Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

FYI everything after the "---" will be discarded on commit; so you
need all the tags (signed-off-by, acked-by, &c) to be before that.

 -George

> ---
>  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 75f00be..057366e 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4824,7 +4824,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, lrc, abort_transaction = 0;
>      uint64_t memorykb;
>      uint32_t videoram = 0;
>      uint32_t current_target_memkb = 0, new_target_memkb = 0;
> @@ -4852,15 +4852,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,
> +        lrc = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
>                                            &current_max_memkb);
> -        if (rc < 0)
> -            goto out_no_transaction;
> +        if (lrc < 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);
> @@ -4868,6 +4868,7 @@ retry_transaction:
>              LOGE(ERROR, "invalid memory target %s from %s/memory/target\n",
>                   target, dompath);
>              abort_transaction = 1;
> +            rc = ERROR_FAIL;
>              goto out;
>          }
>      }
> @@ -4876,6 +4877,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);
> @@ -4883,6 +4885,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;
>      }
>
> @@ -4902,6 +4905,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;
>      }
>
> @@ -4909,33 +4913,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;
>      }
>
> @@ -4949,6 +4956,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

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

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

[Paulina Szubarczyk]

On Wed, 2016-04-06 at 13:54 +0100, George Dunlap wrote:
> On Wed, Apr 6, 2016 at 12:46 PM, Paulina Szubarczyk
> <paulinaszubarczyk@gmail.com> wrote:
> > From: George Dunlap <George.Dunlap@eu.citrix.com>
> >
> > 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)
> >
> > ---
> > Changed since v1:
> >
> > 3. Use 'lrc' for return values to local functions libxl__*
> > where a failure means retry, rather than fail the whole function
> > (libxl__fill_dom0_memory_info), to reduce the risk of that.
> >
> > Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
> > Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> 
> FYI everything after the "---" will be discarded on commit; so you
> need all the tags (signed-off-by, acked-by, &c) to be before that.
> 
>  -George
> 
I did not know that. Thank you. I was suggested by the wiki page,
where the example resend comments starts after the "---" but 
the tags you mentioned are earlier. 

But that confused me a bit, so after that "---" there should be comments
for reviewers rather then information about changes.

Should I re-send it now or may I do it with the changes after review?

Paulina
> > ---
> >  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 75f00be..057366e 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -4824,7 +4824,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, lrc, abort_transaction = 0;
> >      uint64_t memorykb;
> >      uint32_t videoram = 0;
> >      uint32_t current_target_memkb = 0, new_target_memkb = 0;
> > @@ -4852,15 +4852,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,
> > +        lrc = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
> >                                            &current_max_memkb);
> > -        if (rc < 0)
> > -            goto out_no_transaction;
> > +        if (lrc < 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);
> > @@ -4868,6 +4868,7 @@ retry_transaction:
> >              LOGE(ERROR, "invalid memory target %s from %s/memory/target\n",
> >                   target, dompath);
> >              abort_transaction = 1;
> > +            rc = ERROR_FAIL;
> >              goto out;
> >          }
> >      }
> > @@ -4876,6 +4877,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);
> > @@ -4883,6 +4885,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;
> >      }
> >
> > @@ -4902,6 +4905,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;
> >      }
> >
> > @@ -4909,33 +4913,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;
> >      }
> >
> > @@ -4949,6 +4956,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



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

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

[George Dunlap]

On Wed, Apr 6, 2016 at 2:21 PM, Paulina Szubarczyk
<paulinaszubarczyk@gmail.com> wrote:
> On Wed, 2016-04-06 at 13:54 +0100, George Dunlap wrote:
>> On Wed, Apr 6, 2016 at 12:46 PM, Paulina Szubarczyk
>> <paulinaszubarczyk@gmail.com> wrote:
>> > From: George Dunlap <George.Dunlap@eu.citrix.com>
>> >
>> > 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)
>> >
>> > ---
>> > Changed since v1:
>> >
>> > 3. Use 'lrc' for return values to local functions libxl__*
>> > where a failure means retry, rather than fail the whole function
>> > (libxl__fill_dom0_memory_info), to reduce the risk of that.
>> >
>> > Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
>> > Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>>
>> FYI everything after the "---" will be discarded on commit; so you
>> need all the tags (signed-off-by, acked-by, &c) to be before that.
>>
>>  -George
>>
> I did not know that. Thank you. I was suggested by the wiki page,
> where the example resend comments starts after the "---" but
> the tags you mentioned are earlier.
>
> But that confused me a bit, so after that "---" there should be comments
> for reviewers rather then information about changes.

Yes; anything after the --- will automatically be erased by git when
it's checked in.  So you use that to include information that is
useful for reviewers but you don't want to be checked in -- for
example, the difference between versions.

>
> Should I re-send it now or may I do it with the changes after review?

Many committers don't mind making that sort of change when they check
things in; and if there are comments on the series, you may have to
re-send it anyway.  So it's a good idea to just wait until you either
1) know you have to re-send it for other reasons, or 2) are asked by a
committer to re-send it (or not).

 -George

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

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

[Dario Faggioli]

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

On Wed, 2016-04-06 at 13:45 +0200, Paulina Szubarczyk wrote:
> Return error code instead of always 0. Remove assigned-only ret
> variable.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> 
> ---
>
I think a better subject would be:

 "libxl: improve return codes for some pci related functions"

or something like that.

And in the changelog, I'd say something like:

"libxl__device_from_pcidev() can be void, 
 while libxl__create_pci_backend() should propagate the success/error, 
 rather than always returning 0."

or something like that. :-)

> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index dc10cb7..9c9cd04 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
>
The code looks ok to me.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

Re: [PATCH v2 02/10] libxl-pci: removing the extra 'out_closedir' cleaning path

[Dario Faggioli]

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

On Wed, 2016-04-06 at 13:45 +0200, Paulina Szubarczyk wrote:
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> 
> ---
> Changed since v1:
>  * Modify the libxl_device_pci_assignable_list() function to use
>    only one 'out' cleaning path.
>
So, the patch does quite a few style cleanups. This is indeed a good
thing, but both subject and changelog need to better reflect it.

I'd go for the following.

Subject: "libxl: style cleanups in libxl_device_pci_assignable_list()

Changelog:
"various coding style compliance cleanups, such as, arranging for 
 using only one path out of the function, whitespaces in loops ad if-s 
 and r instead of rc for storing non-libxl error codes."

Again, the code looks ok to me. Just one suggestion:

> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -395,34 +395,35 @@ 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 r, num_assigned;
>  
>      *num = 0;
>  
> -    rc = get_all_assigned_devices(gc, &assigned, &num_assigned);
> -    if ( rc )
> +    r = get_all_assigned_devices(gc, &assigned, &num_assigned);
> +    if (r)
>          goto out;
>  
libxl CODING_STYLE allows 'if (r) goto out;', which, since you're
refactoring, you may want to use here.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

Re: [PATCH v2 03/10] libxl: fix return value of libxl__device_pci_destroy_all

[Dario Faggioli]

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

On Wed, 2016-04-06 at 13:45 +0200, Paulina Szubarczyk wrote:
> Return rc value instead of always 0.
> 
I'd say "Propagate the error, insetad of always returning 0", but yeah,
this is nitpicking! :-P

Also, not my call, but I'd include this change inside of patch 1: both
are simple enough that they can be made one, IMO (mentioning this new
thing being done in the changelog of the resulting patch, of course).

> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -1602,7 +1602,7 @@ int libxl__device_pci_destroy_all(libxl__gc
> *gc, uint32_t domid)
>      }
>  
While you're here, libxl conding style says that the 'int rc' variable
used to host libxl error codes, and if necessary, to return them,
should not be initialized, while this function does it.

Again not my call, but I think you can get rid of the initialization
too (and just assign 0 to it before the loop).

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

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

[Dario Faggioli]

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

On Wed, 2016-04-06 at 13:45 +0200, Paulina Szubarczyk wrote:
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3391,15 +3391,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);
> +        return 1;
>
Mmmm.. I see no reason why this can't remain exit(). In fact, it should
be turned int exit(EXIT_FAILURE), and there's Harmandeep's series --
just resubmitted by me tonight-- outstanding that does that [1].

In any case, this patch is probably not necessary any longer, not
because Harmandeep pending series, but because George take care of what
I think you're trying to do in here in
commit 0614c454209ac67016e2296577abfee9e9dcb012 already.

Regards,
Dario

[1] http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg01099.html
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

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

[Dario Faggioli]

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

On Wed, 2016-04-06 at 13:45 +0200, Paulina Szubarczyk wrote:
>  - Use EXIT_{SUCCESS,FAILURE} for main_cd*() function
>  - Use 0/1 as return values of cd_insert function
> 
Done already by commit 04119085f5a2a135e5161535b8821e1aa0d7db8a.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

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

[Dario Faggioli]

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

On Wed, 2016-04-06 at 13:46 +0200, Paulina Szubarczyk wrote:
> From: George Dunlap <George.Dunlap@eu.citrix.com>
> 
> 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.
> 
AFAIUI, that rule applies to 'int rc' variables, meant at hosting libxl
error codes and not equally strictly (although it might be a nice to
have) to 'int r' and alike variables meant at hosting return code from
interna functions, syscalls, libxc calls, etc.

Unless I'm missing or misreading something from libxl's CODING_STYLE.
:-)

> Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> 
> ---
> Changed since v1:
> * Changed exit() calls to 'return 1;'
> 
Why do we need to do that? changing exit(<random_number>) in either
exit(EXIT_SUCCESS) or (in most of the cases) exit(EXIT_FAILURE) is much
better, and is the patter we're (slowly :-/) trying to force into xl.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

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

[Dario Faggioli]

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

On Wed, 2016-04-06 at 13:46 +0200, Paulina Szubarczyk wrote:
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -6238,14 +6238,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,
>
This is ok, but...
>                           _buf);
>
...this now have the wrong indentation: it should be aligned with the
opening '(' in the line above (so, basically, you have to kill one
white space and move it back by one column).

> -    if (rc < 0) {
> -        LOGEV(ERROR, rc, "Can not get tmem list");
> +    if (r < 0) {
> +        LOGEV(ERROR, r, "Can not get tmem list");
>
libxc functions are supposed to, on failure, set errno and alwas return
-1. Using LOGEV and passing r to it means that you're always logging -1
as error code.

I think 'LOG(ERROR, "blabla")' is what should be used here.

And these same comments apply to all the other hunks of the patch.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

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

[Dario Faggioli]

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

On Wed, 2016-04-06 at 13:46 +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.
> 
This is a patch to xl, so the subject should be:
 "xl: improve main_tmem_* return codes"

> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>
With the above done:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

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

[Paulina Szubarczyk]

On Fri, 2016-04-08 at 10:26 +0200, Dario Faggioli wrote:
> On Wed, 2016-04-06 at 13:45 +0200, Paulina Szubarczyk wrote:
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -3391,15 +3391,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);
> > +        return 1;
> >
> Mmmm.. I see no reason why this can't remain exit(). In fact, it should
> be turned int exit(EXIT_FAILURE), and there's Harmandeep's series --
> just resubmitted by me tonight-- outstanding that does that [1].

There was a discussion [2] that the exit() could be replaced by return 1
in the v1 of this patch, that is way I did here and in the following
commits. Should it be rather reverted? 

> 
> In any case, this patch is probably not necessary any longer, not
> because Harmandeep pending series, but because George take care of what
> I think you're trying to do in here in
> commit 0614c454209ac67016e2296577abfee9e9dcb012 already.

In that George's commit the set_memory_* functions return
EXIT_{FAILURE/SUCCESS}. Following the patches from Harmandeep's series I
was going to changed them to return 1/0 and return
EXIT_{FAILURE/SUCCESS} in main_* functions, after asking about that in
that thread [3], though, you said there are exceptions too that in [1].

> 
> Regards,
> Dario
> 
> [1] http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg01099.html
[2]
http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg00384.html
[3]
http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg03422.html

Paulina



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

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

[Dario Faggioli]

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

On Fri, 2016-04-08 at 11:04 +0200, Paulina Szubarczyk wrote:
> On Fri, 2016-04-08 at 10:26 +0200, Dario Faggioli wrote:
> > Mmmm.. I see no reason why this can't remain exit(). In fact, it
> > should
> > be turned int exit(EXIT_FAILURE), and there's Harmandeep's series
> > --
> > just resubmitted by me tonight-- outstanding that does that [1].
> There was a discussion [2] that the exit() could be replaced by
> return 1
> in the v1 of this patch, that is way I did here and in the following
> commits. 
>
Yeah, and in fact, sorry for not participating in that discussion... I
wanted to, but was otherwise engaged at the time.

> Should it be rather reverted? 
> 
Well, this is something Wei and Ian have the final say on.

I'm actually fine with either approaches but, considering that:
 1. calling exit() in situations similar to this is, AFAICT, what xl
    does pretty much always;
 2. converting exit()-s to 'return 1' may (but that's on a case by
    case basis) require a bigger patch;

Right now, given the status this is in xl, I would just focus on making
sure that the program terminates with either EXIT_SUCCESS or
EXIT_FAILURE; if that comes from an exit() being called in a non-top
level functions, so be it... And, in fact, in this case, the quickest
and cleanest way to make that happen, is doing what George's patch
does, which is neither exit() nor 'return 1'. :-/

> > In any case, this patch is probably not necessary any longer, not
> > because Harmandeep pending series, but because George take care of
> > what
> > I think you're trying to do in here in
> > commit 0614c454209ac67016e2296577abfee9e9dcb012 already.
> In that George's commit the set_memory_* functions return
> EXIT_{FAILURE/SUCCESS}. Following the patches from Harmandeep's
> series I
> was going to changed them to return 1/0 and return
> EXIT_{FAILURE/SUCCESS} in main_* functions, after asking about that
> in
> that thread [3], though, you said there are exceptions too that in
> [1].
> 
Yep, it's again, at least up to a cartain extent, a matter of taste and
something that is hard to carve in stone, leaving no room for
exceptions.

For example, in principle, what you say you're planning to do is
correct: internal functions should return 0/1, top level function (and
main_foo() are top level functions) should either exit() or return
EXIT_SUCCESS/EXIT_FAILURE.

But.

Although set_memory_max() certainly is an internal function, it is
_only_ used from main_memmax() like this:

  return set_memory_max(domid, mem);

and since main_memmax() is a top level function, if it were my call,
I'd be more than ok with bending the above stated rule and allow
set_memory_max() to return EXIT_SUCCESS/EXIT_FAILURE.

IOW, since it's return code is always directly returned by a top level
function, I think we can allow set_memory_max() (and functions used in
similar ways) to return just like top level functions do.

I know, it's tricky, because it's not just a matter of "blindly"
applying a set of rules, you have to consider all the implication --and 
there are many of them-- on a case by case basis. It's what (among
other things), IMO, makes Open Source challenging, but also beautiful
and rewarding. :-D

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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