[PATCH v5 0/3] tools: Return failure when the command failed for more xl commands

[PATCH v5 0/3] tools: Return failure when the command failed for more xl commands

[George Dunlap]

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

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.

NB that these three patches are straight forward-ports of
already-acked patches.  (The other two I'll come back to another time,
or may make good OPW / GSoC fodder.)


George Dunlap (3):
  libxl: Remove pointless hypercall from libxl_set_memory_target
  xl: Make set_memory_target return an error code on failure
  xl: Return an error on failed cd-insert

 tools/libxl/libxl.c      |  5 -----
 tools/libxl/xl_cmdimpl.c | 48 +++++++++++++++++++++++++-----------------------
 2 files changed, 25 insertions(+), 28 deletions(-)

-- 
2.1.4


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

[PATCH v5 1/3] libxl: Remove pointless hypercall from libxl_set_memory_target

[George Dunlap]

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

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>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v4:
 - Split this change into a separate patch

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 3471c4c..5c473e7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4810,11 +4810,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


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

[PATCH v5 2/3] xl: Make set_memory_target return an error code on failure

[George Dunlap]

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

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>
Acked-by: Ian Jackson <ian.jackson@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 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 a3610fc..6004f14 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3380,7 +3380,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) {
@@ -3388,9 +3387,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)
@@ -3398,7 +3400,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 */
@@ -3407,18 +3408,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)  {
@@ -3426,7 +3421,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)
@@ -3442,8 +3442,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


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

[PATCH v5 3/3] xl: Return an error on failed cd-insert

[George Dunlap]

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

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>
Acked-by: Ian Jackson <ian.jackson@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 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 6004f14..fdd4877 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3451,7 +3451,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 : "");
@@ -3467,18 +3467,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)
@@ -3512,8 +3516,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_usbctrl_attach(int argc, char **argv)
-- 
2.1.4


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

Re: [PATCH v5 0/3] tools: Return failure when the command failed for more xl commands

[Konrad Rzeszutek Wilk]

On Thu, Mar 24, 2016 at 05:17:21PM +0000, George Dunlap wrote:
> Return failure when the command failed for more xl commands:
> - mem-set
> - cd-insert
> 
> 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.
> 
> NB that these three patches are straight forward-ports of
> already-acked patches.  (The other two I'll come back to another time,
> or may make good OPW / GSoC fodder.)

Applied.
> 
> 
> George Dunlap (3):
>   libxl: Remove pointless hypercall from libxl_set_memory_target
>   xl: Make set_memory_target return an error code on failure
>   xl: Return an error on failed cd-insert
> 
>  tools/libxl/libxl.c      |  5 -----
>  tools/libxl/xl_cmdimpl.c | 48 +++++++++++++++++++++++++-----------------------
>  2 files changed, 25 insertions(+), 28 deletions(-)
> 
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> 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