[PATCH v4 0/5] Return failure on failure for more xl commands

[PATCH v4 0/5] Return failure on failure for more xl commands

[George Dunlap]

Return failure when the command failed for more xl commands:
- mem-set
- cd-insert
- pci-*

This makes xl more useful for scripting.

In the case of mem-set, it means first cleaning up
libxl_set_memory_target() to return useful error codes.

Changes in v4:
- Remove block-attach patch
- Split out removal of spurious getinfolist to a separate patch
- Try to follow CODING_STYLE more closely:
 - 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 rather than magic constants

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

[PATCH v4 1/5] libxl: Remove pointless hypercall from libxl_set_memory_target

[George Dunlap]

There's no obvious reason for the call to xc_domain_getinfolist -- all
it seems to be doing is checking that the domain exists; but if it
doesn't exist, it will have already failed by this point.

NB that this will change the return value for libxl_set_memory_target:
now it will return 0 on success, rather than returning 1 (which was
the previous behavior).  This is more in line with expected behavior,
and also allows the caller to distingiush between success and other
failure modes (some of which also return 1).

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v4:
 - Split this change into a separate patch

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9207621..a36101d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4859,11 +4859,6 @@ retry_transaction:
 
     libxl__xs_printf(gc, t, GCSPRINTF("%s/memory/target", dompath),
                      "%"PRIu32, new_target_memkb);
-    rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
-    if (rc != 1 || info.domain != domid) {
-        abort_transaction = 1;
-        goto out;
-    }
 
     libxl_dominfo_init(&ptr);
     xcinfo2xlinfo(ctx, &info, &ptr);
-- 
2.1.4

[PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value

[George Dunlap]

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@citrix.com>
---
v4:
 - Use 'r' instead of 'lrc'
 - Set 'rc' before jumping to error path, rather than initializing at the beginning
 - Move removal of xc_domain_getinfolist to another patch

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a36101d..d05e58e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4742,7 +4742,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, abort_transaction = 0, r;
     uint64_t memorykb;
     uint32_t videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
@@ -4770,15 +4770,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);
@@ -4786,6 +4786,7 @@ retry_transaction:
             LOGE(ERROR, "invalid memory target %s from %s/memory/target\n",
                  target, dompath);
             abort_transaction = 1;
+            rc = ERROR_FAIL;
             goto out;
         }
     }
@@ -4794,6 +4795,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);
@@ -4801,6 +4803,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;
     }
 
@@ -4820,6 +4823,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;
     }
 
@@ -4827,33 +4831,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;
     }
 
@@ -4867,6 +4874,8 @@ 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)
-- 
2.1.4

[PATCH v4 3/5] xl: Make set_memory_target return an error code on failure

[George Dunlap]

Also move the rc -> shell code translation into set_memory_max() to
make the two functions consistent with each other, and with other
similar examples in xl_cmdimpl.c

Change a 'long long' to "int64_t" while we're at it.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
v4:
 - Move rc -> shell return code translation into set_memory_{max,target}
 - Use EXIT_{SUCCESS,FAILURE} rather than magic constants
 - Note incidental type change in changelog

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Konrad Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/xl_cmdimpl.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f9933cb..1a8b4a1 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3265,7 +3265,6 @@ static int def_getopt(int argc, char * const argv[],
 static int set_memory_max(uint32_t domid, const char *mem)
 {
     int64_t memorykb;
-    int rc;
 
     memorykb = parse_mem_size_kb(mem);
     if (memorykb == -1) {
@@ -3273,9 +3272,12 @@ static int set_memory_max(uint32_t domid, const char *mem)
         exit(3);
     }
 
-    rc = libxl_domain_setmaxmem(ctx, domid, memorykb);
+    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 rc;
+    return EXIT_SUCCESS;
 }
 
 int main_memmax(int argc, char **argv)
@@ -3283,7 +3285,6 @@ int main_memmax(int argc, char **argv)
     uint32_t domid;
     int opt = 0;
     char *mem;
-    int rc;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "mem-max", 2) {
         /* No options */
@@ -3292,18 +3293,12 @@ int main_memmax(int argc, char **argv)
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
 
-    rc = set_memory_max(domid, mem);
-    if (rc) {
-        fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem);
-        return 1;
-    }
-
-    return 0;
+    return set_memory_max(domid, mem);
 }
 
-static void set_memory_target(uint32_t domid, const char *mem)
+static int set_memory_target(uint32_t domid, const char *mem)
 {
-    long long int memorykb;
+    int64_t memorykb;
 
     memorykb = parse_mem_size_kb(mem);
     if (memorykb == -1)  {
@@ -3311,7 +3306,12 @@ static void set_memory_target(uint32_t domid, const char *mem)
         exit(3);
     }
 
-    libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 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 EXIT_SUCCESS;
 }
 
 int main_memset(int argc, char **argv)
@@ -3327,8 +3327,7 @@ int main_memset(int argc, char **argv)
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
 
-    set_memory_target(domid, mem);
-    return 0;
+    return set_memory_target(domid, mem);
 }
 
 static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
-- 
2.1.4

[PATCH v4 4/5] xl: Return an error on failed cd-insert

[George Dunlap]

This makes xl more useful in scripts.

The strange thing about this is that the internal cd_insert function
*already* returned something appropriate, and cd-eject was using it,
but cd-insert wasn't.

Also:

* Rework cd_insert to return EXIT_FAILURE and EXIT_SUCCESS rather than
magic constants

* Use 'r' for non-libxl return code, as specified in CODING_STYLE

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
v4:
 - Use EXIT_{SUCCESS,FAILURE} rather than  magic constants in cd_insert
 - Use 'r' rather than 'rc' for non-libxl return code
 - Fixed typo in changelog

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1a8b4a1..0c3756b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3336,7 +3336,7 @@ static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
     char *buf = NULL;
     XLU_Config *config = 0;
     struct stat b;
-    int rc = 0;
+    int r;
 
     xasprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
               virtdev, phys ? phys : "");
@@ -3352,18 +3352,22 @@ 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);
-        rc = 1;
+        r = EXIT_FAILURE;
+        goto out;
+    }
+
+    if (libxl_cdrom_insert(ctx, domid, &disk, NULL)) {
+        r = EXIT_FAILURE;
         goto out;
     }
 
-    if (libxl_cdrom_insert(ctx, domid, &disk, NULL))
-        rc=1;
+    r = EXIT_SUCCESS;
 
 out:
     libxl_device_disk_dispose(&disk);
     free(buf);
 
-    return rc;
+    return r;
 }
 
 int main_cd_eject(int argc, char **argv)
@@ -3397,8 +3401,7 @@ int main_cd_insert(int argc, char **argv)
     virtdev = argv[optind + 1];
     file = argv[optind + 2];
 
-    cd_insert(domid, virtdev, file);
-    return 0;
+    return cd_insert(domid, virtdev, file);
 }
 
 int main_console(int argc, char **argv)
-- 
2.1.4

[PATCH v4 5/5] xl: Return error codes for pci* commands

[George Dunlap]

Add return codes for pci-detach, pci-attach, pci-asssignable-add, and
pci-assignable-remove.

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 "rc = EXIT_SUCCESS" seemed
a bit pointless.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
v4:
 - Use EXIT_{SUCCESS,FAILURE} rather than magic constants.
 - Use 'r' rather than 'rc' for non-libxl error codes

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 56 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0c3756b..e0c962e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3491,10 +3491,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 = EXIT_SUCCESS;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3505,13 +3506,18 @@ static void pcidetach(uint32_t domid, const char *bdf, int force)
         fprintf(stderr, "pci-detach: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
-    if (force)
-        libxl_device_pci_destroy(ctx, domid, &pcidev, 0);
-    else
-        libxl_device_pci_remove(ctx, domid, &pcidev, 0);
+    if (force) {
+        if(libxl_device_pci_destroy(ctx, domid, &pcidev, 0))
+            r = EXIT_FAILURE;
+    } else {
+        if(libxl_device_pci_remove(ctx, domid, &pcidev, 0))
+            r = EXIT_FAILURE;
+    }
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pcidetach(int argc, char **argv)
@@ -3530,13 +3536,14 @@ int main_pcidetach(int argc, char **argv)
     domid = find_domain(argv[optind]);
     bdf = argv[optind + 1];
 
-    pcidetach(domid, bdf, force);
-    return 0;
+    return pcidetach(domid, bdf, force);
 }
-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 = EXIT_SUCCESS;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3547,10 +3554,14 @@ static void pciattach(uint32_t domid, const char *bdf, const char *vs)
         fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
-    libxl_device_pci_add(ctx, domid, &pcidev, 0);
+
+    if(libxl_device_pci_add(ctx, domid, &pcidev, 0))
+        r = EXIT_FAILURE;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciattach(int argc, char **argv)
@@ -3569,8 +3580,7 @@ int main_pciattach(int argc, char **argv)
     if (optind + 1 < argc)
         vs = argv[optind + 2];
 
-    pciattach(domid, bdf, vs);
-    return 0;
+    return pciattach(domid, bdf, vs);
 }
 
 static void pciassignable_list(void)
@@ -3602,10 +3612,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 = EXIT_SUCCESS;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3616,10 +3627,14 @@ static void pciassignable_add(const char *bdf, int rebind)
         fprintf(stderr, "pci-assignable-add: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
-    libxl_device_pci_assignable_add(ctx, &pcidev, rebind);
+
+    if (libxl_device_pci_assignable_add(ctx, &pcidev, rebind))
+        r = EXIT_FAILURE;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciassignable_add(int argc, char **argv)
@@ -3633,14 +3648,14 @@ int main_pciassignable_add(int argc, char **argv)
 
     bdf = argv[optind];
 
-    pciassignable_add(bdf, 1);
-    return 0;
+    return pciassignable_add(bdf, 1);
 }
 
-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 = EXIT_SUCCESS;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3651,10 +3666,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 = EXIT_FAILURE;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciassignable_remove(int argc, char **argv)
@@ -3671,8 +3690,7 @@ int main_pciassignable_remove(int argc, char **argv)
 
     bdf = argv[optind];
 
-    pciassignable_remove(bdf, rebind);
-    return 0;
+    return pciassignable_remove(bdf, rebind);
 }
 
 static void pause_domain(uint32_t domid)
-- 
2.1.4

Re: [PATCH v4 5/5] xl: Return error codes for pci* commands

[Dario Faggioli]

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

On Fri, 2015-12-18 at 12:23 +0000, George Dunlap wrote:
> Add return codes for pci-detach, pci-attach, pci-asssignable-add, and
> pci-assignable-remove.
> 
> 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 "rc = EXIT_SUCCESS" seemed
> a bit pointless.
>
You mean 'nothing but an "r = EXIT_SUCCESS"'... Or this changelog
violates the CODING_STYLE itself!  ;-P ;-P

> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

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


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

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

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

Re: [PATCH v4 4/5] xl: Return an error on failed cd-insert

[Dario Faggioli]

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

On Fri, 2015-12-18 at 12:23 +0000, George Dunlap wrote:
> This makes xl more useful in scripts.
> 
> The strange thing about this is that the internal cd_insert function
> *already* returned something appropriate, and cd-eject was using it,
> but cd-insert wasn't.
> 
> Also:
> 
> * Rework cd_insert to return EXIT_FAILURE and EXIT_SUCCESS rather
> than
> magic constants
> 
> * Use 'r' for non-libxl return code, as specified in CODING_STYLE
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

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


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

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

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

Re: [PATCH v4 3/5] xl: Make set_memory_target return an error code on failure

[Dario Faggioli]

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

On Fri, 2015-12-18 at 12:23 +0000, George Dunlap wrote:
> Also move the rc -> shell code translation into set_memory_max() to
> make the two functions consistent with each other, and with other
> similar examples in xl_cmdimpl.c
> 
> Change a 'long long' to "int64_t" while we're at it.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

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


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

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

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

Re: [PATCH v4 1/5] libxl: Remove pointless hypercall from libxl_set_memory_target

[Ian Jackson]

George Dunlap writes ("[PATCH v4 1/5] libxl: Remove pointless hypercall from libxl_set_memory_target"):
> There's no obvious reason for the call to xc_domain_getinfolist -- all
> it seems to be doing is checking that the domain exists; but if it
> doesn't exist, it will have already failed by this point.
> 
> NB that this will change the return value for libxl_set_memory_target:
> now it will return 0 on success, rather than returning 1 (which was
> the previous behavior).  This is more in line with expected behavior,
> and also allows the caller to distingiush between success and other
> failure modes (some of which also return 1).

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

Although I don't see the other return paths returning 1 which you
mention.

Ian.

Re: [PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value

[Ian Jackson]

George Dunlap writes ("[PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value"):
> 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

The use of `r' to contain libxl__fill_dom0_memory_info's return value
is IMO confusing and contrary to CODING_STYLE.

Sorry that I didn't spot this last sentence in your point `2' when
reading ijc's review of your v3, where ijc suggested `r'.

> v4:
>  - Use 'r' instead of 'lrc'

Can you go back to `lrc' for just that one use ?  I think that would
be analogous with CODING_STYLE's suggestion to use `rc' for libxl
error codes.


>          abort_transaction = 1;
> +        rc = ERROR_FAIL;

There is an awful lot of this.  I won't object to this in your patch,
as what you have is an improvement, but:

Every `goto out' here is preceded by `abort_transaction = 1'.

If you converted this function to:
  - use libxl__xs_transaction_start et al
  - in the intended pattern as shown in its doc comment
  - use the `out' path only for errors
  - unconditionally call libxl__xs_transaction_abort in the out
    block
  - call libxl__xs_transaction_commit in the success path

Then you could abolish `abort_transaction' and remove all assignments
to it.  You could also abolish `out_no_transaction'.

If you were feeling really gung-ho you could get rid of `goto retry'
and replace it with a loop.

Ian.

Re: [PATCH v4 3/5] xl: Make set_memory_target return an error code on failure

[Ian Jackson]

George Dunlap writes ("[PATCH v4 3/5] xl: Make set_memory_target return an error code on failure"):
> Also move the rc -> shell code translation into set_memory_max() to
> make the two functions consistent with each other, and with other
> similar examples in xl_cmdimpl.c

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

Re: [PATCH v4 4/5] xl: Return an error on failed cd-insert

[Ian Jackson]

George Dunlap writes ("[PATCH v4 4/5] xl: Return an error on failed cd-insert"):
> This makes xl more useful in scripts.

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

Re: [PATCH v4 5/5] xl: Return error codes for pci* commands

[Ian Jackson]

George Dunlap writes ("[PATCH v4 5/5] xl: Return error codes for pci* commands"):
> Add return codes for pci-detach, pci-attach, pci-asssignable-add, and
> pci-assignable-remove.
> 
> 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 "rc = EXIT_SUCCESS" seemed
> a bit pointless.

(You mean `nothing but an "r = EXIT_SUCCESS"'.)

I'm afraid I am going to say that I would prefer the CODING_STYLE
approach.  Most relevantly, while it may seem pointless in this case,
it means that: (a) when another subroutine call is added to one of
these functions, it is not necessary to rework the error handling
again; and (b) everything is the same everywhere so there is less
scope for confusion.

Ian.

Re: [PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value

[Ian Campbell]

On Mon, 2016-01-04 at 17:37 +0000, Ian Jackson wrote:
> George Dunlap writes ("[PATCH v4 2/5] libxl: Fix libxl_set_memory_target
> return value"):
> > 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
> 
> The use of `r' to contain libxl__fill_dom0_memory_info's return value
> is IMO confusing and contrary to CODING_STYLE.
> 
> Sorry that I didn't spot this last sentence in your point `2' when
> reading ijc's review of your v3, where ijc suggested `r'.

And for my part sorry for not spotting the libxl_* in with the xc_*, or I'd
have (hopefully) have thought to mention it.

> 
> > v4:
> >  - Use 'r' instead of 'lrc'
> 
> Can you go back to `lrc' for just that one use ?  I think that would
> be analogous with CODING_STYLE's suggestion to use `rc' for libxl
> error codes.

I was never quite sure what the "l" was supposed to reference, local?

> 
> 
> >          abort_transaction = 1;
> > +        rc = ERROR_FAIL;
> 
> There is an awful lot of this.  I won't object to this in your patch,
> as what you have is an improvement, but:
> 
> Every `goto out' here is preceded by `abort_transaction = 1'.
> 
> If you converted this function to:
>   - use libxl__xs_transaction_start et al
>   - in the intended pattern as shown in its doc comment
>   - use the `out' path only for errors
>   - unconditionally call libxl__xs_transaction_abort in the out
>     block
>   - call libxl__xs_transaction_commit in the success path
> 
> Then you could abolish `abort_transaction' and remove all assignments
> to it.  You could also abolish `out_no_transaction'.
> 
> If you were feeling really gung-ho you could get rid of `goto retry'
> and replace it with a loop.
> 
> Ian.

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

Re: [PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value"):
> On Mon, 2016-01-04 at 17:37 +0000, Ian Jackson wrote:
> > Can you go back to `lrc' for just that one use ?  I think that would
> > be analogous with CODING_STYLE's suggestion to use `rc' for libxl
> > error codes.
> 
> I was never quite sure what the "l" was supposed to reference, local?

It seems to mean `local' to me.  I was going to suggest `lrc' for this
variable name even before I saw that it had already been `lrc' in a
previous patch, which suggests it's a local maximum in probability
space...

Ian.

Re: [PATCH v4 1/5] libxl: Remove pointless hypercall from libxl_set_memory_target

[George Dunlap]

On 04/01/16 17:28, Ian Jackson wrote:
> George Dunlap writes ("[PATCH v4 1/5] libxl: Remove pointless hypercall from libxl_set_memory_target"):
>> There's no obvious reason for the call to xc_domain_getinfolist -- all
>> it seems to be doing is checking that the domain exists; but if it
>> doesn't exist, it will have already failed by this point.
>>
>> NB that this will change the return value for libxl_set_memory_target:
>> now it will return 0 on success, rather than returning 1 (which was
>> the previous behavior).  This is more in line with expected behavior,
>> and also allows the caller to distingiush between success and other
>> failure modes (some of which also return 1).
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Although I don't see the other return paths returning 1 which you
> mention.

rc is initialized to 1, and there are a number of exit paths (I count 7)
which jump to out without setting it; including setting the target above
static_max.

BTW I'm about to re-send the Ack'ed patches in this series (rebased to
staging).

 -George

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