[PATCH v2 0/7] libxl: add support for FreeBSD block hotplug scripts

[PATCH v2 0/7] libxl: add support for FreeBSD block hotplug scripts

[Roger Pau Monne]

This series enables using hotplug scripts with the FreeBSD blkback 
implementation. Since FreeBSD blkback can use both block devices and regular 
RAW files as disks, the physical-device xenstore backend node is now 
OS-specific, Linux and NetBSD will encode the device major and minor numbers 
there, while FreeBSD simply puts an absolute path to a disk image.

The current blkback implementation in FreeBSD HEAD doesn't yet know about 
hotplug scripts, I plan to commit the FreeBSD side once this series is 
reviewed.

Thanks, Roger.

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

[PATCH v2 1/7] blkif: document how FreeBSD uses the physical-device backend node

[Roger Pau Monne]

FreeBSD blkback uses the physical-device xenstore node in order to fetch the
path to the underlying backing storage (either a block device or raw image).
This node is set by the hotplug scripts.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/public/io/blkif.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 99f0326..9a5baa4 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -89,14 +89,18 @@
  *      Values:         string
  *
  *      A free formatted string providing sufficient information for the
- *      backend driver to open the backing device.  (e.g. the path to the
- *      file or block device representing the backing store.)
+ *      hotplug script to attach the device and provide a suitable
+ *      handler (ie: a block device) for blkback to use.
  *
- * physical-device
- *      Values:         "MAJOR:MINOR"
+ * physical-device:
+ *      Values:         OS specific
  *
- *      MAJOR and MINOR are the major number and minor number of the
- *      backing device respectively.
+ *      A free formatted string that contains the OS specific path to the
+ *      backing device for this blkback instance.
+ *
+ *      Linux and NetBSD encode the block device major and minor numbers using
+ *      the following printf format: "%x:%x", while FreeBSD places the path to
+ *      the file or device in the filesystem.
  *
  * type
  *      Values:         "file", "phy", "tap"
-- 
2.5.4 (Apple Git-61)


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

[PATCH v2 2/7] libxl: introduce an OS-specific function to get the physical-device

[Roger Pau Monne]

Linux and NetBSD will return the device major and minor numbers encoded in
hex and separated by a ":". FreeBSD on the other hand returns the path to
the block device or image file.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c          |  9 +++++----
 tools/libxl/libxl_freebsd.c  |  6 ++++++
 tools/libxl/libxl_internal.h |  6 ++++++
 tools/libxl/libxl_linux.c    | 10 ++++++++++
 tools/libxl/libxl_netbsd.c   | 10 ++++++++++
 5 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2d18b8d..6d719d7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2515,10 +2515,11 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
                  */
                 if (!disk->script &&
                     disk->backend_domid == LIBXL_TOOLSTACK_DOMID) {
-                    int major, minor;
-                    if (!libxl__device_physdisk_major_minor(dev, &major, &minor))
-                        flexarray_append_pair(back, "physical-device",
-                                              GCSPRINTF("%x:%x", major, minor));
+                    char *physdev;
+
+                    physdev = libxl__get_physical_device(dev);
+                    if (physdev != NULL)
+                        flexarray_append_pair(back, "physical-device", physdev);
                 }
 
                 assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
index 47c3391..483f36e 100644
--- a/tools/libxl/libxl_freebsd.c
+++ b/tools/libxl/libxl_freebsd.c
@@ -143,3 +143,9 @@ int libxl__pci_topology_init(libxl__gc *gc,
 {
     return ERROR_NI;
 }
+
+char *libxl__get_physical_device(char *dev)
+{
+
+    return dev;
+}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 650a958..1a3fa35 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1346,6 +1346,12 @@ void libxl__domaindeathcheck_stop(libxl__gc *gc, libxl__domaindeathcheck *dc);
  */
 _hidden int libxl__try_phy_backend(mode_t st_mode);
 
+/*
+ * Fetch the "physical-device" backend xenstore field for a local disk
+ * device. The implementation of this function is OS-specific.
+ */
+_hidden char *libxl__get_physical_device(char *dev);
+
 
 _hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
 
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index be4afc6..7e6d7b9 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -347,3 +347,13 @@ int libxl__pci_topology_init(libxl__gc *gc,
 
     return err;
 }
+
+char *libxl__get_physical_device(char *dev)
+{
+    int major, minor;
+
+    if (libxl__device_physdisk_major_minor(dev, &major, &minor))
+        return NULL;
+
+    return GCSPRINTF("%x:%x", major, minor);
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 096c057..e12561f 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -100,3 +100,13 @@ int libxl__pci_topology_init(libxl__gc *gc,
 {
     return ERROR_NI;
 }
+
+char *libxl__get_physical_device(char *dev)
+{
+    int major, minor;
+
+    if (libxl__device_physdisk_major_minor(dev, &major, &minor))
+        return NULL;
+
+    return GCSPRINTF("%x:%x", major, minor);
+}
-- 
2.5.4 (Apple Git-61)


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

[PATCH v2 3/7] hotplug/FreeBSD: add block hotplug script

[Roger Pau Monne]

This is the default hotplug script for block devices. Its only job is to
copy the "params" blkback xenstore node to "physical-device".

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/FreeBSD/Makefile |  2 +-
 tools/hotplug/FreeBSD/block    | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 tools/hotplug/FreeBSD/block

diff --git a/tools/hotplug/FreeBSD/Makefile b/tools/hotplug/FreeBSD/Makefile
index 10fce4f..bd7a86f 100644
--- a/tools/hotplug/FreeBSD/Makefile
+++ b/tools/hotplug/FreeBSD/Makefile
@@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 # Xen script dir and scripts to go there.
-XEN_SCRIPTS = vif-bridge
+XEN_SCRIPTS = vif-bridge block
 
 XEN_SCRIPT_DATA =
 
diff --git a/tools/hotplug/FreeBSD/block b/tools/hotplug/FreeBSD/block
new file mode 100644
index 0000000..1f2a533
--- /dev/null
+++ b/tools/hotplug/FreeBSD/block
@@ -0,0 +1,25 @@
+#!/bin/sh -e
+#
+# FreeBSD hotplug script for attaching disks
+#
+# Parameters:
+#	$1: xenstore backend path of the vbd
+#	$2: action, either "add" or "remove"
+#
+# Environment variables:
+#	None
+#
+
+path=$1
+action=$2
+params=$(xenstore-read "$path/params")
+
+case $action in
+add)
+	xenstore-write $path/physical-device $params
+	exit 0
+	;;
+*)
+	exit 0
+	;;
+esac
-- 
2.5.4 (Apple Git-61)


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

[PATCH v2 4/7] libxl/FreeBSD: add support for disk hotplug scripts

[Roger Pau Monne]

Allow FreeBSD to execute hotplug scripts when attaching disk devices.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_freebsd.c | 114 ++++++++++++++++++++++++++++++++------------
 1 file changed, 83 insertions(+), 31 deletions(-)

diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
index 483f36e..8ad347e 100644
--- a/tools/libxl/libxl_freebsd.c
+++ b/tools/libxl/libxl_freebsd.c
@@ -59,14 +59,36 @@ static int libxl__hotplug_env_nic(libxl__gc *gc, libxl__device *dev, char ***env
     return 0;
 }
 
-static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev, char ***args,
-                              libxl__device_action action)
+static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev,
+                              char ***args, char ***env,
+                              libxl__device_action action,
+                              int num_exec)
 {
+    libxl_nic_type nictype;
     char *be_path = libxl__device_backend_path(gc, dev);
     char *script;
-    int nr = 0, rc = 0, arraysize = 4;
+    int nr = 0, rc;
 
-    assert(dev->backend_kind == LIBXL__DEVICE_KIND_VIF);
+    rc = libxl__nic_type(gc, dev, &nictype);
+    if (rc) {
+        LOG(ERROR, "error when fetching nic type");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /*
+     * For PV domains only one pass is needed (because there's no emulated
+     * interface). For HVM domains two passes are needed in order to add
+     * both the PV and the tap interfaces to the bridge.
+     */
+    if (nictype == LIBXL_NIC_TYPE_VIF && num_exec != 0) {
+        rc = 0;
+        goto out;
+    }
+
+    rc = libxl__hotplug_env_nic(gc, dev, env, num_exec);
+    if (rc)
+        goto out;
 
     script = libxl__xs_read(gc, XBT_NULL,
                             GCSPRINTF("%s/%s", be_path, "script"));
@@ -76,53 +98,83 @@ static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev, char ***args,
         goto out;
     }
 
+    const int arraysize = 4;
     GCNEW_ARRAY(*args, arraysize);
     (*args)[nr++] = script;
     (*args)[nr++] = be_path;
-    (*args)[nr++] = GCSPRINTF("%s", action == LIBXL__DEVICE_ACTION_ADD ?
-                                    "add" : "remove");
+    (*args)[nr++] = (char *) libxl__device_action_to_string(action);
     (*args)[nr++] = NULL;
     assert(nr == arraysize);
 
+    rc = 1;
+
 out:
     return rc;
 }
 
+static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
+                               char ***args, char ***env,
+                               libxl__device_action action)
+{
+    char *be_path = libxl__device_backend_path(gc, dev);
+    char *script;
+    int nr = 0, rc;
+
+    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);
+        rc = ERROR_FAIL;
+        goto error;
+    }
+
+    const int arraysize = 4;
+    GCNEW_ARRAY(*args, arraysize);
+    (*args)[nr++] = script;
+    (*args)[nr++] = be_path;
+    (*args)[nr++] = (char *) libxl__device_action_to_string(action);
+    (*args)[nr++] = NULL;
+    assert(nr == arraysize);
+
+    rc = 1;
+
+error:
+    return rc;
+}
+
 int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
                                    char ***args, char ***env,
                                    libxl__device_action action,
                                    int num_exec)
 {
-    libxl_nic_type nictype;
     int rc;
 
-    if (dev->backend_kind != LIBXL__DEVICE_KIND_VIF || num_exec == 2)
-        return 0;
-
-    rc = libxl__nic_type(gc, dev, &nictype);
-    if (rc) {
-        LOG(ERROR, "error when fetching nic type");
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-    /*
-     * For PV domains only one pass is needed (because there's no emulated
-     * interface). For HVM domains two passes are needed in order to add
-     * both the PV and the tap interfaces to the bridge.
-     */
-    if (nictype == LIBXL_NIC_TYPE_VIF && num_exec != 0) {
+    switch (dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VBD:
+        if (num_exec != 0) {
+            rc = 0;
+            goto out;
+        }
+        rc = libxl__hotplug_disk(gc, dev, args, env, action);
+        break;
+    case LIBXL__DEVICE_KIND_VIF:
+        /*
+         * If domain has a stubdom we don't have to execute hotplug scripts
+         * for emulated interfaces
+         */
+        if ((num_exec > 1) ||
+            (libxl_get_stubdom_id(CTX, dev->domid) && num_exec)) {
+            rc = 0;
+            goto out;
+        }
+        rc = libxl__hotplug_nic(gc, dev, args, env, action, num_exec);
+        break;
+    default:
+        /* No need to execute any hotplug scripts */
         rc = 0;
-        goto out;
+        break;
     }
 
-    rc = libxl__hotplug_env_nic(gc, dev, env, num_exec);
-    if (rc)
-        goto out;
-
-    rc = libxl__hotplug_nic(gc, dev, args, action);
-    if (!rc) rc = 1;
-
 out:
     return rc;
 }
-- 
2.5.4 (Apple Git-61)


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

[PATCH v2 5/7] libxl: properly use vdev vs local device

[Roger Pau Monne]

The current code in libxl assumed that vdev is equal to local device, but
this is only true for Linux systems. In other OSes the local device can use
a nomenclature completely different from the virtual device one.

Move the current libxl__devid_to_localdev Linux implementation out of the
OS-specific file and rename it to libxl__devid_to_vdev, and then make sure
local_device_attach_cb return the local device in the diskpath field.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c          | 12 ++++------
 tools/libxl/libxl_device.c   | 57 ++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  1 +
 tools/libxl/libxl_linux.c    | 49 +------------------------------------
 4 files changed, 64 insertions(+), 55 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 6d719d7..d55c699 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3071,7 +3071,7 @@ static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user,
                     GCSPRINTF("%s/device/vbd/%d/backend",
                         dompath, devid)) == NULL) {
             if (errno == ENOENT)
-                return libxl__devid_to_localdev(gc, devid);
+                return libxl__devid_to_vdev(gc, devid);
             else
                 return NULL;
         }
@@ -3137,7 +3137,7 @@ 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 *be_path = NULL;
     int rc;
     libxl__device device;
     libxl_device_disk *disk = &dls->disk;
@@ -3151,9 +3151,6 @@ static void local_device_attach_cb(libxl__egc *egc, libxl__ao_device *aodev)
         goto out;
     }
 
-    dev = GCSPRINTF("/dev/%s", disk->vdev);
-    LOG(DEBUG, "locally attaching disk %s", dev);
-
     rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk, &device);
     if (rc < 0)
         goto out;
@@ -3162,8 +3159,9 @@ static void local_device_attach_cb(libxl__egc *egc, libxl__ao_device *aodev)
     if (rc < 0)
         goto out;
 
-    if (dev != NULL)
-        dls->diskpath = libxl__strdup(gc, dev);
+    dls->diskpath = GCSPRINTF("/dev/%s",
+                              libxl__devid_to_localdev(gc, device.devid));
+    LOG(DEBUG, "locally attached disk %s", dls->diskpath);
 
     dls->callback(egc, dls, 0);
     return;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 8bb5e93..f5b8bf6 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -436,6 +436,63 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
     return -1;
 }
 
+static char *encode_disk_name(char *ptr, unsigned int n)
+{
+    if (n >= 26)
+        ptr = encode_disk_name(ptr, n / 26 - 1);
+    *ptr = 'a' + n % 26;
+    return ptr + 1;
+}
+
+char *libxl__devid_to_vdev(libxl__gc *gc, int devid)
+{
+    unsigned int minor;
+    int offset;
+    int nr_parts;
+    char *ptr = NULL;
+/* Same as in Linux.
+ * encode_disk_name might end up using up to 29 bytes (BUFFER_SIZE - 3)
+ * including the trailing \0.
+ *
+ * The code is safe because 26 raised to the power of 28 (that is the
+ * maximum offset that can be stored in the allocated buffer as a
+ * string) is far greater than UINT_MAX on 64 bits so offset cannot be
+ * big enough to exhaust the available bytes in ret. */
+#define BUFFER_SIZE 32
+    char *ret = libxl__zalloc(gc, BUFFER_SIZE);
+
+#define EXT_SHIFT 28
+#define EXTENDED (1<<EXT_SHIFT)
+#define VDEV_IS_EXTENDED(dev) ((dev)&(EXTENDED))
+#define BLKIF_MINOR_EXT(dev) ((dev)&(~EXTENDED))
+/* the size of the buffer to store the device name is 32 bytes to match the
+ * equivalent buffer in the Linux kernel code */
+
+    if (!VDEV_IS_EXTENDED(devid)) {
+        minor = devid & 0xff;
+        nr_parts = 16;
+    } else {
+        minor = BLKIF_MINOR_EXT(devid);
+        nr_parts = 256;
+    }
+    offset = minor / nr_parts;
+
+    strcpy(ret, "xvd");
+    ptr = encode_disk_name(ret + 3, offset);
+    if (minor % nr_parts == 0)
+        *ptr = 0;
+    else
+        /* overflow cannot happen, thanks to the upper bound */
+        snprintf(ptr, ret + 32 - ptr,
+                "%d", minor & (nr_parts - 1));
+    return ret;
+#undef BUFFER_SIZE
+#undef EXT_SHIFT
+#undef EXTENDED
+#undef VDEV_IS_EXTENDED
+#undef BLKIF_MINOR_EXT
+}
+
 /* Device AO operations */
 
 void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 1a3fa35..4d1c51f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1156,6 +1156,7 @@ _hidden int libxl__device_disk_set_backend(libxl__gc*, libxl_device_disk*);
 _hidden int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor);
 _hidden int libxl__device_disk_dev_number(const char *virtpath,
                                           int *pdisk, int *ppartition);
+_hidden char *libxl__devid_to_vdev(libxl__gc *gc, int devid);
 
 _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
                                       libxl__device_console *console,
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 7e6d7b9..8073aec 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -26,56 +26,9 @@ int libxl__try_phy_backend(mode_t st_mode)
     return 0;
 }
 
-#define EXT_SHIFT 28
-#define EXTENDED (1<<EXT_SHIFT)
-#define VDEV_IS_EXTENDED(dev) ((dev)&(EXTENDED))
-#define BLKIF_MINOR_EXT(dev) ((dev)&(~EXTENDED))
-/* the size of the buffer to store the device name is 32 bytes to match the
- * equivalent buffer in the Linux kernel code */
-#define BUFFER_SIZE 32
-
-/* Same as in Linux.
- * encode_disk_name might end up using up to 29 bytes (BUFFER_SIZE - 3)
- * including the trailing \0.
- *
- * The code is safe because 26 raised to the power of 28 (that is the
- * maximum offset that can be stored in the allocated buffer as a
- * string) is far greater than UINT_MAX on 64 bits so offset cannot be
- * big enough to exhaust the available bytes in ret. */
-static char *encode_disk_name(char *ptr, unsigned int n)
-{
-    if (n >= 26)
-        ptr = encode_disk_name(ptr, n / 26 - 1);
-    *ptr = 'a' + n % 26;
-    return ptr + 1;
-}
-
 char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
 {
-    unsigned int minor;
-    int offset;
-    int nr_parts;
-    char *ptr = NULL;
-    char *ret = libxl__zalloc(gc, BUFFER_SIZE);
-
-    if (!VDEV_IS_EXTENDED(devid)) {
-        minor = devid & 0xff;
-        nr_parts = 16;
-    } else {
-        minor = BLKIF_MINOR_EXT(devid);
-        nr_parts = 256;
-    }
-    offset = minor / nr_parts;
-
-    strcpy(ret, "xvd");
-    ptr = encode_disk_name(ret + 3, offset);
-    if (minor % nr_parts == 0)
-        *ptr = 0;
-    else
-        /* overflow cannot happen, thanks to the upper bound */
-        snprintf(ptr, ret + 32 - ptr,
-                "%d", minor & (nr_parts - 1));
-    return ret;
+    return libxl__devid_to_vdev(gc, devid);
 }
 
 /* Hotplug scripts helpers */
-- 
2.5.4 (Apple Git-61)


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

[PATCH v2 6/7] libxl: add a FreeBSD implementation of libxl__devid_to_localdev

[Roger Pau Monne]

This code is extracted from the FreeBSD blkfront implementation.

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

diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
index 8ad347e..551338b 100644
--- a/tools/libxl/libxl_freebsd.c
+++ b/tools/libxl/libxl_freebsd.c
@@ -27,8 +27,58 @@ int libxl__try_phy_backend(mode_t st_mode)
 
 char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
 {
-    /* TODO */
-    return NULL;
+    /* This translation table has been copied from the FreeBSD blkfront code. */
+    const static struct vdev_info {
+        int major;
+        int shift;
+        int base;
+        const char *name;
+    } info[] = {
+        {3,     6,  0,      "ada"}, /* ide0 */
+        {22,    6,  2,      "ada"}, /* ide1 */
+        {33,    6,  4,      "ada"}, /* ide2 */
+        {34,    6,  6,      "ada"}, /* ide3 */
+        {56,    6,  8,      "ada"}, /* ide4 */
+        {57,    6,  10,     "ada"}, /* ide5 */
+        {88,    6,  12,     "ada"}, /* ide6 */
+        {89,    6,  14,     "ada"}, /* ide7 */
+        {90,    6,  16,     "ada"}, /* ide8 */
+        {91,    6,  18,     "ada"}, /* ide9 */
+
+        {8,     4,  0,      "da"},  /* scsi disk0 */
+        {65,    4,  16,     "da"},  /* scsi disk1 */
+        {66,    4,  32,     "da"},  /* scsi disk2 */
+        {67,    4,  48,     "da"},  /* scsi disk3 */
+        {68,    4,  64,     "da"},  /* scsi disk4 */
+        {69,    4,  80,     "da"},  /* scsi disk5 */
+        {70,    4,  96,     "da"},  /* scsi disk6 */
+        {71,    4,  112,    "da"},  /* scsi disk7 */
+        {128,   4,  128,    "da"},  /* scsi disk8 */
+        {129,   4,  144,    "da"},  /* scsi disk9 */
+        {130,   4,  160,    "da"},  /* scsi disk10 */
+        {131,   4,  176,    "da"},  /* scsi disk11 */
+        {132,   4,  192,    "da"},  /* scsi disk12 */
+        {133,   4,  208,    "da"},  /* scsi disk13 */
+        {134,   4,  224,    "da"},  /* scsi disk14 */
+        {135,   4,  240,    "da"},  /* scsi disk15 */
+
+        {202,   4,  0,      "xbd"}, /* xbd */
+
+        {0, 0,  0,  NULL},
+    };
+    int major = devid >> 8;
+    int minor = devid & 0xff;
+    int i;
+
+    if (devid & (1 << 28))
+        return GCSPRINTF("%s%d", "xbd", (devid & ((1 << 28) - 1)) >> 8);
+
+    for (i = 0; info[i].major; i++)
+        if (info[i].major == major)
+            return GCSPRINTF("%s%d", info[i].name,
+                             info[i].base + (minor >> info[i].shift));
+
+    return GCSPRINTF("%s%d", "xbd", minor >> 4);
 }
 
 /* Hotplug scripts caller functions */
-- 
2.5.4 (Apple Git-61)


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

[PATCH v2 7/7] libxl: fix error message in local_device_attach_cb

[Roger Pau Monne]

The fields that are printed might not be set in the case of a failure, which
generates a segmentation fault.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d55c699..d5a0a01 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3144,10 +3144,7 @@ static void local_device_attach_cb(libxl__egc *egc, libxl__ao_device *aodev)
 
     rc = aodev->rc;
     if (rc) {
-        LOGE(ERROR, "unable to %s %s with id %u",
-                    libxl__device_action_to_string(aodev->action),
-                    libxl__device_kind_to_string(aodev->dev->kind),
-                    aodev->dev->devid);
+        LOGE(ERROR, "unable locally attach device: %s", disk->pdev_path);
         goto out;
     }
 
-- 
2.5.4 (Apple Git-61)


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

Re: [PATCH v2 0/7] libxl: add support for FreeBSD block hotplug scripts

[George Dunlap]

On Thu, Feb 25, 2016 at 7:25 PM, Roger Pau Monne <roger.pau@citrix.com> wrote:
> This series enables using hotplug scripts with the FreeBSD blkback
> implementation. Since FreeBSD blkback can use both block devices and regular
> RAW files as disks, the physical-device xenstore backend node is now
> OS-specific, Linux and NetBSD will encode the device major and minor numbers
> there, while FreeBSD simply puts an absolute path to a disk image.

Just to catch me up here -- is this an incompatible thing  that
FreeBSD *already* does, or something you're adding with this series?

 -George

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

Re: [PATCH v2 0/7] libxl: add support for FreeBSD block hotplug scripts

[Roger Pau Monné]

El 29/2/16 a les 13:15, George Dunlap ha escrit:
> On Thu, Feb 25, 2016 at 7:25 PM, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> This series enables using hotplug scripts with the FreeBSD blkback
>> implementation. Since FreeBSD blkback can use both block devices and regular
>> RAW files as disks, the physical-device xenstore backend node is now
>> OS-specific, Linux and NetBSD will encode the device major and minor numbers
>> there, while FreeBSD simply puts an absolute path to a disk image.
> 
> Just to catch me up here -- is this an incompatible thing  that
> FreeBSD *already* does, or something you're adding with this series?

I assume you mean the usage of the "physical-device" node, right? In
which case, this is something that I'm adding with this series (and some
FreeBSD kernel changes, of course). Current FreeBSD code doesn't use
physical-device at all.

Roger.


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

Re: [PATCH v2 0/7] libxl: add support for FreeBSD block hotplug scripts

[George Dunlap]

On 29/02/16 12:18, Roger Pau Monné wrote:
> El 29/2/16 a les 13:15, George Dunlap ha escrit:
>> On Thu, Feb 25, 2016 at 7:25 PM, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> This series enables using hotplug scripts with the FreeBSD blkback
>>> implementation. Since FreeBSD blkback can use both block devices and regular
>>> RAW files as disks, the physical-device xenstore backend node is now
>>> OS-specific, Linux and NetBSD will encode the device major and minor numbers
>>> there, while FreeBSD simply puts an absolute path to a disk image.
>>
>> Just to catch me up here -- is this an incompatible thing  that
>> FreeBSD *already* does, or something you're adding with this series?
> 
> I assume you mean the usage of the "physical-device" node, right? In
> which case, this is something that I'm adding with this series (and some
> FreeBSD kernel changes, of course). Current FreeBSD code doesn't use
> physical-device at all.

So there are cases where libxl could use the actual path to the
resulting device (if available) as well.  Rather than overloading
physical-device, what about making a new node, with a path, that can be
used by all of them?

I submitted such a patch here:

<1439233885-22218-4-git-send-email-george.dunlap@eu.citrix.com>

As part of a series allowing HVM domains to use hotplug scripts.

Thoughts?

 -George

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

Re: [PATCH v2 0/7] libxl: add support for FreeBSD block hotplug scripts

[Roger Pau Monné]

El 29/2/16 a les 15:26, George Dunlap ha escrit:
> On 29/02/16 12:18, Roger Pau Monné wrote:
>> El 29/2/16 a les 13:15, George Dunlap ha escrit:
>>> On Thu, Feb 25, 2016 at 7:25 PM, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>>> This series enables using hotplug scripts with the FreeBSD blkback
>>>> implementation. Since FreeBSD blkback can use both block devices and regular
>>>> RAW files as disks, the physical-device xenstore backend node is now
>>>> OS-specific, Linux and NetBSD will encode the device major and minor numbers
>>>> there, while FreeBSD simply puts an absolute path to a disk image.
>>>
>>> Just to catch me up here -- is this an incompatible thing  that
>>> FreeBSD *already* does, or something you're adding with this series?
>>
>> I assume you mean the usage of the "physical-device" node, right? In
>> which case, this is something that I'm adding with this series (and some
>> FreeBSD kernel changes, of course). Current FreeBSD code doesn't use
>> physical-device at all.
> 
> So there are cases where libxl could use the actual path to the
> resulting device (if available) as well.  Rather than overloading
> physical-device, what about making a new node, with a path, that can be
> used by all of them?
> 
> I submitted such a patch here:
> 
> <1439233885-22218-4-git-send-email-george.dunlap@eu.citrix.com>
> 
> As part of a series allowing HVM domains to use hotplug scripts.
> 
> Thoughts?

TBH, I thought we were planning to use local attach to deal with both
HVM guests with hotplug scripts and HVM guests using disks on driver
domains. The solution you propose only solves the first part (hotplug
scripts), but for disks coming from driver domains we would still need
to use local-attach, so I would be in favour of always using local attach.

Roger.


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

Re: [PATCH v2 0/7] libxl: add support for FreeBSD block hotplug scripts

[George Dunlap]

On 29/02/16 16:08, Roger Pau Monné wrote:
> El 29/2/16 a les 15:26, George Dunlap ha escrit:
>> On 29/02/16 12:18, Roger Pau Monné wrote:
>>> El 29/2/16 a les 13:15, George Dunlap ha escrit:
>>>> On Thu, Feb 25, 2016 at 7:25 PM, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>>>> This series enables using hotplug scripts with the FreeBSD blkback
>>>>> implementation. Since FreeBSD blkback can use both block devices and regular
>>>>> RAW files as disks, the physical-device xenstore backend node is now
>>>>> OS-specific, Linux and NetBSD will encode the device major and minor numbers
>>>>> there, while FreeBSD simply puts an absolute path to a disk image.
>>>>
>>>> Just to catch me up here -- is this an incompatible thing  that
>>>> FreeBSD *already* does, or something you're adding with this series?
>>>
>>> I assume you mean the usage of the "physical-device" node, right? In
>>> which case, this is something that I'm adding with this series (and some
>>> FreeBSD kernel changes, of course). Current FreeBSD code doesn't use
>>> physical-device at all.
>>
>> So there are cases where libxl could use the actual path to the
>> resulting device (if available) as well.  Rather than overloading
>> physical-device, what about making a new node, with a path, that can be
>> used by all of them?
>>
>> I submitted such a patch here:
>>
>> <1439233885-22218-4-git-send-email-george.dunlap@eu.citrix.com>
>>
>> As part of a series allowing HVM domains to use hotplug scripts.
>>
>> Thoughts?
> 
> TBH, I thought we were planning to use local attach to deal with both
> HVM guests with hotplug scripts and HVM guests using disks on driver
> domains. The solution you propose only solves the first part (hotplug
> scripts), but for disks coming from driver domains we would still need
> to use local-attach, so I would be in favour of always using local attach.

So the bootloader path basically has a feature where it says, "If you
can find a local path, just use it; otherwise do local attach".  This
seems like a sensible path to take, as it avoids going through the whole
process of doing a local attach / detach when the disk is already
available.  It seems like doing the same thing for qemu, and for disks
with hotplug scripts, makes a lot of sense.

Additionally, I doubt I'm going to have time to do anything wrt local
attach before 4.7; this (with the appropriate libxl additions) could
allow HVM guests for non-driver domains to have full parity with PV
guests in a relatively simple way.

 -George



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

Re: [PATCH v2 1/7] blkif: document how FreeBSD uses the physical-device backend node

[Wei Liu]

On Thu, Feb 25, 2016 at 08:25:12PM +0100, Roger Pau Monne wrote:
> FreeBSD blkback uses the physical-device xenstore node in order to fetch the
> path to the underlying backing storage (either a block device or raw image).
> This node is set by the hotplug scripts.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Tim Deegan <tim@xen.org>
> ---
>  xen/include/public/io/blkif.h | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 99f0326..9a5baa4 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -89,14 +89,18 @@
>   *      Values:         string
>   *
>   *      A free formatted string providing sufficient information for the
> - *      backend driver to open the backing device.  (e.g. the path to the
> - *      file or block device representing the backing store.)
> + *      hotplug script to attach the device and provide a suitable
> + *      handler (ie: a block device) for blkback to use.
>   *
> - * physical-device
> - *      Values:         "MAJOR:MINOR"
> + * physical-device:
> + *      Values:         OS specific
>   *
> - *      MAJOR and MINOR are the major number and minor number of the
> - *      backing device respectively.
> + *      A free formatted string that contains the OS specific path to the
> + *      backing device for this blkback instance.
> + *
> + *      Linux and NetBSD encode the block device major and minor numbers using
> + *      the following printf format: "%x:%x", while FreeBSD places the path to
> + *      the file or device in the filesystem.

I think this is a sensible approach FWIW.

Wei.

>   *
>   * type
>   *      Values:         "file", "phy", "tap"
> -- 
> 2.5.4 (Apple Git-61)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH v2 2/7] libxl: introduce an OS-specific function to get the physical-device

[Wei Liu]

On Thu, Feb 25, 2016 at 08:25:13PM +0100, Roger Pau Monne wrote:
> Linux and NetBSD will return the device major and minor numbers encoded in
> hex and separated by a ":". FreeBSD on the other hand returns the path to
> the block device or image file.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

The code looks sensible.

Acked-by: Wei Liu <wei.liu2@citrix.com>

The acceptance of the patch depends on whether we come to agreement to
use the node as proposed in patch #1.

> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl.c          |  9 +++++----
>  tools/libxl/libxl_freebsd.c  |  6 ++++++
>  tools/libxl/libxl_internal.h |  6 ++++++
>  tools/libxl/libxl_linux.c    | 10 ++++++++++
>  tools/libxl/libxl_netbsd.c   | 10 ++++++++++
>  5 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2d18b8d..6d719d7 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2515,10 +2515,11 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
>                   */
>                  if (!disk->script &&
>                      disk->backend_domid == LIBXL_TOOLSTACK_DOMID) {
> -                    int major, minor;
> -                    if (!libxl__device_physdisk_major_minor(dev, &major, &minor))
> -                        flexarray_append_pair(back, "physical-device",
> -                                              GCSPRINTF("%x:%x", major, minor));
> +                    char *physdev;
> +
> +                    physdev = libxl__get_physical_device(dev);
> +                    if (physdev != NULL)
> +                        flexarray_append_pair(back, "physical-device", physdev);
>                  }
>  
>                  assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
> diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
> index 47c3391..483f36e 100644
> --- a/tools/libxl/libxl_freebsd.c
> +++ b/tools/libxl/libxl_freebsd.c
> @@ -143,3 +143,9 @@ int libxl__pci_topology_init(libxl__gc *gc,
>  {
>      return ERROR_NI;
>  }
> +
> +char *libxl__get_physical_device(char *dev)
> +{
> +

No need to have this extra blank line.


Wei.

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

Re: [PATCH v2 3/7] hotplug/FreeBSD: add block hotplug script

[Wei Liu]

On Thu, Feb 25, 2016 at 08:25:14PM +0100, Roger Pau Monne wrote:
> This is the default hotplug script for block devices. Its only job is to
> copy the "params" blkback xenstore node to "physical-device".
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v2 4/7] libxl/FreeBSD: add support for disk hotplug scripts

[Wei Liu]

On Thu, Feb 25, 2016 at 08:25:15PM +0100, Roger Pau Monne wrote:
> Allow FreeBSD to execute hotplug scripts when attaching disk devices.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_freebsd.c | 114 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 83 insertions(+), 31 deletions(-)
> 

It's better to either break the refactoring to another patch or
explicitly say there is refactoring in this patch.

> +static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
> +                               char ***args, char ***env,
> +                               libxl__device_action action)
> +{
> +    char *be_path = libxl__device_backend_path(gc, dev);
> +    char *script;
> +    int nr = 0, rc;
> +
> +    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);
> +        rc = ERROR_FAIL;
> +        goto error;
> +    }
> +
> +    const int arraysize = 4;
> +    GCNEW_ARRAY(*args, arraysize);
> +    (*args)[nr++] = script;
> +    (*args)[nr++] = be_path;
> +    (*args)[nr++] = (char *) libxl__device_action_to_string(action);
> +    (*args)[nr++] = NULL;
> +    assert(nr == arraysize);
> +
> +    rc = 1;
> +
> +error:
> +    return rc;
> +}
> +

The code looks fine.

Wei.

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

Re: [PATCH v2 5/7] libxl: properly use vdev vs local device

[Wei Liu]

On Thu, Feb 25, 2016 at 08:25:16PM +0100, Roger Pau Monne wrote:
> The current code in libxl assumed that vdev is equal to local device, but
> this is only true for Linux systems. In other OSes the local device can use
> a nomenclature completely different from the virtual device one.
> 
> Move the current libxl__devid_to_localdev Linux implementation out of the
> OS-specific file and rename it to libxl__devid_to_vdev, and then make sure
> local_device_attach_cb return the local device in the diskpath field.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v2 6/7] libxl: add a FreeBSD implementation of libxl__devid_to_localdev

[Wei Liu]

On Thu, Feb 25, 2016 at 08:25:17PM +0100, Roger Pau Monne wrote:
> This code is extracted from the FreeBSD blkfront implementation.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v2 7/7] libxl: fix error message in local_device_attach_cb

[Wei Liu]

On Thu, Feb 25, 2016 at 08:25:18PM +0100, Roger Pau Monne wrote:
> The fields that are printed might not be set in the case of a failure, which
> generates a segmentation fault.
> 

Right, I can see how this can fail.

In device_addrm_aocomplete we check if aodev is set. But I think that's
a bit overkill for local attach.

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index d55c699..d5a0a01 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3144,10 +3144,7 @@ static void local_device_attach_cb(libxl__egc *egc, libxl__ao_device *aodev)
>  
>      rc = aodev->rc;
>      if (rc) {
> -        LOGE(ERROR, "unable to %s %s with id %u",
> -                    libxl__device_action_to_string(aodev->action),
> -                    libxl__device_kind_to_string(aodev->dev->kind),
> -                    aodev->dev->devid);
> +        LOGE(ERROR, "unable locally attach device: %s", disk->pdev_path);
>          goto out;
>      }
>  
> -- 
> 2.5.4 (Apple Git-61)
> 

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

Re: [PATCH v2 0/7] libxl: add support for FreeBSD block hotplug scripts

[Roger Pau Monné]

El 29/2/16 a les 17:45, George Dunlap ha escrit:
> On 29/02/16 16:08, Roger Pau Monné wrote:
>> El 29/2/16 a les 15:26, George Dunlap ha escrit:
>>> On 29/02/16 12:18, Roger Pau Monné wrote:
>>>> El 29/2/16 a les 13:15, George Dunlap ha escrit:
>>>>> On Thu, Feb 25, 2016 at 7:25 PM, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>>>>> This series enables using hotplug scripts with the FreeBSD blkback
>>>>>> implementation. Since FreeBSD blkback can use both block devices and regular
>>>>>> RAW files as disks, the physical-device xenstore backend node is now
>>>>>> OS-specific, Linux and NetBSD will encode the device major and minor numbers
>>>>>> there, while FreeBSD simply puts an absolute path to a disk image.
>>>>>
>>>>> Just to catch me up here -- is this an incompatible thing  that
>>>>> FreeBSD *already* does, or something you're adding with this series?
>>>>
>>>> I assume you mean the usage of the "physical-device" node, right? In
>>>> which case, this is something that I'm adding with this series (and some
>>>> FreeBSD kernel changes, of course). Current FreeBSD code doesn't use
>>>> physical-device at all.
>>>
>>> So there are cases where libxl could use the actual path to the
>>> resulting device (if available) as well.  Rather than overloading
>>> physical-device, what about making a new node, with a path, that can be
>>> used by all of them?
>>>
>>> I submitted such a patch here:
>>>
>>> <1439233885-22218-4-git-send-email-george.dunlap@eu.citrix.com>
>>>
>>> As part of a series allowing HVM domains to use hotplug scripts.
>>>
>>> Thoughts?
>>
>> TBH, I thought we were planning to use local attach to deal with both
>> HVM guests with hotplug scripts and HVM guests using disks on driver
>> domains. The solution you propose only solves the first part (hotplug
>> scripts), but for disks coming from driver domains we would still need
>> to use local-attach, so I would be in favour of always using local attach.
> 
> So the bootloader path basically has a feature where it says, "If you
> can find a local path, just use it; otherwise do local attach".  This
> seems like a sensible path to take, as it avoids going through the whole
> process of doing a local attach / detach when the disk is already
> available.  It seems like doing the same thing for qemu, and for disks
> with hotplug scripts, makes a lot of sense.
> 
> Additionally, I doubt I'm going to have time to do anything wrt local
> attach before 4.7; this (with the appropriate libxl additions) could
> allow HVM guests for non-driver domains to have full parity with PV
> guests in a relatively simple way.

Right, I'm not opposed to this. I can
s/physical-device/physical-device-path/ in my series, but I would like
to have a confirmation that this is the way we are going to proceed forward.

In fact, there's a user on the FreeBSD-Xen ML that's interested in
working on hotplug script support for HVM guests, and I've told him that
the right way to solve this would be to perform a local-attach and use
the resulting device as the disk that's passed to QEMU.

Roger.


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

Re: [PATCH v2 1/7] blkif: document how FreeBSD uses the physical-device backend node

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 1/7] blkif: document how FreeBSD uses the physical-device backend node"):
> FreeBSD blkback uses the physical-device xenstore node in order to fetch the
> path to the underlying backing storage (either a block device or raw image).
> This node is set by the hotplug scripts.

I think it is undesirable that the interface to hotplug scripts would
be platform-specific.

I would prefer it if hotplug scripts always produced a path, and libxl
were responsible for converting it into a major/minor if required.
(libxl already has to have platform-specific code to drive its driver
domain's blkback).

This would be a change to the hotplug script protocol but I think it
can be done in a backward-compatible way.

What do you think ?

(This is related to the conversation you and George had on irc.)

Ian.

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