[RFC PATCH 0/6] libxl: xenstore related changes

[RFC PATCH 0/6] libxl: xenstore related changes

[Paul Durrant]

This patch series is RFC because the documentation changes it is based
upon have not yet been ack-ed or committed.

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 stops mis-use of XS_MKDIR

Patch #3 adds PV control feature checking

Patch #4 adds a xenstore area for PV drivers versions

Patch #5 adds a xenstore area for frontend feature advertisement

Patch #6 adds a xenstore area for frontend network name and address
information

[RFC PATCH 1/6] 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 9c9eaa3..48ff73c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1472,14 +1472,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);
@@ -1753,19 +1753,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);
     }
 
@@ -1775,8 +1775,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 b00add0..4710804 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -666,8 +666,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

[RFC PATCH 2/6] 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   | 44 ++++++++++++++++++++++++++++++++++++++------
 3 files changed, 58 insertions(+), 12 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 4710804..04d8a29 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -666,6 +666,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 3cac4f2..0b56f8b 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -96,25 +96,57 @@ 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;
-    va_list ap;
     int ret;
-    va_start(ap, fmt);
-    ret = vasprintf(&s, fmt, ap);
-    va_end(ap);
 
+    ret = vasprintf(&s, fmt, ap);
     if (ret == -1) {
         return -1;
     }
     xs_write(ctx->xsh, t, path, s, ret);
+    if (perms)
+        xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
     free(s);
     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 = libxl__xs_vprintf_perms(gc, t, path, perms, num_perms, fmt, ap);
+    va_end(ap);
+
+    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)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-- 
2.1.4

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Campbell]

On Wed, 2015-11-11 at 16:18 +0000, Paul Durrant wrote:
> 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   | 44
> ++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 58 insertions(+), 12 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 4710804..04d8a29 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -666,6 +666,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 3cac4f2..0b56f8b 100644
> --- a/tools/libxl/libxl_xshelp.c
> +++ b/tools/libxl/libxl_xshelp.c
> @@ -96,25 +96,57 @@ 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;
> -    va_list ap;
>      int ret;
> -    va_start(ap, fmt);
> -    ret = vasprintf(&s, fmt, ap);
> -    va_end(ap);
>  
> +    ret = vasprintf(&s, fmt, ap);
>      if (ret == -1) {
>          return -1;
>      }
>      xs_write(ctx->xsh, t, path, s, ret);
> +    if (perms)
> +        xs_set_permissions(ctx->xsh, t, path, perms, num_perms);

This can fail, can't it? (OTOH so can xs_write, so maybe there is some
reason we apparently don't care for such things here?)

>      free(s);
>      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 = libxl__xs_vprintf_perms(gc, t, path, perms, num_perms, fmt,
> ap);
> +    va_end(ap);
> +
> +    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)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: 16 November 2015 12:55
> To: Paul Durrant; xen-devel@lists.xenproject.org
> Cc: Ian Jackson; Stefano Stabellini; Wei Liu
> Subject: Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> On Wed, 2015-11-11 at 16:18 +0000, Paul Durrant wrote:
> > 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   | 44
> > ++++++++++++++++++++++++++++++++++++++------
> >  3 files changed, 58 insertions(+), 12 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 4710804..04d8a29 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -666,6 +666,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 3cac4f2..0b56f8b 100644
> > --- a/tools/libxl/libxl_xshelp.c
> > +++ b/tools/libxl/libxl_xshelp.c
> > @@ -96,25 +96,57 @@ 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;
> > -    va_list ap;
> >      int ret;
> > -    va_start(ap, fmt);
> > -    ret = vasprintf(&s, fmt, ap);
> > -    va_end(ap);
> >
> > +    ret = vasprintf(&s, fmt, ap);
> >      if (ret == -1) {
> >          return -1;
> >      }
> >      xs_write(ctx->xsh, t, path, s, ret);
> > +    if (perms)
> > +        xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
> 
> This can fail, can't it? (OTOH so can xs_write, so maybe there is some
> reason we apparently don't care for such things here?)
> 

That's a good point. I would have thought wiring an xs_write() failure back through to the caller would be a good idea (and same goes for xs_set_permissions, I guess).

  Paul

> >      free(s);
> >      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 = libxl__xs_vprintf_perms(gc, t, path, perms, num_perms, fmt,
> > ap);
> > +    va_end(ap);
> > +
> > +    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)
> >  {
> >      libxl_ctx *ctx = libxl__gc_owner(gc);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Campbell]

On Mon, 2015-11-16 at 13:16 +0000, Paul Durrant wrote:
> > 
> > > +    ret = vasprintf(&s, fmt, ap);
> > >      if (ret == -1) {
> > >          return -1;
> > >      }
> > >      xs_write(ctx->xsh, t, path, s, ret);
> > > +    if (perms)
> > > +        xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
> > 
> > This can fail, can't it? (OTOH so can xs_write, so maybe there is some
> > reason we apparently don't care for such things here?)
> > 
> 
> That's a good point. I would have thought wiring an xs_write() failure
> back through to the caller would be a good idea (and same goes for
> xs_set_permissions, I guess).

git tells me it has been this way since libxl was first committed to the
repo. I can't think of a good reason to squash these errors.

Ian.


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

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: 16 November 2015 13:43
> To: Paul Durrant; xen-devel@lists.xenproject.org
> Cc: Ian Jackson; Stefano Stabellini; Wei Liu
> Subject: Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> On Mon, 2015-11-16 at 13:16 +0000, Paul Durrant wrote:
> > >
> > > > +    ret = vasprintf(&s, fmt, ap);
> > > >      if (ret == -1) {
> > > >          return -1;
> > > >      }
> > > >      xs_write(ctx->xsh, t, path, s, ret);
> > > > +    if (perms)
> > > > +        xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
> > >
> > > This can fail, can't it? (OTOH so can xs_write, so maybe there is some
> > > reason we apparently don't care for such things here?)
> > >
> >
> > That's a good point. I would have thought wiring an xs_write() failure
> > back through to the caller would be a good idea (and same goes for
> > xs_set_permissions, I guess).
> 
> git tells me it has been this way since libxl was first committed to the
> repo. I can't think of a good reason to squash these errors.
> 

I'll split out this patch and the name change one and send a small cleanup series. I'll wait for Ian's docs review before sending the rest.

  Paul

> Ian.

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

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Jackson]

Paul Durrant writes ("[RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown"):
> 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.

The way xenstore doesn't distinguish between leaves and directories is
unusual but it is part of the xenstore API.  I think users inside
libxl will have to cope with it anyway.

> 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.

This is a lot of extra boilerplate to clean up one slightly odd call.

Maybe it would be easier to rename libxl__xs_mkdir to
libxl__xs_mknode ?  (It's probably too late to rename XS_MKDIR.)

Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 16 November 2015 17:20
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Ian Campbell; Wei Liu
> Subject: Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> Paul Durrant writes ("[RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown"):
> > 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.
> 
> The way xenstore doesn't distinguish between leaves and directories is
> unusual but it is part of the xenstore API.  I think users inside
> libxl will have to cope with it anyway.
> 
> > 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.
> 
> This is a lot of extra boilerplate to clean up one slightly odd call.
> 
> Maybe it would be easier to rename libxl__xs_mkdir to
> libxl__xs_mknode ?  (It's probably too late to rename XS_MKDIR.)
> 

There is still the need to set the path to an empty value though, which is not implicitly done by the XS_MKDIR.

  Paul

> Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Jackson]

Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown"):
> [Ian Jackson:]
> > Maybe it would be easier to rename libxl__xs_mkdir to
> > libxl__xs_mknode ?  (It's probably too late to rename XS_MKDIR.)
> 
> There is still the need to set the path to an empty value though, which is not implicitly done by the XS_MKDIR.

Under what circumstances would this path not contain an empty value
after XS_MKDIR ?

Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 16 November 2015 17:31
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Ian Campbell; Wei Liu
> Subject: RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir()
> for ~/control/shutdown"):
> > [Ian Jackson:]
> > > Maybe it would be easier to rename libxl__xs_mkdir to
> > > libxl__xs_mknode ?  (It's probably too late to rename XS_MKDIR.)
> >
> > There is still the need to set the path to an empty value though, which is
> not implicitly done by the XS_MKDIR.
> 
> Under what circumstances would this path not contain an empty value
> after XS_MKDIR ?
> 

In this case I believe you are correct, but my feeling was that people reading the code would be lulled into a false sense of security that XS_MKDIR always did the right thing to initialize a new path.

  Paul

> Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Jackson]

Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown"):
> [Ian Jackson]
> > Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir()
> > for ~/control/shutdown"):
> > > [Ian Jackson:]
> > > > Maybe it would be easier to rename libxl__xs_mkdir to
> > > > libxl__xs_mknode ?  (It's probably too late to rename XS_MKDIR.)
> > >
> > > There is still the need to set the path to an empty value though, which is
> > not implicitly done by the XS_MKDIR.
> > 
> > Under what circumstances would this path not contain an empty value
> > after XS_MKDIR ?
> 
> In this case I believe you are correct, but my feeling was that
> people reading the code would be lulled into a false sense of
> security that XS_MKDIR always did the right thing to initialize a
> new path.

I'm not sure I follow this argument.  What did you think of my idea
of renaming libxl__xs_mkdir to libxl__xs_mknode ?

Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 24 November 2015 16:35
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Ian Campbell; Wei Liu
> Subject: RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir()
> for ~/control/shutdown"):
> > [Ian Jackson]
> > > Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using
> libxl__xs_mkdir()
> > > for ~/control/shutdown"):
> > > > [Ian Jackson:]
> > > > > Maybe it would be easier to rename libxl__xs_mkdir to
> > > > > libxl__xs_mknode ?  (It's probably too late to rename XS_MKDIR.)
> > > >
> > > > There is still the need to set the path to an empty value though, which
> is
> > > not implicitly done by the XS_MKDIR.
> > >
> > > Under what circumstances would this path not contain an empty value
> > > after XS_MKDIR ?
> >
> > In this case I believe you are correct, but my feeling was that
> > people reading the code would be lulled into a false sense of
> > security that XS_MKDIR always did the right thing to initialize a
> > new path.
> 
> I'm not sure I follow this argument.  What did you think of my idea
> of renaming libxl__xs_mkdir to libxl__xs_mknode ?
> 

The issue, as I said, is the initial state of the node. If you use XS_MKDIR then it is not guaranteed to be empty. If you use XS_WRITE then it is and I think that is the correct semantic here (even though we happen to get away with it at the moment). I'm happy with the function rename, but would you be happy with the change in semantic?

  Paul

> Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Jackson]

Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown"):
> [Ian Jackson:]
> > I'm not sure I follow this argument.  What did you think of my idea
> > of renaming libxl__xs_mkdir to libxl__xs_mknode ?
> 
> The issue, as I said, is the initial state of the node. If you use
> XS_MKDIR then it is not guaranteed to be empty.

It is guaranteed to be empty if nothing ever wrote anything non-empty
to it.  I think participants in xenstore protocols, and users of
xenstore to store things, are entitled to assume that no-one writes
nonempty values to nodes that aren't supposed to have nonempty values.

And even if the node did somehow end up with a nonempty value, nothing
would break because no-one would read it.

> If you use XS_WRITE then it is and I think that is the correct
> semantic here (even though we happen to get away with it at the
> moment). I'm happy with the function rename, but would you be happy
> with the change in semantic?

XS_MKDIR creates missing parents, which XS_WRITE doesn't.

Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 24 November 2015 17:24
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Ian Campbell; Wei Liu
> Subject: RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir()
> for ~/control/shutdown"):
> > [Ian Jackson:]
> > > I'm not sure I follow this argument.  What did you think of my idea
> > > of renaming libxl__xs_mkdir to libxl__xs_mknode ?
> >
> > The issue, as I said, is the initial state of the node. If you use
> > XS_MKDIR then it is not guaranteed to be empty.
> 
> It is guaranteed to be empty if nothing ever wrote anything non-empty
> to it.  I think participants in xenstore protocols, and users of
> xenstore to store things, are entitled to assume that no-one writes
> nonempty values to nodes that aren't supposed to have nonempty values.
> 
> And even if the node did somehow end up with a nonempty value, nothing
> would break because no-one would read it.

Maybe. Personally I would have thought a toolstack would want to put things into a known state and XS_MKDIR just doesn't do that.

> 
> > If you use XS_WRITE then it is and I think that is the correct
> > semantic here (even though we happen to get away with it at the
> > moment). I'm happy with the function rename, but would you be happy
> > with the change in semantic?
> 
> XS_MKDIR creates missing parents, which XS_WRITE doesn't.
> 

According to the documentation it does: (from xenstore.txt)

READ			<path>|			<value|>
WRITE			<path>|<value|>
	Store and read the octet string <value> at <path>.
	WRITE creates any missing parent paths, with empty values.

  Paul

> Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Jackson]

Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown"):
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > And even if the node did somehow end up with a nonempty value, nothing
> > would break because no-one would read it.
> 
> Maybe. Personally I would have thought a toolstack would want to put things into a known state and XS_MKDIR just doesn't do that.

Hrm.

> > > If you use XS_WRITE then it is and I think that is the correct
> > > semantic here (even though we happen to get away with it at the
> > > moment). I'm happy with the function rename, but would you be happy
> > > with the change in semantic?
> > 
> > XS_MKDIR creates missing parents, which XS_WRITE doesn't.
> 
> According to the documentation it does: (from xenstore.txt)
> 
> READ			<path>|			<value|>
> WRITE			<path>|<value|>
> 	Store and read the octet string <value> at <path>.
> 	WRITE creates any missing parent paths, with empty values.

Oh!

Well so then perhaps we should jsut change libxl__xs_mkdir to use
WRITE.  This rather confusing situation is probably worth a doc
comment next to the prototype in libxl_internal.h.

Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 24 November 2015 17:39
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Ian Campbell; Wei Liu
> Subject: RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir()
> for ~/control/shutdown"):
> > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > And even if the node did somehow end up with a nonempty value,
> nothing
> > > would break because no-one would read it.
> >
> > Maybe. Personally I would have thought a toolstack would want to put
> things into a known state and XS_MKDIR just doesn't do that.
> 
> Hrm.
> 
> > > > If you use XS_WRITE then it is and I think that is the correct
> > > > semantic here (even though we happen to get away with it at the
> > > > moment). I'm happy with the function rename, but would you be
> happy
> > > > with the change in semantic?
> > >
> > > XS_MKDIR creates missing parents, which XS_WRITE doesn't.
> >
> > According to the documentation it does: (from xenstore.txt)
> >
> > READ			<path>|			<value|>
> > WRITE			<path>|<value|>
> > 	Store and read the octet string <value> at <path>.
> > 	WRITE creates any missing parent paths, with empty values.
> 
> Oh!
> 
> Well so then perhaps we should jsut change libxl__xs_mkdir to use
> WRITE.

I think the rename to libxl__xs_mknode() is warranted, and will avoid anyone's belief that the implementer typo-ed the actual XS command :-)

>  This rather confusing situation is probably worth a doc
> comment next to the prototype in libxl_internal.h.

Yep. That sounds like a good idea. I'll do that.

  Paul

> 
> Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Jackson]

Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown"):
> > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > Well so then perhaps we should jsut change libxl__xs_mkdir to use
> > WRITE.
> 
> I think the rename to libxl__xs_mknode() is warranted, and will avoid anyone's belief that the implementer typo-ed the actual XS command :-)

OK.

> >  This rather confusing situation is probably worth a doc
> > comment next to the prototype in libxl_internal.h.
> 
> Yep. That sounds like a good idea. I'll do that.

Thanks,
Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Campbell]

On Tue, 2015-11-24 at 17:20 +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > Sent: 24 November 2015 16:35
> > To: Paul Durrant
> > Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Ian Campbell;
> > Wei Liu
> > Subject: RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> > ~/control/shutdown
> > 
> > Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using
> > libxl__xs_mkdir()
> > for ~/control/shutdown"):
> > > [Ian Jackson]
> > > > Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using
> > libxl__xs_mkdir()
> > > > for ~/control/shutdown"):
> > > > > [Ian Jackson:]
> > > > > > Maybe it would be easier to rename libxl__xs_mkdir to
> > > > > > libxl__xs_mknode ?  (It's probably too late to rename
> > > > > > XS_MKDIR.)
> > > > > 
> > > > > There is still the need to set the path to an empty value though,
> > > > > which
> > is
> > > > not implicitly done by the XS_MKDIR.
> > > > 
> > > > Under what circumstances would this path not contain an empty value
> > > > after XS_MKDIR ?
> > > 
> > > In this case I believe you are correct, but my feeling was that
> > > people reading the code would be lulled into a false sense of
> > > security that XS_MKDIR always did the right thing to initialize a
> > > new path.
> > 
> > I'm not sure I follow this argument.  What did you think of my idea
> > of renaming libxl__xs_mkdir to libxl__xs_mknode ?
> > 
> 
> The issue, as I said, is the initial state of the node. If you use
> XS_MKDIR then it is not guaranteed to be empty.

Just to satisfy my curiosity, how can it be non-empty? What else could it
possibly contain, just garbage?

Or maybe this is the behaviour of XS_MKDIR on a path/node which already
exists?

Ian.

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

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: 25 November 2015 10:43
> To: Paul Durrant; Ian Jackson
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Wei Liu
> Subject: Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> On Tue, 2015-11-24 at 17:20 +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > Sent: 24 November 2015 16:35
> > > To: Paul Durrant
> > > Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Ian Campbell;
> > > Wei Liu
> > > Subject: RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> > > ~/control/shutdown
> > >
> > > Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using
> > > libxl__xs_mkdir()
> > > for ~/control/shutdown"):
> > > > [Ian Jackson]
> > > > > Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using
> > > libxl__xs_mkdir()
> > > > > for ~/control/shutdown"):
> > > > > > [Ian Jackson:]
> > > > > > > Maybe it would be easier to rename libxl__xs_mkdir to
> > > > > > > libxl__xs_mknode ?  (It's probably too late to rename
> > > > > > > XS_MKDIR.)
> > > > > >
> > > > > > There is still the need to set the path to an empty value though,
> > > > > > which
> > > is
> > > > > not implicitly done by the XS_MKDIR.
> > > > >
> > > > > Under what circumstances would this path not contain an empty
> value
> > > > > after XS_MKDIR ?
> > > >
> > > > In this case I believe you are correct, but my feeling was that
> > > > people reading the code would be lulled into a false sense of
> > > > security that XS_MKDIR always did the right thing to initialize a
> > > > new path.
> > >
> > > I'm not sure I follow this argument.  What did you think of my idea
> > > of renaming libxl__xs_mkdir to libxl__xs_mknode ?
> > >
> >
> > The issue, as I said, is the initial state of the node. If you use
> > XS_MKDIR then it is not guaranteed to be empty.
> 
> Just to satisfy my curiosity, how can it be non-empty? What else could it
> possibly contain, just garbage?
> 
> Or maybe this is the behaviour of XS_MKDIR on a path/node which already
> exists?
> 

Yes, that's exactly it. I believe XS_MKDIR does guarantee to create a path empty, but will not clear an existing one.

  Paul

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

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Campbell]

On Wed, 2015-11-25 at 10:46 +0000, Paul Durrant wrote:
> > Or maybe this is the behaviour of XS_MKDIR on a path/node which already
> > exists?
> > 
> 
> Yes, that's exactly it. I believe XS_MKDIR does guarantee to create a
> path empty, but will not clear an existing one.

Thanks.

I suppose clearing a node on mkdir would be an equally surprising semantic,
after all mkdir(1) doesn't remove all files in an existing directory.

It's a pity MKDIR doesn't follow mkdir(1) and return EEXIST, but we are
stuck with that I guess.

As well as the changes being considered here I wonder if any
libxl__xs_mkdir alike ought to include the existence check and error
return?

Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: 25 November 2015 11:02
> To: Paul Durrant; Ian Jackson
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Wei Liu
> Subject: Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> On Wed, 2015-11-25 at 10:46 +0000, Paul Durrant wrote:
> > > Or maybe this is the behaviour of XS_MKDIR on a path/node which
> already
> > > exists?
> > >
> >
> > Yes, that's exactly it. I believe XS_MKDIR does guarantee to create a
> > path empty, but will not clear an existing one.
> 
> Thanks.
> 
> I suppose clearing a node on mkdir would be an equally surprising semantic,
> after all mkdir(1) doesn't remove all files in an existing directory.
> 
> It's a pity MKDIR doesn't follow mkdir(1) and return EEXIST, but we are
> stuck with that I guess.
> 
> As well as the changes being considered here I wonder if any
> libxl__xs_mkdir alike ought to include the existence check and error
> return?

Looking at the current callers of libxl__xs_mkdir(), I think they all expect to be creating new (empty) paths so I was planning a global replace of libxl__xs_mkdir() with libxl__xs_mknod() and I'll put a comment in the header to explain the semantics of that function.

  Paul

> 
> Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Jackson]

Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown"):
> Looking at the current callers of libxl__xs_mkdir(), I think they all expect to be creating new (empty) paths so I was planning a global replace of libxl__xs_mkdir() with libxl__xs_mknod() and I'll put a comment in the header to explain the semantics of that function.

Can I ask you to split the patch up into two, one of which is the
entirely mechanical function rename, and one of which is the change to
use xs_write ?

Thanks,
Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 25 November 2015 11:14
> To: Paul Durrant
> Cc: Ian Campbell; xen-devel@lists.xenproject.org; Stefano Stabellini; Wei Liu
> Subject: RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> Paul Durrant writes ("RE: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir()
> for ~/control/shutdown"):
> > Looking at the current callers of libxl__xs_mkdir(), I think they all expect to
> be creating new (empty) paths so I was planning a global replace of
> libxl__xs_mkdir() with libxl__xs_mknod() and I'll put a comment in the
> header to explain the semantics of that function.
> 
> Can I ask you to split the patch up into two, one of which is the
> entirely mechanical function rename, and one of which is the change to
> use xs_write ?

Sure. I'll do that.

  Paul

> 
> Thanks,
> Ian.

Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Ian Jackson]

Ian Campbell writes ("Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown"):
> On Tue, 2015-11-24 at 17:20 +0000, Paul Durrant wrote:
> > The issue, as I said, is the initial state of the node. If you use
> > XS_MKDIR then it is not guaranteed to be empty.
> 
> Just to satisfy my curiosity, how can it be non-empty? What else could it
> possibly contain, just garbage?

AIUI Paul is worried about the hypothetical possibility that someone
might call libxl__xs_mkdir
  - on a path which already exists;
  - and has a nonempty value;
  - expecting the nonempty value to be cleared.

I think this is too remote a possibility to be concerned about.  But
since the WRITE operation has identical semantics to MKDIR _except_
that it overwrites the existing value, I don't mind if Paul cares
about this and wants to change libxl to always use WRITE and never
MKDIR.[1]

Ian.

[1] There are AFAIAA no existing xenstore protocols or uses where a
node is deliberately used both to store a value, and as a parent for
descendent nodes.  We don't seem likely to introduce any such thing,
because it would be confusing.

[RFC PATCH 3/6] libxl: implement control feature checking

[Paul Durrant]

docs/misc/xenstore-paths documents a set of feature flags that can be used
to determine whether values written to ~/control/shutdown will result in
actions in the guest.

This patch adds empty guest writable values into xenstore for each
documented feature flag (to allow the guest to update them) and also code
to check these flags to determine whether a specific use of
libxl__domain_pvcontrol() will definitely *not* result in action and fail
the call immediately.

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             | 51 ++++++++++++++++++++++-------------------
 tools/libxl/libxl_create.c      | 22 ++++++++++++++++++
 tools/libxl/libxl_dom_suspend.c |  6 ++++-
 tools/libxl/libxl_internal.h    |  8 +++----
 4 files changed, 58 insertions(+), 29 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0cb4a15..d7dd081 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1082,11 +1082,13 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
     return rc;
 }
 
-int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid)
+int libxl__domain_pvcontrol_available(libxl__gc *gc, xs_transaction_t t,
+                                      uint32_t domid, const char *cmd)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-
     uint64_t pvdriver = 0;
+    const char *dom_path;
+    char *flag;
     int ret;
 
     libxl_domain_type domtype = libxl__domain_type(gc, domid);
@@ -1101,34 +1103,37 @@ int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid)
         LOGE(ERROR, "getting HVM callback IRQ");
         return ERROR_FAIL;
     }
-    return !!pvdriver;
-}
 
-const char *libxl__domain_pvcontrol_xspath(libxl__gc *gc, uint32_t domid)
-{
-    const char *dom_path;
+    if (!pvdriver)
+        return 0;
 
     dom_path = libxl__xs_get_dompath(gc, domid);
     if (!dom_path)
-        return NULL;
+        return ERROR_FAIL;
 
-    return GCSPRINTF("%s/control/shutdown", dom_path);
+    /* Check to see if the command is explicitly not supported */
+    flag = libxl__xs_read(gc, t,
+                          libxl__sprintf(gc, "%s/control/feature-%s",
+                                         dom_path, cmd));
+    if (flag && !strcmp(flag, "0"))
+        return 0;
+
+    return 1;
 }
 
-char * libxl__domain_pvcontrol_read(libxl__gc *gc, xs_transaction_t t,
-                                    uint32_t domid)
+const char *libxl__domain_pvcontrol_xspath(libxl__gc *gc, uint32_t domid)
 {
-    const char *shutdown_path;
+    const char *dom_path;
 
-    shutdown_path = libxl__domain_pvcontrol_xspath(gc, domid);
-    if (!shutdown_path)
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    if (!dom_path)
         return NULL;
 
-    return libxl__xs_read(gc, t, shutdown_path);
+    return GCSPRINTF("%s/control/shutdown", dom_path);
 }
 
-int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
-                                  uint32_t domid, const char *cmd)
+static int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
+                                         uint32_t domid, const char *cmd)
 {
     const char *shutdown_path;
 
@@ -1139,26 +1144,26 @@ int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
     return libxl__xs_printf(gc, t, shutdown_path, "%s", cmd);
 }
 
-static int libxl__domain_pvcontrol(libxl__gc *gc, uint32_t domid,
-                                   const char *cmd)
+int libxl__domain_pvcontrol(libxl__gc *gc, xs_transaction_t t,
+                            uint32_t domid, const char *cmd)
 {
     int ret;
 
-    ret = libxl__domain_pvcontrol_available(gc, domid);
+    ret = libxl__domain_pvcontrol_available(gc, t, domid, cmd);
     if (ret < 0)
         return ret;
 
     if (!ret)
         return ERROR_NOPARAVIRT;
 
-    return libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, cmd);
+    return libxl__domain_pvcontrol_write(gc, t, domid, cmd);
 }
 
 int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid)
 {
     GC_INIT(ctx);
     int ret;
-    ret = libxl__domain_pvcontrol(gc, domid, "poweroff");
+    ret = libxl__domain_pvcontrol(gc, XBT_NULL, domid, "poweroff");
     GC_FREE;
     return ret;
 }
@@ -1167,7 +1172,7 @@ int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid)
 {
     GC_INIT(ctx);
     int ret;
-    ret = libxl__domain_pvcontrol(gc, domid, "reboot");
+    ret = libxl__domain_pvcontrol(gc, XBT_NULL, domid, "reboot");
     GC_FREE;
     return ret;
 }
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 279deda..cc82311 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -619,6 +619,28 @@ retry_transaction:
                            libxl__sprintf(gc, "%s/shutdown", control_path),
                            rwperm, ARRAY_SIZE(rwperm),
                            "");
+    libxl__xs_printf_perms(gc, t,
+                           libxl__sprintf(gc, "%s/feature-poweroff", control_path),
+                           rwperm, ARRAY_SIZE(rwperm),
+                           "");
+    libxl__xs_printf_perms(gc, t,
+                           libxl__sprintf(gc, "%s/feature-reboot", control_path),
+                           rwperm, ARRAY_SIZE(rwperm),
+                           "");
+    libxl__xs_printf_perms(gc, t,
+                           libxl__sprintf(gc, "%s/feature-suspend", control_path),
+                           rwperm, ARRAY_SIZE(rwperm),
+                           "");
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        libxl__xs_printf_perms(gc, t,
+                               libxl__sprintf(gc, "%s/feature-s3", control_path),
+                               rwperm, ARRAY_SIZE(rwperm),
+                               "");
+        libxl__xs_printf_perms(gc, t,
+                               libxl__sprintf(gc, "%s/feature-s4", control_path),
+                               rwperm, ARRAY_SIZE(rwperm),
+                               "");
+    }
 
     libxl__xs_mkdir(gc, t,
                     libxl__sprintf(gc, "%s/device/suspend/event-channel", dom_path),
diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
index 4cc01ad..5edfef2 100644
--- a/tools/libxl/libxl_dom_suspend.c
+++ b/tools/libxl/libxl_dom_suspend.c
@@ -143,7 +143,11 @@ static void domain_suspend_callback_common(libxl__egc *egc,
     LOG(DEBUG, "issuing %s suspend request via XenBus control node",
         dss->hvm ? "PVHVM" : "PV");
 
-    libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "suspend");
+    rc = libxl__domain_pvcontrol(gc, XBT_NULL, domid, "suspend");
+    if (rc < 0) {
+        LOGE(ERROR, "suspend failed");
+        goto err;
+    }
 
     dss->pvcontrol.path = libxl__domain_pvcontrol_xspath(gc, domid);
     if (!dss->pvcontrol.path) { rc = ERROR_FAIL; goto err; }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 04d8a29..ab62e6f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1135,13 +1135,11 @@ _hidden int libxl__domain_resume(libxl__gc *gc, uint32_t domid,
                                  int suspend_cancel);
 
 /* returns 0 or 1, or a libxl error code */
-_hidden int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid);
+_hidden int libxl__domain_pvcontrol_available(libxl__gc *gc, xs_transaction_t t, uint32_t domid,const char *cmd);
 
 _hidden const char *libxl__domain_pvcontrol_xspath(libxl__gc*, uint32_t domid);
-_hidden char * libxl__domain_pvcontrol_read(libxl__gc *gc,
-                                            xs_transaction_t t, uint32_t domid);
-_hidden int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
-                                          uint32_t domid, const char *cmd);
+_hidden int libxl__domain_pvcontrol(libxl__gc *gc, xs_transaction_t t,
+                                    uint32_t domid, const char *cmd);
 
 /* from xl_device */
 _hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend backend);
-- 
2.1.4

Re: [RFC PATCH 3/6] libxl: implement control feature checking

[Ian Jackson]

Paul Durrant writes ("[RFC PATCH 3/6] libxl: implement control feature checking"):
> docs/misc/xenstore-paths documents a set of feature flags that can be used
> to determine whether values written to ~/control/shutdown will result in
> actions in the guest.
> 
> This patch adds empty guest writable values into xenstore for each
> documented feature flag (to allow the guest to update them) and also code
> to check these flags to determine whether a specific use of
> libxl__domain_pvcontrol() will definitely *not* result in action and fail
> the call immediately.

I am going to go back and see what you wrote in the spec, but this
patch is only correct if the guest is not permitted to infer anything
from the existence of these nodes.

> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 279deda..cc82311 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -619,6 +619,28 @@ retry_transaction:
>                             libxl__sprintf(gc, "%s/shutdown", control_path),
>                             rwperm, ARRAY_SIZE(rwperm),
>                             "");
> +    libxl__xs_printf_perms(gc, t,
> +                           libxl__sprintf(gc, "%s/feature-poweroff", control_path),
> +                           rwperm, ARRAY_SIZE(rwperm),
> +                           "");
> +    libxl__xs_printf_perms(gc, t,
> +                           libxl__sprintf(gc, "%s/feature-reboot", control_path),
> +                           rwperm, ARRAY_SIZE(rwperm),
> +                           "");

This is really very repetitive and also needs more energetic wrapping
and also needs to use GCSPRINTF.  (Indeed, in several parts of this
patch you replace GCSPRINTF calls, but your new code is all
libxl__sprintf(gc,)).

Thanks,
Ian.

[RFC PATCH 4/6] libxl: add guest writable xenstore area for driver versions

[Paul Durrant]

docs/misc/xenstore-paths documents a path format that can be used by
guests to supply version information for PV drivers.

This patch creates a guest writable ~/drivers area for these paths.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index cc82311..389427f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -648,6 +648,9 @@ retry_transaction:
     libxl__xs_mkdir(gc, t,
                     libxl__sprintf(gc, "%s/data", dom_path),
                     rwperm, ARRAY_SIZE(rwperm));
+    libxl__xs_mkdir(gc, t,
+                    libxl__sprintf(gc, "%s/drivers", dom_path),
+                    rwperm, ARRAY_SIZE(rwperm));
 
     if (libxl_defbool_val(info->driver_domain)) {
         /*
-- 
2.1.4

Re: [RFC PATCH 4/6] libxl: add guest writable xenstore area for driver versions

[Ian Jackson]

Paul Durrant writes ("[RFC PATCH 4/6] libxl: add guest writable xenstore area for driver versions"):
> docs/misc/xenstore-paths documents a path format that can be used by
> guests to supply version information for PV drivers.
> 
> This patch creates a guest writable ~/drivers area for these paths.
> 
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index cc82311..389427f 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -648,6 +648,9 @@ retry_transaction:
>      libxl__xs_mkdir(gc, t,
>                      libxl__sprintf(gc, "%s/data", dom_path),
>                      rwperm, ARRAY_SIZE(rwperm));
> +    libxl__xs_mkdir(gc, t,
> +                    libxl__sprintf(gc, "%s/drivers", dom_path),
> +                    rwperm, ARRAY_SIZE(rwperm));

I see there are already many of these open-coded calls to
libxl__xs_mkdir with libxl__sprintf (rather than GCSPRINTF).  Oh well.

Ian.

Re: [RFC PATCH 4/6] libxl: add guest writable xenstore area for driver versions

[Ian Jackson]

Ian Jackson writes ("Re: [RFC PATCH 4/6] libxl: add guest writable xenstore area for driver versions"):
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index cc82311..389427f 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -648,6 +648,9 @@ retry_transaction:
> >      libxl__xs_mkdir(gc, t,
> >                      libxl__sprintf(gc, "%s/data", dom_path),
> >                      rwperm, ARRAY_SIZE(rwperm));
> > +    libxl__xs_mkdir(gc, t,
> > +                    libxl__sprintf(gc, "%s/drivers", dom_path),
> > +                    rwperm, ARRAY_SIZE(rwperm));
> 
> I see there are already many of these open-coded calls to
> libxl__xs_mkdir with libxl__sprintf (rather than GCSPRINTF).  Oh well.

Which means, I should say:

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

[RFC PATCH 5/6] libxl: add guest writable xenstore area for feature advertisement

[Paul Durrant]

docs/misc/xenstore-paths documents a paths under a top-level ~/features
area that a guest can use to advertise the ability to react to vif
or vbd hotplug.

This patch creates the guest writable ~/features area.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 389427f..97ff215 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -651,6 +651,9 @@ retry_transaction:
     libxl__xs_mkdir(gc, t,
                     libxl__sprintf(gc, "%s/drivers", dom_path),
                     rwperm, ARRAY_SIZE(rwperm));
+    libxl__xs_mkdir(gc, t,
+                    libxl__sprintf(gc, "%s/feature", dom_path),
+                    rwperm, ARRAY_SIZE(rwperm));
 
     if (libxl_defbool_val(info->driver_domain)) {
         /*
-- 
2.1.4

Re: [RFC PATCH 5/6] libxl: add guest writable xenstore area for feature advertisement

[Ian Jackson]

Paul Durrant writes ("[RFC PATCH 5/6] libxl: add guest writable xenstore area for feature advertisement"):
> docs/misc/xenstore-paths documents a paths under a top-level ~/features
> area that a guest can use to advertise the ability to react to vif
> or vbd hotplug.

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

As before.

Ian.

[RFC PATCH 6/6] libxl: create path for advertisement of network attributes...

[Paul Durrant]

...at vif instantiation time.

The xenstore path ~attr/vif/$DEVID is documented for the purposes of
a frontend advertising network attributes.

This patch adds an extra bool argument to libxl__device_generic_add()
(the function which creates frontend and backend xenstore paths) which
will cause creation of guest writable xenstore path ~/attr/$DEVICE/$DEVID
if true. libxl__device_nic_add() (i.e. $DEVICE == 'vif') is modified
to pass true for this argument. All other callers pass false.

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          | 18 ++++++++++++------
 tools/libxl/libxl_device.c   | 30 ++++++++++++++++++++++++------
 tools/libxl/libxl_internal.h |  5 ++++-
 tools/libxl/libxl_pci.c      |  3 ++-
 4 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d7dd081..1689274 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2186,7 +2186,8 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
                                                              back->count),
                                   libxl__xs_kvs_of_flexarray(gc, front,
                                                              front->count),
-                                  NULL);
+                                  NULL,
+                                  false);
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
@@ -2628,7 +2629,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
         libxl__device_generic_add(gc, t, device,
                                   libxl__xs_kvs_of_flexarray(gc, back, back->count),
                                   libxl__xs_kvs_of_flexarray(gc, front, front->count),
-                                  NULL);
+                                  NULL,
+                                  false);
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
@@ -3442,7 +3444,8 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
                                                              back->count),
                                   libxl__xs_kvs_of_flexarray(gc, front,
                                                              front->count),
-                                  NULL);
+                                  NULL,
+                                  true);
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
@@ -3731,7 +3734,8 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
     libxl__device_generic_add(gc, XBT_NULL, device,
                               libxl__xs_kvs_of_flexarray(gc, back, back->count),
                               libxl__xs_kvs_of_flexarray(gc, front, front->count),
-                              libxl__xs_kvs_of_flexarray(gc, ro_front, ro_front->count));
+                              libxl__xs_kvs_of_flexarray(gc, ro_front, ro_front->count),
+                              false);
     rc = 0;
 out:
     return rc;
@@ -4031,7 +4035,8 @@ int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
     libxl__device_generic_add(gc, XBT_NULL, &device,
                               libxl__xs_kvs_of_flexarray(gc, back, back->count),
                               libxl__xs_kvs_of_flexarray(gc, front, front->count),
-                              NULL);
+                              NULL,
+                              false);
     rc = 0;
 out:
     return rc;
@@ -4144,7 +4149,8 @@ int libxl__device_vfb_add(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb)
     libxl__device_generic_add(gc, XBT_NULL, &device,
                               libxl__xs_kvs_of_flexarray(gc, back, back->count),
                               libxl__xs_kvs_of_flexarray(gc, front, front->count),
-                              NULL);
+                              NULL,
+                              false);
     rc = 0;
 out:
     return rc;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 8bb5e93..3b2cbcd 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -40,6 +40,15 @@ char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device)
                      device->domid, device->devid);
 }
 
+char *libxl__device_attr_path(libxl__gc *gc, libxl__device *device)
+{
+    char *dom_path = libxl__xs_get_dompath(gc, device->domid);
+
+    return GCSPRINTF("%s/attr/%s/%d", dom_path,
+                     libxl__device_kind_to_string(device->kind),
+                     device->devid);
+}
+
 /* Returns 1 if device exists, 0 if not, ERROR_* (<0) on error. */
 int libxl__device_exists(libxl__gc *gc, xs_transaction_t t,
                          libxl__device *device)
@@ -102,22 +111,25 @@ out:
 }
 
 int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
-        libxl__device *device, char **bents, char **fents, char **ro_fents)
+                              libxl__device *device, char **bents, char **fents, char **ro_fents,
+                              bool attr)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    char *frontend_path, *backend_path;
+    char *frontend_path, *backend_path, *attr_path;
     struct xs_permissions frontend_perms[2];
     struct xs_permissions ro_frontend_perms[2];
     struct xs_permissions backend_perms[2];
+    struct xs_permissions attr_perms[2];
     int create_transaction = t == XBT_NULL;
 
     frontend_path = libxl__device_frontend_path(gc, device);
     backend_path = libxl__device_backend_path(gc, device);
+    attr_path = libxl__device_attr_path(gc, device);
 
-    frontend_perms[0].id = device->domid;
-    frontend_perms[0].perms = XS_PERM_NONE;
-    frontend_perms[1].id = device->backend_domid;
-    frontend_perms[1].perms = XS_PERM_READ;
+    attr_perms[0].id = frontend_perms[0].id = device->domid;
+    attr_perms[0].perms = frontend_perms[0].perms = XS_PERM_NONE;
+    attr_perms[1].id = frontend_perms[1].id = device->backend_domid;
+    attr_perms[1].perms = frontend_perms[1].perms = XS_PERM_READ;
 
     ro_frontend_perms[0].id = backend_perms[0].id = device->backend_domid;
     ro_frontend_perms[0].perms = backend_perms[0].perms = XS_PERM_NONE;
@@ -162,6 +174,12 @@ retry_transaction:
         libxl__xs_writev(gc, t, backend_path, bents);
     }
 
+    if (attr) {
+        xs_rm(ctx->xsh, t, attr_path);
+        xs_mkdir(ctx->xsh, t, attr_path);
+        xs_set_permissions(ctx->xsh, t, attr_path, attr_perms, ARRAY_SIZE(attr_perms));
+    }
+
     if (!create_transaction)
         return 0;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ab62e6f..89d893f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1159,9 +1159,12 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
 _hidden int libxl__device_exists(libxl__gc *gc, xs_transaction_t t,
                                  libxl__device *device);
 _hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
-        libxl__device *device, char **bents, char **fents, char **ro_fents);
+                                      libxl__device *device, char **bents,
+                                      char **fents, char **ro_fents,
+                                      bool attr);
 _hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
 _hidden char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device);
+_hidden char *libxl__device_attr_path(libxl__gc *gc, libxl__device *device);
 _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
                                       libxl__device *dev);
 _hidden int libxl__device_destroy(libxl__gc *gc, libxl__device *dev);
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index de3976b..c175939 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -111,7 +111,8 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
     libxl__device_generic_add(gc, XBT_NULL, &device,
                               libxl__xs_kvs_of_flexarray(gc, back, back->count),
                               libxl__xs_kvs_of_flexarray(gc, front, front->count),
-                              NULL);
+                              NULL,
+                              false);
 
     return 0;
 }
-- 
2.1.4

Re: [RFC PATCH 6/6] libxl: create path for advertisement of network attributes...

[Ian Jackson]

Paul Durrant writes ("[RFC PATCH 6/6] libxl: create path for advertisement of network attributes..."):
> -    frontend_perms[0].id = device->domid;
> -    frontend_perms[0].perms = XS_PERM_NONE;
> -    frontend_perms[1].id = device->backend_domid;
> -    frontend_perms[1].perms = XS_PERM_READ;
> +    attr_perms[0].id = frontend_perms[0].id = device->domid;
> +    attr_perms[0].perms = frontend_perms[0].perms = XS_PERM_NONE;
> +    attr_perms[1].id = frontend_perms[1].id = device->backend_domid;
> +    attr_perms[1].perms = frontend_perms[1].perms = XS_PERM_READ;

Why not just use frontend_perms throughout rather than creating a new
identical structure ?

If you do want another structure, please line up the assignemnts
horizontally so that it is easy to read !

Also there is a lot of wrap damage in this patch.  Can you please
rewrap the lines you touch, at least ?  At the moment it is hard for
me to read.

Thanks,
Ian.

Re: [RFC PATCH 6/6] libxl: create path for advertisement of network attributes...

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 16 November 2015 17:30
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini; Ian Campbell; Wei Liu
> Subject: Re: [RFC PATCH 6/6] libxl: create path for advertisement of network
> attributes...
> 
> Paul Durrant writes ("[RFC PATCH 6/6] libxl: create path for advertisement of
> network attributes..."):
> > -    frontend_perms[0].id = device->domid;
> > -    frontend_perms[0].perms = XS_PERM_NONE;
> > -    frontend_perms[1].id = device->backend_domid;
> > -    frontend_perms[1].perms = XS_PERM_READ;
> > +    attr_perms[0].id = frontend_perms[0].id = device->domid;
> > +    attr_perms[0].perms = frontend_perms[0].perms = XS_PERM_NONE;
> > +    attr_perms[1].id = frontend_perms[1].id = device->backend_domid;
> > +    attr_perms[1].perms = frontend_perms[1].perms = XS_PERM_READ;
> 
> Why not just use frontend_perms throughout rather than creating a new
> identical structure ?
>

I could do that. I felt that a separate name was more illustrative though.
 
> If you do want another structure, please line up the assignemnts
> horizontally so that it is easy to read !
> 

Sure.

> Also there is a lot of wrap damage in this patch.  Can you please
> rewrap the lines you touch, at least ?  At the moment it is hard for
> me to read.
> 

I'll fix when I post the non-RFC version following commit of my docs patches.

  Paul

> Thanks,
> Ian.