xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts
@ 2016-03-16 16:09 George Dunlap
  2016-03-16 16:09 ` [PATCH 1/8] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
                   ` (7 more replies)
  0 siblings, 8 replies; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:09 UTC (permalink / raw)
  To: xen-devel; +Cc: 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.


George Dunlap (7):
  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: 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         |  84 ++++++++++++++++++++++++
 tools/hotplug/Linux/Makefile        |   1 +
 tools/hotplug/Linux/block-common.sh |  16 ++---
 tools/hotplug/Linux/block-dummy     | 107 ++++++++++++++++++++++++++++++
 tools/libxl/libxl.c                 | 126 +++++++++++++++++++++++++-----------
 tools/libxl/libxl_dm.c              |  65 ++++++++++++-------
 tools/libxl/libxl_internal.h        |  11 +++-
 tools/libxl/libxl_linux.c           |  70 +++++++++++++++++++-
 8 files changed, 407 insertions(+), 73 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>


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

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

* [PATCH 1/8] tools/hotplug: Add a "dummy" hotplug script for testing
  2016-03-16 16:09 [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
@ 2016-03-16 16:09 ` George Dunlap
  2016-03-16 16:54   ` Ian Jackson
  2016-03-16 16:09 ` [PATCH 2/8] libxl: Remove redundant setting of phyical-device George Dunlap
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:09 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>
---
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] 25+ messages in thread

* [PATCH 2/8] libxl: Remove redundant setting of phyical-device
  2016-03-16 16:09 [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
  2016-03-16 16:09 ` [PATCH 1/8] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
@ 2016-03-16 16:09 ` George Dunlap
  2016-03-16 16:54   ` Ian Jackson
  2016-03-16 16:09 ` [PATCH 3/8] tools/hotplug: Write physical-device-path in addition to physical-device George Dunlap
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:09 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>
---
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 4cdc169..7ce3ebb 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2441,21 +2441,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] 25+ messages in thread

* [PATCH 3/8] tools/hotplug: Write physical-device-path in addition to physical-device
  2016-03-16 16:09 [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
  2016-03-16 16:09 ` [PATCH 1/8] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
  2016-03-16 16:09 ` [PATCH 2/8] libxl: Remove redundant setting of phyical-device George Dunlap
@ 2016-03-16 16:09 ` George Dunlap
  2016-03-16 16:56   ` Ian Jackson
  2016-03-16 16:09 ` [PATCH 4/8] libxl: Move check for local access to a funciton George Dunlap
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:09 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>
---

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] 25+ messages in thread

* [PATCH 4/8] libxl: Move check for local access to a funciton
  2016-03-16 16:09 [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (2 preceding siblings ...)
  2016-03-16 16:09 ` [PATCH 3/8] tools/hotplug: Write physical-device-path in addition to physical-device George Dunlap
@ 2016-03-16 16:09 ` George Dunlap
  2016-03-16 16:58   ` Ian Jackson
  2016-03-17 18:11   ` Ian Jackson
  2016-03-16 16:09 ` [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub George Dunlap
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:09 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>
---
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 | 66 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 7ce3ebb..a57568f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3001,13 +3001,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
+        && disk->pdev_path) {
+        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;
@@ -3015,34 +3040,35 @@ 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");
+
+    if ((dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk))) {
+        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] 25+ messages in thread

* [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub
  2016-03-16 16:09 [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (3 preceding siblings ...)
  2016-03-16 16:09 ` [PATCH 4/8] libxl: Move check for local access to a funciton George Dunlap
@ 2016-03-16 16:09 ` George Dunlap
  2016-03-17 18:22   ` Ian Jackson
  2016-03-16 16:09 ` [PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts George Dunlap
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:09 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.

Reorganize the qemuu disk argument code to make a clean separation
between finding a file to use (if any), and constructing the
parameters.

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.

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.

This should fix two bugs:

* In the case of a block script, or a non-dom0 backend, qemuu will now
print a warning and skip the disk, rather than adding bogus
parameters that will cause qemuu to error out.

* You should now be able to use any backend for a cdrom that you can
use for a normal disk.  (Before it was limited to RAW files or things
that qemu could handle directly.)

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>
---
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       | 66 +++++++++++++++++++++++++++++---------------
 tools/libxl/libxl_internal.h |  8 ++++++
 3 files changed, 66 insertions(+), 25 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a57568f..d6302a9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3001,8 +3001,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;
 
     /* No local paths for driver domains */
@@ -3023,6 +3024,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;
 }
@@ -3047,7 +3058,7 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
 
     LOG(DEBUG, "Trying to find local path");
 
-    if ((dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk))) {
+    if ((dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk, false))) {
         LOG(DEBUG, "Local path found, executing callback.");
         dls->callback(egc, dls, 0);
     } else {
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4aca38e..917ebbf 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,36 @@ 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,
+                /* 
+                 * 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 {
-                    pdev_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", disks[i].vdev);
+                    continue;
                 }
+            }
+
+            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.
                  *
@@ -1211,7 +1233,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                 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");
+                         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 +1247,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 +1258,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 */
                 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 005fe53..a511c6d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2670,6 +2670,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] 25+ messages in thread

* [PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts
  2016-03-16 16:09 [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (4 preceding siblings ...)
  2016-03-16 16:09 ` [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub George Dunlap
@ 2016-03-16 16:09 ` George Dunlap
  2016-03-17 18:36   ` Ian Jackson
  2016-03-16 16:09 ` [PATCH 7/8] docs: Document block-script protocol George Dunlap
  2016-03-16 16:09 ` [PATCH 8/8] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path George Dunlap
  7 siblings, 1 reply; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:09 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; meaning that disk hotplug scripts cannot be
used with 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 bre written to
physical-device-path.

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

We need the target domid to find the appropriate xenstore node, so add
that to libxl__disk_local_state.

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

This allows qemuu to boot with block devices created with hotplug
scripts.

Signed-off-by: George Dunlap <george.dunlap@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/libxl/libxl.c          | 38 +++++++++++++++++++++++++++++++++++---
 tools/libxl/libxl_dm.c       |  3 +--
 tools/libxl/libxl_internal.h |  3 ++-
 3 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d6302a9..490a7d2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2296,7 +2296,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;
@@ -3002,6 +3002,7 @@ static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user,
 /* Callbacks */
 
 char * libxl__device_disk_find_local_path(libxl__gc *gc, 
+                                          libxl_domid domid,
                                           const libxl_device_disk *disk,
                                           bool qdisk_direct) {
     char *path = NULL;
@@ -3032,6 +3033,37 @@ 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 && 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, 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:
@@ -3058,7 +3090,8 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
 
     LOG(DEBUG, "Trying to find local path");
 
-    if ((dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk, false))) {
+    if ((dls->diskpath = libxl__device_disk_find_local_path(gc, INVALID_DOMID,
+                                                            in_disk, false))) {
         LOG(DEBUG, "Local path found, executing callback.");
         dls->callback(egc, dls, 0);
     } else {
@@ -3073,7 +3106,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 917ebbf..646a49c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1206,8 +1206,7 @@ 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", disks[i].vdev);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a511c6d..6291b9c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1709,7 +1709,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
@@ -2674,6 +2674,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 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] 25+ messages in thread

* [PATCH 7/8] docs: Document block-script protocol
  2016-03-16 16:09 [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (5 preceding siblings ...)
  2016-03-16 16:09 ` [PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts George Dunlap
@ 2016-03-16 16:09 ` George Dunlap
  2016-03-16 22:57   ` Jim Fehlig
  2016-03-17 18:38   ` Ian Jackson
  2016-03-16 16:09 ` [PATCH 8/8] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path George Dunlap
  7 siblings, 2 replies; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:09 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>
---
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 | 100 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
new file mode 100644
index 0000000..ef19207
--- /dev/null
+++ b/docs/misc/block-scripts.txt
@@ -0,0 +1,100 @@
+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 script.
+
+Setup
+-----
+
+It is highly recommended that custom 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 do away with the block scripts
+altogether when not actually running any scripts, and do the duplicate
+checking inside of libxl.  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] 25+ messages in thread

* [PATCH 8/8] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path
  2016-03-16 16:09 [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
                   ` (6 preceding siblings ...)
  2016-03-16 16:09 ` [PATCH 7/8] docs: Document block-script protocol George Dunlap
@ 2016-03-16 16:09 ` George Dunlap
  7 siblings, 0 replies; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:09 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 ef19207..06afb35 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 do away with the block scripts
 altogether when not actually running any scripts, and do the duplicate
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] 25+ messages in thread

* Re: [PATCH 1/8] tools/hotplug: Add a "dummy" hotplug script for testing
  2016-03-16 16:09 ` [PATCH 1/8] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
@ 2016-03-16 16:54   ` Ian Jackson
  0 siblings, 0 replies; 25+ messages in thread
From: Ian Jackson @ 2016-03-16 16:54 UTC (permalink / raw)
  To: George Dunlap; +Cc: George Dunlap, Roger Pau Monne, Wei Liu, xen-devel

George Dunlap writes ("[PATCH 1/8] tools/hotplug: Add a "dummy" hotplug script for testing"):
> 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>

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

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

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

* Re: [PATCH 2/8] libxl: Remove redundant setting of phyical-device
  2016-03-16 16:09 ` [PATCH 2/8] libxl: Remove redundant setting of phyical-device George Dunlap
@ 2016-03-16 16:54   ` Ian Jackson
  0 siblings, 0 replies; 25+ messages in thread
From: Ian Jackson @ 2016-03-16 16:54 UTC (permalink / raw)
  To: George Dunlap; +Cc: Roger Pau Monne, Wei Liu, xen-devel

George Dunlap writes ("[PATCH 2/8] libxl: Remove redundant setting of phyical-device"):
> 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>
> ---
> 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.

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

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

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

* Re: [PATCH 3/8] tools/hotplug: Write physical-device-path in addition to physical-device
  2016-03-16 16:09 ` [PATCH 3/8] tools/hotplug: Write physical-device-path in addition to physical-device George Dunlap
@ 2016-03-16 16:56   ` Ian Jackson
  2016-03-16 16:57     ` George Dunlap
  0 siblings, 1 reply; 25+ messages in thread
From: Ian Jackson @ 2016-03-16 16:56 UTC (permalink / raw)
  To: George Dunlap; +Cc: Roger Pau Monne, Wei Liu, xen-devel

George Dunlap writes ("[PATCH 3/8] tools/hotplug: Write physical-device-path in addition to physical-device"):
> 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.

I would like to see some corresponding updates to the documentation of
the hotplug script invocation protocol (such as it is - maybe just
some file in docs/misc/)

Ian.

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

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

* Re: [PATCH 3/8] tools/hotplug: Write physical-device-path in addition to physical-device
  2016-03-16 16:56   ` Ian Jackson
@ 2016-03-16 16:57     ` George Dunlap
  0 siblings, 0 replies; 25+ messages in thread
From: George Dunlap @ 2016-03-16 16:57 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Roger Pau Monne, Wei Liu, xen-devel

On 16/03/16 16:56, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 3/8] tools/hotplug: Write physical-device-path in addition to physical-device"):
>> 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.
> 
> I would like to see some corresponding updates to the documentation of
> the hotplug script invocation protocol (such as it is - maybe just
> some file in docs/misc/)

See patch 7. :-)

 -George


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

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

* Re: [PATCH 4/8] libxl: Move check for local access to a funciton
  2016-03-16 16:09 ` [PATCH 4/8] libxl: Move check for local access to a funciton George Dunlap
@ 2016-03-16 16:58   ` Ian Jackson
  2016-03-16 17:02     ` George Dunlap
  2016-03-17 18:11   ` Ian Jackson
  1 sibling, 1 reply; 25+ messages in thread
From: Ian Jackson @ 2016-03-16 16:58 UTC (permalink / raw)
  To: George Dunlap; +Cc: George Dunlap, Roger Pau Monne, Wei Liu, xen-devel

George Dunlap writes ("[PATCH 4/8] libxl: Move check for local access to a funciton"):
> 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.
...
> +static char * libxl__device_disk_find_local_path(libxl__gc *gc, 
> +                                                 const libxl_device_disk *disk) {
                ^ whitespace
    ^ line length

I think I need to review this with `git diff -b' - can you give me a
git branch for your series or shall I git-am it ?

Thanks,
Ian.

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

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

* Re: [PATCH 4/8] libxl: Move check for local access to a funciton
  2016-03-16 16:58   ` Ian Jackson
@ 2016-03-16 17:02     ` George Dunlap
  0 siblings, 0 replies; 25+ messages in thread
From: George Dunlap @ 2016-03-16 17:02 UTC (permalink / raw)
  To: Ian Jackson; +Cc: George Dunlap, Roger Pau Monne, Wei Liu, xen-devel

On 16/03/16 16:58, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 4/8] libxl: Move check for local access to a funciton"):
>> 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.
> ...
>> +static char * libxl__device_disk_find_local_path(libxl__gc *gc, 
>> +                                                 const libxl_device_disk *disk) {
>                 ^ whitespace

Oops..


>     ^ line length

I was about to say "it's nto clear to me how to make this shorter", but
then I could just not have the const align with the '(', couldn't I?

There's a line in another patch that has a similar problem / solution.

> I think I need to review this with `git diff -b' - can you give me a
> git branch for your series or shall I git-am it ?

I'm actually rather in the middle of some further work (I'm trying to
get the script to fail on a duplicate block device) -- would you mind
running git am?

 -George


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

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

* Re: [PATCH 7/8] docs: Document block-script protocol
  2016-03-16 16:09 ` [PATCH 7/8] docs: Document block-script protocol George Dunlap
@ 2016-03-16 22:57   ` Jim Fehlig
  2016-03-17  9:55     ` George Dunlap
  2016-03-17 18:38   ` Ian Jackson
  1 sibling, 1 reply; 25+ messages in thread
From: Jim Fehlig @ 2016-03-16 22:57 UTC (permalink / raw)
  To: George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu, Roger Pau Monne

On 03/16/2016 10:09 AM, George Dunlap wrote:
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> 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 | 100 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
>
> diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
> new file mode 100644
> index 0000000..ef19207
> --- /dev/null
> +++ b/docs/misc/block-scripts.txt
> @@ -0,0 +1,100 @@
> +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 script.
> +
> +Setup
> +-----
> +
> +It is highly recommended that custom 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 do away with the block scripts
> +altogether when not actually running any scripts,

Just to clarify, do you mean drop support for all block scripts?

>  and do the duplicate
> +checking inside of libxl.  The rationale for doing this in block
> +scripts rather than in libxl isn't clear at thes point.

Block scripts like block-iscsi and block-drbd also "cook" $XENBUS/params into
$XENBUS_PATH/physical-device{,-path} right?

Regards,
Jim

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

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

* Re: [PATCH 7/8] docs: Document block-script protocol
  2016-03-16 22:57   ` Jim Fehlig
@ 2016-03-17  9:55     ` George Dunlap
  0 siblings, 0 replies; 25+ messages in thread
From: George Dunlap @ 2016-03-17  9:55 UTC (permalink / raw)
  To: Jim Fehlig
  Cc: Ian Jackson, Roger Pau Monne, Wei Liu, George Dunlap, xen-devel

On Wed, Mar 16, 2016 at 10:57 PM, Jim Fehlig <jfehlig@suse.com> wrote:
> On 03/16/2016 10:09 AM, George Dunlap wrote:
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> ---
>> 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 | 100 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 100 insertions(+)
>>
>> diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
>> new file mode 100644
>> index 0000000..ef19207
>> --- /dev/null
>> +++ b/docs/misc/block-scripts.txt
>> @@ -0,0 +1,100 @@
>> +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 script.
>> +
>> +Setup
>> +-----
>> +
>> +It is highly recommended that custom 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 do away with the block scripts
>> +altogether when not actually running any scripts,
>
> Just to clarify, do you mean drop support for all block scripts?

No -- although I can see why "do away with block scripts when not
running any scripts" is a bit confusing. :-)

What I meant was this: at the moment, when we *aren't* using custom
hotplug scripts, but we are using physical devices or files, we call a
script called "block".  What I meant was, to only run a script when a
custom hotplug script is actually requested; and in the case of
physical devices or files, get rid of the current block script and do
all the necessary work in libxl instead.

Having custom block scripts (like block-iscsi and block-drbd) is a
feature I definitely think we should keep.

I'll clarify the wording here.

>>  and do the duplicate
>> +checking inside of libxl.  The rationale for doing this in block
>> +scripts rather than in libxl isn't clear at thes point.
>
> Block scripts like block-iscsi and block-drbd also "cook" $XENBUS/params into
> $XENBUS_PATH/physical-device{,-path} right?

block-iscsi calls write_dev (as recommended in this document), so it
will always DTRT.  block-drbd must currently at least write
physical-device, or it wouldn't work.  (The "Linux block scripts must
write physical-device" is describing the implicit current protocol.)
And at least in this version of the script, block-drbd also calls
write_dev, so it will automatically be updated to the new behavior as
well:

https://www.meb.uni-bonn.de/imbie/dokus/dateien/xen-scripte/scripts/block-drbd

Nonetheless, we always need to support scripts that only write
physical-device -- at least as well as they have ever worked (which
for the time being is pv-only).  I'll try to make this clear in the
document.

 -George

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

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

* Re: [PATCH 4/8] libxl: Move check for local access to a funciton
  2016-03-16 16:09 ` [PATCH 4/8] libxl: Move check for local access to a funciton George Dunlap
  2016-03-16 16:58   ` Ian Jackson
@ 2016-03-17 18:11   ` Ian Jackson
  2016-03-21 15:35     ` George Dunlap
  1 sibling, 1 reply; 25+ messages in thread
From: Ian Jackson @ 2016-03-17 18:11 UTC (permalink / raw)
  To: George Dunlap; +Cc: George Dunlap, Roger Pau Monne, Wei Liu, xen-devel

George Dunlap writes ("[PATCH 4/8] libxl: Move check for local access to a funciton"):
> +static char * libxl__device_disk_find_local_path(libxl__gc *gc, 
> +                                                 const libxl_device_disk *disk) {
...
> +    if (disk->format == LIBXL_DISK_FORMAT_RAW
> +        && disk->script == NULL
> +        && disk->pdev_path) {

libxl__device_disk_local_initiate_attach asserts pdev_path.  Do you
foresee it maybe being NULL and if so how ?

> -    rc = libxl__device_disk_setdefault(gc, disk);
> -    if (rc) goto out;
> +    if(dls->diskpath)
         ^
Missing space.

> +        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");
> +
> +    if ((dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk))) {

We normally seem to avoid this kind of   if ((x = ...))  construction
in libxl.

But apart from that and my previous stylistic comments, this patch
looks OK.  I didn't see any other unexpected functional change.

ian.

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

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

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

George Dunlap writes ("[PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub"):
> From: George Dunlap <george.dunlap@eu.citrix.com>

Thanks.  There are a few format inconsistencies (long lines, spaces)
which I won't enumerate.

> * In the case of a block script, or a non-dom0 backend, qemuu will now
> print a warning and skip the disk, rather than adding bogus
> parameters that will cause qemuu to error out.

I think the consequences here ought to be better spelled out.  AFAICT
the implication is that, in this case, the disk will be available to
the guest as a PV disk (because libxl will select a backend other than
qdisk) but not via the emulated IDE ?

IWBNI the resulting functional restriction were documented, but I
won't insist on that given that you're improving matters.

> @@ -3023,6 +3024,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;

Is this really true ?  What if the format is qcow2 ?  I think that
might result in feeding pygrub the qcow2 file rather than the virtual
block image it contains.

Overall I think this patch is otherwise probably good but it it's a
bit hard to see the wood for the trees.  If you felt like factoring
out some of the refactoring and variable renaming, from the functional
change, that would be very nice.

Ian.

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

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

* Re: [PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts
  2016-03-16 16:09 ` [PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts George Dunlap
@ 2016-03-17 18:36   ` Ian Jackson
  2016-03-18 17:17     ` George Dunlap
  0 siblings, 1 reply; 25+ messages in thread
From: Ian Jackson @ 2016-03-17 18:36 UTC (permalink / raw)
  To: George Dunlap; +Cc: George Dunlap, Roger Pau Monne, Wei Liu, xen-devel

George Dunlap writes ("[PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts"):
> 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.

Some long lines, `if(', etc., in this patch, I'm afraid.

> Unfortunately, while the plugging machinery works for pygrub, it does
> not yet work for qemuu; meaning that disk hotplug scripts cannot be
> used with 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 bre written to
> physical-device-path.
> 
> Modify libxl__device_disk_setdefault() to be able to fish this path
> out of xenstore and pass it back.
> 
> We need the target domid to find the appropriate xenstore node, so add
> that to libxl__disk_local_state.

I couldn't find that change in this patch.

> +    /* 
> +     * 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 && domid != INVALID_DOMID) {
> +        libxl__device device;
> +        char *be_path, *pdpath;
> +        int rc;

I don't see where you check that the disk is not being provided by a
driver domain - in which case the hotplug script ran in the driver
domain and the device path is also only in the driver domain.

(By `driver domain' I mean, really, a domain other than this one.)

>  _hidden char * libxl__device_disk_find_local_path(libxl__gc *gc, 
> +                                                  libxl_domid domid,

This new parameter would maybe be a little less confusing if it were
explicitly `guest_domid'; after all it might be the driver domain
domid.

Thanks,
Ian.

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

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

* Re: [PATCH 7/8] docs: Document block-script protocol
  2016-03-16 16:09 ` [PATCH 7/8] docs: Document block-script protocol George Dunlap
  2016-03-16 22:57   ` Jim Fehlig
@ 2016-03-17 18:38   ` Ian Jackson
  1 sibling, 0 replies; 25+ messages in thread
From: Ian Jackson @ 2016-03-17 18:38 UTC (permalink / raw)
  To: George Dunlap; +Cc: Roger Pau Monne, Wei Liu, xen-devel

George Dunlap writes ("[PATCH 7/8] docs: Document block-script protocol"):
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

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

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

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

* Re: [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub
  2016-03-17 18:22   ` Ian Jackson
@ 2016-03-18 17:09     ` George Dunlap
  2016-04-01 14:17       ` Ian Jackson
  0 siblings, 1 reply; 25+ messages in thread
From: George Dunlap @ 2016-03-18 17:09 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Wei Liu, George Dunlap, xen-devel, Stefano Stabellini,
	Anthony Perard, Roger Pau Monne

On Thu, Mar 17, 2016 at 6:22 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> George Dunlap writes ("[PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub"):
>> From: George Dunlap <george.dunlap@eu.citrix.com>
>
> Thanks.  There are a few format inconsistencies (long lines, spaces)
> which I won't enumerate.
>
>> * In the case of a block script, or a non-dom0 backend, qemuu will now
>> print a warning and skip the disk, rather than adding bogus
>> parameters that will cause qemuu to error out.
>
> I think the consequences here ought to be better spelled out.  AFAICT
> the implication is that, in this case, the disk will be available to
> the guest as a PV disk (because libxl will select a backend other than
> qdisk) but not via the emulated IDE ?
>
> IWBNI the resulting functional restriction were documented, but I
> won't insist on that given that you're improving matters.
>
>> @@ -3023,6 +3024,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;
>
> Is this really true ?  What if the format is qcow2 ?  I think that
> might result in feeding pygrub the qcow2 file rather than the virtual
> block image it contains.

That's what the "qdisk_direct" argument is for -- it tells
libxl__device_disk_find_local_path, "I can interpret QDISK backends".
Pygrub will call this function with "qdisk_direct" set to "false",
since it can't interpret QDISK backends; this will result in it doing
a local attach instead.

> Overall I think this patch is otherwise probably good but it it's a
> bit hard to see the wood for the trees.  If you felt like factoring
> out some of the refactoring and variable renaming, from the functional
> change, that would be very nice.

Yes, there are a lot of things going on, aren't there.  I think I
remember trying to make it simpler when I first wrote it last year,
but not finding any really sensible in-between points; but I'll give
it another look.

 -George

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

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

* Re: [PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts
  2016-03-17 18:36   ` Ian Jackson
@ 2016-03-18 17:17     ` George Dunlap
  0 siblings, 0 replies; 25+ messages in thread
From: George Dunlap @ 2016-03-18 17:17 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Wei Liu, George Dunlap, Roger Pau Monne

On Thu, Mar 17, 2016 at 6:36 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> George Dunlap writes ("[PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts"):
>> 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.
>
> Some long lines, `if(', etc., in this patch, I'm afraid.
>
>> Unfortunately, while the plugging machinery works for pygrub, it does
>> not yet work for qemuu; meaning that disk hotplug scripts cannot be
>> used with 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 bre written to
>> physical-device-path.
>>
>> Modify libxl__device_disk_setdefault() to be able to fish this path
>> out of xenstore and pass it back.
>>
>> We need the target domid to find the appropriate xenstore node, so add
>> that to libxl__disk_local_state.
>
> I couldn't find that change in this patch.

Oops -- forgot to take this paragraph out after porting it on top of
patch 2 (which changes libxl not to duplicate the work of 'block'
anymore).  I'll remove this.

>
>> +    /*
>> +     * 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 && domid != INVALID_DOMID) {
>> +        libxl__device device;
>> +        char *be_path, *pdpath;
>> +        int rc;
>
> I don't see where you check that the disk is not being provided by a
> driver domain - in which case the hotplug script ran in the driver
> domain and the device path is also only in the driver domain.
>
> (By `driver domain' I mean, really, a domain other than this one.)

It's in patch 4 (still at the top of this function):

+    /* No local paths for driver domains */
+    if (disk->backend_domname != NULL) {
+        LOG(DEBUG, "Non-local backend, can't access locally.\n");
+        goto out;
+    }

>
>>  _hidden char * libxl__device_disk_find_local_path(libxl__gc *gc,
>> +                                                  libxl_domid domid,
>
> This new parameter would maybe be a little less confusing if it were
> explicitly `guest_domid'; after all it might be the driver domain
> domid.

Ack.

 -George

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

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

* Re: [PATCH 4/8] libxl: Move check for local access to a funciton
  2016-03-17 18:11   ` Ian Jackson
@ 2016-03-21 15:35     ` George Dunlap
  0 siblings, 0 replies; 25+ messages in thread
From: George Dunlap @ 2016-03-21 15:35 UTC (permalink / raw)
  To: Ian Jackson; +Cc: George Dunlap, Roger Pau Monne, Wei Liu, xen-devel

On 17/03/16 18:11, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 4/8] libxl: Move check for local access to a funciton"):
>> +static char * libxl__device_disk_find_local_path(libxl__gc *gc, 
>> +                                                 const libxl_device_disk *disk) {
> ...
>> +    if (disk->format == LIBXL_DISK_FORMAT_RAW
>> +        && disk->script == NULL
>> +        && disk->pdev_path) {
> 
> libxl__device_disk_local_initiate_attach asserts pdev_path.  Do you
> foresee it maybe being NULL and if so how ?

I think I must have been copying the existing code, which checks for
pdev_path of disk (rather than in_disk, which is what's being passed to
this function).

In the next patch this will also be called from the devicemodel code,
but that code dereferences disk[i].pdev_path (implicitly assuming that
it will never be null), so I think we can drop this check.

Ack on the other two comments.

 -George


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

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

* Re: [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub
  2016-03-18 17:09     ` George Dunlap
@ 2016-04-01 14:17       ` Ian Jackson
  0 siblings, 0 replies; 25+ messages in thread
From: Ian Jackson @ 2016-04-01 14:17 UTC (permalink / raw)
  To: George Dunlap
  Cc: Wei Liu, George Dunlap, xen-devel, Stefano Stabellini,
	Anthony Perard, Roger Pau Monne

George Dunlap writes ("Re: [Xen-devel] [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub"):
> That's what the "qdisk_direct" argument is for -- it tells
> libxl__device_disk_find_local_path, "I can interpret QDISK backends".
> Pygrub will call this function with "qdisk_direct" set to "false",
> since it can't interpret QDISK backends; this will result in it doing
> a local attach instead.

Thanks for the explanation.

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

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

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

end of thread, other threads:[~2016-04-01 14:17 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-16 16:09 [PATCH 0/8] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
2016-03-16 16:09 ` [PATCH 1/8] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
2016-03-16 16:54   ` Ian Jackson
2016-03-16 16:09 ` [PATCH 2/8] libxl: Remove redundant setting of phyical-device George Dunlap
2016-03-16 16:54   ` Ian Jackson
2016-03-16 16:09 ` [PATCH 3/8] tools/hotplug: Write physical-device-path in addition to physical-device George Dunlap
2016-03-16 16:56   ` Ian Jackson
2016-03-16 16:57     ` George Dunlap
2016-03-16 16:09 ` [PATCH 4/8] libxl: Move check for local access to a funciton George Dunlap
2016-03-16 16:58   ` Ian Jackson
2016-03-16 17:02     ` George Dunlap
2016-03-17 18:11   ` Ian Jackson
2016-03-21 15:35     ` George Dunlap
2016-03-16 16:09 ` [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub George Dunlap
2016-03-17 18:22   ` Ian Jackson
2016-03-18 17:09     ` George Dunlap
2016-04-01 14:17       ` Ian Jackson
2016-03-16 16:09 ` [PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts George Dunlap
2016-03-17 18:36   ` Ian Jackson
2016-03-18 17:17     ` George Dunlap
2016-03-16 16:09 ` [PATCH 7/8] docs: Document block-script protocol George Dunlap
2016-03-16 22:57   ` Jim Fehlig
2016-03-17  9:55     ` George Dunlap
2016-03-17 18:38   ` Ian Jackson
2016-03-16 16:09 ` [PATCH 8/8] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path 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).