Xen devel - Dec 2012 - [PATCH RFC 00/10] libxl: new hotplug calling convention

This series implements a new hoplug calling convention for libxl.

The aim of this new convention is to reduce the blackout phase of 
migration when using complex hotplug scripts, like iSCSI or other kind 
of storage backends that might have a non trivial setup time.

There are some issues that I would like to discuss, the first one is 
the fact that pdev_path field in libxl_device_disk is no longuer used 
to store a physical path, since diskspec "target" can now contain 
"random" information to connect a block device.

To solve this I would like to introduce a new field in 
libxl_device_disk called "target", that will be used to store the 
diskspec target parameter. This can later be copied to pdev_path if 
using the old hotplug calling convention.

The second issue is related to iSCSI and iscsiadm, specifically the 
way to set authentication parameters, which is done with command line 
parameters or editing files (which each distro seems to place in 
different locations). I will look into this to see if we can find a 
suitable solution.

Thanks for the comments, Roger.

num_exec was not cleared when calling libxl__prepare_ao_device.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl_device.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 51dd06e..58d3f35 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -409,6 +409,7 @@ void libxl__prepare_ao_device(libxl__ao *ao,
libxl__ao_device *aodev)
     aodev->ao = ao;
     aodev->rc = 0;
     aodev->dev = NULL;
+    aodev->num_exec = 0;
     /* Initialize timer for QEMU Bodge and hotplug execution */
     libxl__ev_time_init(&aodev->timeout);
     aodev->active = 1;
-- 
1.7.7.5 (Apple Git-26)


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

Add support to call hotplug scripts that use the new hotplug interface
to the low-level functions in charge of calling hotplug scripts. This
new calling convention will be used when the hotplug_version aodev
field is 1.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl_device.c   |   33 +++++++++++++++++++-
 tools/libxl/libxl_internal.h |   33 ++++++++++++++++++--
 tools/libxl/libxl_linux.c    |   68 ++++++++++++++++++++++++++++++++----------
 tools/libxl/libxl_netbsd.c   |    2 +-
 4 files changed, 115 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 58d3f35..85c9953 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -83,6 +83,35 @@ out:
     return rc;
 }
 
+char *libxl__device_hotplug_action(libxl__gc *gc,
+                                   libxl__device_action action)
+{
+    switch (action) {
+    case DEVICE_CONNECT:
+        return "add";
+    case DEVICE_DISCONNECT:
+        return "remove";
+    case DEVICE_PREPARE:
+        return "prepare";
+    case DEVICE_UNPREPARE:
+        return "unprepare";
+    case DEVICE_LOCALATTACH:
+        return "localattach";
+    case DEVICE_LOCALDETACH:
+        return "localdetach";
+    default:
+        LOG(ERROR, "invalid action (%d) for device", action);
+        break;
+    }
+    return NULL;
+}
+
+char *libxl__device_xs_hotplug_path(libxl__gc *gc, libxl__device *dev)
+{
+    return GCSPRINTF("/local/domain/%u/libxl/hotplug/%u/%u",
+                     dev->backend_domid, dev->domid, dev->devid);
+}
+
 int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
         libxl__device *device, char **bents, char **fents)
 {
@@ -410,6 +439,7 @@ void libxl__prepare_ao_device(libxl__ao *ao,
libxl__ao_device *aodev)
     aodev->rc = 0;
     aodev->dev = NULL;
     aodev->num_exec = 0;
+    aodev->hotplug_version = 0;
     /* Initialize timer for QEMU Bodge and hotplug execution */
     libxl__ev_time_init(&aodev->timeout);
     aodev->active = 1;
@@ -891,7 +921,8 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device
*aodev)
      * and return the necessary args/env vars for execution */
     hotplug = libxl__get_hotplug_script_info(gc, aodev->dev, &args,
&env,
                                              aodev->action,
-                                             aodev->num_exec);
+                                             aodev->num_exec,
+                                             aodev->hotplug_version);
     switch (hotplug) {
     case 0:
         /* no hotplug script to execute */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0b38e3e..c41e608 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1829,12 +1829,19 @@ _hidden const char *libxl__run_dir_path(void);
 
 /*----- device addition/removal -----*/
 
-/* Action to perform (either connect or disconnect) */
+/* Action to perform */
 typedef enum {
     DEVICE_CONNECT,
-    DEVICE_DISCONNECT
+    DEVICE_DISCONNECT,
+    DEVICE_PREPARE,
+    DEVICE_UNPREPARE,
+    DEVICE_LOCALATTACH,
+    DEVICE_LOCALDETACH
 } libxl__device_action;
 
+_hidden char *libxl__device_hotplug_action(libxl__gc *gc,
+                                           libxl__device_action action);
+
 typedef struct libxl__ao_device libxl__ao_device;
 typedef struct libxl__multidev libxl__multidev;
 typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
@@ -1852,6 +1859,14 @@ typedef void libxl__device_callback(libxl__egc*,
libxl__ao_device*);
  * Once _prepare has been called on a libxl__ao_device, it is safe to just
  * discard this struct, there's no need to call any destroy function.
  * _prepare can also be called multiple times with the same libxl__ao_device.
+ *
+ * If hotplug_version field is 1, the new hotplug script calling convention
+ * will be used to call the hotplug script. This new convention provides
+ * two new actions to hotplug scripts, "prepare",
"unprepare", "localattach"
+ * and "localdetach". This new actions have been added to offload
work done
+ * by hotplug scripts during the blackout phase of migration.
"prepare" is
+ * called before the remote domain is paused, so as much operations as
+ * possible should be done in this phase.
  */
 _hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev);
 
@@ -1862,6 +1877,7 @@ struct libxl__ao_device {
     libxl__device *dev;
     int force;
     libxl__device_callback *callback;
+    int hotplug_version;
     /* return value, zeroed by user on entry, is valid on callback */
     int rc;
     /* private for multidev */
@@ -2045,6 +2061,17 @@ _hidden void libxl__initiate_device_remove(libxl__egc
*egc,
                                            libxl__ao_device *aodev);
 
 /*
+ * libxl__device_xs_hotplug_path returns the xenstore hotplug
+ * path that is used to share information with the hotplug
+ * script.
+ *
+ * This path is only used by new hotplug scripts, that are
+ * specified using "method" instead of "script" in the disk
+ * parameters.
+ */
+_hidden char *libxl__device_xs_hotplug_path(libxl__gc *gc, libxl__device *dev);
+
+/*
  * libxl__get_hotplug_script_info returns the args and env that should
  * be passed to the hotplug script for the requested device.
  *
@@ -2065,7 +2092,7 @@ _hidden void libxl__initiate_device_remove(libxl__egc
*egc,
 _hidden int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
                                            char ***args, char ***env,
                                            libxl__device_action action,
-                                           int num_exec);
+                                           int num_exec, int hotplug_version);
 
 /*----- local disk attach: attach a disk locally to run the bootloader -----*/
 
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 1fed3cd..43f23dd 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -190,32 +190,68 @@ out:
 
 static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
                                char ***args, char ***env,
-                               libxl__device_action action)
+                               libxl__device_action action,
+                               int hotplug_version)
 {
     char *be_path = libxl__device_backend_path(gc, dev);
-    char *script;
+    char *hotplug_path = libxl__device_xs_hotplug_path(gc, dev);
+    char *script, *saction;
     int nr = 0, rc = 0;
+    const int env_arraysize = 5;
+    const int arg_arraysize = 3;
 
-    script = libxl__xs_read(gc, XBT_NULL,
-                            GCSPRINTF("%s/%s", be_path,
"script"));
-    if (!script) {
-        LOGEV(ERROR, errno, "unable to read script from %s",
be_path);
+    switch (hotplug_version) {
+    case 0:
+        script = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/script",
be_path));
+        if (!script) {
+            LOGEV(ERROR, errno, "unable to read script from %s",
be_path);
+            rc = ERROR_FAIL;
+            goto error;
+        }
+        *env = get_hotplug_env(gc, script, dev);
+        if (!*env) {
+            rc = ERROR_FAIL;
+            goto error;
+        }
+        break;
+    case 1:
+        /* The new hotplug calling convention only uses two ENV variables:
+         * BACKEND_PATH: path to xenstore backend of the related device.
+         * HOTPLUG_PATH: path to the xenstore directory that can be used to
+         * pass extra parameters to the script.
+         */
+        script = libxl__xs_read(gc, XBT_NULL,
+                                GCSPRINTF("%s/script",
hotplug_path));
+        if (!script) {
+            LOGEV(ERROR, errno, "unable to read script from %s",
hotplug_path);
+            rc = ERROR_FAIL;
+            goto error;
+        }
+        GCNEW_ARRAY(*env, env_arraysize);
+        (*env)[nr++] = "BACKEND_PATH";
+        (*env)[nr++] = be_path;
+        (*env)[nr++] = "HOTPLUG_PATH";
+        (*env)[nr++] = hotplug_path;
+        (*env)[nr++] = NULL;
+        assert(nr == env_arraysize);
+        nr = 0;
+        break;
+    default:
+        LOG(ERROR, "unknown hotplug script version %d",
hotplug_version);
         rc = ERROR_FAIL;
         goto error;
     }
 
-    *env = get_hotplug_env(gc, script, dev);
-    if (!*env) {
+    GCNEW_ARRAY(*args, arg_arraysize);
+    (*args)[nr++] = script;
+    saction = libxl__device_hotplug_action(gc, action);
+    if (!saction) {
         rc = ERROR_FAIL;
         goto error;
     }
-
-    const int arraysize = 3;
-    GCNEW_ARRAY(*args, arraysize);
-    (*args)[nr++] = script;
-    (*args)[nr++] = action == DEVICE_CONNECT ? "add" :
"remove";
+    (*args)[nr++] = saction;
     (*args)[nr++] = NULL;
-    assert(nr == arraysize);
+    assert(nr == arg_arraysize);
 
     rc = 1;
 
@@ -226,7 +262,7 @@ error:
 int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
                                    char ***args, char ***env,
                                    libxl__device_action action,
-                                   int num_exec)
+                                   int num_exec, int hotplug_version)
 {
     char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
     int rc;
@@ -243,7 +279,7 @@ int libxl__get_hotplug_script_info(libxl__gc *gc,
libxl__device *dev,
             rc = 0;
             goto out;
         }
-        rc = libxl__hotplug_disk(gc, dev, args, env, action);
+        rc = libxl__hotplug_disk(gc, dev, args, env, action, hotplug_version);
         break;
     case LIBXL__DEVICE_KIND_VIF:
         /*
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9587833..8061e7a 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -62,7 +62,7 @@ out:
 int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
                                    char ***args, char ***env,
                                    libxl__device_action action,
-                                   int num_exec)
+                                   int num_exec, int hotplug_version)
 {
     char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
     int rc;
-- 
1.7.7.5 (Apple Git-26)


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

This new diskspec parameters will set script to the value passed in
method, and hotplug_version to 1.

This patch adds the basic support to handle this new scripts, by
adding the prepare/unprepare private libxl functions, and modifying
the current domain creation/destruction to call this functions at the
appropriate spots.

This patch also adds some helpers, that will be used here and in later
patches.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl.c          |  131 +++++++++++++++++++++++++++++++++++++++++-
 tools/libxl/libxl_create.c   |   62 +++++++++++++++++++-
 tools/libxl/libxl_device.c   |   80 +++++++++++++++++++------
 tools/libxl/libxl_internal.h |   34 +++++++++++
 tools/libxl/libxl_types.idl  |    1 +
 tools/libxl/libxlu_disk_l.l  |    2 +
 6 files changed, 288 insertions(+), 22 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 8d921bc..e141379 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1999,6 +1999,108 @@ int libxl__device_from_disk(libxl__gc *gc, uint32_t
domid,
     return 0;
 }
 
+void libxl__device_disk_prepare(libxl__egc *egc, uint32_t domid,
+                                libxl_device_disk *disk,
+                                libxl__ao_device *aodev)
+{
+    STATE_AO_GC(aodev->ao);
+    char *hotplug_path;
+    char *script_path;
+    int rc;
+    xs_transaction_t t = XBT_NULL;
+
+    if (disk->hotplug_version == 0) {
+        aodev->callback(egc, aodev);
+        return;
+    }
+
+    GCNEW(aodev->dev);
+    rc = libxl__device_from_disk(gc, domid, disk, aodev->dev);
+    if (rc != 0) {
+        LOG(ERROR, "Invalid or unsupported virtual disk identifier
%s",
+                   disk->vdev);
+        goto error;
+    }
+
+    script_path = libxl__abs_path(gc, disk->script,
+                                  libxl__xen_script_dir_path());
+
+    hotplug_path = libxl__device_xs_hotplug_path(gc, aodev->dev);
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto error;
+
+        rc = libxl__xs_write_checked(gc, t,
+                                 GCSPRINTF("%s/params",
hotplug_path),
+                                 disk->pdev_path);
+        if (rc)
+            goto error;
+
+        rc = libxl__xs_write_checked(gc, t,
+                                 GCSPRINTF("%s/script",
hotplug_path),
+                                 script_path);
+        if (rc)
+            goto error;
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc < 0) goto error;
+    }
+
+    aodev->action = DEVICE_PREPARE;
+    aodev->hotplug_version = disk->hotplug_version;
+    libxl__device_hotplug(egc, aodev);
+    return;
+
+error:
+    assert(rc);
+    libxl__xs_transaction_abort(gc, &t);
+    aodev->rc = rc;
+    aodev->callback(egc, aodev);
+    return;
+}
+
+void libxl__device_disk_unprepare(libxl__egc *egc, uint32_t domid,
+                                  libxl_device_disk *disk,
+                                  libxl__ao_device *aodev)
+{
+    STATE_AO_GC(aodev->ao);
+    char *hotplug_path;
+    int rc;
+
+    if (disk->hotplug_version == 0) {
+        aodev->callback(egc, aodev);
+        return;
+    }
+
+    GCNEW(aodev->dev);
+    rc = libxl__device_from_disk(gc, domid, disk, aodev->dev);
+    if (rc != 0) {
+        LOG(ERROR, "Invalid or unsupported virtual disk identifier
%s",
+                   disk->vdev);
+        goto error;
+    }
+
+    hotplug_path = libxl__device_xs_hotplug_path(gc, aodev->dev);
+    if (!libxl__xs_read(gc, XBT_NULL, hotplug_path)) {
+        LOG(DEBUG, "unable to unprepare device because %s doesn't
exist",
+                   hotplug_path);
+        aodev->callback(egc, aodev);
+        return;
+    }
+
+    aodev->action = DEVICE_UNPREPARE;
+    aodev->hotplug_version = disk->hotplug_version;
+    libxl__device_hotplug(egc, aodev);
+    return;
+
+error:
+    assert(rc);
+    aodev->rc = rc;
+    aodev->callback(egc, aodev);
+    return;
+}
+
 /* Specific function called directly only by local disk attach,
  * all other users should instead use the regular
  * libxl__device_disk_add wrapper
@@ -2058,8 +2160,15 @@ static void device_disk_add(libxl__egc *egc, uint32_t
domid,
                 dev = disk->pdev_path;
 
         do_backend_phy:
-                flexarray_append(back, "params");
-                flexarray_append(back, dev);
+                if (disk->hotplug_version == 0) {
+                    /*
+                     * If the new hotplug version is used params is
+                     * stored under a private path, since it can contain
+                     * data that the guest should not see.
+                     */
+                    flexarray_append(back, "params");
+                    flexarray_append(back, dev);
+                }
 
                 script = libxl__abs_path(gc, disk->script?:
"block",
                                          libxl__xen_script_dir_path());
@@ -2152,6 +2261,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t
domid,
 
     aodev->dev = device;
     aodev->action = DEVICE_CONNECT;
+    aodev->hotplug_version = disk->hotplug_version;
     libxl__wait_device_connection(egc, aodev);
 
     rc = 0;
@@ -2245,6 +2355,7 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t
domid,
     GC_INIT(ctx);
     char *dompath, *path;
     int devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
+    libxl__device dev;
     int rc = ERROR_FAIL;
 
     if (devid < 0)
@@ -2263,6 +2374,22 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t
domid,
         goto out;
 
     rc = libxl__device_disk_from_xs_be(gc, path, disk);
+    if (rc) {
+        LOG(ERROR, "unable to parse disk device from path %s", path);
+        goto out;
+    }
+
+    /* Check if the device is using the new hotplug interface */
+    rc = libxl__device_from_disk(gc, domid, disk, &dev);
+    if (rc) {
+        LOG(ERROR, "invalid or unsupported virtual disk identifier
%s",
+                    disk->vdev);
+        goto out;
+    }
+    path = libxl__device_xs_hotplug_path(gc, &dev);
+    if (libxl__xs_read(gc, XBT_NULL, path))
+        disk->hotplug_version = 1;
+
 out:
     GC_FREE;
     return rc;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 9d20086..c176967 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -591,6 +591,10 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
  */
 
 /* Event callbacks, in this order: */
+
+static void domcreate_launch_bootloader(libxl__egc *egc,
+                                        libxl__multidev *multidev,
+                                        int ret);
 static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int rc);
@@ -627,6 +631,12 @@ static void domcreate_destruction_cb(libxl__egc *egc,
                                      libxl__domain_destroy_state *dds,
                                      int rc);
 
+/* If creation is not successful, this callback will be executed
+ * when devices have been unprepared */
+static void domcreate_unprepare_cb(libxl__egc *egc,
+                                   libxl__multidev *multidev,
+                                   int ret);
+
 static void initiate_domain_create(libxl__egc *egc,
                                    libxl__domain_create_state *dcs)
 {
@@ -637,7 +647,6 @@ static void initiate_domain_create(libxl__egc *egc,
 
     /* convenience aliases */
     libxl_domain_config *const d_config = dcs->guest_config;
-    const int restore_fd = dcs->restore_fd;
     memset(&dcs->build_state, 0, sizeof(dcs->build_state));
 
     domid = 0;
@@ -670,6 +679,33 @@ static void initiate_domain_create(libxl__egc *egc,
         if (ret) goto error_out;
     }
 
+    libxl__multidev_begin(ao, &dcs->multidev);
+    dcs->multidev.callback = domcreate_launch_bootloader;
+    libxl__prepare_disks(egc, ao, domid, d_config, &dcs->multidev);
+    libxl__multidev_prepared(egc, &dcs->multidev, 0);
+    return;
+
+error_out:
+    assert(ret);
+    domcreate_complete(egc, dcs, ret);
+}
+
+static void domcreate_launch_bootloader(libxl__egc *egc,
+                                        libxl__multidev *multidev,
+                                        int ret)
+{
+    libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
+    STATE_AO_GC(dcs->ao);
+
+    /* convenience aliases */
+    const int restore_fd = dcs->restore_fd;
+    libxl_domain_config *const d_config = dcs->guest_config;
+
+    if (ret) {
+        LOG(ERROR, "unable to prepare devices");
+        goto error_out;
+    }
+
     dcs->bl.ao = ao;
     libxl_device_disk *bootdisk          d_config->num_disks > 0 ?
&d_config->disks[0] : NULL;
@@ -1202,11 +1238,35 @@ static void domcreate_destruction_cb(libxl__egc *egc,
 {
     STATE_AO_GC(dds->ao);
     libxl__domain_create_state *dcs = CONTAINER_OF(dds, *dcs, dds);
+    uint32_t domid = dcs->guest_domid;
+    libxl_domain_config *const d_config = dcs->guest_config;
 
     if (rc)
         LOG(ERROR, "unable to destroy domain %u following failed
creation",
                    dds->domid);
 
+    /*
+     * We might have devices that have been prepared, but with no
+     * frontend xenstore entries, so domain destruction fails to
+     * find them, that is why we have to unprepare them manually.
+     */
+    libxl__multidev_begin(ao, &dcs->multidev);
+    dcs->multidev.callback = domcreate_unprepare_cb;
+    libxl__unprepare_disks(egc, ao, domid, d_config, &dcs->multidev);
+    libxl__multidev_prepared(egc, &dcs->multidev, 0);
+    return;
+}
+
+static void domcreate_unprepare_cb(libxl__egc *egc,
+                                   libxl__multidev *multidev,
+                                   int ret)
+{
+    libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
+    STATE_AO_GC(dcs->ao);
+
+    if (ret)
+        LOG(ERROR, "unable to unprepare devices");
+
     dcs->callback(egc, dcs, ERROR_FAIL, dcs->guest_domid);
 }
 
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 85c9953..ce99d99 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -521,18 +521,21 @@ void libxl__multidev_prepared(libxl__egc *egc,
 
 /******************************************************************************/
 
-/* Macro for defining the functions that will add a bunch of disks when
+/* Macro for defining the functions that will operate a bunch of devices when
  * inside an async op with multidev.
  * This macro is added to prevent repetition of code.
  *
  * The following functions are defined:
+ * libxl__prepare_disks
+ * libxl__unprepare_disks
  * libxl__add_disks
  * libxl__add_nics
  * libxl__add_vtpms
  */
 
-#define DEFINE_DEVICES_ADD(type)                                        \
-    void libxl__add_##type##s(libxl__egc *egc, libxl__ao *ao, uint32_t domid, \
+#define DEFINE_DEVICES_FUNC(type, op)                                   \
+    void libxl__##op##_##type##s(libxl__egc *egc, libxl__ao *ao,        \
+                              uint32_t domid,                           \
                               libxl_domain_config *d_config,            \
                               libxl__multidev *multidev)                \
     {                                                                   \
@@ -540,16 +543,19 @@ void libxl__multidev_prepared(libxl__egc *egc,
         int i;                                                          \
         for (i = 0; i < d_config->num_##type##s; i++) {                 \
             libxl__ao_device *aodev = libxl__multidev_prepare(multidev);  \
-            libxl__device_##type##_add(egc, domid,
&d_config->type##s[i], \
+            libxl__device_##type##_##op(egc, domid,
&d_config->type##s[i], \
                                        aodev);                          \
         }                                                               \
     }
 
-DEFINE_DEVICES_ADD(disk)
-DEFINE_DEVICES_ADD(nic)
-DEFINE_DEVICES_ADD(vtpm)
+DEFINE_DEVICES_FUNC(disk, add)
+DEFINE_DEVICES_FUNC(nic, add)
+DEFINE_DEVICES_FUNC(vtpm, add)
 
-#undef DEFINE_DEVICES_ADD
+DEFINE_DEVICES_FUNC(disk, prepare)
+DEFINE_DEVICES_FUNC(disk, unprepare)
+
+#undef DEFINE_DEVICES_FUNC
 
 /******************************************************************************/
 
@@ -557,6 +563,7 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
 {
     const char *be_path = libxl__device_backend_path(gc, dev);
     const char *fe_path = libxl__device_frontend_path(gc, dev);
+    const char *hotplug_path = libxl__device_xs_hotplug_path(gc, dev);
     const char *tapdisk_path = GCSPRINTF("%s/%s", be_path,
"tapdisk-params");
     const char *tapdisk_params;
     xs_transaction_t t = 0;
@@ -572,6 +579,7 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
 
         libxl__xs_path_cleanup(gc, t, fe_path);
         libxl__xs_path_cleanup(gc, t, be_path);
+        libxl__xs_path_cleanup(gc, t, hotplug_path);
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
@@ -644,6 +652,14 @@ void libxl__devices_destroy(libxl__egc *egc,
libxl__devices_remove_state *drs)
                     continue;
                 }
                 aodev = libxl__multidev_prepare(multidev);
+                /*
+                 * Check if the device has hotplug entries in xenstore,
+                 * which would mean it's using the new hotplug calling
+                 * convention.
+                 */
+                path = libxl__device_xs_hotplug_path(gc, dev);
+                if (libxl__xs_read(gc, XBT_NULL, path))
+                    aodev->hotplug_version = 1;
                 aodev->action = DEVICE_DISCONNECT;
                 aodev->dev = dev;
                 aodev->force = drs->force;
@@ -696,8 +712,6 @@ static void device_backend_callback(libxl__egc *egc,
libxl__ev_devstate *ds,
 static void device_backend_cleanup(libxl__gc *gc,
                                    libxl__ao_device *aodev);
 
-static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev);
-
 static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
                                       const struct timeval *requested_abs);
 
@@ -732,7 +746,7 @@ void libxl__wait_device_connection(libxl__egc *egc,
libxl__ao_device *aodev)
          * If Qemu is running, it will set the state of the device to
          * 4 directly, without waiting in state 2 for any hotplug execution.
          */
-        device_hotplug(egc, aodev);
+        libxl__device_hotplug(egc, aodev);
         return;
     }
 
@@ -864,7 +878,7 @@ static void device_qemu_timeout(libxl__egc *egc,
libxl__ev_time *ev,
     rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
     if (rc) goto out;
 
-    device_hotplug(egc, aodev);
+    libxl__device_hotplug(egc, aodev);
     return;
 
 out:
@@ -893,7 +907,7 @@ static void device_backend_callback(libxl__egc *egc,
libxl__ev_devstate *ds,
         goto out;
     }
 
-    device_hotplug(egc, aodev);
+    libxl__device_hotplug(egc, aodev);
     return;
 
 out:
@@ -908,7 +922,7 @@ static void device_backend_cleanup(libxl__gc *gc,
libxl__ao_device *aodev)
     libxl__ev_devstate_cancel(gc, &aodev->backend_ds);
 }
 
-static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
+void libxl__device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
 {
     STATE_AO_GC(aodev->ao);
     char *be_path = libxl__device_backend_path(gc, aodev->dev);
@@ -1012,10 +1026,13 @@ static void device_hotplug_child_death_cb(libxl__egc
*egc,
         if (hotplug_error)
             LOG(ERROR, "script: %s", hotplug_error);
         aodev->rc = ERROR_FAIL;
-        if (aodev->action == DEVICE_CONNECT)
+        if (aodev->action == DEVICE_CONNECT ||
+            aodev->action == DEVICE_PREPARE ||
+            aodev->action == DEVICE_LOCALATTACH)
             /*
-             * Only fail on device connection, on disconnection
-             * ignore error, and continue with the remove process
+             * Only fail on device connection, prepare or localattach,
+             * on other cases ignore error, and continue with the remove
+             * process.
              */
              goto error;
     }
@@ -1025,7 +1042,7 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
      * device_hotplug_done breaking the loop.
      */
     aodev->num_exec++;
-    device_hotplug(egc, aodev);
+    libxl__device_hotplug(egc, aodev);
 
     return;
 
@@ -1041,8 +1058,33 @@ static void device_hotplug_done(libxl__egc *egc,
libxl__ao_device *aodev)
 
     device_hotplug_clean(gc, aodev);
 
-    /* Clean xenstore if it's a disconnection */
     if (aodev->action == DEVICE_DISCONNECT) {
+        switch (aodev->hotplug_version) {
+        case 0:
+            /* Clean xenstore if it's a disconnection */
+            rc = libxl__device_destroy(gc, aodev->dev);
+            if (!aodev->rc)
+                aodev->rc = rc;
+            break;
+        case 1:
+            /*
+             * Chain unprepare hotplug execution
+             * after disconnection of device.
+             */
+            aodev->num_exec = 0;
+            aodev->action = DEVICE_UNPREPARE;
+            libxl__device_hotplug(egc, aodev);
+            return;
+        default:
+            LOG(ERROR, "unknown hotplug script version (%d)",
+                       aodev->hotplug_version);
+            if (!aodev->rc)
+                aodev->rc = ERROR_FAIL;
+            break;
+        }
+    }
+    /* Clean hotplug xenstore path */
+    if (aodev->action == DEVICE_UNPREPARE) {
         rc = libxl__device_destroy(gc, aodev->dev);
         if (!aodev->rc)
             aodev->rc = rc;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c41e608..16907ff 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2018,6 +2018,12 @@ struct libxl__multidev {
 _hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
                                     libxl_device_disk *disk,
                                     libxl__ao_device *aodev);
+_hidden void libxl__device_disk_prepare(libxl__egc *egc, uint32_t domid,
+                                        libxl_device_disk *disk,
+                                        libxl__ao_device *aodev);
+_hidden void libxl__device_disk_unprepare(libxl__egc *egc, uint32_t domid,
+                                          libxl_device_disk *disk,
+                                          libxl__ao_device *aodev);
 
 /* AO operation to connect a nic device */
 _hidden void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
@@ -2094,6 +2100,19 @@ _hidden int libxl__get_hotplug_script_info(libxl__gc *gc,
libxl__device *dev,
                                            libxl__device_action action,
                                            int num_exec, int hotplug_version);
 
+/*
+ * libxl__device_hotplug runs the hotplug script associated
+ * with the device passed in aodev->dev.
+ *
+ * The libxl__ao_device passed to this function should be
+ * prepared using libxl__prepare_ao_device prior to calling
+ * this function.
+ *
+ * Once finished, aodev->callback will be executed.
+ */
+_hidden void libxl__device_hotplug(libxl__egc *egc,
+                                   libxl__ao_device *aodev);
+
 /*----- local disk attach: attach a disk locally to run the bootloader -----*/
 
 typedef struct libxl__disk_local_state libxl__disk_local_state;
@@ -2467,6 +2486,21 @@ _hidden void libxl__add_disks(libxl__egc *egc, libxl__ao
*ao, uint32_t domid,
                               libxl_domain_config *d_config,
                               libxl__multidev *multidev);
 
+_hidden void libxl__prepare_disks(libxl__egc *egc, libxl__ao *ao,
+                                  uint32_t domid,
+                                  libxl_domain_config *d_config,
+                                  libxl__multidev *multidev);
+
+_hidden void libxl__prepare_undisks(libxl__egc *egc, libxl__ao *ao,
+                                    uint32_t domid,
+                                    libxl_domain_config *d_config,
+                                    libxl__multidev *multidev);
+
+_hidden void libxl__unprepare_disks(libxl__egc *egc, libxl__ao *ao,
+                                    uint32_t domid,
+                                    libxl_domain_config *d_config,
+                                    libxl__multidev *multidev);
+
 _hidden void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
                              libxl_domain_config *d_config,
                              libxl__multidev *multidev);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 7eac4a8..272ff54 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -366,6 +366,7 @@ libxl_device_disk = Struct("device_disk", [
     ("removable", integer),
     ("readwrite", integer),
     ("is_cdrom", integer),
+    ("hotplug_version", integer),
     ])
 
 libxl_device_nic = Struct("device_nic", [
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index bee16a1..4486c1a 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -172,6 +172,8 @@ backendtype=[^,]*,? { STRIP(',');
setbackendtype(DPC,FROMEQUALS); }
 
 vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev,
FROMEQUALS); }
 script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script,
FROMEQUALS); }
+method=[^,]*,?	{ STRIP(','); SAVESTRING("script", script,
FROMEQUALS);
+		  DPC->disk->hotplug_version = 1; }
 
  /* the target magic parameter, eats the rest of the string */
 
-- 
1.7.7.5 (Apple Git-26)


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

Add public functions for the prepare/unprepare disk operations.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl.c |   26 +++++++++++++++-----------
 tools/libxl/libxl.h |    8 ++++++++
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e141379..82034f1 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1688,19 +1688,19 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid,
int autopass)
 /******************************************************************************/
 
 /* generic callback for devices that only need to set ao_complete */
-static void device_addrm_aocomplete(libxl__egc *egc, libxl__ao_device *aodev)
+static void device_aocomplete(libxl__egc *egc, libxl__ao_device *aodev)
 {
     STATE_AO_GC(aodev->ao);
 
     if (aodev->rc) {
         if (aodev->dev) {
             LOG(ERROR, "unable to %s %s with id %u",
-                        aodev->action == DEVICE_CONNECT ? "add" :
"remove",
+                        libxl__device_hotplug_action(gc, aodev->action),
                         libxl__device_kind_to_string(aodev->dev->kind),
                         aodev->dev->devid);
         } else {
             LOG(ERROR, "unable to %s device",
-                       aodev->action == DEVICE_CONNECT ? "add" :
"remove");
+                       libxl__device_hotplug_action(gc, aodev->action));
         }
         goto out;
     }
@@ -3480,7 +3480,7 @@ out:
         libxl__prepare_ao_device(ao, aodev);                            \
         aodev->action = DEVICE_DISCONNECT;                              \
         aodev->dev = device;                                            \
-        aodev->callback = device_addrm_aocomplete;                      \
+        aodev->callback = device_aocomplete;                            \
         aodev->force = f;                                               \
         libxl__initiate_device_remove(egc, aodev);                      \
                                                                         \
@@ -3521,10 +3521,12 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
  * libxl_device_disk_add
  * libxl_device_nic_add
  * libxl_device_vtpm_add
+ * libxl_device_disk_prepare
+ * libxl_device_disk_unprepare
  */
 
-#define DEFINE_DEVICE_ADD(type)                                         \
-    int libxl_device_##type##_add(libxl_ctx *ctx,                       \
+#define DEFINE_DEVICE_FUNC(type, op)                                    \
+    int libxl_device_##type##_##op(libxl_ctx *ctx,                      \
         uint32_t domid, libxl_device_##type *type,                      \
         const libxl_asyncop_how *ao_how)                                \
     {                                                                   \
@@ -3533,8 +3535,8 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
                                                                         \
         GCNEW(aodev);                                                   \
         libxl__prepare_ao_device(ao, aodev);                            \
-        aodev->callback = device_addrm_aocomplete;                      \
-        libxl__device_##type##_add(egc, domid, type, aodev);            \
+        aodev->callback = device_aocomplete;                            \
+        libxl__device_##type##_##op(egc, domid, type, aodev);           \
                                                                         \
         return AO_INPROGRESS;                                           \
     }
@@ -3542,13 +3544,15 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
 /* Define alladd functions and undef the macro */
 
 /* disk */
-DEFINE_DEVICE_ADD(disk)
+DEFINE_DEVICE_FUNC(disk, add)
+DEFINE_DEVICE_FUNC(disk, prepare)
+DEFINE_DEVICE_FUNC(disk, unprepare)
 
 /* nic */
-DEFINE_DEVICE_ADD(nic)
+DEFINE_DEVICE_FUNC(nic, add)
 
 /* vtpm */
-DEFINE_DEVICE_ADD(vtpm)
+DEFINE_DEVICE_FUNC(vtpm, add)
 
 #undef DEFINE_DEVICE_ADD
 
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index e2ba549..f3426f7 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -707,6 +707,10 @@ void libxl_vtpminfo_list_free(libxl_vtpminfo *, int
nr_vtpms);
  */
 
 /* Disks */
+int libxl_device_disk_prepare(libxl_ctx *ctx, uint32_t domid,
+                              libxl_device_disk *disk,
+                              const libxl_asyncop_how *ao_how)
+                              LIBXL_EXTERNAL_CALLERS_ONLY;
 int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
                           libxl_device_disk *disk,
                           const libxl_asyncop_how *ao_how)
@@ -719,6 +723,10 @@ int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t
domid,
                               libxl_device_disk *disk,
                               const libxl_asyncop_how *ao_how)
                               LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_disk_unprepare(libxl_ctx *ctx, uint32_t domid,
+                                libxl_device_disk *disk,
+                                const libxl_asyncop_how *ao_how)
+                                LIBXL_EXTERNAL_CALLERS_ONLY;
 
 libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int
*num);
 int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
-- 
1.7.7.5 (Apple Git-26)


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

Add a specific macro to generate libxl_device_disk_{remove/destroy}
functions that passes the hotplug_version field down to the aodev
struct.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl.c |   35 +++++++++++++++++++++++++++++++++--
 1 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 82034f1..29b2765 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3489,11 +3489,41 @@ out:
         return AO_INPROGRESS;                                           \
     }
 
+/* Specific for disk devices, that have to set aodev->hotplug_version */
+#define DEFINE_DISK_REMOVE(type, removedestroy, f)                      \
+    int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,           \
+        uint32_t domid, libxl_device_##type *type,                      \
+        const libxl_asyncop_how *ao_how)                                \
+    {                                                                   \
+        AO_CREATE(ctx, domid, ao_how);                                  \
+        libxl__device *device;                                          \
+        libxl__ao_device *aodev;                                        \
+        int rc;                                                         \
+                                                                        \
+        GCNEW(device);                                                  \
+        rc = libxl__device_from_##type(gc, domid, type, device);        \
+        if (rc != 0) goto out;                                          \
+                                                                        \
+        GCNEW(aodev);                                                   \
+        libxl__prepare_ao_device(ao, aodev);                            \
+        aodev->action = DEVICE_DISCONNECT;                              \
+        aodev->dev = device;                                            \
+        aodev->callback = device_aocomplete;                            \
+        aodev->force = f;                                               \
+        aodev->hotplug_version = type->hotplug_version;                 \
+        LOG(DEBUG, "hotplug version: %d", aodev->hotplug_version);
\
+        libxl__initiate_device_remove(egc, aodev);                      \
+                                                                        \
+    out:                                                                \
+        if (rc) return AO_ABORT(rc);                                    \
+        return AO_INPROGRESS;                                           \
+    }
+
 /* Define all remove/destroy functions and undef the macro */
 
 /* disk */
-DEFINE_DEVICE_REMOVE(disk, remove, 0)
-DEFINE_DEVICE_REMOVE(disk, destroy, 1)
+DEFINE_DISK_REMOVE(disk, remove, 0)
+DEFINE_DISK_REMOVE(disk, destroy, 1)
 
 /* nic */
 DEFINE_DEVICE_REMOVE(nic, remove, 0)
@@ -3512,6 +3542,7 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
 DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
 DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
 
+#undef DEFINE_DISK_REMOVE
 #undef DEFINE_DEVICE_REMOVE
 
 /******************************************************************************/
-- 
1.7.7.5 (Apple Git-26)


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

Call prepare before attaching a block device.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 4b75fc3..781201a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5631,6 +5631,11 @@ int main_blockattach(int argc, char **argv)
         return 0;
     }
 
+    if (libxl_device_disk_prepare(ctx, fe_domid, &disk, 0)) {
+        fprintf(stderr, "libxl_device_disk_prepare failed.\n");
+        return 1;
+    }
+
     if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
         fprintf(stderr, "libxl_device_disk_add failed.\n");
     }
-- 
1.7.7.5 (Apple Git-26)


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

Adds support for locally attaching disks that use the new hotplug
script interface, by calling the localattach/localdetach operations
and returning a block device that can be used to execute pygrub.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl.c            |   88 ++++++++++++++++++++++++++++++++++------
 tools/libxl/libxl_bootloader.c |    1 +
 tools/libxl/libxl_internal.h   |    1 +
 3 files changed, 77 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 29b2765..b21eefc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2657,6 +2657,7 @@ void libxl__device_disk_local_initiate_attach(libxl__egc
*egc,
     const libxl_device_disk *in_disk = dls->in_disk;
     libxl_device_disk *disk = &dls->disk;
     const char *blkdev_start = dls->blkdev_start;
+    uint32_t domid = dls->domid;
 
     assert(in_disk->pdev_path);
 
@@ -2673,6 +2674,23 @@ void libxl__device_disk_local_initiate_attach(libxl__egc
*egc,
         case LIBXL_DISK_BACKEND_PHY:
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk
%s",
                        disk->pdev_path);
+            if (disk->hotplug_version != 0) {
+                disk->vdev = libxl__strdup(gc, in_disk->vdev);
+                libxl__prepare_ao_device(ao, &dls->aodev);
+                GCNEW(dls->aodev.dev);
+                rc = libxl__device_from_disk(gc, domid, disk,
+                                             dls->aodev.dev);
+                if (rc != 0) {
+                    LOG(ERROR, "Invalid or unsupported virtual disk "
+                               "identifier %s", disk->vdev);
+                    goto out;
+                }
+                dls->aodev.action = DEVICE_LOCALATTACH;
+                dls->aodev.hotplug_version = disk->hotplug_version;
+                dls->aodev.callback = local_device_attach_cb;
+                libxl__device_hotplug(egc, &dls->aodev);
+                return;
+            }
             dev = disk->pdev_path;
             break;
         case LIBXL_DISK_BACKEND_TAP:
@@ -2735,7 +2753,8 @@ static void local_device_attach_cb(libxl__egc *egc,
libxl__ao_device *aodev)
 {
     STATE_AO_GC(aodev->ao);
     libxl__disk_local_state *dls = CONTAINER_OF(aodev, *dls, aodev);
-    char *dev = NULL, *be_path = NULL;
+    char *dev = NULL, *be_path = NULL, *hotplug_path;
+    const char *pdev = NULL;
     int rc;
     libxl__device device;
     libxl_device_disk *disk = &dls->disk;
@@ -2748,20 +2767,45 @@ static void local_device_attach_cb(libxl__egc *egc,
libxl__ao_device *aodev)
                     aodev->dev->devid);
         goto out;
     }
+    switch (disk->backend) {
+    case LIBXL_DISK_BACKEND_PHY:
+        /* Fetch hotplug output to obtain the block device */
+        rc = libxl__device_from_disk(gc, dls->domid, disk, &device);
+        if (rc)
+            goto out;
+        hotplug_path = libxl__device_xs_hotplug_path(gc, &device);
+        rc = libxl__xs_read_checked(gc, XBT_NULL,
+                                    GCSPRINTF("%s/pdev",
hotplug_path),
+                                    &pdev);
+        if (rc)
+            goto out;
+        if (!pdev) {
+            rc = ERROR_FAIL;
+            goto out;
+        }
+        LOG(DEBUG, "attached local block device %s", pdev);
+        dls->diskpath = libxl__strdup(gc, pdev);
+        break;
+    case LIBXL_DISK_BACKEND_QDISK:
+        dev = GCSPRINTF("/dev/%s", disk->vdev);
+        LOG(DEBUG, "locally attaching qdisk %s", dev);
 
-    dev = GCSPRINTF("/dev/%s", disk->vdev);
-    LOG(DEBUG, "locally attaching qdisk %s", dev);
+        rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk,
&device);
+        if (rc < 0)
+            goto out;
+        be_path = libxl__device_backend_path(gc, &device);
+        rc = libxl__wait_for_backend(gc, be_path, "4");
+        if (rc < 0)
+            goto out;
 
-    rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk, &device);
-    if (rc < 0)
-        goto out;
-    be_path = libxl__device_backend_path(gc, &device);
-    rc = libxl__wait_for_backend(gc, be_path, "4");
-    if (rc < 0)
+        if (dev != NULL)
+            dls->diskpath = libxl__strdup(gc, dev);
+        break;
+    default:
+        LOG(ERROR, "invalid disk backend for local attach callback");
+        rc = ERROR_FAIL;
         goto out;
-
-    if (dev != NULL)
-        dls->diskpath = libxl__strdup(gc, dev);
+    }
 
     dls->callback(egc, dls, 0);
     return;
@@ -2785,11 +2829,29 @@ void libxl__device_disk_local_initiate_detach(libxl__egc
*egc,
     libxl_device_disk *disk = &dls->disk;
     libxl__device *device;
     libxl__ao_device *aodev = &dls->aodev;
+    uint32_t domid = dls->domid;
     libxl__prepare_ao_device(ao, aodev);
 
     if (!dls->diskpath) goto out;
 
     switch (disk->backend) {
+        case LIBXL_DISK_BACKEND_PHY:
+            if (disk->hotplug_version != 0) {
+                GCNEW(aodev->dev);
+                rc = libxl__device_from_disk(gc, domid, disk,
+                                             aodev->dev);
+                if (rc != 0) {
+                    LOG(ERROR, "Invalid or unsupported virtual disk "
+                               "identifier %s", disk->vdev);
+                    goto out;
+                }
+                aodev->action = DEVICE_LOCALDETACH;
+                aodev->hotplug_version = disk->hotplug_version;
+                aodev->callback = local_device_detach_cb;
+                libxl__device_hotplug(egc, aodev);
+                return;
+            }
+            break;
         case LIBXL_DISK_BACKEND_QDISK:
             if (disk->vdev != NULL) {
                 GCNEW(device);
@@ -2828,7 +2890,7 @@ static void local_device_detach_cb(libxl__egc *egc,
libxl__ao_device *aodev)
 
     if (aodev->rc) {
         LOGE(ERROR, "unable to %s %s with id %u",
-                    aodev->action == DEVICE_CONNECT ? "add" :
"remove",
+                    libxl__device_hotplug_action(gc, aodev->action),
                     libxl__device_kind_to_string(aodev->dev->kind),
                     aodev->dev->devid);
         goto out;
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index e103ee9..ff2e48c 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -381,6 +381,7 @@ void libxl__bootloader_run(libxl__egc *egc,
libxl__bootloader_state *bl)
     bl->dls.ao = ao;
     bl->dls.in_disk = bl->disk;
     bl->dls.blkdev_start = info->blkdev_start;
+    bl->dls.domid = domid;
     bl->dls.callback = bootloader_disk_attached_cb;
     libxl__device_disk_local_initiate_attach(egc, &bl->dls);
     return;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 16907ff..67c56a6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2130,6 +2130,7 @@ struct libxl__disk_local_state {
     libxl_device_disk disk;
     const char *blkdev_start;
     libxl__disk_local_state_callback *callback;
+    uint32_t domid;
     /* filled by libxl__device_disk_local_initiate_attach */
     char *diskpath;
     /* private for implementation of local detach */
-- 
1.7.7.5 (Apple Git-26)


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

Reads the results of the hotplug script execution and changes
pdev_path libxl_device_disk to point to the block device. This will be
used later when Qemu is launched.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl_create.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index c176967..1d5410f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -964,6 +964,10 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
     libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
     STATE_AO_GC(dcs->ao);
     int i;
+    char *hotplug_path;
+    const char *pdev;
+    libxl__device dev;
+    libxl_device_disk *disk;
 
     /* convenience aliases */
     const uint32_t domid = dcs->guest_domid;
@@ -975,6 +979,35 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
         goto error_out;
     }
 
+    /*
+     * If using the new hotplug interface change disks pdev_path
+     * to point to the real physical path, so it can be used by Qemu.
+     */
+    for (i = 0; i < d_config->num_disks; i++) {
+        disk = &d_config->disks[i];
+        if (disk->hotplug_version != 0) {
+            /* Update pdev_path */
+            ret = libxl__device_from_disk(gc, domid, disk, &dev);
+            if (ret != 0) {
+                LOG(ERROR, "Invalid or unsupported virtual disk identifier
%s",
+                           disk->vdev);
+                goto error_out;
+            }
+            hotplug_path = libxl__device_xs_hotplug_path(gc, &dev);
+            ret = libxl__xs_read_checked(gc, XBT_NULL,
+                                         GCSPRINTF("%s/pdev",
hotplug_path),
+                                         &pdev);
+            if (ret)
+                goto error_out;
+            if (!pdev) {
+                ret = ERROR_FAIL;
+                goto error_out;
+            }
+            free(disk->pdev_path);
+            disk->pdev_path = libxl__strdup(NOGC, pdev);
+        }
+    }
+
     for (i = 0; i < d_config->b_info.num_ioports; i++) {
         libxl_ioport_range *io = &d_config->b_info.ioports[i];
 
-- 
1.7.7.5 (Apple Git-26)


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

Mention the new diskspec parameter and add a document explaining the
new hotplug interface.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 docs/misc/libxl-hotplug-interface.txt |  156 +++++++++++++++++++++++++++++++++
 docs/misc/xl-disk-configuration.txt   |   11 +++
 2 files changed, 167 insertions(+), 0 deletions(-)
 create mode 100644 docs/misc/libxl-hotplug-interface.txt

diff --git a/docs/misc/libxl-hotplug-interface.txt
b/docs/misc/libxl-hotplug-interface.txt
new file mode 100644
index 0000000..5187e44
--- /dev/null
+++ b/docs/misc/libxl-hotplug-interface.txt
@@ -0,0 +1,156 @@
+                     -----------------------
+                     LIBXL HOTPLUG INTERFACE
+                     -----------------------
+
+This document specifies the new libxl hotplug interface. This new
+interface has been designed to operate better with complex hotplug
+scripts, that need to perform several operations and can take a
+considerable time to execute.
+
+Hotplug scripts are expected to take a parameter, passed by the caller
+and provide a block device as output, that will be attached to the guest.
+
+
+
+====================+ENVIRONMENT VARIABLES
+====================+
+
+The following environment variables will be set before calling
+the hotplug script.
+
+
+HOTPLUG_PATH
+------------
+
+Points to the xenstore directory that holds information relative
+to this hotplug script. At present only one parameter is passed by
+the toolstack, the "params" xenstore entry which contains the
"target"
+line specified in the diskspec xl disk configuration (pdev_path in
+libxl_device_disk struct).
+
+This xenstore directory will be used to communicate between the
+hotplug script and the toolstack, and it can also be used by the
+hotplug script to store temporary information. This directory is created
+before calling the "prepare" operation, and the toolstack guarantees
+that it will not be removed before the "unprepare" operation has been
+finished. After that, the toolstack will take the appropriate actions
+to remove it. The toolstack guarantees that HOTPLUG_PATH will always
+point to a valid xenstore path for all operations.
+
+The path of this directory follows the syntax:
+
+/local/domain/<local_domid>/libxl/hotplug/<guest_domid>/<device_id>
+
+(Note that there is no end slash appended to HOTPLUG_PATH)
+
+
+BACKEND_PATH
+------------
+
+Points to the xenstore backend path of the corresponding block device.
+Since hotplug scripts are always executed in the Domain that acts as
+backend for a device, it will always have the following syntax:
+
+/local/domain/<local_domain>/backend/vbd/<guest_domid>/<device_id>
+
+(Note that there is no end slash appended to BACKEND_PATH)
+
+This environment variable is not set for all operations, since some
+hotplug operations are executed before the backend xenstore is set up.
+
+
+
+======================+COMMAND LINE PARAMETERS
+======================+
+
+Script will be called with only one parameter, that is either prepare,
+add, remove, unprepare, localattach or localdetach.
+
+
+PREPARE
+-------
+
+This is the first operation that the hotplug script will be requested to
+execute. This operation is executed before the disk is connected, to
+give the hotplug script the chance to offload some work from the
"add"
+operation, that is performed later.
+
+BACKEND_PATH: not valid
+
+Expected output:
+None
+
+
+ADD
+---
+
+This operation should connect the device to the domain. Will only be called
+after the "prepare" operation has finished successfully.
+
+BACKEND_PATH: valid
+
+Expected output:
+BACKEND_PATH/physical-device = block device major:minor
+BACKEND_PATH/params = block device path
+HOTPLUG_PATH/pdev = block device path
+
+
+REMOVE
+------
+
+
+Disconnects a block device from a domain. Will only be called
+after the "prepare" operation has finished successfully.
Implementations
+should take into account that the "remove" operation will also be
called if
+the "add" operation has failed.
+
+BACKEND_PATH: valid
+
+Expected output:
+None
+
+
+LOCALATTACH
+-----------
+
+
+Creates a block device in the current domain that points to the guest
+disk device. Will only be called after the "prepare" operation has
+finished successfully.
+
+BACKEND_PATH: not valid
+
+Expected output:
+HOTPLUG_PATH/pdev = block device path
+
+
+LOCALDETACH
+-----------
+
+
+Disconnects a device (previously connected with the localattach
+operation) from the current domain. Will only be called after
+the "prepare" operation has finished successfully. Implementations
+should take into account that the "localdetach" operation will
+also be called if the "localattach" operation has failed.
+
+BACKEND_PATH: not valid
+
+Expected output:
+None
+
+
+UNPREPARE
+---------
+
+
+Performs the necessary actions to unprepare the device.
+
+BACKEND_PATH: not valid
+
+Expected output:
+None
diff --git a/docs/misc/xl-disk-configuration.txt
b/docs/misc/xl-disk-configuration.txt
index 86c16be..d78acdb 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -166,6 +166,17 @@ information to be interpreted by the executable program
<script>,
 These scripts are normally called "block-<script>".
 
 
+method=<script>
+---------------
+
+Specifies that <target> is not a normal host path, but rather
+information to be interpreted by the executable program <script>,
+(looked for in /etc/xen/scripts, if it doesn't contain a slash).
+
+The script passed as parameter should support the new hotplug
+script interface, which is defined in libxl-hotplug-interface.txt.
+
+
 
 =========================================== DEPRECATED PARAMETERS, PREFIXES AND
SYNTAXES
-- 
1.7.7.5 (Apple Git-26)


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

This hotplug script has been tested with IET and NetBSD iSCSI targets,
without authentication.

Authentication parameters, including the password are passed as
parameters to iscsiadm, which is not recommended because other users
of the system can see them. This parameters could also be set by
editing a corresponding file directly, but the location of this file
seems to be different depending on the distribution used.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/hotplug/Linux/Makefile    |    1 +
 tools/hotplug/Linux/block-iscsi |  254 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 255 insertions(+), 0 deletions(-)
 create mode 100644 tools/hotplug/Linux/block-iscsi

diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 0605559..98c7738 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -21,6 +21,7 @@ XEN_SCRIPTS += blktap
 XEN_SCRIPTS += xen-hotplug-cleanup
 XEN_SCRIPTS += external-device-migrate
 XEN_SCRIPTS += vscsi
+XEN_SCRIPTS += block-iscsi
 XEN_SCRIPT_DATA = xen-script-common.sh locking.sh logging.sh
 XEN_SCRIPT_DATA += xen-hotplug-common.sh xen-network-common.sh vif-common.sh
 XEN_SCRIPT_DATA += block-common.sh
diff --git a/tools/hotplug/Linux/block-iscsi b/tools/hotplug/Linux/block-iscsi
new file mode 100644
index 0000000..ddb96c5
--- /dev/null
+++ b/tools/hotplug/Linux/block-iscsi
@@ -0,0 +1,254 @@
+#!/bin/sh
+#
+# Open-iSCSI Xen block device hotplug script
+#
+# Author Roger Pau Monné <roger.pau@citrix.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published
+# by the Free Software Foundation; version 2.1 only. with the special
+# exception on linking described in file LICENSE.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+
+remove_label()
+{
+    echo $1 | sed "s/^\("$2"\)//"
+}
+
+check_tools()
+{
+    if ! type iscsiadm > /dev/null 2>&1; then
+        echo "Unable to find iscsiadm tool"
+        return 1
+    fi
+    if [ "$multipath" = "y" ] && ! type multipath
> /dev/null 2>&1; then
+        echo "Unable to find multipath"
+        return 1
+    fi
+}
+
+# Sets the following global variables based on the params field passed in as
+# a parameter: iqn, portal, auth_method, user, multipath, password
+parse_target()
+{
+    # set multipath default value
+    multipath="n"
+    for param in $(echo "$1" | tr "," "\n")
+    do
+        if [ -n "$password" ]; then
+            password="$password,$param"
+            continue
+        fi
+        case $param in
+        iqn=*)
+            iqn=$(remove_label $param "iqn=")
+            ;;
+        portal=*)
+            portal=$(remove_label $param "portal=")
+            ;;
+        auth_method=*)
+            auth_method=$(remove_label $param "auth_method=")
+            ;;
+        user=*)
+            user=$(remove_label $param "user=")
+            ;;
+        multipath=*)
+            multipath=$(remove_label $param "multipath=")
+            ;;
+        password=*)
+            password=$(remove_label $param "password=")
+            ;;
+        esac
+    done
+    if [ -z "$iqn" ] || [ -z "$portal" ]; then
+        echo "iqn= and portal= are required parameters"
+        return 1
+    fi
+    if [ "$multipath" != "y" ] && [
"$multipath" != "n" ]; then
+        echo "multipath valid values are y and n, $multipath not a valid
value"
+        return 1
+    fi
+    return 0
+}
+
+# Outputs the block device major:minor
+device_major_minor()
+{
+    stat -L -c %t:%T "$1"
+}
+
+# Sets $dev to point to the device associated with the value in iqn
+find_device()
+{
+    while [ ! -e /dev/disk/by-path/*"$iqn"-lun-0 ]; do
+        sleep 0.1
+    done
+    sddev=$(readlink -f /dev/disk/by-path/*"$iqn"-lun-0)
+    if [ ! -b "$sddev" ]; then
+        echo "Unable to find attached device path"
+        return 1
+    fi
+    if [ "$multipath" = "y" ]; then
+        mdev=$(multipath -ll "$sddev" | head -1 | awk '{ print
$1}')
+        if [ ! -b /dev/mapper/"$mdev" ]; then
+            echo "Unable to find attached device multipath"
+            return 1
+        fi
+        dev="/dev/mapper/$mdev"
+    else
+        dev="$sddev"
+    fi
+    return 0
+}
+
+# Attaches the target $iqn in $portal and sets $dev to point to the
+# multipath device
+attach()
+{
+    iscsiadm -m node --targetname "$iqn" -p "$portal"
--login > /dev/null 2>&1
+    if [ $? -ne 0 ]; then
+        echo "Unable to connect to target"
+        return 1
+    fi
+    find_device
+    if [ $? -ne 0 ]; then
+        echo "Unable to find iSCSI device"
+        return 1
+    fi
+    return 0
+}
+
+# Discovers targets in $portal and checks that $iqn is one of those targets
+# Also sets the auth parameters to attach the device
+prepare()
+{
+    # Check if target is already opened
+    iscsiadm -m session 2>&1 | grep -q "$iqn"
+    if [ $? -eq 0 ]; then
+        echo "Device already opened"
+        return 1
+    fi
+    # Discover portal targets
+    iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q
"$iqn"
+    if [ $? -ne 0 ]; then
+        echo "No matching target iqn found"
+        return 1
+    fi
+    # Set auth method if necessary
+    if [ -n "$auth_method" ] || [ -n "$user" ] || [ -n
"$password" ]; then
+        iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=update --name \
+            node.session.auth.authmethod --value="$auth_method" \
+            > /dev/null 2>&1 && \
+        iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=update --name \
+            node.session.auth.username --value="$user" \
+            > /dev/null 2>&1 && \
+        iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=update --name \
+            node.session.auth.password --value="$password" \
+            > /dev/null 2>&1
+        if [ $? -ne 0 ]; then
+            echo "Unable to set authentication parameters"
+            return 1
+        fi
+    fi
+    return 0
+}
+
+# Attaches the device and writes xenstore backend entries to connect
+# the device
+add()
+{
+    localattach
+    if [ $? -ne 0 ]; then
+        echo "Failed to attach device"
+        return 1
+    fi
+    mm=$(device_major_minor "$dev")
+    xenstore-write "$BACKEND_PATH/physical-device" "$mm"
+    xenstore-write "$BACKEND_PATH/params" "$dev"
+    return 0
+}
+
+# Disconnects the device
+remove()
+{
+    find_device
+    if [ $? -ne 0 ]; then
+        echo "Unable to find device"
+        return 1
+    fi
+    iscsiadm -m node --targetname "$iqn" -p "$portal"
--logout > /dev/null 2>&1
+    if [ $? -ne 0 ]; then
+        echo "Unable to disconnect target"
+        return 1
+    fi
+    return 0
+}
+
+# Removes the auth params set by prepare
+unprepare()
+{
+    if [ -n "$auth_method" ] || [ -n "$user" ] || [ -n
"$password" ]; then
+        iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=delete --name \
+            node.session.auth.authmethod > /dev/null 2>&1
+        iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=delete --name \
+            node.session.auth.username > /dev/null 2>&1
+        iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=delete --name \
+            node.session.auth.password > /dev/null 2>&1
+    fi
+}
+
+# Attaches the device to the current domain, and writes the resulting
+# block device to xenstore
+localattach()
+{
+    attach
+    if [ $? -ne 0 ]; then
+        echo "Failed to attach device"
+        return 1
+    fi
+    xenstore-write "$HOTPLUG_PATH/pdev" "$dev"
+    return 0
+}
+
+command=$1
+target=$(xenstore-read $HOTPLUG_PATH/params)
+
+if [ -z "$target" ]; then
+    echo "No information about the target"
+    exit 1
+fi
+
+check_tools || exit 1
+
+parse_target "$target"
+
+case $command in
+prepare)
+    prepare
+    exit $?
+    ;;
+add)
+    add
+    exit $?
+    ;;
+remove)
+    remove
+    exit $?
+    ;;
+unprepare)
+    unprepare
+    exit $?
+    ;;
+localattach)
+    localattach
+    exit $?
+    ;;
+localdetach)
+    remove
+    exit $?
+    ;;
+esac
-- 
1.7.7.5 (Apple Git-26)


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

Ping!

On 21/12/12 17:59, Roger Pau Monne wrote:
> This series implements a new hoplug calling convention for libxl.
> 
> The aim of this new convention is to reduce the blackout phase of 
> migration when using complex hotplug scripts, like iSCSI or other kind 
> of storage backends that might have a non trivial setup time.
> 
> There are some issues that I would like to discuss, the first one is 
> the fact that pdev_path field in libxl_device_disk is no longer used 
> to store a physical path, since diskspec "target" can now contain
> "random" information to connect a block device.
Does someone has any feedback about this proposed change? Will it be OK
to insert this new field? I guess I will have to use some kind of
backward-compatible mechanism, like:

if (disk->pdev_path)
    disk->target = disk->pdev_path;

So old clients that use pdev_path still work as expected.
> 
> To solve this I would like to introduce a new field in 
> libxl_device_disk called "target", that will be used to store the
> diskspec target parameter. This can later be copied to pdev_path if 
> using the old hotplug calling convention.
> 
> The second issue is related to iSCSI and iscsiadm, specifically the 
> way to set authentication parameters, which is done with command line 
> parameters or editing files (which each distro seems to place in 
> different locations). I will look into this to see if we can find a 
> suitable solution.
> 
> Thanks for the comments, Roger.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

On Fri, 2012-12-21 at 16:59 +0000, Roger Pau Monne
wrote:
> This series implements a new hoplug calling convention for libxl.
> 
> The aim of this new convention is to reduce the blackout phase of 
> migration when using complex hotplug scripts, like iSCSI or other kind 
> of storage backends that might have a non trivial setup time.
> 
> There are some issues that I would like to discuss, the first one is 
> the fact that pdev_path field in libxl_device_disk is no longuer used 
> to store a physical path, since diskspec "target" can now contain
> "random" information to connect a block device.
> 
> To solve this I would like to introduce a new field in 
> libxl_device_disk called "target", that will be used to store the
> diskspec target parameter. This can later be copied to pdev_path if 
> using the old hotplug calling convention.
Perhaps we could arrange either via a union or via LIBXL_API_VERSION
ifdefs that "pdev_path" and "target" are in the same place?
> The second issue is related to iSCSI and iscsiadm, specifically the 
> way to set authentication parameters, which is done with command line 
> parameters or editing files (which each distro seems to place in 
> different locations). I will look into this to see if we can find a 
> suitable solution.
> 
> Thanks for the comments, Roger.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Fri, 2012-12-21 at 16:59 +0000, Roger Pau Monne
wrote:
> num_exec was not cleared when calling libxl__prepare_ao_device.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl_device.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 51dd06e..58d3f35 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -409,6 +409,7 @@ void libxl__prepare_ao_device(libxl__ao *ao,
libxl__ao_device *aodev)
>      aodev->ao = ao;
>      aodev->rc = 0;
>      aodev->dev = NULL;
> +    aodev->num_exec = 0;
>      /* Initialize timer for QEMU Bodge and hotplug execution */
>      libxl__ev_time_init(&aodev->timeout);
>      aodev->active = 1;


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

On 17/01/13 14:56, Ian Campbell wrote:
> On Fri, 2012-12-21 at 16:59 +0000, Roger Pau Monne wrote:
>> This series implements a new hoplug calling convention for libxl.
>>
>> The aim of this new convention is to reduce the blackout phase of 
>> migration when using complex hotplug scripts, like iSCSI or other kind 
>> of storage backends that might have a non trivial setup time.
>>
>> There are some issues that I would like to discuss, the first one is 
>> the fact that pdev_path field in libxl_device_disk is no longuer used 
>> to store a physical path, since diskspec "target" can now
contain
>> "random" information to connect a block device.
>>
>> To solve this I would like to introduce a new field in 
>> libxl_device_disk called "target", that will be used to store
the
>> diskspec target parameter. This can later be copied to pdev_path if 
>> using the old hotplug calling convention.
> 
> Perhaps we could arrange either via a union or via LIBXL_API_VERSION
> ifdefs that "pdev_path" and "target" are in the same
place?
We still need pdev_path, I would say only internally because once the
"target" has been attached to the Dom0, pdev_path has the path to the
block device that will be used in blkback, but the caller doesn''t need
to know about this.

Old callers can continue to pass pdev_path as currently doing, but
callers wanting to use the new hotplug interface should instead use
target, and set hotplug_version to 1.

What I''m doing in this series is overwriting pdev_path with the real
device path once the hotplug script has been executed, but that''s
clumsy.
>> The second issue is related to iSCSI and iscsiadm, specifically the 
>> way to set authentication parameters, which is done with command line 
>> parameters or editing files (which each distro seems to place in 
>> different locations). I will look into this to see if we can find a 
>> suitable solution.
>>
>> Thanks for the comments, Roger.
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
>

On Thu, 2013-01-17 at 15:30 +0000, Roger Pau Monne
wrote:
> On 17/01/13 14:56, Ian Campbell wrote:
> > On Fri, 2012-12-21 at 16:59 +0000, Roger Pau Monne wrote:
> >> This series implements a new hoplug calling convention for libxl.
> >>
> >> The aim of this new convention is to reduce the blackout phase of 
> >> migration when using complex hotplug scripts, like iSCSI or other
kind
> >> of storage backends that might have a non trivial setup time.
> >>
> >> There are some issues that I would like to discuss, the first one
is
> >> the fact that pdev_path field in libxl_device_disk is no longuer
used
> >> to store a physical path, since diskspec "target" can
now contain
> >> "random" information to connect a block device.
> >>
> >> To solve this I would like to introduce a new field in 
> >> libxl_device_disk called "target", that will be used to
store the
> >> diskspec target parameter. This can later be copied to pdev_path
if
> >> using the old hotplug calling convention.
> > 
> > Perhaps we could arrange either via a union or via LIBXL_API_VERSION
> > ifdefs that "pdev_path" and "target" are in the
same place?
> 
> We still need pdev_path, I would say only internally because once the
> "target" has been attached to the Dom0, pdev_path has the path to
the
> block device that will be used in blkback, but the caller doesn''t
need
> to know about this.
So in the new scheme both pdev_path are valid and contain different
information?
> Old callers can continue to pass pdev_path as currently doing, but
> callers wanting to use the new hotplug interface should instead use
> target, and set hotplug_version to 1.
> 
> What I''m doing in this series is overwriting pdev_path with the
real
> device path once the hotplug script has been executed, but that''s
clumsy.
> 
> >> The second issue is related to iSCSI and iscsiadm, specifically
the
> >> way to set authentication parameters, which is done with command
line
> >> parameters or editing files (which each distro seems to place in 
> >> different locations). I will look into this to see if we can find
a
> >> suitable solution.
> >>
> >> Thanks for the comments, Roger.
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xen.org
> >> http://lists.xen.org/xen-devel
> > 
> > 
>

On 17/01/13 16:40, Ian Campbell wrote:
> On Thu, 2013-01-17 at 15:30 +0000, Roger Pau Monne wrote:
>> We still need pdev_path, I would say only internally because once the
>> "target" has been attached to the Dom0, pdev_path has the
path to the
>> block device that will be used in blkback, but the caller
doesn''t need
>> to know about this.
> 
> So in the new scheme both pdev_path are valid and contain different
> information?
Yes, target would be what''s passed by the user, like:

"iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128"

And pdev_path would be filled after the device has been attached, and it
will be something like:

/dev/sdb

The main reason we need the block device is to be able to call pygrub or
to pass it to Qemu.

On Fri, 2012-12-21 at 17:00 +0000, Roger Pau Monne
wrote:
> diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
> index bee16a1..4486c1a 100644
> --- a/tools/libxl/libxlu_disk_l.l
> +++ b/tools/libxl/libxlu_disk_l.l
> @@ -172,6 +172,8 @@ backendtype=[^,]*,? { STRIP('','');
> setbackendtype(DPC,FROMEQUALS); }
> 
>  vdev=[^,]*,?   { STRIP('','');
SAVESTRING("vdev", vdev, FROMEQUALS); }
>  script=[^,]*,? { STRIP('','');
SAVESTRING("script", script, FROMEQUALS); }
> +method=[^,]*,? { STRIP('','');
SAVESTRING("script", script, FROMEQUALS);
> +                 DPC->disk->hotplug_version = 1; }
Did mean "method" here ------------------/^ ?

Are there any docs for this new option?

Are there docs for the new protocol somewhere? I think I should probably
read them first...

Ian

On Thu, 2013-01-17 at 15:47 +0000, Roger Pau Monne
wrote:
> On 17/01/13 16:40, Ian Campbell wrote:
> > On Thu, 2013-01-17 at 15:30 +0000, Roger Pau Monne wrote:
> >> We still need pdev_path, I would say only internally because once
the
> >> "target" has been attached to the Dom0, pdev_path has
the path to the
> >> block device that will be used in blkback, but the caller
doesn''t need
> >> to know about this.
> > 
> > So in the new scheme both pdev_path are valid and contain different
> > information?
> 
> Yes, target would be what''s passed by the user, like:
> 
>
"iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128"
> 
> And pdev_path would be filled after the device has been attached, and it
> will be something like:
> 
> /dev/sdb
I see. When we login and create /dev/sdb we can''t overwrite target? Or
move pdev_path into some purely internal datastructure?
> The main reason we need the block device is to be able to call pygrub or
> to pass it to Qemu.
I see.

Ian.

On 17/01/13 16:57, Ian Campbell wrote:
> On Thu, 2013-01-17 at 15:47 +0000, Roger Pau Monne wrote:
>> On 17/01/13 16:40, Ian Campbell wrote:
>>> On Thu, 2013-01-17 at 15:30 +0000, Roger Pau Monne wrote:
>>>> We still need pdev_path, I would say only internally because
once the
>>>> "target" has been attached to the Dom0, pdev_path has
the path to the
>>>> block device that will be used in blkback, but the caller
doesn''t need
>>>> to know about this.
>>>
>>> So in the new scheme both pdev_path are valid and contain different
>>> information?
>>
>> Yes, target would be what''s passed by the user, like:
>>
>>
"iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128"
>>
>> And pdev_path would be filled after the device has been attached, and
it
>> will be something like:
>>
>> /dev/sdb
> 
> I see. When we login and create /dev/sdb we can''t overwrite
target? Or
> move pdev_path into some purely internal datastructure?
Yes that''s another option, what I''m doing in this series is
overwriting
pdev_path, we could just change the name of pdev_path to target (with a
union as you said) and keep doing it (overwriting target with the
physical path of the attached device).

I''m afraid there''s no internal disk related structure, only
libxl_device_disk (public) or libxl__device, but libxl__device is too
general, and I don''t want to start overloading it with disk specific
values.
> 
>> The main reason we need the block device is to be able to call pygrub
or
>> to pass it to Qemu.
> 
> I see.
> 
> Ian.
>

On Thu, 2013-01-17 at 16:10 +0000, Roger Pau Monne
wrote:
> On 17/01/13 16:57, Ian Campbell wrote:
> > On Thu, 2013-01-17 at 15:47 +0000, Roger Pau Monne wrote:
> >> On 17/01/13 16:40, Ian Campbell wrote:
> >>> On Thu, 2013-01-17 at 15:30 +0000, Roger Pau Monne wrote:
> >>>> We still need pdev_path, I would say only internally
because once the
> >>>> "target" has been attached to the Dom0,
pdev_path has the path to the
> >>>> block device that will be used in blkback, but the caller
doesn''t need
> >>>> to know about this.
> >>>
> >>> So in the new scheme both pdev_path are valid and contain
different
> >>> information?
> >>
> >> Yes, target would be what''s passed by the user, like:
> >>
> >>
"iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128"
> >>
> >> And pdev_path would be filled after the device has been attached,
and it
> >> will be something like:
> >>
> >> /dev/sdb
> > 
> > I see. When we login and create /dev/sdb we can''t overwrite
target? Or
> > move pdev_path into some purely internal datastructure?
> 
> Yes that''s another option, what I''m doing in this series
is overwriting
> pdev_path, we could just change the name of pdev_path to target (with a
> union as you said) and keep doing it (overwriting target with the
> physical path of the attached device).
> 
> I''m afraid there''s no internal disk related structure,
only
> libxl_device_disk (public) or libxl__device, but libxl__device is too
> general, and I don''t want to start overloading it with disk
specific values.
You could add such an internal struct?

Ian.

On 17/01/13 17:15, Ian Campbell wrote:
> On Thu, 2013-01-17 at 16:10 +0000, Roger Pau Monne wrote:
>> On 17/01/13 16:57, Ian Campbell wrote:
>>> On Thu, 2013-01-17 at 15:47 +0000, Roger Pau Monne wrote:
>>>> On 17/01/13 16:40, Ian Campbell wrote:
>>>>> On Thu, 2013-01-17 at 15:30 +0000, Roger Pau Monne wrote:
>>>>>> We still need pdev_path, I would say only internally
because once the
>>>>>> "target" has been attached to the Dom0,
pdev_path has the path to the
>>>>>> block device that will be used in blkback, but the
caller doesn''t need
>>>>>> to know about this.
>>>>>
>>>>> So in the new scheme both pdev_path are valid and contain
different
>>>>> information?
>>>>
>>>> Yes, target would be what''s passed by the user, like:
>>>>
>>>>
"iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128"
>>>>
>>>> And pdev_path would be filled after the device has been
attached, and it
>>>> will be something like:
>>>>
>>>> /dev/sdb
>>>
>>> I see. When we login and create /dev/sdb we can''t
overwrite target? Or
>>> move pdev_path into some purely internal datastructure?
>>
>> Yes that''s another option, what I''m doing in this
series is overwriting
>> pdev_path, we could just change the name of pdev_path to target (with a
>> union as you said) and keep doing it (overwriting target with the
>> physical path of the attached device).
>>
>> I''m afraid there''s no internal disk related
structure, only
>> libxl_device_disk (public) or libxl__device, but libxl__device is too
>> general, and I don''t want to start overloading it with disk
specific values.
> 
> You could add such an internal struct?
Yes, will try to come up with something that''s not too disruptive. I
was
wrong regarding when we need to modify the pdev_path value, we only need
to modify it when booting HVM guests (see patch 08), for pygrub we use
the local attach mechanism that already has a related struct with a
field to store the physical path.

On Fri, 2012-12-21 at 17:00 +0000, Roger Pau Monne
wrote:
> Add support to call hotplug scripts that use the new hotplug interface
> to the low-level functions in charge of calling hotplug scripts. This
> new calling convention will be used when the hotplug_version aodev
> field is 1.
Is there perhaps some way we can avoid users having to think about the
hotplug_version?

For example if we export XENBUGS_PATH as a synonym for BACKEND_PATH and
arrange that the new protocol involves calling action "add" and
"remove" (as well as the other new ones) do old scripts mostly Just
Work
because they ignore the prepare/unprepare calls? (as a small sample
vif-bridge and block-nbd seem to).

Alternatively by way of backwards compat perhaps we could provide a
wrapper script? So if today you use script="/my-custom-script" you
would
switch to script="/etc/xen/hotplug-compat-wrapper /my-custom-script"
and
the wrapper would shim or ignore the new calls into the old?
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/libxl/libxl_device.c   |   33 +++++++++++++++++++-
>  tools/libxl/libxl_internal.h |   33 ++++++++++++++++++--
>  tools/libxl/libxl_linux.c    |   68
++++++++++++++++++++++++++++++++----------
>  tools/libxl/libxl_netbsd.c   |    2 +-
>  4 files changed, 115 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 58d3f35..85c9953 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -83,6 +83,35 @@ out:
>      return rc;
>  }
> 
> +char *libxl__device_hotplug_action(libxl__gc *gc,
> +                                   libxl__device_action action)
> +{
If you define libxl__device_action in
tools/libxl/libxl_types_internal.idl you'll get this for free.

[...]
> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 9587833..8061e7a 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
> @@ -62,7 +62,7 @@ out:
>  int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
>                                     char ***args, char ***env,
>                                     libxl__device_action action,
> -                                   int num_exec)
> +                                   int num_exec, int hotplug_version)
Is it worth wrapping verison and action into a little
libxl__hotplug_action struct? 



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

On 18/01/13 14:29, Ian Campbell wrote:
> On Fri, 2012-12-21 at 17:00 +0000, Roger Pau Monne wrote:
>> Add support to call hotplug scripts that use the new hotplug interface
>> to the low-level functions in charge of calling hotplug scripts. This
>> new calling convention will be used when the hotplug_version aodev
>> field is 1.
> 
> Is there perhaps some way we can avoid users having to think about the
> hotplug_version?
> 
> For example if we export XENBUGS_PATH as a synonym for BACKEND_PATH and
> arrange that the new protocol involves calling action "add" and
> "remove" (as well as the other new ones) do old scripts mostly
Just Work
> because they ignore the prepare/unprepare calls? (as a small sample
> vif-bridge and block-nbd seem to).
I'm afraid old scripts will return an error when called with commands
different than "add" or "remove", this is from
block-common.sh:

if [ "$command" != "add" ] &&
   [ "$command" != "remove" ]
then
  log err "Invalid command: $command"
  exit 1
fi
> Alternatively by way of backwards compat perhaps we could provide a
> wrapper script? So if today you use script="/my-custom-script"
you would
> switch to script="/etc/xen/hotplug-compat-wrapper
/my-custom-script" and
> the wrapper would shim or ignore the new calls into the old?
Well, from a user point of view, this will still be the same, old
hotplug scripts will be called using the "script" config option, so
there will be no need to change configuration files. The only difference
is that when calling hotplug scripts using the new hotplug interface
users should use "method" instead of "script" in their
config file, as
an example this would be the diskspec line to attach a disk using the
iSCSI block script:

'method=block-iscsi,vdev=xvda,target=iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128'

Users don't have to deal directly with hotplug_version, I think forcing
them to user a wrapper is worse, because that will mean modifications to
config files when updating.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  tools/libxl/libxl_device.c   |   33 +++++++++++++++++++-
>>  tools/libxl/libxl_internal.h |   33 ++++++++++++++++++--
>>  tools/libxl/libxl_linux.c    |   68
++++++++++++++++++++++++++++++++----------
>>  tools/libxl/libxl_netbsd.c   |    2 +-
>>  4 files changed, 115 insertions(+), 21 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 58d3f35..85c9953 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -83,6 +83,35 @@ out:
>>      return rc;
>>  }
>>
>> +char *libxl__device_hotplug_action(libxl__gc *gc,
>> +                                   libxl__device_action action)
>> +{
> 
> If you define libxl__device_action in
> tools/libxl/libxl_types_internal.idl you'll get this for free.
Thanks, will do that then.
> [...]
>> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
>> index 9587833..8061e7a 100644
>> --- a/tools/libxl/libxl_netbsd.c
>> +++ b/tools/libxl/libxl_netbsd.c
>> @@ -62,7 +62,7 @@ out:
>>  int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
>>                                     char ***args, char ***env,
>>                                     libxl__device_action action,
>> -                                   int num_exec)
>> +                                   int num_exec, int hotplug_version)
> 
> Is it worth wrapping verison and action into a little
> libxl__hotplug_action struct? 
It will slim down the list of parameters to pass to this function, which
is already too long for my taste.


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

On Fri, 2013-01-18 at 16:24 +0000, Roger Pau Monne
wrote:
> On 18/01/13 14:29, Ian Campbell wrote:
> > On Fri, 2012-12-21 at 17:00 +0000, Roger Pau Monne wrote:
> >> Add support to call hotplug scripts that use the new hotplug
interface
> >> to the low-level functions in charge of calling hotplug scripts.
This
> >> new calling convention will be used when the hotplug_version aodev
> >> field is 1.
> > 
> > Is there perhaps some way we can avoid users having to think about the
> > hotplug_version?
> > 
> > For example if we export XENBUGS_PATH as a synonym for BACKEND_PATH
and
> > arrange that the new protocol involves calling action "add"
and
> > "remove" (as well as the other new ones) do old scripts
mostly Just Work
> > because they ignore the prepare/unprepare calls? (as a small sample
> > vif-bridge and block-nbd seem to).
> 
> I''m afraid old scripts will return an error when called with
commands
> different than "add" or "remove", this is from
block-common.sh:
> 
> if [ "$command" != "add" ] &&
>    [ "$command" != "remove" ]
> then
>   log err "Invalid command: $command"
>   exit 1
> fi
That''s a shame.
> > Alternatively by way of backwards compat perhaps we could provide a
> > wrapper script? So if today you use
script="/my-custom-script" you would
> > switch to script="/etc/xen/hotplug-compat-wrapper
/my-custom-script" and
> > the wrapper would shim or ignore the new calls into the old?
> 
> Well, from a user point of view, this will still be the same, old
> hotplug scripts will be called using the "script" config option,
so
> there will be no need to change configuration files. The only difference
> is that when calling hotplug scripts using the new hotplug interface
> users should use "method" instead of "script" in their
config file, as
> an example this would be the diskspec line to attach a disk using the
> iSCSI block script:
> 
>
''method=block-iscsi,vdev=xvda,target=iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128''
> 
> Users don''t have to deal directly with hotplug_version, I think
forcing
> them to user a wrapper is worse, because that will mean modifications to
> config files when updating.
But "users" of libxl do need to care about hotplug_version?

I''m not sure what the answer is here but it would be good if everyone
writing a toolstack using libxl didn''t have to think about what kind of
script each one was calling.

Could we perhaps mandate the new scripts have a certain comment or other
grepable property or that they must react without error to a particular
probing command line option "--are-you-a-v2-script" and have libxl
probe
using that?

Ian.

On 21/01/13 11:07, Ian Campbell wrote:
> On Fri, 2013-01-18 at 16:24 +0000, Roger Pau Monne wrote:
>> On 18/01/13 14:29, Ian Campbell wrote:
>>> On Fri, 2012-12-21 at 17:00 +0000, Roger Pau Monne wrote:
>>>> Add support to call hotplug scripts that use the new hotplug
interface
>>>> to the low-level functions in charge of calling hotplug
scripts. This
>>>> new calling convention will be used when the hotplug_version
aodev
>>>> field is 1.
>>>
>>> Is there perhaps some way we can avoid users having to think about
the
>>> hotplug_version?
>>>
>>> For example if we export XENBUGS_PATH as a synonym for BACKEND_PATH
and
>>> arrange that the new protocol involves calling action
"add" and
>>> "remove" (as well as the other new ones) do old scripts
mostly Just Work
>>> because they ignore the prepare/unprepare calls? (as a small sample
>>> vif-bridge and block-nbd seem to).
>>
>> I''m afraid old scripts will return an error when called with
commands
>> different than "add" or "remove", this is from
block-common.sh:
>>
>> if [ "$command" != "add" ] &&
>>    [ "$command" != "remove" ]
>> then
>>   log err "Invalid command: $command"
>>   exit 1
>> fi
> 
> That''s a shame.
> 
>>> Alternatively by way of backwards compat perhaps we could provide a
>>> wrapper script? So if today you use
script="/my-custom-script" you would
>>> switch to script="/etc/xen/hotplug-compat-wrapper
/my-custom-script" and
>>> the wrapper would shim or ignore the new calls into the old?
>>
>> Well, from a user point of view, this will still be the same, old
>> hotplug scripts will be called using the "script" config
option, so
>> there will be no need to change configuration files. The only
difference
>> is that when calling hotplug scripts using the new hotplug interface
>> users should use "method" instead of "script" in
their config file, as
>> an example this would be the diskspec line to attach a disk using the
>> iSCSI block script:
>>
>>
''method=block-iscsi,vdev=xvda,target=iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128''
>>
>> Users don''t have to deal directly with hotplug_version, I
think forcing
>> them to user a wrapper is worse, because that will mean modifications
to
>> config files when updating.
> 
> But "users" of libxl do need to care about hotplug_version?
Yes libxl users need to care about hotplug_version, I misunderstood you
and thought you were talking about xl users, not libxl users.
> 
> I''m not sure what the answer is here but it would be good if
everyone
> writing a toolstack using libxl didn''t have to think about what
kind of
> script each one was calling.
> 
> Could we perhaps mandate the new scripts have a certain comment or other
> grepable property or that they must react without error to a particular
> probing command line option "--are-you-a-v2-script" and have
libxl probe
> using that?
If we choose to use think approach we can also get rid of "method",
since libxl will be able to auto-detect script type and react
accordingly. I don''t have any preference regarding the way to do this
auto-detection, but maybe stating that the script should return 0
(success) when called with the action "support_v2" could be a good
way.

I prefer this rather that having a comment somewhere, since hotplug
scripts can be in any kind of programming language (or even in binary
form AFAIK), this could become more complicated that it seems now.

On Mon, 2013-01-21 at 12:11 +0000, Roger Pau Monne
wrote:
> >>> Alternatively by way of backwards compat perhaps we could
provide a
> >>> wrapper script? So if today you use
script="/my-custom-script" you would
> >>> switch to script="/etc/xen/hotplug-compat-wrapper
/my-custom-script" and
> >>> the wrapper would shim or ignore the new calls into the old?
> >>
> >> Well, from a user point of view, this will still be the same, old
> >> hotplug scripts will be called using the "script" config
option, so
> >> there will be no need to change configuration files. The only
difference
> >> is that when calling hotplug scripts using the new hotplug
interface
> >> users should use "method" instead of "script"
in their config file, as
> >> an example this would be the diskspec line to attach a disk using
the
> >> iSCSI block script:
> >>
> >>
''method=block-iscsi,vdev=xvda,target=iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128''
> >>
> >> Users don''t have to deal directly with hotplug_version, I
think forcing
> >> them to user a wrapper is worse, because that will mean
modifications to
> >> config files when updating.
> > 
> > But "users" of libxl do need to care about hotplug_version?
> 
> Yes libxl users need to care about hotplug_version, I misunderstood you
> and thought you were talking about xl users, not libxl users.
I think I was initially talking about both.
> > I''m not sure what the answer is here but it would be good if
everyone
> > writing a toolstack using libxl didn''t have to think about
what kind of
> > script each one was calling.
> > 
> > Could we perhaps mandate the new scripts have a certain comment or
other
> > grepable property or that they must react without error to a
particular
> > probing command line option "--are-you-a-v2-script" and have
libxl probe
> > using that?
> 
> If we choose to use think approach we can also get rid of
"method",
> since libxl will be able to auto-detect script type and react
> accordingly. I don''t have any preference regarding the way to do
this
> auto-detection, but maybe stating that the script should return 0
> (success) when called with the action "support_v2" could be a
good way.
Perhaps by way of future-proofing it should be a command
"supported-hotplug-protocols" which prints a list of supported
versions,
initially just "2" for new scripts with failure taken to mean
"1".
Combined with also setting $XEN_HOTPLUG_PROTOCOL=<N> that would give us
some scope for future evolution.
> I prefer this rather that having a comment somewhere, since hotplug
> scripts can be in any kind of programming language (or even in binary
> form AFAIK), this could become more complicated that it seems now.
Ack.

On 18/01/13 14:29, Ian Campbell wrote:
>> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
>> index 9587833..8061e7a 100644
>> --- a/tools/libxl/libxl_netbsd.c
>> +++ b/tools/libxl/libxl_netbsd.c
>> @@ -62,7 +62,7 @@ out:
>>  int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
>>                                     char ***args, char ***env,
>>                                     libxl__device_action action,
>> -                                   int num_exec)
>> +                                   int num_exec, int hotplug_version)
> 
> Is it worth wrapping verison and action into a little
> libxl__hotplug_action struct? 
I''m going to pack hotplug_version and num_exec inside a
libxl__hotplug_action struct, but not device_action, because
device_action is not only used by hotplug scripts, it is also used by
the general device connection/disconnection flow, and it will be strange
to put it inside a hotplug specific struct.