[PATCH v2 0/3] libxl: xenstore related cleanup

[PATCH v2 0/3] libxl: xenstore related cleanup

[Paul Durrant]

This series is v2 since patches #1 and #3 were part of my previous
RFC posting.

Patch #1 is a cosmetic change to re-name a function so that the name of
a function introduced in a subsequent patch makes more sense

Patch #2 passes back errors from libx__xs_printf() and
libxl__xs_writev_perms() if the underlying xenstore operations fail

Patch #3 stops mis-use of XS_MKDIR

[PATCH v2 1/3] libxl: re-name libxl__xs_write() to libxl__xs_printf()...

[Paul Durrant]

...to denote what it actually does.

The name libxl__xs_write() suggests something taking a buffer and length,
akin to write(2), whereas the semantics of the function are actually more
akin to printf(3).

This patch is a textual substitution of libxl__xs_write with
libxl__xs_printf with some associated formatting fixes.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c            | 49 ++++++++++++++++++++++--------------------
 tools/libxl/libxl_bootloader.c |  4 ++--
 tools/libxl/libxl_create.c     |  4 ++--
 tools/libxl/libxl_dm.c         | 40 +++++++++++++++++-----------------
 tools/libxl/libxl_dom.c        | 23 ++++++++++----------
 tools/libxl/libxl_exec.c       |  2 +-
 tools/libxl/libxl_genid.c      |  6 +++---
 tools/libxl/libxl_internal.h   |  4 ++--
 tools/libxl/libxl_pci.c        | 22 +++++++++----------
 tools/libxl/libxl_qmp.c        |  4 ++--
 tools/libxl/libxl_xshelp.c     |  4 ++--
 11 files changed, 83 insertions(+), 79 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 854e957..0cb4a15 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1136,7 +1136,7 @@ int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
     if (!shutdown_path)
         return ERROR_FAIL;
 
-    return libxl__xs_write(gc, t, shutdown_path, "%s", cmd);
+    return libxl__xs_printf(gc, t, shutdown_path, "%s", cmd);
 }
 
 static int libxl__domain_pvcontrol(libxl__gc *gc, uint32_t domid,
@@ -1364,7 +1364,7 @@ static void disk_eject_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *w,
     if (!value || strcmp(value,  "eject"))
         return;
 
-    if (libxl__xs_write(gc, XBT_NULL, wpath, "")) {
+    if (libxl__xs_printf(gc, XBT_NULL, wpath, "")) {
         LIBXL__EVENT_DISASTER(egc, "xs_write failed acknowledging eject",
                               errno, LIBXL_EVENT_TYPE_DISK_EJECT);
         return;
@@ -4700,13 +4700,13 @@ retry_transaction:
         goto out;
 
     if (target == NULL) {
-        libxl__xs_write(gc, t, target_path, "%"PRIu32,
-                (uint32_t) info.current_memkb);
+        libxl__xs_printf(gc, t, target_path, "%"PRIu32,
+                         (uint32_t) info.current_memkb);
         *target_memkb = (uint32_t) info.current_memkb;
     }
     if (staticmax == NULL) {
-        libxl__xs_write(gc, t, max_path, "%"PRIu32,
-                        (uint32_t) info.max_memkb);
+        libxl__xs_printf(gc, t, max_path, "%"PRIu32,
+                         (uint32_t) info.max_memkb);
         *max_memkb = (uint32_t) info.max_memkb;
     }
 
@@ -4845,8 +4845,9 @@ retry_transaction:
         goto out;
     }
 
-    libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
-                dompath), "%"PRIu32, new_target_memkb);
+    libxl__xs_printf(gc, t,
+                     libxl__sprintf(gc, "%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;
@@ -4856,8 +4857,8 @@ retry_transaction:
     libxl_dominfo_init(&ptr);
     xcinfo2xlinfo(ctx, &info, &ptr);
     uuid = libxl__uuid2string(gc, ptr.uuid);
-    libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
-            "%"PRIu32, new_target_memkb / 1024);
+    libxl__xs_printf(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
+                     "%"PRIu32, new_target_memkb / 1024);
     libxl_dominfo_dispose(&ptr);
 
 out:
@@ -5492,9 +5493,9 @@ static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
 retry_transaction:
     t = xs_transaction_start(CTX->xsh);
     for (i = 0; i <= info->vcpu_max_id; i++)
-        libxl__xs_write(gc, t,
-                       libxl__sprintf(gc, "%s/cpu/%u/availability", dompath, i),
-                       "%s", libxl_bitmap_test(cpumap, i) ? "online" : "offline");
+        libxl__xs_printf(gc, t,
+                         libxl__sprintf(gc, "%s/cpu/%u/availability", dompath, i),
+                         "%s", libxl_bitmap_test(cpumap, i) ? "online" : "offline");
     if (!xs_transaction_end(CTX->xsh, t, 0)) {
         if (errno == EAGAIN)
             goto retry_transaction;
@@ -5990,7 +5991,9 @@ int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq)
     GC_INIT(ctx);
     char *dompath = libxl__xs_get_dompath(gc, domid);
 
-    libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "%s/control/sysrq", dompath), "%c", sysrq);
+    libxl__xs_printf(gc, XBT_NULL,
+                     libxl__sprintf(gc, "%s/control/sysrq", dompath),
+                     "%c", sysrq);
 
     GC_FREE;
     return 0;
@@ -6268,12 +6271,12 @@ int libxl_cpupool_create(libxl_ctx *ctx, const char *name,
         t = xs_transaction_start(ctx->xsh);
 
         xs_mkdir(ctx->xsh, t, libxl__sprintf(gc, "/local/pool/%d", *poolid));
-        libxl__xs_write(gc, t,
-                        libxl__sprintf(gc, "/local/pool/%d/uuid", *poolid),
-                        "%s", uuid_string);
-        libxl__xs_write(gc, t,
-                        libxl__sprintf(gc, "/local/pool/%d/name", *poolid),
-                        "%s", name);
+        libxl__xs_printf(gc, t,
+                         libxl__sprintf(gc, "/local/pool/%d/uuid", *poolid),
+                         "%s", uuid_string);
+        libxl__xs_printf(gc, t,
+                         libxl__sprintf(gc, "/local/pool/%d/name", *poolid),
+                         "%s", name);
 
         if (xs_transaction_end(ctx->xsh, t, 0) || (errno != EAGAIN)) {
             GC_FREE;
@@ -6364,9 +6367,9 @@ int libxl_cpupool_rename(libxl_ctx *ctx, const char *name, uint32_t poolid)
     for (;;) {
         t = xs_transaction_start(ctx->xsh);
 
-        libxl__xs_write(gc, t,
-                        libxl__sprintf(gc, "/local/pool/%d/name", poolid),
-                        "%s", name);
+        libxl__xs_printf(gc, t,
+                         libxl__sprintf(gc, "/local/pool/%d/name", poolid),
+                         "%s", name);
 
         if (xs_transaction_end(ctx->xsh, t, 0))
             break;
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 95dde98..ba519e5 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -485,8 +485,8 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
 
     dom_console_xs_path = GCSPRINTF("%s/console/tty", dompath);
 
-    rc = libxl__xs_write(gc, XBT_NULL, dom_console_xs_path, "%s",
-                         dom_console_slave_tty_path);
+    rc = libxl__xs_printf(gc, XBT_NULL, dom_console_xs_path, "%s",
+                          dom_console_slave_tty_path);
     if (rc) {
         LOGE(ERROR,"xs write console path %s := %s failed",
              dom_console_xs_path, dom_console_slave_tty_path);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f0fee00..921d155 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -678,8 +678,8 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
 
     path = libxl__xs_libxl_path(gc, domid);
     path = libxl__sprintf(gc, "%s/dm-version", path);
-    return libxl__xs_write(gc, XBT_NULL, path, "%s",
-        libxl_device_model_version_to_string(b_info->device_model_version));
+    return libxl__xs_printf(gc, XBT_NULL, path, "%s",
+                            libxl_device_model_version_to_string(b_info->device_model_version));
 }
 
 /*----- remus asynchronous checkpoint callback -----*/
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 175accd..a917c2c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1540,14 +1540,14 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     }
 
     libxl__write_stub_dmargs(gc, dm_domid, guest_domid, args);
-    libxl__xs_write(gc, XBT_NULL,
-                   libxl__sprintf(gc, "%s/image/device-model-domid",
-                                  libxl__xs_get_dompath(gc, guest_domid)),
-                   "%d", dm_domid);
-    libxl__xs_write(gc, XBT_NULL,
-                   libxl__sprintf(gc, "%s/target",
-                                  libxl__xs_get_dompath(gc, dm_domid)),
-                   "%d", guest_domid);
+    libxl__xs_printf(gc, XBT_NULL,
+                     libxl__sprintf(gc, "%s/image/device-model-domid",
+                                    libxl__xs_get_dompath(gc, guest_domid)),
+                     "%d", dm_domid);
+    libxl__xs_printf(gc, XBT_NULL,
+                     libxl__sprintf(gc, "%s/target",
+                                    libxl__xs_get_dompath(gc, dm_domid)),
+                     "%d", guest_domid);
     ret = xc_domain_set_target(ctx->xch, dm_domid, guest_domid);
     if (ret<0) {
         LOGE(ERROR, "setting target domain %d -> %d", dm_domid, guest_domid);
@@ -1821,19 +1821,19 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         path = xs_get_domain_path(ctx->xsh, domid);
-        libxl__xs_write(gc, XBT_NULL,
-                        libxl__sprintf(gc, "%s/hvmloader/bios", path),
-                        "%s", libxl_bios_type_to_string(b_info->u.hvm.bios));
+        libxl__xs_printf(gc, XBT_NULL,
+                         libxl__sprintf(gc, "%s/hvmloader/bios", path),
+                         "%s", libxl_bios_type_to_string(b_info->u.hvm.bios));
         /* Disable relocating memory to make the MMIO hole larger
          * unless we're running qemu-traditional and vNUMA is not
          * configured. */
-        libxl__xs_write(gc, XBT_NULL,
-                        libxl__sprintf(gc,
-                                       "%s/hvmloader/allow-memory-relocate",
-                                       path),
-                        "%d",
-                        b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL &&
-                        !libxl__vnuma_configured(b_info));
+        libxl__xs_printf(gc, XBT_NULL,
+                         libxl__sprintf(gc,
+                                        "%s/hvmloader/allow-memory-relocate",
+                                        path),
+                         "%d",
+                         b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL &&
+                         !libxl__vnuma_configured(b_info));
         free(path);
     }
 
@@ -1843,8 +1843,8 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
         b_info->device_model_version
         == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL)
-        libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "%s/disable_pf", path),
-                    "%d", !libxl_defbool_val(b_info->u.hvm.xen_platform_pci));
+        libxl__xs_printf(gc, XBT_NULL, libxl__sprintf(gc, "%s/disable_pf", path),
+                         "%d", !libxl_defbool_val(b_info->u.hvm.xen_platform_pci));
 
     logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qemu-dm-%s",
                                                          c_info->name));
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 44d481b..517e838 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -825,15 +825,15 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
     if (dom->smbios_module.guest_addr_out) {
         path = GCSPRINTF("/local/domain/%d/"HVM_XS_SMBIOS_PT_ADDRESS, domid);
 
-        ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
-                              dom->smbios_module.guest_addr_out);
+        ret = libxl__xs_printf(gc, XBT_NULL, path, "0x%"PRIx64,
+                               dom->smbios_module.guest_addr_out);
         if (ret)
             goto err;
 
         path = GCSPRINTF("/local/domain/%d/"HVM_XS_SMBIOS_PT_LENGTH, domid);
 
-        ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
-                              dom->smbios_module.length);
+        ret = libxl__xs_printf(gc, XBT_NULL, path, "0x%x",
+                               dom->smbios_module.length);
         if (ret)
             goto err;
     }
@@ -841,15 +841,15 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
     if (dom->acpi_module.guest_addr_out) {
         path = GCSPRINTF("/local/domain/%d/"HVM_XS_ACPI_PT_ADDRESS, domid);
 
-        ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
-                              dom->acpi_module.guest_addr_out);
+        ret = libxl__xs_printf(gc, XBT_NULL, path, "0x%"PRIx64,
+                               dom->acpi_module.guest_addr_out);
         if (ret)
             goto err;
 
         path = GCSPRINTF("/local/domain/%d/"HVM_XS_ACPI_PT_LENGTH, domid);
 
-        ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
-                              dom->acpi_module.length);
+        ret = libxl__xs_printf(gc, XBT_NULL, path, "0x%x",
+                               dom->acpi_module.length);
         if (ret)
             goto err;
     }
@@ -1074,7 +1074,7 @@ int libxl__qemu_traditional_cmd(libxl__gc *gc, uint32_t domid,
     char *path = NULL;
     uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
     path = libxl__device_model_xs_path(gc, dm_domid, domid, "/command");
-    return libxl__xs_write(gc, XBT_NULL, path, "%s", cmd);
+    return libxl__xs_printf(gc, XBT_NULL, path, "%s", cmd);
 }
 
 /*
@@ -1137,8 +1137,9 @@ int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
             goto out;
         }
 
-        libxl__xs_write(gc, XBT_NULL,
-                        GCSPRINTF("%s/%s", xs_root, key), "%s", val);
+        libxl__xs_printf(gc, XBT_NULL,
+                         GCSPRINTF("%s/%s", xs_root, key),
+                         "%s", val);
     }
 
     rc = 0;
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index df4aead..02e6c91 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -144,7 +144,7 @@ int libxl__spawn_record_pid(libxl__gc *gc, libxl__spawn_state *spawn, pid_t pid)
     rc = libxl__ev_child_xenstore_reopen(gc, spawn->what);
     if (rc) goto out;
 
-    r = libxl__xs_write(gc, XBT_NULL, spawn->pidpath, "%d", pid);
+    r = libxl__xs_printf(gc, XBT_NULL, spawn->pidpath, "%d", pid);
     if (r) {
         LOGE(ERROR,
              "write %s = %d: xenstore write failed", spawn->pidpath, pid);
diff --git a/tools/libxl/libxl_genid.c b/tools/libxl/libxl_genid.c
index 4e2f013..f1c4eb7 100644
--- a/tools/libxl/libxl_genid.c
+++ b/tools/libxl/libxl_genid.c
@@ -77,9 +77,9 @@ int libxl__ms_vm_genid_set(libxl__gc *gc, uint32_t domid,
         rc = ERROR_FAIL;
         goto out;
     }
-    rc = libxl__xs_write(gc, XBT_NULL,
-                         GCSPRINTF("%s/platform/generation-id", dom_path),
-                         "%"PRIu64 ":%" PRIu64, genid[0], genid[1]);
+    rc = libxl__xs_printf(gc, XBT_NULL,
+			  GCSPRINTF("%s/platform/generation-id", dom_path),
+			  "%"PRIu64 ":%" PRIu64, genid[0], genid[1]);
     if (rc < 0)
         goto out;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 590870a..e51d57f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -667,8 +667,8 @@ _hidden int libxl__xs_writev_perms(libxl__gc *gc, xs_transaction_t t,
 _hidden int libxl__xs_writev_atonce(libxl__gc *gc,
                              const char *dir, char **kvs);
 
-_hidden int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
-               const char *path, const char *fmt, ...) PRINTF_ATTRIBUTE(4, 5);
+_hidden int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
+                             const char *path, const char *fmt, ...) PRINTF_ATTRIBUTE(4, 5);
    /* Each fn returns 0 on success.
     * On error: returns -1, sets errno (no logging) */
 
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index fad2eb6..de3976b 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -254,7 +254,7 @@ retry_transaction2:
     xs_rm(ctx->xsh, t, libxl__sprintf(gc, "%s/vdev-%d", be_path, i));
     xs_rm(ctx->xsh, t, libxl__sprintf(gc, "%s/opts-%d", be_path, i));
     xs_rm(ctx->xsh, t, libxl__sprintf(gc, "%s/vdevfn-%d", be_path, i));
-    libxl__xs_write(gc, t, num_devs_path, "%d", num - 1);
+    libxl__xs_printf(gc, t, num_devs_path, "%d", num - 1);
     for (j = i + 1; j < num; j++) {
         tmppath = libxl__sprintf(gc, "%s/state-%d", be_path, j);
         tmp = libxl__xs_read(gc, t, tmppath);
@@ -733,7 +733,7 @@ static void pci_assignable_driver_path_write(libxl__gc *gc,
                           pcidev->bus,
                           pcidev->dev,
                           pcidev->func);
-    if ( libxl__xs_write(gc, XBT_NULL, path, "%s", driver_path) < 0 ) {
+    if ( libxl__xs_printf(gc, XBT_NULL, path, "%s", driver_path) < 0 ) {
         LOGE(WARN, "Write of %s to node %s failed.", driver_path, path);
     }
 }
@@ -971,14 +971,14 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     state = libxl__xs_read(gc, XBT_NULL, path);
     path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
     if (pcidev->vdevfn) {
-        libxl__xs_write(gc, XBT_NULL, path, PCI_BDF_VDEVFN","PCI_OPTIONS,
-                        pcidev->domain, pcidev->bus, pcidev->dev,
-                        pcidev->func, pcidev->vdevfn, pcidev->msitranslate,
-                        pcidev->power_mgmt);
+        libxl__xs_printf(gc, XBT_NULL, path, PCI_BDF_VDEVFN","PCI_OPTIONS,
+                         pcidev->domain, pcidev->bus, pcidev->dev,
+                         pcidev->func, pcidev->vdevfn, pcidev->msitranslate,
+                         pcidev->power_mgmt);
     } else {
-        libxl__xs_write(gc, XBT_NULL, path, PCI_BDF","PCI_OPTIONS,
-                        pcidev->domain,  pcidev->bus, pcidev->dev,
-                        pcidev->func, pcidev->msitranslate, pcidev->power_mgmt);
+        libxl__xs_printf(gc, XBT_NULL, path, PCI_BDF","PCI_OPTIONS,
+                         pcidev->domain,  pcidev->bus, pcidev->dev,
+                         pcidev->func, pcidev->msitranslate, pcidev->power_mgmt);
     }
 
     libxl__qemu_traditional_cmd(gc, domid, "pci-ins");
@@ -1310,8 +1310,8 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
     path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
     path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
-    libxl__xs_write(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
-                    pcidev->bus, pcidev->dev, pcidev->func);
+    libxl__xs_printf(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
+                     pcidev->bus, pcidev->dev, pcidev->func);
 
     /* Remove all functions at once atomically by only signalling
      * device-model for function 0 */
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index f798de7..714038b 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -103,7 +103,7 @@ static int store_serial_port_info(libxl__qmp_handler *qmp,
     path = libxl__xs_get_dompath(gc, qmp->domid);
     path = GCSPRINTF("%s/serial/%d/tty", path, port);
 
-    ret = libxl__xs_write(gc, XBT_NULL, path, "%s", chardev + 4);
+    ret = libxl__xs_printf(gc, XBT_NULL, path, "%s", chardev + 4);
 
     GC_FREE;
     return ret;
@@ -162,7 +162,7 @@ static int qmp_write_domain_console_item(libxl__gc *gc, int domid,
     path = libxl__xs_get_dompath(gc, domid);
     path = GCSPRINTF("%s/console/%s", path, item);
 
-    return libxl__xs_write(gc, XBT_NULL, path, "%s", value);
+    return libxl__xs_printf(gc, XBT_NULL, path, "%s", value);
 }
 
 static int qmp_register_vnc_callback(libxl__qmp_handler *qmp,
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index b0db062..3cac4f2 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -96,8 +96,8 @@ out:
 
 }
 
-int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
-                    const char *path, const char *fmt, ...)
+int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
+                     const char *path, const char *fmt, ...)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *s;
-- 
2.1.4

[PATCH v2 2/3] libxl: check for underlying xenstore operation failure...

[Paul Durrant]

...in libxl__xs_writev_perms() and libxl__xs_printf()

ERROR_FAIL is returned when any underlying operation fails. This is a
semantic change in the case of a vasprintf() failure in libxl__xs_printf(),
but appears to be better than returning a hardcoded -1.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 tools/libxl/libxl_xshelp.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 3cac4f2..a178039 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -57,9 +57,11 @@ int libxl__xs_writev_perms(libxl__gc *gc, xs_transaction_t t,
         path = libxl__sprintf(gc, "%s/%s", dir, kvs[i]);
         if (path && kvs[i + 1]) {
             int length = strlen(kvs[i + 1]);
-            xs_write(ctx->xsh, t, path, kvs[i + 1], length);
-            if (perms)
-                xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
+            if (!xs_write(ctx->xsh, t, path, kvs[i + 1], length))
+                return ERROR_FAIL;
+            if (perms &&
+                !xs_set_permissions(ctx->xsh, t, path, perms, num_perms))
+                return ERROR_FAIL;
         }
     }
     return 0;
@@ -108,11 +110,10 @@ int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
     va_end(ap);
 
     if (ret == -1) {
-        return -1;
+        return ERROR_FAIL;
     }
-    xs_write(ctx->xsh, t, path, s, ret);
-    free(s);
-    return 0;
+    libxl__ptr_add(gc, s);
+    return xs_write(ctx->xsh, t, path, s, ret) ? 0 : ERROR_FAIL;
 }
 
 char * libxl__xs_read(libxl__gc *gc, xs_transaction_t t, const char *path)
-- 
2.1.4

[PATCH v2 3/3] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

As documented in docs/misc/xenstore, the XS_MKDIR operation merely makes
sure a path exists whereas ~/control/shutdown needs to start empty. Also
using XS_MKDIR for a node which is never supposed to have children is
somewhat counterintuitive.

This patch introduces a new libxl__xs_printf_perms() function analogous
to libxl__xs_printf() but taking explicit permissions arguments, and then
uses this to create an empty ~/control/shutdown node.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_create.c   | 16 +++++++++-----
 tools/libxl/libxl_internal.h | 10 +++++++++
 tools/libxl/libxl_xshelp.c   | 52 +++++++++++++++++++++++++++++++++++++-------
 3 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 921d155..279deda 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -484,7 +484,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int flags, ret, rc, nb_vm;
     char *uuid_string;
-    char *dom_path, *vm_path, *libxl_path;
+    char *dom_path, *vm_path, *libxl_path, *control_path;
     struct xs_permissions roperm[2];
     struct xs_permissions rwperm[1];
     struct xs_permissions noperm[1];
@@ -605,17 +605,21 @@ retry_transaction:
     libxl__xs_mkdir(gc, t,
                     libxl__sprintf(gc, "%s/device", dom_path),
                     roperm, ARRAY_SIZE(roperm));
-    libxl__xs_mkdir(gc, t,
-                    libxl__sprintf(gc, "%s/control", dom_path),
+
+    control_path = libxl__sprintf(gc, "%s/control", dom_path);
+
+    libxl__xs_mkdir(gc, t, control_path,
                     roperm, ARRAY_SIZE(roperm));
     if (info->type == LIBXL_DOMAIN_TYPE_HVM)
         libxl__xs_mkdir(gc, t,
                         libxl__sprintf(gc, "%s/hvmloader", dom_path),
                         roperm, ARRAY_SIZE(roperm));
 
-    libxl__xs_mkdir(gc, t,
-                    libxl__sprintf(gc, "%s/control/shutdown", dom_path),
-                    rwperm, ARRAY_SIZE(rwperm));
+    libxl__xs_printf_perms(gc, t,
+                           libxl__sprintf(gc, "%s/shutdown", control_path),
+                           rwperm, ARRAY_SIZE(rwperm),
+                           "");
+
     libxl__xs_mkdir(gc, t,
                     libxl__sprintf(gc, "%s/device/suspend/event-channel", dom_path),
                     rwperm, ARRAY_SIZE(rwperm));
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e51d57f..94f05cd 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -667,6 +667,16 @@ _hidden int libxl__xs_writev_perms(libxl__gc *gc, xs_transaction_t t,
 _hidden int libxl__xs_writev_atonce(libxl__gc *gc,
                              const char *dir, char **kvs);
 
+_hidden int libxl__xs_vprintf_perms(libxl__gc *gc, xs_transaction_t t,
+                                    const char *path,
+                                    struct xs_permissions *perms,
+                                    unsigned int num_perms,
+                                    const char *fmt, va_list ap);
+_hidden int libxl__xs_printf_perms(libxl__gc *gc, xs_transaction_t t,
+                                   const char *path,
+                                   struct xs_permissions *perms,
+                                   unsigned int num_perms,
+                                   const char *fmt, ...) PRINTF_ATTRIBUTE(6, 7);
 _hidden int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
                              const char *path, const char *fmt, ...) PRINTF_ATTRIBUTE(4, 5);
    /* Each fn returns 0 on success.
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index a178039..6d4f33d 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -98,22 +98,58 @@ out:
 
 }
 
-int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
-                     const char *path, const char *fmt, ...)
+int libxl__xs_vprintf_perms(libxl__gc *gc, xs_transaction_t t,
+                            const char *path,
+                            struct xs_permissions *perms,
+                            unsigned int num_perms,
+                            const char *fmt,
+                            va_list ap)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *s;
+    int ret;
+
+    ret = vasprintf(&s, fmt, ap);
+    if (ret == -1)
+        return ERROR_FAIL;
+
+    libxl__ptr_add(gc, s);
+
+    if (!xs_write(ctx->xsh, t, path, s, ret))
+        return ERROR_FAIL;
+    if (perms && !xs_set_permissions(ctx->xsh, t, path, perms, num_perms))
+        return ERROR_FAIL;
+
+    return 0;
+}
+
+int libxl__xs_printf_perms(libxl__gc *gc, xs_transaction_t t,
+                           const char *path,
+                           struct xs_permissions *perms,
+                           unsigned int num_perms,
+                           const char *fmt, ...)
+{
     va_list ap;
     int ret;
+
     va_start(ap, fmt);
-    ret = vasprintf(&s, fmt, ap);
+    ret = libxl__xs_vprintf_perms(gc, t, path, perms, num_perms, fmt, ap);
     va_end(ap);
 
-    if (ret == -1) {
-        return ERROR_FAIL;
-    }
-    libxl__ptr_add(gc, s);
-    return xs_write(ctx->xsh, t, path, s, ret) ? 0 : ERROR_FAIL;
+    return ret;
+}
+
+int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
+                     const char *path, const char *fmt, ...)
+{
+    va_list ap;
+    int ret;
+
+    va_start(ap, fmt);
+    ret = libxl__xs_vprintf_perms(gc, t, path, NULL, 0, fmt, ap);
+    va_end(ap);
+
+    return ret;
 }
 
 char * libxl__xs_read(libxl__gc *gc, xs_transaction_t t, const char *path)
-- 
2.1.4

Re: [PATCH v2 1/3] libxl: re-name libxl__xs_write() to libxl__xs_printf()...

[Ian Jackson]

Paul Durrant writes ("[PATCH v2 1/3] libxl: re-name libxl__xs_write() to libxl__xs_printf()..."):
> ...to denote what it actually does.
> 
> The name libxl__xs_write() suggests something taking a buffer and length,
> akin to write(2), whereas the semantics of the function are actually more
> akin to printf(3).
> 
> This patch is a textual substitution of libxl__xs_write with
> libxl__xs_printf with some associated formatting fixes.

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

Thanks,
Ian.

Re: [PATCH v2 1/3] libxl: re-name libxl__xs_write() to libxl__xs_printf()...

[Ian Campbell]

On Mon, 2015-11-16 at 17:15 +0000, Ian Jackson wrote:
> Paul Durrant writes ("[PATCH v2 1/3] libxl: re-name libxl__xs_write() to
> libxl__xs_printf()..."):
> > ...to denote what it actually does.
> > 
> > The name libxl__xs_write() suggests something taking a buffer and
> > length,
> > akin to write(2), whereas the semantics of the function are actually
> > more
> > akin to printf(3).
> > 
> > This patch is a textual substitution of libxl__xs_write with
> > libxl__xs_printf with some associated formatting fixes.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Did you have opinions on the other two patches and/or shall I just apply
this one in the meantime?

Ian.

Re: [PATCH v2 2/3] libxl: check for underlying xenstore operation failure...

[Ian Jackson]

(Added CC to Ian C and Wei.)

Paul Durrant writes ("[Xen-devel] [PATCH v2 2/3] libxl: check for underlying xenstore operation failure..."):
> ...in libxl__xs_writev_perms() and libxl__xs_printf()

Thanks for looking at this.


> ERROR_FAIL is returned when any underlying operation fails. This is a
> semantic change in the case of a vasprintf() failure in libxl__xs_printf(),
> but appears to be better than returning a hardcoded -1.

This is what libxl__alloc_failed is for.  But, surely it would be
better to replace the call to vasprintf with one to libxl__vsprintf ?

>          path = libxl__sprintf(gc, "%s/%s", dir, kvs[i]);
>          if (path && kvs[i + 1]) {
>              int length = strlen(kvs[i + 1]);
> -            xs_write(ctx->xsh, t, path, kvs[i + 1], length);
> -            if (perms)
> -                xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
> +            if (!xs_write(ctx->xsh, t, path, kvs[i + 1], length))
> +                return ERROR_FAIL;

This is a good change semantically but I'm not keen on this error
handling style.  CODING_STYLE says:

  * Function calls which might fail (ie most function calls) are
    handled by putting the return/status value into a variable, and
    then checking it in a separate statement:
            char *dompath = libxl__xs_get_dompath(gc, bl->domid);
            if (!dompath) { rc = ERROR_FAIL; goto out; }

In this case the libxenstore return value variable should be called
`r', I think.  CODING_STYLE says:

    int r;     /* the return value from a system call (or libxc call) */

> +            if (perms &&
> +                !xs_set_permissions(ctx->xsh, t, path, perms, num_perms))
> +                return ERROR_FAIL;

And this is worse, I'm afraid.  Nested ifs are not expensive and there
is no risk of overloading the compiler.  So I would like to see this:

  if (perms) {
      r = xs_set ...
      if (r) ...
  }


I won't insist on you changing the error exits to use `goto out',
although I tend to think that would be better.


Thanks,
Ian.

Re: [PATCH v2 2/3] libxl: check for underlying xenstore operation failure...

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 24 November 2015 16:49
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Ian Campbell; Wei Liu
> Subject: Re: [Xen-devel] [PATCH v2 2/3] libxl: check for underlying xenstore
> operation failure...
> 
> (Added CC to Ian C and Wei.)
> 
> Paul Durrant writes ("[Xen-devel] [PATCH v2 2/3] libxl: check for underlying
> xenstore operation failure..."):
> > ...in libxl__xs_writev_perms() and libxl__xs_printf()
> 
> Thanks for looking at this.
> 
> 
> > ERROR_FAIL is returned when any underlying operation fails. This is a
> > semantic change in the case of a vasprintf() failure in libxl__xs_printf(),
> > but appears to be better than returning a hardcoded -1.
> 
> This is what libxl__alloc_failed is for.  But, surely it would be
> better to replace the call to vasprintf with one to libxl__vsprintf ?
> 

Ah yes, that would work nicely.

> >          path = libxl__sprintf(gc, "%s/%s", dir, kvs[i]);
> >          if (path && kvs[i + 1]) {
> >              int length = strlen(kvs[i + 1]);
> > -            xs_write(ctx->xsh, t, path, kvs[i + 1], length);
> > -            if (perms)
> > -                xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
> > +            if (!xs_write(ctx->xsh, t, path, kvs[i + 1], length))
> > +                return ERROR_FAIL;
> 
> This is a good change semantically but I'm not keen on this error
> handling style.  CODING_STYLE says:
> 
>   * Function calls which might fail (ie most function calls) are
>     handled by putting the return/status value into a variable, and
>     then checking it in a separate statement:
>             char *dompath = libxl__xs_get_dompath(gc, bl->domid);
>             if (!dompath) { rc = ERROR_FAIL; goto out; }
> 
> In this case the libxenstore return value variable should be called
> `r', I think.  CODING_STYLE says:
> 
>     int r;     /* the return value from a system call (or libxc call) */
> 
> > +            if (perms &&
> > +                !xs_set_permissions(ctx->xsh, t, path, perms, num_perms))
> > +                return ERROR_FAIL;
> 
> And this is worse, I'm afraid.  Nested ifs are not expensive and there
> is no risk of overloading the compiler.  So I would like to see this:
> 
>   if (perms) {
>       r = xs_set ...
>       if (r) ...
>   }
> 
> 
> I won't insist on you changing the error exits to use `goto out',
> although I tend to think that would be better.
> 

I'm completely happy with that style. I'll fix accordingly.

  Paul

> 
> Thanks,
> Ian.

Re: [PATCH v2 1/3] libxl: re-name libxl__xs_write() to libxl__xs_printf()...

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: 24 November 2015 16:23
> To: Ian Jackson; Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Wei Liu
> Subject: Re: [PATCH v2 1/3] libxl: re-name libxl__xs_write() to
> libxl__xs_printf()...
> 
> On Mon, 2015-11-16 at 17:15 +0000, Ian Jackson wrote:
> > Paul Durrant writes ("[PATCH v2 1/3] libxl: re-name libxl__xs_write() to
> > libxl__xs_printf()..."):
> > > ...to denote what it actually does.
> > >
> > > The name libxl__xs_write() suggests something taking a buffer and
> > > length,
> > > akin to write(2), whereas the semantics of the function are actually
> > > more
> > > akin to printf(3).
> > >
> > > This patch is a textual substitution of libxl__xs_write with
> > > libxl__xs_printf with some associated formatting fixes.
> >
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Did you have opinions on the other two patches and/or shall I just apply
> this one in the meantime?
> 

Given the discussion (on IRC) about using 'ok' for boolean returns, I'll re-work this patch.

  Paul

> Ian.