[PATCH v3 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts

[PATCH v3 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts

[George Dunlap]

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.

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

Changes in v3:
- Fix stray comma
- Make it clear that block-script.txt inputs are Linux-specific

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         |  96 +++++++++++++++++++++++++++
 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, 433 insertions(+), 78 deletions(-)
 create mode 100644 docs/misc/block-scripts.txt
 create mode 100644 tools/hotplug/Linux/block-dummy

-- 
2.1.4


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

[PATCH v3 1/9] tools/hotplug: Add a "dummy" hotplug script for testing

[George Dunlap]

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

[PATCH v3 2/9] libxl: Remove redundant setting of phyical-device

[George Dunlap]

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

[PATCH v3 3/9] tools/hotplug: Write physical-device-path in addition to physical-device

[George Dunlap]

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

[PATCH v3 4/9] libxl: Move check for local access to a funciton

[George Dunlap]

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

[PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code

[George Dunlap]

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 cfda24c..1effd71 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1167,9 +1167,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",
@@ -1177,22 +1177,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",
@@ -1200,14 +1200,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.
                  *
@@ -1215,9 +1223,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
@@ -1231,7 +1240,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;
@@ -1242,7 +1251,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

[PATCH v3 6/9] libxl: Share logic for finding path between qemuu and pygrub

[George Dunlap]

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 v2:
- Fix stray comma

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 1effd71..2eef53a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1188,23 +1188,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

[PATCH v3 7/9] libxl: Allow local access for block devices with hotplug scripts

[George Dunlap]

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 2eef53a..07559b6 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1214,8 +1214,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

[PATCH v3 8/9] docs: Document block-script protocol

[George Dunlap]

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Changes since v2:
- Note that the "Inputs" describe Linux only

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

diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
new file mode 100644
index 0000000..f9a001e
--- /dev/null
+++ b/docs/misc/block-scripts.txt
@@ -0,0 +1,112 @@
+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
+------
+
+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
+
+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

[PATCH v3 9/9] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path

[George Dunlap]

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 f9a001e..5390e7c 100644
--- a/docs/misc/block-scripts.txt
+++ b/docs/misc/block-scripts.txt
@@ -59,18 +59,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
 -------------------------
@@ -85,25 +85,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

Re: [PATCH v3 3/9] tools/hotplug: Write physical-device-path in addition to physical-device

[Ian Jackson]

George Dunlap writes ("[PATCH v3 3/9] 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.
> 
> 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>

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

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

Re: [PATCH v3 4/9] libxl: Move check for local access to a funciton

[Ian Jackson]

George Dunlap writes ("[PATCH v3 4/9] 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.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.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

Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code

[Ian Jackson]

George Dunlap writes ("[PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code"):
> 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

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

However, I think that this is going to have a conflict with the colo
series...

Ian.

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

Re: [PATCH v3 6/9] libxl: Share logic for finding path between qemuu and pygrub

[Ian Jackson]

George Dunlap writes ("[PATCH v3 6/9] libxl: Share logic for finding path between qemuu and pygrub"):
> 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.

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

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

Re: [PATCH v3 7/9] libxl: Allow local access for block devices with hotplug scripts

[Ian Jackson]

George Dunlap writes ("[PATCH v3 7/9] 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.

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

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

Re: [PATCH v3 8/9] docs: Document block-script protocol

[Ian Jackson]

George Dunlap writes ("[PATCH v3 8/9] docs: Document block-script protocol"):
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Changes since v2:
> - Note that the "Inputs" describe Linux only
> 
> Changes since v1:
> - Attempt to make a clear distinction between custom hotplug scripts
> and the script called for raw physical devices and files

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

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

Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code

[Ian Jackson]

George Dunlap writes ("[PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code"):
> Reorganize the qemuu disk argument code to make a clean separation
> between finding a file to use, and constructing the parameters:

This didn't apply to staging, since colo went in.
I have tried to rebase it and the result compiles.

Can you check it's right please ?

Thanks,
Ian.

From 6ab86b63462c8e6dc243c796f5ea10240aadc5de Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 24 Mar 2016 17:18:33 +0000
Subject: [PATCH] libxl: Rearrange qemu upstream disk argument code

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>
Signed-off-by: Ian Jackson <ian.jackson@eu.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 |   76 ++++++++++++++++++++++++++----------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3522fbf..c11e9b8 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -761,7 +761,7 @@ enum {
     LIBXL__COLO_SECONDARY,
 };
 
-static char *qemu_disk_scsi_drive_string(libxl__gc *gc, const char *pdev_path,
+static char *qemu_disk_scsi_drive_string(libxl__gc *gc, const char *target_path,
                                          int unit, const char *format,
                                          const libxl_device_disk *disk,
                                          int colo_mode)
@@ -775,14 +775,14 @@ static char *qemu_disk_scsi_drive_string(libxl__gc *gc, const char *pdev_path,
     case LIBXL__COLO_NONE:
         drive = libxl__sprintf
             (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
-             pdev_path, unit, format);
+             target_path, unit, format);
         break;
     case LIBXL__COLO_PRIMARY:
         /*
          * primary:
          *  -dirve if=scsi,bus=0,unit=x,cache=writeback,driver=quorum,\
          *  id=exportname,\
-         *  children.0.file.filename=pdev_path,\
+         *  children.0.file.filename=target_path,\
          *  children.0.driver=format,\
          *  read-pattern=fifo,\
          *  vote-threshold=1
@@ -794,7 +794,7 @@ static char *qemu_disk_scsi_drive_string(libxl__gc *gc, const char *pdev_path,
             "children.0.driver=%s,"
             "read-pattern=fifo,"
             "vote-threshold=1",
-            unit, exportname, pdev_path, format);
+            unit, exportname, target_path, format);
         break;
     case LIBXL__COLO_SECONDARY:
         /*
@@ -823,7 +823,7 @@ static char *qemu_disk_scsi_drive_string(libxl__gc *gc, const char *pdev_path,
     return drive;
 }
 
-static char *qemu_disk_ide_drive_string(libxl__gc *gc, const char *pdev_path,
+static char *qemu_disk_ide_drive_string(libxl__gc *gc, const char *target_path,
                                         int unit, const char *format,
                                         const libxl_device_disk *disk,
                                         int colo_mode)
@@ -837,14 +837,14 @@ static char *qemu_disk_ide_drive_string(libxl__gc *gc, const char *pdev_path,
     case LIBXL__COLO_NONE:
         drive = GCSPRINTF
             ("file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
-             pdev_path, unit, format);
+             target_path, unit, format);
         break;
     case LIBXL__COLO_PRIMARY:
         /*
          * primary:
          *  -dirve if=ide,index=x,media=disk,cache=writeback,driver=quorum,\
          *  id=exportname,\
-         *  children.0.file.filename=pdev_path,\
+         *  children.0.file.filename=target_path,\
          *  children.0.driver=format,\
          *  read-pattern=fifo,\
          *  vote-threshold=1
@@ -856,7 +856,7 @@ static char *qemu_disk_ide_drive_string(libxl__gc *gc, const char *pdev_path,
             "children.0.driver=%s,"
             "read-pattern=fifo,"
             "vote-threshold=1",
-             unit, exportname, pdev_path, format);
+             unit, exportname, target_path, format);
         break;
     case LIBXL__COLO_SECONDARY:
         /*
@@ -1305,9 +1305,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;
             int colo_mode;
 
             if (dev_number == -1) {
@@ -1316,22 +1316,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",
@@ -1339,14 +1339,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.
                  *
@@ -1367,12 +1375,12 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                     if (colo_mode == LIBXL__COLO_SECONDARY) {
                         drive = libxl__sprintf
                             (gc, "if=none,driver=%s,file=%s,id=%s",
-                             format, pdev_path, disks[i].colo_export);
+                             format, target_path, disks[i].colo_export);
 
                         flexarray_append(dm_args, "-drive");
                         flexarray_append(dm_args, drive);
                     }
-                    drive = qemu_disk_scsi_drive_string(gc, pdev_path, disk,
+                    drive = qemu_disk_scsi_drive_string(gc, target_path, disk,
                                                         format,
                                                         &disks[i],
                                                         colo_mode);
@@ -1389,7 +1397,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;
@@ -1401,12 +1409,12 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                     if (colo_mode == LIBXL__COLO_SECONDARY) {
                         drive = libxl__sprintf
                             (gc, "if=none,driver=%s,file=%s,id=%s",
-                             format, pdev_path, disks[i].colo_export);
+                             format, target_path, disks[i].colo_export);
 
                         flexarray_append(dm_args, "-drive");
                         flexarray_append(dm_args, drive);
                     }
-                    drive = qemu_disk_ide_drive_string(gc, pdev_path, disk,
+                    drive = qemu_disk_ide_drive_string(gc, target_path, disk,
                                                        format,
                                                        &disks[i],
                                                        colo_mode);
-- 
1.7.10.4


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

Re: [PATCH v3 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts

[Ian Jackson]

(Firstly, did you not intend to CC tools maintainers on your 0/9 ?)

George Dunlap writes ("[Xen-devel] [PATCH v3 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts"):
> 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.
> 
> git://xenbits.xenproject.org/people/gdunlap/xen.git out/hotplug-script-improvements/v3

Thanks.  See my other mails about this.  I have rebased it to staging,
fixed up a conflict (hopefully!), and transcribed my acks.

The result is here:

  git://xenbits.xenproject.org/people/iwj/xen.git wip.gwd.hotplug-v3.1

Ian.

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

Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code

[George Dunlap]

On 01/04/16 15:31, Ian Jackson wrote:
> George Dunlap writes ("[PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code"):
>> Reorganize the qemuu disk argument code to make a clean separation
>> between finding a file to use, and constructing the parameters:
> 
> This didn't apply to staging, since colo went in.
> I have tried to rebase it and the result compiles.
> 
> Can you check it's right please ?
> 
> Thanks,
> Ian.
> 
> From 6ab86b63462c8e6dc243c796f5ea10240aadc5de Mon Sep 17 00:00:00 2001
> From: George Dunlap <george.dunlap@citrix.com>
> Date: Thu, 24 Mar 2016 17:18:33 +0000
> Subject: [PATCH] libxl: Rearrange qemu upstream disk argument code
> 
> 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>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

I looked through the patch in the branch provided in your reply to 0/9
[1], and it looks correct; morever, I tested it and it and the basic
functionality (using the "dummy" block script) still works.

Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Tested-by: George Dunlap <george.dunlap@citrix.com>

[1] git://xenbits.xenproject.org/people/iwj/xen.git wip.gwd.hotplug-v3.1


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

Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code

[Ian Jackson]

George Dunlap writes ("Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code"):
> I looked through the patch in the branch provided in your reply to 0/9
> [1], and it looks correct; morever, I tested it and it and the basic
> functionality (using the "dummy" block script) still works.
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
> Tested-by: George Dunlap <george.dunlap@citrix.com>
> 
> [1] git://xenbits.xenproject.org/people/iwj/xen.git wip.gwd.hotplug-v3.1

Thanks, I have pushed it (rebased) to staging.

Ian.

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

Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code

[Andrew Cooper]

On 04/04/16 17:59, Ian Jackson wrote:
> George Dunlap writes ("Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code"):
>> I looked through the patch in the branch provided in your reply to 0/9
>> [1], and it looks correct; morever, I tested it and it and the basic
>> functionality (using the "dummy" block script) still works.
>>
>> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
>> Tested-by: George Dunlap <george.dunlap@citrix.com>
>>
>> [1] git://xenbits.xenproject.org/people/iwj/xen.git wip.gwd.hotplug-v3.1
> Thanks, I have pushed it (rebased) to staging.

New build failure on CentOS 7

libxl_dm.c: In function 'libxl__build_device_model_args':
libxl_dm.c:1374:27: error: 'target_path' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
                     drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
                           ^
libxl_dm.c:1310:25: note: 'target_path' was declared here
             const char *target_path;
                         ^
cc1: all warnings being treated as errors

~Andrew

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

Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code

[Andrew Cooper]

On 04/04/16 18:11, Andrew Cooper wrote:
> On 04/04/16 17:59, Ian Jackson wrote:
>> George Dunlap writes ("Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code"):
>>> I looked through the patch in the branch provided in your reply to 0/9
>>> [1], and it looks correct; morever, I tested it and it and the basic
>>> functionality (using the "dummy" block script) still works.
>>>
>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
>>> Tested-by: George Dunlap <george.dunlap@citrix.com>
>>>
>>> [1] git://xenbits.xenproject.org/people/iwj/xen.git wip.gwd.hotplug-v3.1
>> Thanks, I have pushed it (rebased) to staging.
> New build failure on CentOS 7
>
> libxl_dm.c: In function 'libxl__build_device_model_args':
> libxl_dm.c:1374:27: error: 'target_path' may be used uninitialized in
> this function [-Werror=maybe-uninitialized]
>                      drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
>                            ^
> libxl_dm.c:1310:25: note: 'target_path' was declared here
>              const char *target_path;
>                          ^
> cc1: all warnings being treated as errors

Sorry - sent too early.  Specifically, target_path is genuinely
uninitialised in the case that there is an empty CDROM specified.

~Andrew

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

Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code

[George Dunlap]

On Mon, Apr 4, 2016 at 6:16 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 04/04/16 18:11, Andrew Cooper wrote:
>> On 04/04/16 17:59, Ian Jackson wrote:
>>> George Dunlap writes ("Re: [PATCH v3 5/9] libxl: Rearrange qemu upstream disk argument code"):
>>>> I looked through the patch in the branch provided in your reply to 0/9
>>>> [1], and it looks correct; morever, I tested it and it and the basic
>>>> functionality (using the "dummy" block script) still works.
>>>>
>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
>>>> Tested-by: George Dunlap <george.dunlap@citrix.com>
>>>>
>>>> [1] git://xenbits.xenproject.org/people/iwj/xen.git wip.gwd.hotplug-v3.1
>>> Thanks, I have pushed it (rebased) to staging.
>> New build failure on CentOS 7
>>
>> libxl_dm.c: In function 'libxl__build_device_model_args':
>> libxl_dm.c:1374:27: error: 'target_path' may be used uninitialized in
>> this function [-Werror=maybe-uninitialized]
>>                      drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
>>                            ^
>> libxl_dm.c:1310:25: note: 'target_path' was declared here
>>              const char *target_path;
>>                          ^
>> cc1: all warnings being treated as errors
>
> Sorry - sent too early.  Specifically, target_path is genuinely
> uninitialised in the case that there is an empty CDROM specified.

Indeed -- what's strange is that I actually tested this on a CentOS 7
image and it didn't complain.

Anyway, a patch will be on its way...

 -George

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