[PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach

[PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach

[George Dunlap]

Return proper error codes on failure so that scripts can tell whether
the command completed properly or not.

Also while we're at it, normalize init and dispose of
libxl_device_disk structures.  This means calling init and dispose in
the actual functions where they are declared.

This in turn means changing parse_disk_config_multistring() to not
call libxl_device_disk_init.  And that in turn means changes to
callers of parse_disk_config().

It also means removing libxl_device_disk_init() from
libxl.c:libxl_vdev_to_device_disk().  This is only called from
xl_cmdimpl.c.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
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>

v3:
 - Rebased to tip

v2:
 - Restructure functions to make sure init and dispose are properly
 called.
---
 tools/libxl/libxl.c      |  2 --
 tools/libxl/xl_cmdimpl.c | 29 ++++++++++++++++++++---------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index bd3aac8..814d056 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2738,8 +2738,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
     if (devid < 0)
         return ERROR_INVAL;
 
-    libxl_device_disk_init(disk);
-
     dompath = libxl__xs_get_dompath(gc, domid);
     if (!dompath) {
         goto out;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2b6371d..2ba2393 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -570,8 +570,6 @@ static void parse_disk_config_multistring(XLU_Config **config,
 {
     int e;
 
-    libxl_device_disk_init(disk);
-
     if (!*config) {
         *config = xlu_cfg_init(stderr, "command line");
         if (!*config) { perror("xlu_cfg_init"); exit(-1); }
@@ -3340,6 +3338,8 @@ static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
     xasprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
               virtdev, phys ? phys : "");
 
+    libxl_device_disk_init(&disk);
+
     parse_disk_config(&config, buf, &disk);
 
     /* ATM the existence of the backing file is not checked for qdisk
@@ -6649,6 +6649,7 @@ int main_blockattach(int argc, char **argv)
     uint32_t fe_domid;
     libxl_device_disk disk;
     XLU_Config *config = 0;
+    int rc = 0;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) {
         /* No options */
@@ -6660,6 +6661,8 @@ int main_blockattach(int argc, char **argv)
     }
     optind++;
 
+    libxl_device_disk_init(&disk);
+
     parse_disk_config_multistring
         (&config, argc-optind, (const char* const*)argv + optind, &disk);
 
@@ -6668,14 +6671,17 @@ int main_blockattach(int argc, char **argv)
         printf("disk: %s\n", json);
         free(json);
         if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
-        return 0;
+        goto out;
     }
 
     if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
         fprintf(stderr, "libxl_device_disk_add failed.\n");
-        return 1;
+        rc = 1;
     }
-    return 0;
+out:
+    libxl_device_disk_dispose(&disk);
+
+    return rc;
 }
 
 int main_blocklist(int argc, char **argv)
@@ -6726,18 +6732,23 @@ int main_blockdetach(int argc, char **argv)
         /* No options */
     }
 
+    libxl_device_disk_init(&disk);
+
     domid = find_domain(argv[optind]);
 
     if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], &disk)) {
         fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]);
-        return 1;
+        rc = 1;
+        goto out;
     }
-    rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
-    if (rc) {
+    if (libxl_device_disk_remove(ctx, domid, &disk, 0)) {
         fprintf(stderr, "libxl_device_disk_remove failed.\n");
-        return 1;
+        rc = 1;
     }
+
+out:
     libxl_device_disk_dispose(&disk);
+
     return rc;
 }
 
-- 
2.1.4

[PATCH v3 2/6] 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

* 1 on success (!)

Make it consistenstly return ERROR_FAIL, unless the parameters were
invalid.

To make this more robust, use 'lrc' 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 future code
shuffles will accidentally clobber the return value again.

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

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
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 | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 814d056..f8a0642 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4722,7 +4722,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 = ERROR_FAIL, abort_transaction = 0, lrc;
     uint64_t memorykb;
     uint32_t videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
@@ -4750,9 +4750,9 @@ 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)
+        if (lrc < 0)
             goto out_no_transaction;
         goto retry_transaction;
     } else if (!target) {
@@ -4800,6 +4800,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;
     }
 
@@ -4807,43 +4808,39 @@ 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 +
+        lrc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
                 LIBXL_MAXMEM_CONSTANT);
-        if (rc != 0) {
+        if (lrc != 0) {
             LOGE(ERROR,
                  "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed ""rc=%d\n",
                  domid,
                  memorykb + LIBXL_MAXMEM_CONSTANT,
-                 rc);
+                 lrc);
             abort_transaction = 1;
             goto out;
         }
     }
 
-    rc = xc_domain_set_pod_target(ctx->xch, domid,
+    lrc = xc_domain_set_pod_target(ctx->xch, domid,
             (new_target_memkb + LIBXL_MAXMEM_CONSTANT) / 4, NULL, NULL, NULL);
-    if (rc != 0) {
+    if (lrc != 0) {
         LOGE(ERROR,
              "xc_domain_set_pod_target domid=%d, memkb=%d ""failed rc=%d\n",
              domid,
              new_target_memkb / 4,
-             rc);
+             lrc);
         abort_transaction = 1;
         goto out;
     }
 
     libxl__xs_write(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);
@@ -4852,6 +4849,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 v3 3/6] xl: Make set_memory_target return an error code on failure

[George Dunlap]

Bring set_memory_target into line with set_memory_max (which does
return an error code.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
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 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2ba2393..4455d73 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3297,9 +3297,10 @@ int main_memmax(int argc, char **argv)
     return 0;
 }
 
-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;
+    int rc;
 
     memorykb = parse_mem_size_kb(mem);
     if (memorykb == -1)  {
@@ -3307,7 +3308,9 @@ static void set_memory_target(uint32_t domid, const char *mem)
         exit(3);
     }
 
-    libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1);
+    rc = libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1);
+
+    return rc;
 }
 
 int main_memset(int argc, char **argv)
@@ -3315,6 +3318,7 @@ int main_memset(int argc, char **argv)
     uint32_t domid;
     int opt = 0;
     const char *mem;
+    int rc;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "mem-set", 2) {
         /* No options */
@@ -3323,7 +3327,12 @@ int main_memset(int argc, char **argv)
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
 
-    set_memory_target(domid, mem);
+    rc = set_memory_target(domid, mem);
+    if (rc) {
+        fprintf(stderr, "cannot set domid %d dynamic max memory to : %s\n", domid, mem);
+        return 1;
+    }
+
     return 0;
 }
 
-- 
2.1.4

[PATCH v3 4/6] 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 wasnt.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 4455d73..72ece2e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3405,8 +3405,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 v3 5/6] 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.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
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 72ece2e..5f21c37 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3495,10 +3495,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 rc = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3509,13 +3510,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))
+            rc = 1;
+    } else {
+        if(libxl_device_pci_remove(ctx, domid, &pcidev, 0))
+            rc = 1;
+    }
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return rc;
 }
 
 int main_pcidetach(int argc, char **argv)
@@ -3534,13 +3540,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 rc;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3551,10 +3558,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))
+        rc = 1;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return rc;
 }
 
 int main_pciattach(int argc, char **argv)
@@ -3573,8 +3584,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)
@@ -3606,10 +3616,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 rc = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3620,10 +3631,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))
+        rc = 1;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return rc;
 }
 
 int main_pciassignable_add(int argc, char **argv)
@@ -3637,14 +3652,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 rc = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3655,10 +3670,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))
+        rc = 1;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return rc;
 }
 
 int main_pciassignable_remove(int argc, char **argv)
@@ -3675,8 +3694,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

[PATCH v3 6/6] xl: Return error code on save

[George Dunlap]

save_domain was already constructing an error code; it just wasn't
being used.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5f21c37..52aedcf 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4712,8 +4712,7 @@ int main_save(int argc, char **argv)
     if ( argc - optind >= 3 )
         config_filename = argv[optind + 2];
 
-    save_domain(domid, filename, checkpoint, leavepaused, config_filename);
-    return 0;
+    return save_domain(domid, filename, checkpoint, leavepaused, config_filename);
 }
 
 int main_migrate(int argc, char **argv)
-- 
2.1.4

Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach

[George Dunlap]

On Tue, Dec 1, 2015 at 11:53 AM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> Return proper error codes on failure so that scripts can tell whether
> the command completed properly or not.
>
> Also while we're at it, normalize init and dispose of
> libxl_device_disk structures.  This means calling init and dispose in
> the actual functions where they are declared.
>
> This in turn means changing parse_disk_config_multistring() to not
> call libxl_device_disk_init.  And that in turn means changes to
> callers of parse_disk_config().
>
> It also means removing libxl_device_disk_init() from
> libxl.c:libxl_vdev_to_device_disk().  This is only called from
> xl_cmdimpl.c.
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Ping?
 -George

> ---
> 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>
>
> v3:
>  - Rebased to tip
>
> v2:
>  - Restructure functions to make sure init and dispose are properly
>  called.
> ---
>  tools/libxl/libxl.c      |  2 --
>  tools/libxl/xl_cmdimpl.c | 29 ++++++++++++++++++++---------
>  2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index bd3aac8..814d056 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2738,8 +2738,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
>      if (devid < 0)
>          return ERROR_INVAL;
>
> -    libxl_device_disk_init(disk);
> -
>      dompath = libxl__xs_get_dompath(gc, domid);
>      if (!dompath) {
>          goto out;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2b6371d..2ba2393 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -570,8 +570,6 @@ static void parse_disk_config_multistring(XLU_Config **config,
>  {
>      int e;
>
> -    libxl_device_disk_init(disk);
> -
>      if (!*config) {
>          *config = xlu_cfg_init(stderr, "command line");
>          if (!*config) { perror("xlu_cfg_init"); exit(-1); }
> @@ -3340,6 +3338,8 @@ static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
>      xasprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
>                virtdev, phys ? phys : "");
>
> +    libxl_device_disk_init(&disk);
> +
>      parse_disk_config(&config, buf, &disk);
>
>      /* ATM the existence of the backing file is not checked for qdisk
> @@ -6649,6 +6649,7 @@ int main_blockattach(int argc, char **argv)
>      uint32_t fe_domid;
>      libxl_device_disk disk;
>      XLU_Config *config = 0;
> +    int rc = 0;
>
>      SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) {
>          /* No options */
> @@ -6660,6 +6661,8 @@ int main_blockattach(int argc, char **argv)
>      }
>      optind++;
>
> +    libxl_device_disk_init(&disk);
> +
>      parse_disk_config_multistring
>          (&config, argc-optind, (const char* const*)argv + optind, &disk);
>
> @@ -6668,14 +6671,17 @@ int main_blockattach(int argc, char **argv)
>          printf("disk: %s\n", json);
>          free(json);
>          if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
> -        return 0;
> +        goto out;
>      }
>
>      if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
>          fprintf(stderr, "libxl_device_disk_add failed.\n");
> -        return 1;
> +        rc = 1;
>      }
> -    return 0;
> +out:
> +    libxl_device_disk_dispose(&disk);
> +
> +    return rc;
>  }
>
>  int main_blocklist(int argc, char **argv)
> @@ -6726,18 +6732,23 @@ int main_blockdetach(int argc, char **argv)
>          /* No options */
>      }
>
> +    libxl_device_disk_init(&disk);
> +
>      domid = find_domain(argv[optind]);
>
>      if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], &disk)) {
>          fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]);
> -        return 1;
> +        rc = 1;
> +        goto out;
>      }
> -    rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
> -    if (rc) {
> +    if (libxl_device_disk_remove(ctx, domid, &disk, 0)) {
>          fprintf(stderr, "libxl_device_disk_remove failed.\n");
> -        return 1;
> +        rc = 1;
>      }
> +
> +out:
>      libxl_device_disk_dispose(&disk);
> +
>      return rc;
>  }
>
> --
> 2.1.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach

[Ian Campbell]

On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> Return proper error codes on failure so that scripts can tell whether
> the command completed properly or not.
> 
> Also while we're at it, normalize init and dispose of
> libxl_device_disk structures.  This means calling init and dispose in
> the actual functions where they are declared.
> 
> This in turn means changing parse_disk_config_multistring() to not
> call libxl_device_disk_init.  And that in turn means changes to
> callers of parse_disk_config().
> 
> It also means removing libxl_device_disk_init() from
> libxl.c:libxl_vdev_to_device_disk().  This is only called from
> xl_cmdimpl.c.

... and the ocaml bindings.

I can't remember what we decided regarding libxl "getter" functions and
initialisation of the data type (i.e. whose responsibility it is), but it
seems that changing a given calls semantics is rather dangerous API-wise.

Wei, ISTR you stumbling over this once and the formulation of A Plan(tm),
but I can't remember what it was, can you?

> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> 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>
> 
> v3:
>  - Rebased to tip
> 
> v2:
>  - Restructure functions to make sure init and dispose are properly
>  called.
> ---
>  tools/libxl/libxl.c      |  2 --
>  tools/libxl/xl_cmdimpl.c | 29 ++++++++++++++++++++---------
>  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index bd3aac8..814d056 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2738,8 +2738,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx,
> uint32_t domid,
>      if (devid < 0)
>          return ERROR_INVAL;
>  
> -    libxl_device_disk_init(disk);
> -
>      dompath = libxl__xs_get_dompath(gc, domid);
>      if (!dompath) {
>          goto out;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2b6371d..2ba2393 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -570,8 +570,6 @@ static void parse_disk_config_multistring(XLU_Config
> **config,
>  {
>      int e;
>  
> -    libxl_device_disk_init(disk);
> -
>      if (!*config) {
>          *config = xlu_cfg_init(stderr, "command line");
>          if (!*config) { perror("xlu_cfg_init"); exit(-1); }
> @@ -3340,6 +3338,8 @@ static int cd_insert(uint32_t domid, const char
> *virtdev, char *phys)
>      xasprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
>                virtdev, phys ? phys : "");
>  
> +    libxl_device_disk_init(&disk);
> +
>      parse_disk_config(&config, buf, &disk);
>  
>      /* ATM the existence of the backing file is not checked for qdisk
> @@ -6649,6 +6649,7 @@ int main_blockattach(int argc, char **argv)
>      uint32_t fe_domid;
>      libxl_device_disk disk;
>      XLU_Config *config = 0;
> +    int rc = 0;
>  
>      SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) {
>          /* No options */
> @@ -6660,6 +6661,8 @@ int main_blockattach(int argc, char **argv)
>      }
>      optind++;
>  
> +    libxl_device_disk_init(&disk);
> +
>      parse_disk_config_multistring
>          (&config, argc-optind, (const char* const*)argv + optind,
> &disk);
>  
> @@ -6668,14 +6671,17 @@ int main_blockattach(int argc, char **argv)
>          printf("disk: %s\n", json);
>          free(json);
>          if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-
> 1); }
> -        return 0;
> +        goto out;
>      }
>  
>      if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
>          fprintf(stderr, "libxl_device_disk_add failed.\n");
> -        return 1;
> +        rc = 1;
>      }
> -    return 0;
> +out:
> +    libxl_device_disk_dispose(&disk);
> +
> +    return rc;
>  }
>  
>  int main_blocklist(int argc, char **argv)
> @@ -6726,18 +6732,23 @@ int main_blockdetach(int argc, char **argv)
>          /* No options */
>      }
>  
> +    libxl_device_disk_init(&disk);
> +
>      domid = find_domain(argv[optind]);
>  
>      if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], &disk)) {
>          fprintf(stderr, "Error: Device %s not connected.\n",
> argv[optind+1]);
> -        return 1;
> +        rc = 1;
> +        goto out;
>      }
> -    rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
> -    if (rc) {
> +    if (libxl_device_disk_remove(ctx, domid, &disk, 0)) {
>          fprintf(stderr, "libxl_device_disk_remove failed.\n");
> -        return 1;
> +        rc = 1;
>      }
> +
> +out:
>      libxl_device_disk_dispose(&disk);
> +
>      return rc;
>  }
>  

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

Re: [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value

[Ian Campbell]

On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> libxl_set_memory_target seems to have the following return values:
> 
> * 1 on failure, if the failure happens because of a xenstore error *or*
> invalid target
> 
> * -1 if the setmaxmem hypercall
> 
> * -errno if the set_pod_target hypercall target fails
> 
> * 1 on success (!)
> 
> Make it consistenstly return ERROR_FAIL, unless the parameters were

"consistently"

> invalid.
> 
> To make this more robust, use 'lrc' for return values to functions

tools/libxl/CODING_STYLE recommends "r" for such variables (return values
of syscalls or libxc calls).

> 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 future code
> shuffles will accidentally clobber the return value again.
> 
> Also remove the final call to xc_domain_getinfolist. There's no
> obvious reason for this call -- all it seems to be doing is checking
> to see if the domain exists; but if it doesn't exist, it will have
> already failed by this point.

If we aren't sure what it is for then I'd rather remove it in an
independent change, just in case.

> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> 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 | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 814d056..f8a0642 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4722,7 +4722,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 = ERROR_FAIL, abort_transaction = 0, lrc;

CODING_STYLE asks that rc not be initialised on declaration but set on the
failure paths (it allows a single line if () { rc = ; goto ... } to
mitigate the verbosity of this somewhat).

Ian.

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

Re: [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value

[George Dunlap]

On 08/12/15 17:19, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
>> libxl_set_memory_target seems to have the following return values:
>>
>> * 1 on failure, if the failure happens because of a xenstore error *or*
>> invalid target
>>
>> * -1 if the setmaxmem hypercall
>>
>> * -errno if the set_pod_target hypercall target fails
>>
>> * 1 on success (!)
>>
>> Make it consistenstly return ERROR_FAIL, unless the parameters were
> 
> "consistently"
> 
>> invalid.
>>
>> To make this more robust, use 'lrc' for return values to functions
> 
> tools/libxl/CODING_STYLE recommends "r" for such variables (return values
> of syscalls or libxc calls).
> 
>> 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 future code
>> shuffles will accidentally clobber the return value again.
>>
>> Also remove the final call to xc_domain_getinfolist. There's no
>> obvious reason for this call -- all it seems to be doing is checking
>> to see if the domain exists; but if it doesn't exist, it will have
>> already failed by this point.
> 
> If we aren't sure what it is for then I'd rather remove it in an
> independent change, just in case.
> 
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> ---
>> 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 | 27 +++++++++++++--------------
>>  1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 814d056..f8a0642 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -4722,7 +4722,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 = ERROR_FAIL, abort_transaction = 0, lrc;
> 
> CODING_STYLE asks that rc not be initialised on declaration but set on the
> failure paths (it allows a single line if () { rc = ; goto ... } to
> mitigate the verbosity of this somewhat).

Ack to all of the above.

 -George

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

[Ian Campbell]

On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> Bring set_memory_target into line with set_memory_max (which does
> return an error code.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> 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 | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2ba2393..4455d73 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3297,9 +3297,10 @@ int main_memmax(int argc, char **argv)
>      return 0;
>  }
>  
> -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;

The switch from long long to int64_t here is just incidental, right?

It did cause me to notice that both libxl_set_memory_target
and libxl_domain_setmaxmem take a 32bit (inconsistently signed vs unsigned)
argument for the memkb, so apart from the loss of range vs
parse_mem_size_kb you also can't set the target as high as you can set the
maximum. Nice.

Ian.

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

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

[Ian Campbell]

On Tue, 2015-12-01 at 11:53 +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 wasnt.

Missing an apostrophe in wasnt.

> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Ian Campbell <Ian.campbell@citrix.com>

> ---
> 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 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 4455d73..72ece2e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3405,8 +3405,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)

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

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

[Ian Campbell]

On Tue, 2015-12-01 at 11:53 +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.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> 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 72ece2e..5f21c37 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3495,10 +3495,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 rc = 0;

I think we should probably avoid "rc" for non-libxl error codes even in xl.

Also, I'd forgotten that since 00e110e44a0eb we are trying to have xl
main_* functions return either EXIT_SUCCESS or EXIT_FAILURE rather than 0,
1, -foo etc. That probably applies to many of the  earlier patches which
I've already commented on. I shan't go back and correct myself.

Ian.



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

Re: [PATCH v3 6/6] xl: Return error code on save

[Ian Campbell]

On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> save_domain was already constructing an error code; it just wasn't
> being used.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>


> ---
> 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 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5f21c37..52aedcf 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4712,8 +4712,7 @@ int main_save(int argc, char **argv)
>      if ( argc - optind >= 3 )
>          config_filename = argv[optind + 2];
>  
> -    save_domain(domid, filename, checkpoint, leavepaused,
> config_filename);
> -    return 0;
> +    return save_domain(domid, filename, checkpoint, leavepaused,
> config_filename);
>  }
>  
>  int main_migrate(int argc, char **argv)

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

Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach

[George Dunlap]

On 08/12/15 17:15, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
>> Return proper error codes on failure so that scripts can tell whether
>> the command completed properly or not.
>>
>> Also while we're at it, normalize init and dispose of
>> libxl_device_disk structures.  This means calling init and dispose in
>> the actual functions where they are declared.
>>
>> This in turn means changing parse_disk_config_multistring() to not
>> call libxl_device_disk_init.  And that in turn means changes to
>> callers of parse_disk_config().
>>
>> It also means removing libxl_device_disk_init() from
>> libxl.c:libxl_vdev_to_device_disk().  This is only called from
>> xl_cmdimpl.c.
> 
> ... and the ocaml bindings.
> 
> I can't remember what we decided regarding libxl "getter" functions and
> initialisation of the data type (i.e. whose responsibility it is), but it
> seems that changing a given calls semantics is rather dangerous API-wise.

Right -- looks like there are similar issues with the nic lookup
routines as well.  I'll drop that bit from the patch.

 -George

Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach

[George Dunlap]

On 16/12/15 16:53, George Dunlap wrote:
> On 08/12/15 17:15, Ian Campbell wrote:
>> On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
>>> Return proper error codes on failure so that scripts can tell whether
>>> the command completed properly or not.
>>>
>>> Also while we're at it, normalize init and dispose of
>>> libxl_device_disk structures.  This means calling init and dispose in
>>> the actual functions where they are declared.
>>>
>>> This in turn means changing parse_disk_config_multistring() to not
>>> call libxl_device_disk_init.  And that in turn means changes to
>>> callers of parse_disk_config().
>>>
>>> It also means removing libxl_device_disk_init() from
>>> libxl.c:libxl_vdev_to_device_disk().  This is only called from
>>> xl_cmdimpl.c.
>>
>> ... and the ocaml bindings.
>>
>> I can't remember what we decided regarding libxl "getter" functions and
>> initialisation of the data type (i.e. whose responsibility it is), but it
>> seems that changing a given calls semantics is rather dangerous API-wise.
> 
> Right -- looks like there are similar issues with the nic lookup
> routines as well.  I'll drop that bit from the patch.

Hmm -- and upon further inspection, it appears that the headline feature
(returning appropriate error codes) was already checked in. What was
left was only the "while we're at it" bit. :-/

I'll drop this from the series...

 -George

Re: [PATCH v3 6/6] xl: Return error code on save

[George Dunlap]

On 01/12/15 11:53, George Dunlap wrote:
> save_domain was already constructing an error code; it just wasn't
> being used.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> 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 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5f21c37..52aedcf 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4712,8 +4712,7 @@ int main_save(int argc, char **argv)
>      if ( argc - optind >= 3 )
>          config_filename = argv[optind + 2];
>  
> -    save_domain(domid, filename, checkpoint, leavepaused, config_filename);
> -    return 0;
> +    return save_domain(domid, filename, checkpoint, leavepaused, config_filename);

Ah -- turns out the reason the return value "wasn't being used" was that
save_domain() actually calls exit() itself directly.

I'll drop this patch, as the headline feature (xl failing on the save
failing) appears to be working properly.  Maybe we can put this on a
list of clean-up items.

 -George

Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach

[Wei Liu]

On Tue, Dec 08, 2015 at 05:15:28PM +0000, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> > Return proper error codes on failure so that scripts can tell whether
> > the command completed properly or not.
> > 
> > Also while we're at it, normalize init and dispose of
> > libxl_device_disk structures.  This means calling init and dispose in
> > the actual functions where they are declared.
> > 
> > This in turn means changing parse_disk_config_multistring() to not
> > call libxl_device_disk_init.  And that in turn means changes to
> > callers of parse_disk_config().
> > 
> > It also means removing libxl_device_disk_init() from
> > libxl.c:libxl_vdev_to_device_disk().  This is only called from
> > xl_cmdimpl.c.
> 
> ... and the ocaml bindings.
> 
> I can't remember what we decided regarding libxl "getter" functions and
> initialisation of the data type (i.e. whose responsibility it is), but it
> seems that changing a given calls semantics is rather dangerous API-wise.
> 
> Wei, ISTR you stumbling over this once and the formulation of A Plan(tm),
> but I can't remember what it was, can you?
> 

I vaguely remember getting into something about libxl_device_disk_init
when I was discussing with with Jim regarding libvirt, but I can't
remember exactly what. It's probably a different issue.

Wei.