xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts
@ 2016-03-21 18:16 George Dunlap
  2016-03-21 18:16 ` [PATCH v2 1/9] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
                   ` (9 more replies)
  0 siblings, 10 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:16 UTC (permalink / raw)
  To: xen-devel
  Cc: Anthony Perard, Ian Jackson, Wei Liu, George Dunlap, Roger Pau Monne

In order for HVM domains to provide emulated access to disks provided
by hotplug scripts, qemu needs access to a "cooked" version of the
disk.  In the case of hotplug scripts, this "cooked" version is
available in the form of a block device passed to blkback.  Make this
"cooked" version available to qemu.

This series also starts to work towards a rationalized interface to
the block hotplug scripts, on which hotplug scripts for FreeBSD can be
added.

Changes since v1:
- Split one of the patches into two


George Dunlap (8):
  tools/hotplug: Add a "dummy" hotplug script for testing
  libxl: Remove redundant setting of phyical-device
  tools/hotplug: Write physical-device-path in addition to
    physical-device
  libxl: Move check for local access to a funciton
  libxl: Rearrange qemu upstream disk argument code
  libxl: Share logic for finding path between qemuu and pygrub
  libxl: Allow local access for block devices with hotplug scripts
  docs: Document block-script protocol

Ian Jackson (1):
  DO NOT APPLY libxl: Change hotplug script interface to use
    physical-device-path

 docs/misc/block-scripts.txt         |  85 ++++++++++++++++++++++++
 tools/hotplug/Linux/Makefile        |   1 +
 tools/hotplug/Linux/block-common.sh |  16 ++---
 tools/hotplug/Linux/block-dummy     | 107 ++++++++++++++++++++++++++++++
 tools/libxl/libxl.c                 | 128 ++++++++++++++++++++++++++----------
 tools/libxl/libxl_dm.c              |  82 +++++++++++++++--------
 tools/libxl/libxl_internal.h        |  11 +++-
 tools/libxl/libxl_linux.c           |  70 +++++++++++++++++++-
 8 files changed, 422 insertions(+), 78 deletions(-)
 create mode 100644 docs/misc/block-scripts.txt
 create mode 100644 tools/hotplug/Linux/block-dummy

-- 
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
CC: Anthony Perard <anthony.perard@citrix.com>


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 1/9] tools/hotplug: Add a "dummy" hotplug script for testing
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
@ 2016-03-21 18:16 ` George Dunlap
  2016-03-21 18:16 ` [PATCH v2 2/9] libxl: Remove redundant setting of phyical-device George Dunlap
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:16 UTC (permalink / raw)
  To: xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monne

From: George Dunlap <george.dunlap@eu.citrix.com>

Testing the hotplug external script path at the moment involves
actually setting up one of the alternate datapaths (blktap, iscsi,
&c).  Simplify testing by making a script which does a simple loopback,
but still has a target that can't be used directly.

To use:

script=block-dummy,vdev=xvda,target=dummy:<file>

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/hotplug/Linux/Makefile    |   1 +
 tools/hotplug/Linux/block-dummy | 107 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+)

diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 6e10118..dcfd58a 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -27,6 +27,7 @@ XEN_SCRIPTS += vscsi
 XEN_SCRIPTS += block-iscsi
 XEN_SCRIPTS += block-tap
 XEN_SCRIPTS += block-drbd-probe
+XEN_SCRIPTS += block-dummy
 XEN_SCRIPTS += $(XEN_SCRIPTS-y)
 
 SUBDIRS-$(CONFIG_SYSTEMD) += systemd
diff --git a/tools/hotplug/Linux/block-dummy b/tools/hotplug/Linux/block-dummy
new file mode 100644
index 0000000..57d40b5
--- /dev/null
+++ b/tools/hotplug/Linux/block-dummy
@@ -0,0 +1,107 @@
+#!/bin/bash -e
+#
+# dummy Xen block device hotplug script
+#
+# Author George Dunlap <george.dunlap@eu.citrix.com>
+#
+# Based on block-iscsi by 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.
+#
+# Usage:
+#
+# Target should be specified using the following syntax:
+#
+# script=block-dummy,vdev=xvda,target=dummy:<file>
+
+dir=$(dirname "$0")
+. "$dir/block-common.sh"
+
+check_tools()
+{
+    if ! command -v losetup > /dev/null 2>&1; then
+        fatal "Unable to find losetup"
+    fi
+}
+
+# Sets the following global variables based on the params field passed in as
+# a parameter: type file
+parse_target()
+{
+    params=($(echo "$1" | tr ":" "\n"))
+
+    type=${params[0]}
+    file=${params[1]}
+    if [ -z "$type" ] || [ -z "$file" ]; then
+        fatal "Cannot parse required parameters"
+    fi
+
+    if [ "$type" != "dummy" ] ; then
+	fatal "Invalid type: $type"
+    fi
+}
+
+# Attaches the device and writes xenstore backend entries to connect
+# the device
+add()
+{
+    test -f "$file" || fatal "$file does not exist."
+
+    loopdev=$(losetup -f 2>/dev/null || find_free_loopback_dev)
+    if [ "$loopdev" = '' ]
+    then
+        fatal 'Failed to find an unused loop device'
+    fi
+    
+    if LANG=C losetup -h 2>&1 | grep read-only >/dev/null
+    then
+        roflag="-$mode"; roflag="${roflag#-w}"; roflag="${roflag#-!}"
+    else
+        roflag=''
+    fi
+
+    do_or_die losetup $roflag "$loopdev" "$file"
+    # FIXME Is this OK?
+    xenstore_write "$XENBUS_PATH/node" "$loopdev"
+    write_dev "$loopdev"
+}
+
+# Disconnects the device
+remove()
+{
+    node=$(xenstore_read "$XENBUS_PATH/node")
+    losetup -d "$node"
+}
+
+command=$1
+target=$(xenstore-read $XENBUS_PATH/params || true)
+if [ -z "$target" ]; then
+    fatal "No information about the target"
+fi
+
+parse_target "$target"
+
+mode=$(xenstore_read "$XENBUS_PATH/mode")
+mode=$(canonicalise_mode "$mode")
+
+check_tools || exit 1
+
+case $command in
+add)
+    add
+    ;;
+remove)
+    remove
+    ;;
+*)
+    exit 1
+    ;;
+esac
-- 
2.1.4


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 2/9] libxl: Remove redundant setting of phyical-device
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
  2016-03-21 18:16 ` [PATCH v2 1/9] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
@ 2016-03-21 18:16 ` George Dunlap
  2016-03-21 18:16 ` [PATCH v2 3/9] tools/hotplug: Write physical-device-path in addition to physical-device George Dunlap
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:16 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Wei Liu, George Dunlap, Roger Pau Monne

Regardless of whether we're running a custom hotplug script or using
normal phy: or file:, the "block" script will be run, which will set
all the necessary xenstore nodes.

In fact, writing this value here prevents the block script from
accomplishing its only purpose: to detect duplicate physical block
devices used in different virtual devices.  The first thing the block
script does is check to see if this node is written; and if it is, it
silently exits.

Remove this, and let the block script perform its duplicate checking
function.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
NOTE: It's likely that the duplicate checking for physical devices has
never been run under libxl (at least since this bug was introduced);
this may shake out some issues.

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/libxl/libxl.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3471c4c..93f50d3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2440,21 +2440,6 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
                                          libxl__xen_script_dir_path());
                 flexarray_append_pair(back, "script", script);
 
-                /* If the user did not supply a block script then we
-                 * write the physical-device node ourselves.
-                 *
-                 * If the user did supply a script then that script is
-                 * responsible for this since the block device may not
-                 * exist yet.
-                 */
-                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));
-                }
-
                 assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
                 break;
 
-- 
2.1.4


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 3/9] tools/hotplug: Write physical-device-path in addition to physical-device
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
  2016-03-21 18:16 ` [PATCH v2 1/9] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
  2016-03-21 18:16 ` [PATCH v2 2/9] libxl: Remove redundant setting of phyical-device George Dunlap
@ 2016-03-21 18:16 ` George Dunlap
  2016-03-21 18:16 ` [PATCH v2 4/9] libxl: Move check for local access to a funciton George Dunlap
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:16 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson, George Dunlap, Roger Pau Monne

Change block-common.sh on Linux to write physical-device-path with the
path of the device node, in addition to physical-device with its
major:minor numbers.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
NB that a later patch in this series introduces some documentation for
the block script protocol.  If it's wanted I could fold that into this
patch; or if we wanted to be really strict, I could check it in before
this patch (modifying it so that it's accurate at that point in time)
and then have this patch update it.

CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/block-common.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/hotplug/Linux/block-common.sh b/tools/hotplug/Linux/block-common.sh
index ee95009..35110b4 100644
--- a/tools/hotplug/Linux/block-common.sh
+++ b/tools/hotplug/Linux/block-common.sh
@@ -67,7 +67,8 @@ write_dev() {
   fi
  
   xenstore_write "$XENBUS_PATH/physical-device" "$mm"
-
+  xenstore_write "$XENBUS_PATH/physical-device-path" "$1"
+  
   success
 }
 
-- 
2.1.4


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 4/9] libxl: Move check for local access to a funciton
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (2 preceding siblings ...)
  2016-03-21 18:16 ` [PATCH v2 3/9] tools/hotplug: Write physical-device-path in addition to physical-device George Dunlap
@ 2016-03-21 18:16 ` George Dunlap
  2016-03-21 18:16 ` [PATCH v2 5/9] libxl: Rearrange qemu upstream disk argument code George Dunlap
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:16 UTC (permalink / raw)
  To: xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monne

From: George Dunlap <george.dunlap@eu.citrix.com>

Move pygrub checks for local access ability into a separate function.

Also reorganize libxl__device_disk_local_initiate_attach so that we
don't initialize dls->disk unless we actually end up doing a local
attach.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes since v1:
- Stylistic updates
 - No space between * and function name in libxl__device_disk_find_local_path
 - { on start of newline for function
 - Long line now under 80 characters
 - Remove redundant check for pdev_path
 - Space between if and condition
 - Avoid if ((x=...)) construction

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/libxl/libxl.c | 67 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 93f50d3..2d7a154 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3000,13 +3000,38 @@ static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user,
 
 /* Callbacks */
 
+static char *libxl__device_disk_find_local_path(libxl__gc *gc, 
+                                                const libxl_device_disk *disk)
+{
+    char *path = NULL;
+
+    /* No local paths for driver domains */
+    if (disk->backend_domname != NULL) {
+        LOG(DEBUG, "Non-local backend, can't access locally.\n");
+        goto out;
+    }
+
+    /*
+     * If this is in raw format, and we're not using a script or a
+     * driver domain, we can access the target path directly.
+     */
+    if (disk->format == LIBXL_DISK_FORMAT_RAW
+        && disk->script == NULL) {
+        path = libxl__strdup(gc, disk->pdev_path);
+        LOG(DEBUG, "Directly accessing local RAW disk %s", path);
+        goto out;
+    } 
+
+ out:
+    return path;
+}
+
 static void local_device_attach_cb(libxl__egc *egc, libxl__ao_device *aodev);
 
 void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
                                      libxl__disk_local_state *dls)
 {
     STATE_AO_GC(dls->ao);
-    char *dev = NULL;
     int rc;
     const libxl_device_disk *in_disk = dls->in_disk;
     libxl_device_disk *disk = &dls->disk;
@@ -3014,34 +3039,36 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
 
     assert(in_disk->pdev_path);
 
-    memcpy(disk, in_disk, sizeof(libxl_device_disk));
-    disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
-    if (in_disk->script != NULL)
-        disk->script = libxl__strdup(gc, in_disk->script);
     disk->vdev = NULL;
 
-    rc = libxl__device_disk_setdefault(gc, disk);
-    if (rc) goto out;
+    if (dls->diskpath)
+        LOG(DEBUG, "Strange, dls->diskpath already set: %s", dls->diskpath);
 
-    /* If this is in a driver domain, or it's not a raw format, or it involves
-     * running a script, we have to do a local attach. */
-    if (disk->backend_domname != NULL
-        || disk->format != LIBXL_DISK_FORMAT_RAW
-        || disk->script != NULL) {
+    LOG(DEBUG, "Trying to find local path");
+
+    dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk);
+    if (dls->diskpath) {
+        LOG(DEBUG, "Local path found, executing callback.");
+        dls->callback(egc, dls, 0);
+    } else {
+        LOG(DEBUG, "Local path not found, initiating attach.");
+
+        memcpy(disk, in_disk, sizeof(libxl_device_disk));
+        disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
+        if (in_disk->script != NULL)
+            disk->script = libxl__strdup(gc, in_disk->script);
+        disk->vdev = NULL;
+        
+        rc = libxl__device_disk_setdefault(gc, disk);
+        if (rc) goto out;
+
+        /* If we can't find a local path, attach it */
         libxl__prepare_ao_device(ao, &dls->aodev);
         dls->aodev.callback = local_device_attach_cb;
         device_disk_add(egc, LIBXL_TOOLSTACK_DOMID, disk, &dls->aodev,
                         libxl__alloc_vdev, (void *) blkdev_start);
-        return;
     }
 
-    LOG(DEBUG, "locally attaching RAW disk %s", disk->pdev_path);
-    dev = disk->pdev_path;
-
-    if (dev != NULL)
-        dls->diskpath = libxl__strdup(gc, dev);
-
-    dls->callback(egc, dls, 0);
     return;
 
  out:
-- 
2.1.4


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 5/9] libxl: Rearrange qemu upstream disk argument code
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (3 preceding siblings ...)
  2016-03-21 18:16 ` [PATCH v2 4/9] libxl: Move check for local access to a funciton George Dunlap
@ 2016-03-21 18:16 ` George Dunlap
  2016-03-21 18:16 ` [PATCH v2 6/9] libxl: Share logic for finding path between qemuu and pygrub George Dunlap
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:16 UTC (permalink / raw)
  To: xen-devel
  Cc: Anthony Perard, Ian Jackson, Wei Liu, George Dunlap, Stefano Stabellini

Reorganize the qemuu disk argument code to make a clean separation
between finding a file to use, and constructing the parameters:

* Rename pdev_path to target_path

* Only use qemu_disk_format_string() in circumstances where qemu may
be interpreting the disk (i.e., backend==QDISK).  In all other cases,
it should use RAW.

* Share as much as possible between the is_cdrom path and the normal
path.

This is mainly prep for sharing the local path finder with the
bootloader; but it does allow cdroms to use any backend that a normal
disk can use. Previously this was limited to RAW files or things that
qemu could handle directly; as of this changeset, it now includes tap
disks; and in future changesets it will include backends with custom
block scripts.

NB that this retains an existing bug, that disks with custom block
scripts or non-dom0 backends will have the bogus pdev_path passed in
to qemu, most likely resulting in qemu exiting with an error.  This
will be fixed in follow-up patches.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Changes since v1:
- Move qemuu disk argument refactoring into a separate patch

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Anthony Perard <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dm.c | 61 +++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4aca38e..dfcf141 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1161,9 +1161,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             int disk, part;
             int dev_number =
                 libxl__device_disk_dev_number(disks[i].vdev, &disk, &part);
-            const char *format = qemu_disk_format_string(disks[i].format);
+            const char *format;
             char *drive;
-            const char *pdev_path;
+            const char *target_path;
 
             if (dev_number == -1) {
                 LOG(WARN, "unable to determine"" disk number for %s",
@@ -1171,22 +1171,22 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                 continue;
             }
 
-            if (disks[i].is_cdrom) {
-                if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
-                    drive = libxl__sprintf
-                        (gc, "if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
-                         disk, disks[i].readwrite ? "off" : "on", dev_number);
-                else
-                    drive = libxl__sprintf
-                        (gc, "file=%s,if=ide,index=%d,readonly=%s,media=cdrom,format=%s,cache=writeback,id=ide-%i",
-                         disks[i].pdev_path, disk, disks[i].readwrite ? "off" : "on", format, dev_number);
-            } else {
-                if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
+            /* 
+             * If qemu isn't doing the interpreting, the parameter is
+             * always raw
+             */
+            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK)
+                format = qemu_disk_format_string(disks[i].format);
+            else
+                format = qemu_disk_format_string(LIBXL_DISK_FORMAT_RAW);
+
+            if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
+                if (!disks[i].is_cdrom) {
                     LOG(WARN, "cannot support"" empty disk format for %s",
                         disks[i].vdev);
                     continue;
                 }
-
+            } else {
                 if (format == NULL) {
                     LOG(WARN,
                         "unable to determine"" disk image format %s",
@@ -1194,14 +1194,22 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                     continue;
                 }
 
-                if (disks[i].backend == LIBXL_DISK_BACKEND_TAP) {
-                    format = qemu_disk_format_string(LIBXL_DISK_FORMAT_RAW);
-                    pdev_path = libxl__blktap_devpath(gc, disks[i].pdev_path,
-                                                      disks[i].format);
-                } else {
-                    pdev_path = disks[i].pdev_path;
-                }
+                if (disks[i].backend == LIBXL_DISK_BACKEND_TAP)
+                    target_path = libxl__blktap_devpath(gc, disks[i].pdev_path,
+                                                        disks[i].format);
+                else     
+                    target_path = disks[i].pdev_path;
+            }
+
+            if (disks[i].is_cdrom) {
+                drive = libxl__sprintf(gc,
+                         "if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
+                         disk, disks[i].readwrite ? "off" : "on", dev_number);
 
+                if (target_path)
+                    drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
+                                           drive, target_path, format);
+            } else {
                 /*
                  * Explicit sd disks are passed through as is.
                  *
@@ -1209,9 +1217,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                  * hd[a-d] and ignore the rest.
                  */
                 if (strncmp(disks[i].vdev, "sd", 2) == 0) {
-                    drive = libxl__sprintf
-                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,readonly=%s,cache=writeback",
-                         pdev_path, disk, format, disks[i].readwrite ? "off" : "on");
+                    drive = libxl__sprintf(gc,
+                             "file=%s,if=scsi,bus=0,unit=%d,format=%s,readonly=%s,cache=writeback",
+                             target_path, disk, format,
+                             disks[i].readwrite ? "off" : "on");
                 } else if (strncmp(disks[i].vdev, "xvd", 3) == 0) {
                     /*
                      * Do not add any emulated disk when PV disk are
@@ -1225,7 +1234,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                     }
                     flexarray_vappend(dm_args, "-drive",
                         GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,cache=writeback",
-                        pdev_path, disk, format),
+                        target_path, disk, format),
                         "-device", GCSPRINTF("ide-hd,bus=ahci0.%d,unit=0,drive=ahcidisk-%d",
                         disk, disk), NULL);
                     continue;
@@ -1236,7 +1245,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                     }
                     drive = libxl__sprintf
                         (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
-                         pdev_path, disk, format);
+                         target_path, disk, format);
                 } else {
                     continue; /* Do not emulate this disk */
                 }
-- 
2.1.4


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 6/9] libxl: Share logic for finding path between qemuu and pygrub
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (4 preceding siblings ...)
  2016-03-21 18:16 ` [PATCH v2 5/9] libxl: Rearrange qemu upstream disk argument code George Dunlap
@ 2016-03-21 18:16 ` George Dunlap
  2016-03-22 10:38   ` George Dunlap
  2016-03-21 18:16 ` [PATCH v2 7/9] libxl: Allow local access for block devices with hotplug scripts George Dunlap
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:16 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, George Dunlap, Stefano Stabellini, Anthony Perard,
	Ian Jackson, Roger Pau Monne

From: George Dunlap <george.dunlap@eu.citrix.com>

qemu can also access disks which will be provided with a qdisk backend
directly; add a flag to libxl__device_disk_find_local_path to indicate
whether to check for qdisk direct access.

Call libxl__device_disk_find_local_path() for most paths.  If we can't
find a local path, print an error and skip the disk, rather than using
a bogus path.

Now if there is no local access to the disk (i.e., because the disk
has a non-local backend, or relies on a custom hotplug script), libxl
will now print a warning and not provide the emulated disk, rather
than providing bogus parameters to qemu which cause it to error out.
(Such disks will still be available via the PV backend.)

I left the libxl__blktap_devpath in the qemuu-specific code rather
than sharing it with the pyrgub code because:

1) When the pygrub path runs the guest disks have not yet been set up

2) libxl__blktap_devpath() will give you the existing devpath if it
already exists, but will set one up for you if you don't.  So on the
pygrub path, this would end up setting up a new tap device.

3) There is no tap-specific teardown code on the pygrub path, and I
don't want to add any (particularly since I'm hoping to remove tapdisk
altogether soon).

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes since v1:
- Re-port on top of revisions to previous patches
- Address line length issues
- Remove stray space at the end of an if (xxx )
- Break out devicemodel argument rearrangement into a separate patch
- Spell out implications of warning messages which skip emulated disks

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Anthony Perard <anthony.perard@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/libxl/libxl.c          | 17 ++++++++++++++---
 tools/libxl/libxl_dm.c       | 27 +++++++++++++++++++++++----
 tools/libxl/libxl_internal.h |  8 ++++++++
 3 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2d7a154..602bec2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3000,8 +3000,9 @@ static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user,
 
 /* Callbacks */
 
-static char *libxl__device_disk_find_local_path(libxl__gc *gc, 
-                                                const libxl_device_disk *disk)
+char *libxl__device_disk_find_local_path(libxl__gc *gc, 
+                                         const libxl_device_disk *disk,
+                                         bool qdisk_direct)
 {
     char *path = NULL;
 
@@ -3022,6 +3023,16 @@ static char *libxl__device_disk_find_local_path(libxl__gc *gc,
         goto out;
     } 
 
+    /*
+     * If we're being called for a qemu path, we can pass the target
+     * string directly as well
+     */
+    if (qdisk_direct && disk->backend == LIBXL_DISK_BACKEND_QDISK) {
+        path = libxl__strdup(gc, disk->pdev_path);
+        LOG(DEBUG, "Directly accessing local QDISK target %s", path);
+        goto out;
+    }
+
  out:
     return path;
 }
@@ -3046,7 +3057,7 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
 
     LOG(DEBUG, "Trying to find local path");
 
-    dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk);
+    dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk, false);
     if (dls->diskpath) {
         LOG(DEBUG, "Local path found, executing callback.");
         dls->callback(egc, dls, 0);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index dfcf141..1493b84 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1182,23 +1182,42 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
 
             if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
                 if (!disks[i].is_cdrom) {
-                    LOG(WARN, "cannot support"" empty disk format for %s",
+                    LOG(WARN, "Cannot support empty disk format for %s",
                         disks[i].vdev);
                     continue;
                 }
             } else {
                 if (format == NULL) {
                     LOG(WARN,
-                        "unable to determine"" disk image format %s",
+                        "Unable to determine disk image format: %s\n",
+                        "Disk will be available via PV drivers but not as an"
+                        "emulated disk.",
                         disks[i].vdev);
                     continue;
                 }
 
+                /* 
+                 * We can't call libxl__blktap_devpath from
+                 * libxl__device_disk_find_local_path for now because
+                 * the bootloader is called before the disks are set
+                 * up, so this function would set up a blktap node,
+                 * but there's no TAP tear-down on error conditions in
+                 * the bootloader path.
+                 */
                 if (disks[i].backend == LIBXL_DISK_BACKEND_TAP)
                     target_path = libxl__blktap_devpath(gc, disks[i].pdev_path,
                                                         disks[i].format);
-                else     
-                    target_path = disks[i].pdev_path;
+                else
+                    target_path = libxl__device_disk_find_local_path(gc, 
+                                                            &disks[i], true);
+
+                if (!target_path) {
+                    LOG(WARN, "No way to get local access disk to image: %s\n"
+                        "Disk will be available via PV drivers but not as an"
+                        "emulated disk.",
+                        disks[i].vdev);
+                    continue;
+                }
             }
 
             if (disks[i].is_cdrom) {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 345a764..268a2f4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2688,6 +2688,14 @@ static inline void libxl__device_disk_local_init(libxl__disk_local_state *dls)
     dls->rc = 0;
 }
 
+/* 
+ * See if we can find a way to access a disk locally
+ */
+_hidden char * libxl__device_disk_find_local_path(libxl__gc *gc, 
+                                                  const libxl_device_disk *disk,
+                                                  bool qdisk_direct);
+
+
 /* Make a disk available in this (the control) domain. Always calls
  * dls->callback when finished.
  * State Idle -> Attaching
-- 
2.1.4


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 7/9] libxl: Allow local access for block devices with hotplug scripts
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (5 preceding siblings ...)
  2016-03-21 18:16 ` [PATCH v2 6/9] libxl: Share logic for finding path between qemuu and pygrub George Dunlap
@ 2016-03-21 18:16 ` George Dunlap
  2016-03-21 18:17 ` [PATCH v2 8/9] docs: Document block-script protocol George Dunlap
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:16 UTC (permalink / raw)
  To: xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monne

From: George Dunlap <george.dunlap@eu.citrix.com>

pygrub and qemuu need to be able to access a VM's disks locally in
order to be able to pull out the kernel and provide emulated disk
access, respectively.  This can be done either by accessing the local
disk directly, or by plugging the target disk into dom0 to allow
access.

Unfortunately, while the plugging machinery works for pygrub, it does
not yet work for qemuu; this means that at the moment, disks with
hotplug scripts or disks with non-dom0 backends cannot be provided as
emulated devices to HVM domains.

Fortunately, disks using hotplug scripts created in dom0 do create a
block device as part of set-up, which can be accessed locally; and if
they use block-common.sh:write_dev, this path will be written to
physical-device-path.

Modify libxl__device_disk_setdefault() to be able to fish this path
out of xenstore and pass it to the caller.

Unfortunately, at the time pygrub runs, the devices have not yet been
set up.  Rather than try to stash the domid somewhere to pass, we just
pass INVALID_DOMID.

This allows qemuu to emulate block devices created with custom hotplug
scripts.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes since v1:
- Ported on top of previous changed patches
- Removed vestigal paragraph from changelog
- Change domid argument to guest_domid
- Added a missing space after an if ()
- Trimmed some long lines

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/libxl/libxl.c          | 43 ++++++++++++++++++++++++++++++++++++++-----
 tools/libxl/libxl_dm.c       |  4 ++--
 tools/libxl/libxl_internal.h |  3 ++-
 3 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 602bec2..944f4d1 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2295,7 +2295,7 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
 }
 
 int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
-                                   libxl_device_disk *disk,
+                                   const libxl_device_disk *disk,
                                    libxl__device *device)
 {
     int devid;
@@ -3001,8 +3001,9 @@ static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user,
 /* Callbacks */
 
 char *libxl__device_disk_find_local_path(libxl__gc *gc, 
-                                         const libxl_device_disk *disk,
-                                         bool qdisk_direct)
+                                          libxl_domid guest_domid,
+                                          const libxl_device_disk *disk,
+                                          bool qdisk_direct)
 {
     char *path = NULL;
 
@@ -3031,6 +3032,38 @@ char *libxl__device_disk_find_local_path(libxl__gc *gc,
         path = libxl__strdup(gc, disk->pdev_path);
         LOG(DEBUG, "Directly accessing local QDISK target %s", path);
         goto out;
+    } 
+
+    /* 
+     * If the format isn't raw and / or we're using a script, then see
+     * if the script has written a path to the "cooked" node
+     */
+    if (disk->script && guest_domid != INVALID_DOMID) {
+        libxl__device device;
+        char *be_path, *pdpath;
+        int rc;
+
+        LOG(DEBUG,
+            "Run from a script; checking for physical-device-path (vdev %s)",
+            disk->vdev);
+
+        rc = libxl__device_from_disk(gc, guest_domid, disk, &device);
+        if (rc < 0)
+            goto out;
+
+        be_path = libxl__device_backend_path(gc, &device);
+
+        pdpath = libxl__sprintf(gc, "%s/physical-device-path", be_path);
+
+        LOG(DEBUG, "Attempting to read node %s", pdpath);
+        path = libxl__xs_read(gc, XBT_NULL, pdpath);
+
+        if (path)
+            LOG(DEBUG, "Accessing cooked block device %s", path);
+        else
+            LOG(DEBUG, "No physical-device-path, can't access locally.");
+
+        goto out;
     }
 
  out:
@@ -3057,7 +3090,8 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
 
     LOG(DEBUG, "Trying to find local path");
 
-    dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk, false);
+    dls->diskpath = libxl__device_disk_find_local_path(gc, INVALID_DOMID,
+                                                       in_disk, false);
     if (dls->diskpath) {
         LOG(DEBUG, "Local path found, executing callback.");
         dls->callback(egc, dls, 0);
@@ -3073,7 +3107,6 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
         rc = libxl__device_disk_setdefault(gc, disk);
         if (rc) goto out;
 
-        /* If we can't find a local path, attach it */
         libxl__prepare_ao_device(ao, &dls->aodev);
         dls->aodev.callback = local_device_attach_cb;
         device_disk_add(egc, LIBXL_TOOLSTACK_DOMID, disk, &dls->aodev,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 1493b84..b581657 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1208,8 +1208,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                     target_path = libxl__blktap_devpath(gc, disks[i].pdev_path,
                                                         disks[i].format);
                 else
-                    target_path = libxl__device_disk_find_local_path(gc, 
-                                                            &disks[i], true);
+                    target_path = libxl__device_disk_find_local_path(gc,
+                                                 guest_domid, &disks[i], true);
 
                 if (!target_path) {
                     LOG(WARN, "No way to get local access disk to image: %s\n"
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 268a2f4..9fde212 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1712,7 +1712,7 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
 _hidden int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params);
 
 _hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
-                                   libxl_device_disk *disk,
+                                   const libxl_device_disk *disk,
                                    libxl__device *device);
 
 /* Calls poll() again - useful to check whether a signaled condition
@@ -2692,6 +2692,7 @@ static inline void libxl__device_disk_local_init(libxl__disk_local_state *dls)
  * See if we can find a way to access a disk locally
  */
 _hidden char * libxl__device_disk_find_local_path(libxl__gc *gc, 
+                                                  libxl_domid guest_domid,
                                                   const libxl_device_disk *disk,
                                                   bool qdisk_direct);
 
-- 
2.1.4


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 8/9] docs: Document block-script protocol
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (6 preceding siblings ...)
  2016-03-21 18:16 ` [PATCH v2 7/9] libxl: Allow local access for block devices with hotplug scripts George Dunlap
@ 2016-03-21 18:17 ` George Dunlap
  2016-03-22 12:52   ` Roger Pau Monné
  2016-03-21 18:17 ` [PATCH v2 9/9] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path George Dunlap
  2016-03-23 10:10 ` [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts Roger Pau Monné
  9 siblings, 1 reply; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:17 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Wei Liu, George Dunlap, Roger Pau Monne

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Changes since v1:
- Attempt to make a clear distinction between custom hotplug scripts
and the script called for raw physical devices and files

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
---
 docs/misc/block-scripts.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
new file mode 100644
index 0000000..6dd5d48
--- /dev/null
+++ b/docs/misc/block-scripts.txt
@@ -0,0 +1,101 @@
+Block scripts
+=============
+
+Block scripts are called at the moment anytime blkback is directly
+involved in providing access to a backend.  There are three general
+cases this happens:
+
+1. When a user passes a block device in the 'target' field of the disk
+specification
+
+2. When a user passes a file in the 'target' field of the disk
+specification
+
+3. When a user specifies a custom hotplug script.
+
+Setup
+-----
+
+It is highly recommended that custom hotplug scripts as much as
+possible include and use the common Xen functionality.  If the script
+is run from the normal block script location (/etc/xen/scripts by
+default), then this can be done by adding the following to the top of
+the script:
+
+dir=$(dirname "$0")
+. "$dir/block-common.sh"
+
+
+Inputs
+------
+
+In all cases, the scripts are called with either "add" or "remove" as
+the command.  For custom scripts, the command will be the first
+argument of the script (i.e. $1).
+
+The environment variable XENBUS_PATH will be set to the
+path for the block device to be created.
+
+When the script is run, the following nodes shall already have been
+written into xenstore:
+
+ $XENBUS/params    The contents of the 'target' section of the disk specification verbatim.
+ $XENBUS/mode      'r' (for readonly) or 'w' (for read-write)
+
+Output
+-------
+
+Block scripts are responsible for making sure that if a file is
+provided to a VM read/write, that it is not provided to any other VM.
+
+FreeBSD block hotplug scripts must write
+"$XENBUS_PATH/physical-device-path" with the path to the physical
+device or file.  Linux and NetBSD block hotplug scripts *should* also
+write this node.
+
+For the time being, Linux and NetBSD block hotplug scripts must write
+"$XENBUS_PATH/physical-device" with the device's major and minor
+numbers, written in hex, and separated by a colon.
+
+Scripts which include block-common.sh can simply call write_dev "$dev"
+with a path to the device, and write_dev will do the right thing, now
+and going forward.  (See the discussion below.)
+
+Rationale and future work
+-------------------------
+
+Historically, the block scripts wrote a node called "physical-device",
+which contains the major and minor numbers, written in hex, and
+separated by a colon (e.g., "1a:2").  This is required by the Linux
+blkback driver.
+
+FreeBSD blkback, on the other hand, does not have the concept of
+major/minor numbers, and can give direct access to a file without
+going through loopback; so its driver will consume
+physical-device-path.
+
+On Linux, the device model (qemu) needs access to a file it can
+interpret to provide emulated disks before paravirtualized drivers are
+marked as up.  The easiest way to accomplish this is to allow qemu to
+consume physical-device-path (rather than, say, having dom0 act as
+both a frontend and a backend).
+
+Going forward, the plan is at some point to have all block scripts
+simply write "physical-device-path", and then have libxl write the
+other nodes.  The reason we haven't done this yet is that the main
+block script wants to check to make sure the *major/minor* number
+hasn't been re-used, rather than just checking that the *specific
+device node* isn't re-used.  To do this it currently uses
+physical-device; and to do this *safely* it needs physical-device to
+be written with the lock held.
+
+The simplest solution for sorting this out would be to have the block
+script use physical-device if it's present, but if not, to directly
+stat physical-device-path.  But there's not time before the 4.7
+release to make sure all that works.
+
+Another possibility would be to only call out to scripts when using
+custom hotplug scripts; and when doing files or physical devices, to
+do the duplicate checking inside of libxl instead.  The rationale for
+doing this in block scripts rather than in libxl isn't clear at thes
+point.
-- 
2.1.4


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 9/9] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (7 preceding siblings ...)
  2016-03-21 18:17 ` [PATCH v2 8/9] docs: Document block-script protocol George Dunlap
@ 2016-03-21 18:17 ` George Dunlap
  2016-03-23 10:10 ` [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts Roger Pau Monné
  9 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-21 18:17 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson, George Dunlap, Roger Pau Monne

From: Ian Jackson <ian.jackson@eu.citrix.com>

DO NOT APPLY.  This is provided for future reference, as a starting
point to clean up the disk path.  A simple fix will make it "work",
but will introduce a subtle race condition.

* Change block-common.sh on Linux to only write physical-device-path
  with the path of the device node, rather than also writing
  physical-device with its major:minor numbers.

* Have the libxl Linux hotplug script scheduler convert this, by
  reading physical-device-path and writing physical-device.  This
  happens just when we have decided that there is no more script to
  run.

(As a reminder: Many hotplug scripts are called multiple times; so
libxl__get_hotplug_script_info() is called repeatedly until it returns
'0'.  Block scripts are only called once; but this gives us the
opportunity to do the translation at any point when
libxl__get_hotplug_script_info() *would* return zero.)

* Add an xxx about the fact that the sharing check in
  tools/hotplug/Linux/block needs adjusting.  Note that WITHOUT THIS
  OTHER FIX THE CHANGE TO BLOCK-COMMON.SH IS BROKEN.

* This has been tested (with the xxx commented out) and works.

The reason the block script is broken with this change is that
block:check_sharing() reads physical-device to check the major:minor
for duplicates rather than checking the path.  But since this is (now)
written by libxl without the block script lock held, it's possible
that a duplicate instance will run the check_sharing() check before
libxl gets a chance to write that node.

It should be sufficient to modify check_sharing() to read that node if
it's avialable, and if it's not available, to instead read the
major/minor from physical-device-path.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Chances since rfc v1:
- Fixed two bugs in the patch
 - Use backend xenstore node rather than frondend
 - Correctly interpret return value for libxl__device_physdisk_major_minor()
- Remove erroneous comment about libxl__device_physdisk_major_minor()'s return value
- Port to libxl script

CC: Roger Pau Monne <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 docs/misc/block-scripts.txt         | 38 ++++++--------------
 tools/hotplug/Linux/block-common.sh | 15 ++------
 tools/libxl/libxl_linux.c           | 70 +++++++++++++++++++++++++++++++++++--
 3 files changed, 82 insertions(+), 41 deletions(-)

diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
index 6dd5d48..58c6955 100644
--- a/docs/misc/block-scripts.txt
+++ b/docs/misc/block-scripts.txt
@@ -48,18 +48,18 @@ Output
 Block scripts are responsible for making sure that if a file is
 provided to a VM read/write, that it is not provided to any other VM.
 
-FreeBSD block hotplug scripts must write
-"$XENBUS_PATH/physical-device-path" with the path to the physical
-device or file.  Linux and NetBSD block hotplug scripts *should* also
-write this node.
+Block hotplug scripts must write "$XENBUS_PATH/physical-device-path"
+with the path to the physical device or file.
 
-For the time being, Linux and NetBSD block hotplug scripts must write
-"$XENBUS_PATH/physical-device" with the device's major and minor
-numbers, written in hex, and separated by a colon.
+Linux scripts used to write "$XENBUS_PATH/physical-device" with the
+major and minor number of a block device, separated by a colon,
+instead of physical-device-path.  This interface style is deprecated
+but still supported.  However, some functionality, such as emulated
+devices for HVM guests, may not work.
 
 Scripts which include block-common.sh can simply call write_dev "$dev"
 with a path to the device, and write_dev will do the right thing, now
-and going forward.  (See the discussion below.)
+and going forward.
 
 Rationale and future work
 -------------------------
@@ -74,25 +74,9 @@ major/minor numbers, and can give direct access to a file without
 going through loopback; so its driver will consume
 physical-device-path.
 
-On Linux, the device model (qemu) needs access to a file it can
-interpret to provide emulated disks before paravirtualized drivers are
-marked as up.  The easiest way to accomplish this is to allow qemu to
-consume physical-device-path (rather than, say, having dom0 act as
-both a frontend and a backend).
-
-Going forward, the plan is at some point to have all block scripts
-simply write "physical-device-path", and then have libxl write the
-other nodes.  The reason we haven't done this yet is that the main
-block script wants to check to make sure the *major/minor* number
-hasn't been re-used, rather than just checking that the *specific
-device node* isn't re-used.  To do this it currently uses
-physical-device; and to do this *safely* it needs physical-device to
-be written with the lock held.
-
-The simplest solution for sorting this out would be to have the block
-script use physical-device if it's present, but if not, to directly
-stat physical-device-path.  But there's not time before the 4.7
-release to make sure all that works.
+Rather than have different interfaces for different operating systems,
+we just have the block script write the path, and libxl do the
+tranlation when necessary.
 
 Another possibility would be to only call out to scripts when using
 custom hotplug scripts; and when doing files or physical devices, to
diff --git a/tools/hotplug/Linux/block-common.sh b/tools/hotplug/Linux/block-common.sh
index 35110b4..2fcbf76 100644
--- a/tools/hotplug/Linux/block-common.sh
+++ b/tools/hotplug/Linux/block-common.sh
@@ -50,23 +50,14 @@ device_major_minor()
 
 
 ##
-# Write physical-device = MM,mm to the store, where MM and mm are the major 
-# and minor numbers of device respectively.
+# Write physical-device-path = "$1" to the store
 #
 # @param device The device from which major and minor numbers are read, which
 #               will be written into the store.
 #
 write_dev() {
-  local mm
-  
-  mm=$(device_major_minor "$1")
- 
-  if [ -z $mm ]
-  then
-    fatal "Backend device does not exist"
-  fi
- 
-  xenstore_write "$XENBUS_PATH/physical-device" "$mm"
+  xxx check_sharing needs to be fixed or this introduces a race
+    
   xenstore_write "$XENBUS_PATH/physical-device-path" "$1"
   
   success
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index be4afc6..9526dbd 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -233,6 +233,65 @@ error:
     return rc;
 }
 
+static int disk_copy_block_device(libxl__gc *gc, libxl__device *dev,
+                                  libxl__device_action action)
+{
+    int rc;
+    xs_transaction_t t = 0;
+    const char *be_path = libxl__device_backend_path(gc, dev);
+    const char *majmin_path = GCSPRINTF("%s/physical-device", be_path);
+    const char *path_path = GCSPRINTF("%s/physical-device-path", be_path);
+    int major, minor;
+
+    if (action != LIBXL__DEVICE_ACTION_ADD)
+        return 0;
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto out;
+
+        const char *majmin;
+        rc = libxl__xs_read_checked(gc,t, majmin_path, &majmin);
+        if (rc) goto out;
+        if (majmin) {
+            /* Old-style Linux-only hotplug script wrote physical-device */
+            rc = 0;
+            goto out;
+        }
+
+        const char *bdev;
+        rc = libxl__xs_read_checked(gc,t, path_path, &bdev);
+        if (rc) goto out;
+        if (!bdev) {
+            LOGE(ERROR, "nothing wrote physical device to %s", path_path);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        if (libxl__device_physdisk_major_minor(bdev, &major, &minor)<0) {
+            LOG(ERROR, "libxl__device_physdisk_major_minor failed (on %s)",
+                bdev);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        majmin = GCSPRINTF("%x:%x", major, minor);
+        rc = libxl__xs_write_checked(gc,t, majmin_path, majmin);
+        if (rc) goto out;
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc<0) goto out;
+    }
+
+    return rc;
+
+ out:
+    libxl__xs_transaction_abort(gc, &t);
+    return rc;
+}
+
+
 int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
                                    char ***args, char ***env,
                                    libxl__device_action action,
@@ -245,9 +304,16 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
         if (num_exec != 0) {
             LOG(DEBUG, "num_exec %d, not running hotplug scripts", num_exec);
             rc = 0;
-            goto out;
+        } else {
+            rc = libxl__hotplug_disk(gc, dev, args, env, action);
+        }
+        if (!rc) {
+            /* No more hotplug scripts to run.  But, the hotplug
+             * scripts write physical-device-path but we blkback wants
+             * physical-device (major:minor).  So now we adjust
+             * that. */
+            rc = disk_copy_block_device(gc, dev, action);
         }
-        rc = libxl__hotplug_disk(gc, dev, args, env, action);
         break;
     case LIBXL__DEVICE_KIND_VIF:
         /*
-- 
2.1.4


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 6/9] libxl: Share logic for finding path between qemuu and pygrub
  2016-03-21 18:16 ` [PATCH v2 6/9] libxl: Share logic for finding path between qemuu and pygrub George Dunlap
@ 2016-03-22 10:38   ` George Dunlap
  0 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-22 10:38 UTC (permalink / raw)
  To: George Dunlap
  Cc: Wei Liu, xen-devel, Stefano Stabellini, Anthony Perard,
	Ian Jackson, Roger Pau Monne

On Mon, Mar 21, 2016 at 6:16 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> From: George Dunlap <george.dunlap@eu.citrix.com>
>
> qemu can also access disks which will be provided with a qdisk backend
> directly; add a flag to libxl__device_disk_find_local_path to indicate
> whether to check for qdisk direct access.
>
> Call libxl__device_disk_find_local_path() for most paths.  If we can't
> find a local path, print an error and skip the disk, rather than using
> a bogus path.
>
> Now if there is no local access to the disk (i.e., because the disk
> has a non-local backend, or relies on a custom hotplug script), libxl
> will now print a warning and not provide the emulated disk, rather
> than providing bogus parameters to qemu which cause it to error out.
> (Such disks will still be available via the PV backend.)
>
> I left the libxl__blktap_devpath in the qemuu-specific code rather
> than sharing it with the pyrgub code because:
>
> 1) When the pygrub path runs the guest disks have not yet been set up
>
> 2) libxl__blktap_devpath() will give you the existing devpath if it
> already exists, but will set one up for you if you don't.  So on the
> pygrub path, this would end up setting up a new tap device.
>
> 3) There is no tap-specific teardown code on the pygrub path, and I
> don't want to add any (particularly since I'm hoping to remove tapdisk
> altogether soon).
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Changes since v1:
> - Re-port on top of revisions to previous patches
> - Address line length issues
> - Remove stray space at the end of an if (xxx )
> - Break out devicemodel argument rearrangement into a separate patch
> - Spell out implications of warning messages which skip emulated disks
>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Anthony Perard <anthony.perard@citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> ---
>  tools/libxl/libxl.c          | 17 ++++++++++++++---
>  tools/libxl/libxl_dm.c       | 27 +++++++++++++++++++++++----
>  tools/libxl/libxl_internal.h |  8 ++++++++
>  3 files changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2d7a154..602bec2 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3000,8 +3000,9 @@ static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user,
>
>  /* Callbacks */
>
> -static char *libxl__device_disk_find_local_path(libxl__gc *gc,
> -                                                const libxl_device_disk *disk)
> +char *libxl__device_disk_find_local_path(libxl__gc *gc,
> +                                         const libxl_device_disk *disk,
> +                                         bool qdisk_direct)
>  {
>      char *path = NULL;
>
> @@ -3022,6 +3023,16 @@ static char *libxl__device_disk_find_local_path(libxl__gc *gc,
>          goto out;
>      }
>
> +    /*
> +     * If we're being called for a qemu path, we can pass the target
> +     * string directly as well
> +     */
> +    if (qdisk_direct && disk->backend == LIBXL_DISK_BACKEND_QDISK) {
> +        path = libxl__strdup(gc, disk->pdev_path);
> +        LOG(DEBUG, "Directly accessing local QDISK target %s", path);
> +        goto out;
> +    }
> +
>   out:
>      return path;
>  }
> @@ -3046,7 +3057,7 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
>
>      LOG(DEBUG, "Trying to find local path");
>
> -    dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk);
> +    dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk, false);
>      if (dls->diskpath) {
>          LOG(DEBUG, "Local path found, executing callback.");
>          dls->callback(egc, dls, 0);
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index dfcf141..1493b84 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1182,23 +1182,42 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>
>              if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
>                  if (!disks[i].is_cdrom) {
> -                    LOG(WARN, "cannot support"" empty disk format for %s",
> +                    LOG(WARN, "Cannot support empty disk format for %s",
>                          disks[i].vdev);
>                      continue;
>                  }
>              } else {
>                  if (format == NULL) {
>                      LOG(WARN,
> -                        "unable to determine"" disk image format %s",
> +                        "Unable to determine disk image format: %s\n",
> +                        "Disk will be available via PV drivers but not as an"
> +                        "emulated disk.",

Sorry, there's a stray comma after the first line of the multi-line
string.  Let me know if you want me to re-send the series with the
comma removed, or whether someone can take it out when committing.

 -George

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 8/9] docs: Document block-script protocol
  2016-03-21 18:17 ` [PATCH v2 8/9] docs: Document block-script protocol George Dunlap
@ 2016-03-22 12:52   ` Roger Pau Monné
  2016-03-23 11:19     ` George Dunlap
  0 siblings, 1 reply; 18+ messages in thread
From: Roger Pau Monné @ 2016-03-22 12:52 UTC (permalink / raw)
  To: George Dunlap; +Cc: Ian Jackson, Roger Pau Monne, Wei Liu, xen-devel

On Mon, 21 Mar 2016, George Dunlap wrote:
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Changes since v1:
> - Attempt to make a clear distinction between custom hotplug scripts
> and the script called for raw physical devices and files
> 
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> ---
>  docs/misc/block-scripts.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
> new file mode 100644
> index 0000000..6dd5d48
> --- /dev/null
> +++ b/docs/misc/block-scripts.txt
> @@ -0,0 +1,101 @@
[...]
> +Inputs
> +------
> +
> +In all cases, the scripts are called with either "add" or "remove" as
> +the command.  For custom scripts, the command will be the first
> +argument of the script (i.e. $1).
> +
> +The environment variable XENBUS_PATH will be set to the
> +path for the block device to be created.

This is true for Linux, but not for NetBSD. On NetBSD no env variables are 
needed, and everything is passed as arguments using the following format:

./<script> <backend_path> <xenbus state>

Where xenbus state is either 2 or 6.

On FreeBSD I'm aiming of using the same input interface for both block and 
network scripts, and it is the following:

./<script> {add/remove} <backend path>

With no env variables provided at all. So either this section is expanded, 
or it is labelled as "Linux Inputs".

Roger.

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts
  2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (8 preceding siblings ...)
  2016-03-21 18:17 ` [PATCH v2 9/9] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path George Dunlap
@ 2016-03-23 10:10 ` Roger Pau Monné
  2016-03-23 10:36   ` George Dunlap
  9 siblings, 1 reply; 18+ messages in thread
From: Roger Pau Monné @ 2016-03-23 10:10 UTC (permalink / raw)
  To: George Dunlap
  Cc: Anthony Perard, Ian Jackson, Roger Pau Monne, Wei Liu, xen-devel

On Mon, 21 Mar 2016, George Dunlap wrote:

> In order for HVM domains to provide emulated access to disks provided
> by hotplug scripts, qemu needs access to a "cooked" version of the
> disk.  In the case of hotplug scripts, this "cooked" version is
> available in the form of a block device passed to blkback.  Make this
> "cooked" version available to qemu.
> 
> This series also starts to work towards a rationalized interface to
> the block hotplug scripts, on which hotplug scripts for FreeBSD can be
> added.
> 
> Changes since v1:
> - Split one of the patches into two
> 
> 
> George Dunlap (8):
>   tools/hotplug: Add a "dummy" hotplug script for testing
>   libxl: Remove redundant setting of phyical-device
>   tools/hotplug: Write physical-device-path in addition to
>     physical-device
>   libxl: Move check for local access to a funciton
>   libxl: Rearrange qemu upstream disk argument code
>   libxl: Share logic for finding path between qemuu and pygrub
>   libxl: Allow local access for block devices with hotplug scripts
>   docs: Document block-script protocol
> 
> Ian Jackson (1):
>   DO NOT APPLY libxl: Change hotplug script interface to use
>     physical-device-path

Hello,

Is this available in some git repo? It would be easier for me to rebase my 
FreeBSD hotplug series on top of it, but if not I can pick the patches 
myself from the mailing list.

Thanks, Roger.

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts
  2016-03-23 10:10 ` [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts Roger Pau Monné
@ 2016-03-23 10:36   ` George Dunlap
  0 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2016-03-23 10:36 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: Anthony Perard, Ian Jackson, Wei Liu, xen-devel

On 23/03/16 10:10, Roger Pau Monné wrote:
> On Mon, 21 Mar 2016, George Dunlap wrote:
> 
>> In order for HVM domains to provide emulated access to disks provided
>> by hotplug scripts, qemu needs access to a "cooked" version of the
>> disk.  In the case of hotplug scripts, this "cooked" version is
>> available in the form of a block device passed to blkback.  Make this
>> "cooked" version available to qemu.
>>
>> This series also starts to work towards a rationalized interface to
>> the block hotplug scripts, on which hotplug scripts for FreeBSD can be
>> added.
>>
>> Changes since v1:
>> - Split one of the patches into two
>>
>>
>> George Dunlap (8):
>>   tools/hotplug: Add a "dummy" hotplug script for testing
>>   libxl: Remove redundant setting of phyical-device
>>   tools/hotplug: Write physical-device-path in addition to
>>     physical-device
>>   libxl: Move check for local access to a funciton
>>   libxl: Rearrange qemu upstream disk argument code
>>   libxl: Share logic for finding path between qemuu and pygrub
>>   libxl: Allow local access for block devices with hotplug scripts
>>   docs: Document block-script protocol
>>
>> Ian Jackson (1):
>>   DO NOT APPLY libxl: Change hotplug script interface to use
>>     physical-device-path
> 
> Hello,
> 
> Is this available in some git repo? It would be easier for me to rebase my 
> FreeBSD hotplug series on top of it, but if not I can pick the patches 
> myself from the mailing list.

I've pushed it here:

git://xenbits.xenproject.org/people/gdunlap/xen.git
out/hotplug-script-improvements/v2

NB this has the fix to patch 6 as well.

 -George

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 8/9] docs: Document block-script protocol
  2016-03-22 12:52   ` Roger Pau Monné
@ 2016-03-23 11:19     ` George Dunlap
  2016-03-23 12:14       ` Roger Pau Monné
  0 siblings, 1 reply; 18+ messages in thread
From: George Dunlap @ 2016-03-23 11:19 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: Ian Jackson, Wei Liu, xen-devel

On 22/03/16 12:52, Roger Pau Monné wrote:
> On Mon, 21 Mar 2016, George Dunlap wrote:
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> ---
>> Changes since v1:
>> - Attempt to make a clear distinction between custom hotplug scripts
>> and the script called for raw physical devices and files
>>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monne <roger.pau@citrix.com>
>> ---
>>  docs/misc/block-scripts.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 101 insertions(+)
>>
>> diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
>> new file mode 100644
>> index 0000000..6dd5d48
>> --- /dev/null
>> +++ b/docs/misc/block-scripts.txt
>> @@ -0,0 +1,101 @@
> [...]
>> +Inputs
>> +------
>> +
>> +In all cases, the scripts are called with either "add" or "remove" as
>> +the command.  For custom scripts, the command will be the first
>> +argument of the script (i.e. $1).
>> +
>> +The environment variable XENBUS_PATH will be set to the
>> +path for the block device to be created.
> 
> This is true for Linux, but not for NetBSD. On NetBSD no env variables are 
> needed, and everything is passed as arguments using the following format:
> 
> ./<script> <backend_path> <xenbus state>
> 
> Where xenbus state is either 2 or 6.
> 
> On FreeBSD I'm aiming of using the same input interface for both block and 
> network scripts, and it is the following:
> 
> ./<script> {add/remove} <backend path>
> 
> With no env variables provided at all. So either this section is expanded, 
> or it is labelled as "Linux Inputs".

Nothing like consistency across implementations. :-)

So in the case of NetBSD, "2" means 'add' and "6" means 'remove'?  Or
how does that work?

Presumably there's not much we can do about NetBSD at this point, if
there are (or may be) out-of-tree scripts that expect the new format.
But unless there's a good reason, it seems like we should try to
converge the hotplug script protocol.

Was there a particular reason you wanted to use an argument instead of
an environment variable?  If not, it's probably better to just follow
suit with the Linux protocol.

If there is a good reason, we could consider changing the Linux protocol
as well (leaving the environment variable in for backwards-compatibility
purposes).

 -George

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 8/9] docs: Document block-script protocol
  2016-03-23 11:19     ` George Dunlap
@ 2016-03-23 12:14       ` Roger Pau Monné
  2016-03-24 16:29         ` George Dunlap
  0 siblings, 1 reply; 18+ messages in thread
From: Roger Pau Monné @ 2016-03-23 12:14 UTC (permalink / raw)
  To: George Dunlap; +Cc: Ian Jackson, xen-devel, Wei Liu, Roger Pau Monné

[-- Attachment #1: Type: text/plain, Size: 3291 bytes --]

On Wed, 23 Mar 2016, George Dunlap wrote:
> On 22/03/16 12:52, Roger Pau Monné wrote:
> > On Mon, 21 Mar 2016, George Dunlap wrote:
> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> >> ---
> >> Changes since v1:
> >> - Attempt to make a clear distinction between custom hotplug scripts
> >> and the script called for raw physical devices and files
> >>
> >> CC: Ian Jackson <ian.jackson@citrix.com>
> >> CC: Wei Liu <wei.liu2@citrix.com>
> >> CC: Roger Pau Monne <roger.pau@citrix.com>
> >> ---
> >>  docs/misc/block-scripts.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 101 insertions(+)
> >>
> >> diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
> >> new file mode 100644
> >> index 0000000..6dd5d48
> >> --- /dev/null
> >> +++ b/docs/misc/block-scripts.txt
> >> @@ -0,0 +1,101 @@
> > [...]
> >> +Inputs
> >> +------
> >> +
> >> +In all cases, the scripts are called with either "add" or "remove" as
> >> +the command.  For custom scripts, the command will be the first
> >> +argument of the script (i.e. $1).
> >> +
> >> +The environment variable XENBUS_PATH will be set to the
> >> +path for the block device to be created.
> > 
> > This is true for Linux, but not for NetBSD. On NetBSD no env variables are 
> > needed, and everything is passed as arguments using the following format:
> > 
> > ./<script> <backend_path> <xenbus state>
> > 
> > Where xenbus state is either 2 or 6.
> > 
> > On FreeBSD I'm aiming of using the same input interface for both block and 
> > network scripts, and it is the following:
> > 
> > ./<script> {add/remove} <backend path>
> > 
> > With no env variables provided at all. So either this section is expanded, 
> > or it is labelled as "Linux Inputs".
> 
> Nothing like consistency across implementations. :-)
> 
> So in the case of NetBSD, "2" means 'add' and "6" means 'remove'?  Or
> how does that work?

Yes, 2 means add and 6 remove.
 
> Presumably there's not much we can do about NetBSD at this point, if
> there are (or may be) out-of-tree scripts that expect the new format.
> But unless there's a good reason, it seems like we should try to
> converge the hotplug script protocol.
> 
> Was there a particular reason you wanted to use an argument instead of
> an environment variable?  If not, it's probably better to just follow
> suit with the Linux protocol.

Don't get me wrong, but the Linux protocol is all but consistent :). I'm 
not sure if those other env variables are used by the block hotplug 
scripts, but we also set:

script=<script_name>
XENBUS_TYPE=<vbd/vif>
XENBUS_PATH=<be_path>
XENBUS_BASE_PATH="backend"

And it's even worse for vifs, where the action parameter that we pass to 
the hotplug script is different depending on whether we are dealing with 
a PV or an emulated interface (PV uses "online/offline", while emulated 
use "add/remove").

I would like to share the same interface, but I think the Linux one is 
simply too broken, and I don't want to put this anywhere close to FreeBSD. 
There at least I have a chance of having something that's simple and 
rational.

> If there is a good reason, we could consider changing the Linux protocol
> as well (leaving the environment variable in for backwards-compatibility
> purposes).
> 
>  -George
> 

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 8/9] docs: Document block-script protocol
  2016-03-23 12:14       ` Roger Pau Monné
@ 2016-03-24 16:29         ` George Dunlap
  2016-03-24 16:51           ` Roger Pau Monné
  0 siblings, 1 reply; 18+ messages in thread
From: George Dunlap @ 2016-03-24 16:29 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: Ian Jackson, Wei Liu, George Dunlap, xen-devel

On Wed, Mar 23, 2016 at 12:14 PM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> On Wed, 23 Mar 2016, George Dunlap wrote:
>> On 22/03/16 12:52, Roger Pau Monné wrote:
>> > On Mon, 21 Mar 2016, George Dunlap wrote:
>> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> >> ---
>> >> Changes since v1:
>> >> - Attempt to make a clear distinction between custom hotplug scripts
>> >> and the script called for raw physical devices and files
>> >>
>> >> CC: Ian Jackson <ian.jackson@citrix.com>
>> >> CC: Wei Liu <wei.liu2@citrix.com>
>> >> CC: Roger Pau Monne <roger.pau@citrix.com>
>> >> ---
>> >>  docs/misc/block-scripts.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 101 insertions(+)
>> >>
>> >> diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
>> >> new file mode 100644
>> >> index 0000000..6dd5d48
>> >> --- /dev/null
>> >> +++ b/docs/misc/block-scripts.txt
>> >> @@ -0,0 +1,101 @@
>> > [...]
>> >> +Inputs
>> >> +------
>> >> +
>> >> +In all cases, the scripts are called with either "add" or "remove" as
>> >> +the command.  For custom scripts, the command will be the first
>> >> +argument of the script (i.e. $1).
>> >> +
>> >> +The environment variable XENBUS_PATH will be set to the
>> >> +path for the block device to be created.
>> >
>> > This is true for Linux, but not for NetBSD. On NetBSD no env variables are
>> > needed, and everything is passed as arguments using the following format:
>> >
>> > ./<script> <backend_path> <xenbus state>
>> >
>> > Where xenbus state is either 2 or 6.
>> >
>> > On FreeBSD I'm aiming of using the same input interface for both block and
>> > network scripts, and it is the following:
>> >
>> > ./<script> {add/remove} <backend path>
>> >
>> > With no env variables provided at all. So either this section is expanded,
>> > or it is labelled as "Linux Inputs".
>>
>> Nothing like consistency across implementations. :-)
>>
>> So in the case of NetBSD, "2" means 'add' and "6" means 'remove'?  Or
>> how does that work?
>
> Yes, 2 means add and 6 remove.
>
>> Presumably there's not much we can do about NetBSD at this point, if
>> there are (or may be) out-of-tree scripts that expect the new format.
>> But unless there's a good reason, it seems like we should try to
>> converge the hotplug script protocol.
>>
>> Was there a particular reason you wanted to use an argument instead of
>> an environment variable?  If not, it's probably better to just follow
>> suit with the Linux protocol.
>
> Don't get me wrong, but the Linux protocol is all but consistent :). I'm
> not sure if those other env variables are used by the block hotplug
> scripts, but we also set:
>
> script=<script_name>
> XENBUS_TYPE=<vbd/vif>
> XENBUS_PATH=<be_path>
> XENBUS_BASE_PATH="backend"
>
> And it's even worse for vifs, where the action parameter that we pass to
> the hotplug script is different depending on whether we are dealing with
> a PV or an emulated interface (PV uses "online/offline", while emulated
> use "add/remove").
>
> I would like to share the same interface, but I think the Linux one is
> simply too broken, and I don't want to put this anywhere close to FreeBSD.
> There at least I have a chance of having something that's simple and
> rational.

OK, so how about this.  I'll post the document with the input sections
like this:
[snip]

Inputs
------

Unfortunately the inputs to the block scripts look completely
different for each operating system.

Inputs (Linux)
--------------

In all cases, the scripts are called with either "add" or "remove" as
the command.  For custom scripts, the command will be the first
argument of the script (i.e. $1).

The environment variable XENBUS_PATH will be set to the
path for the block device to be created.

When the script is run, the following nodes shall already have been
written into xenstore:

 $XENBUS/params    The contents of the 'target' section of the disk
specification verbatim.
 $XENBUS/mode      'r' (for readonly) or 'w' (for read-write)

Inputs (NetBSD)
---------------

TODO

[snip]

And you add a FreeBSD section in your series.  If that sounds good
I'll make the above modification and send v3.

 -George

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 8/9] docs: Document block-script protocol
  2016-03-24 16:29         ` George Dunlap
@ 2016-03-24 16:51           ` Roger Pau Monné
  0 siblings, 0 replies; 18+ messages in thread
From: Roger Pau Monné @ 2016-03-24 16:51 UTC (permalink / raw)
  To: George Dunlap
  Cc: Ian Jackson, xen-devel, Wei Liu, George Dunlap, Roger Pau Monné

[-- Attachment #1: Type: text/plain, Size: 4480 bytes --]

On Thu, 24 Mar 2016, George Dunlap wrote:
> On Wed, Mar 23, 2016 at 12:14 PM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> > On Wed, 23 Mar 2016, George Dunlap wrote:
> >> On 22/03/16 12:52, Roger Pau Monné wrote:
> >> > On Mon, 21 Mar 2016, George Dunlap wrote:
> >> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> >> >> ---
> >> >> Changes since v1:
> >> >> - Attempt to make a clear distinction between custom hotplug scripts
> >> >> and the script called for raw physical devices and files
> >> >>
> >> >> CC: Ian Jackson <ian.jackson@citrix.com>
> >> >> CC: Wei Liu <wei.liu2@citrix.com>
> >> >> CC: Roger Pau Monne <roger.pau@citrix.com>
> >> >> ---
> >> >>  docs/misc/block-scripts.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++
> >> >>  1 file changed, 101 insertions(+)
> >> >>
> >> >> diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
> >> >> new file mode 100644
> >> >> index 0000000..6dd5d48
> >> >> --- /dev/null
> >> >> +++ b/docs/misc/block-scripts.txt
> >> >> @@ -0,0 +1,101 @@
> >> > [...]
> >> >> +Inputs
> >> >> +------
> >> >> +
> >> >> +In all cases, the scripts are called with either "add" or "remove" as
> >> >> +the command.  For custom scripts, the command will be the first
> >> >> +argument of the script (i.e. $1).
> >> >> +
> >> >> +The environment variable XENBUS_PATH will be set to the
> >> >> +path for the block device to be created.
> >> >
> >> > This is true for Linux, but not for NetBSD. On NetBSD no env variables are
> >> > needed, and everything is passed as arguments using the following format:
> >> >
> >> > ./<script> <backend_path> <xenbus state>
> >> >
> >> > Where xenbus state is either 2 or 6.
> >> >
> >> > On FreeBSD I'm aiming of using the same input interface for both block and
> >> > network scripts, and it is the following:
> >> >
> >> > ./<script> {add/remove} <backend path>
> >> >
> >> > With no env variables provided at all. So either this section is expanded,
> >> > or it is labelled as "Linux Inputs".
> >>
> >> Nothing like consistency across implementations. :-)
> >>
> >> So in the case of NetBSD, "2" means 'add' and "6" means 'remove'?  Or
> >> how does that work?
> >
> > Yes, 2 means add and 6 remove.
> >
> >> Presumably there's not much we can do about NetBSD at this point, if
> >> there are (or may be) out-of-tree scripts that expect the new format.
> >> But unless there's a good reason, it seems like we should try to
> >> converge the hotplug script protocol.
> >>
> >> Was there a particular reason you wanted to use an argument instead of
> >> an environment variable?  If not, it's probably better to just follow
> >> suit with the Linux protocol.
> >
> > Don't get me wrong, but the Linux protocol is all but consistent :). I'm
> > not sure if those other env variables are used by the block hotplug
> > scripts, but we also set:
> >
> > script=<script_name>
> > XENBUS_TYPE=<vbd/vif>
> > XENBUS_PATH=<be_path>
> > XENBUS_BASE_PATH="backend"
> >
> > And it's even worse for vifs, where the action parameter that we pass to
> > the hotplug script is different depending on whether we are dealing with
> > a PV or an emulated interface (PV uses "online/offline", while emulated
> > use "add/remove").
> >
> > I would like to share the same interface, but I think the Linux one is
> > simply too broken, and I don't want to put this anywhere close to FreeBSD.
> > There at least I have a chance of having something that's simple and
> > rational.
> 
> OK, so how about this.  I'll post the document with the input sections
> like this:
> [snip]
> 
> Inputs
> ------
> 
> Unfortunately the inputs to the block scripts look completely
> different for each operating system.
> 
> Inputs (Linux)
> --------------
> 
> In all cases, the scripts are called with either "add" or "remove" as
> the command.  For custom scripts, the command will be the first
> argument of the script (i.e. $1).
> 
> The environment variable XENBUS_PATH will be set to the
> path for the block device to be created.
> 
> When the script is run, the following nodes shall already have been
> written into xenstore:
> 
>  $XENBUS/params    The contents of the 'target' section of the disk
> specification verbatim.
>  $XENBUS/mode      'r' (for readonly) or 'w' (for read-write)
> 
> Inputs (NetBSD)
> ---------------
> 
> TODO
> 
> [snip]
> 
> And you add a FreeBSD section in your series.  If that sounds good
> I'll make the above modification and send v3.

LGTM :).

Roger.

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2016-03-24 16:51 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
2016-03-21 18:16 ` [PATCH v2 1/9] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
2016-03-21 18:16 ` [PATCH v2 2/9] libxl: Remove redundant setting of phyical-device George Dunlap
2016-03-21 18:16 ` [PATCH v2 3/9] tools/hotplug: Write physical-device-path in addition to physical-device George Dunlap
2016-03-21 18:16 ` [PATCH v2 4/9] libxl: Move check for local access to a funciton George Dunlap
2016-03-21 18:16 ` [PATCH v2 5/9] libxl: Rearrange qemu upstream disk argument code George Dunlap
2016-03-21 18:16 ` [PATCH v2 6/9] libxl: Share logic for finding path between qemuu and pygrub George Dunlap
2016-03-22 10:38   ` George Dunlap
2016-03-21 18:16 ` [PATCH v2 7/9] libxl: Allow local access for block devices with hotplug scripts George Dunlap
2016-03-21 18:17 ` [PATCH v2 8/9] docs: Document block-script protocol George Dunlap
2016-03-22 12:52   ` Roger Pau Monné
2016-03-23 11:19     ` George Dunlap
2016-03-23 12:14       ` Roger Pau Monné
2016-03-24 16:29         ` George Dunlap
2016-03-24 16:51           ` Roger Pau Monné
2016-03-21 18:17 ` [PATCH v2 9/9] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path George Dunlap
2016-03-23 10:10 ` [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts Roger Pau Monné
2016-03-23 10:36   ` George Dunlap

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).