snapshot: also support disks by path

I got confused when 'virsh domblkinfo dom disk' required the
path to a disk (which can be ambiguous, since a single file
can back multiple disks), rather than the unambiguous target
device name that I was using in disk snapshots.  So, in true
developer fashion, I went for the best of both worlds - all
interfaces that operate on a disk (aka block) now accept
either the target name or the unambiguous path to the backing
file used by the disk.

* src/conf/domain_conf.h (virDomainDiskIndexByName): Add
parameter.
(virDomainDiskPathByName): New prototype.
* src/libvirt_private.syms (domain_conf.h): Export it.
* src/conf/domain_conf.c (virDomainDiskIndexByName): Also allow
searching by path, and decide whether ambiguity is okay.
(virDomainDiskPathByName): New function.
(virDomainDiskRemoveByName, virDomainSnapshotAlignDisks): Update
callers.
* src/qemu/qemu_driver.c (qemudDomainBlockPeek)
(qemuDomainAttachDeviceConfig, qemuDomainUpdateDeviceConfig)
(qemuDomainGetBlockInfo, qemuDiskPathToAlias): Likewise.
* src/qemu/qemu_process.c (qemuProcessFindDomainDiskByPath):
Likewise.
* src/libxl/libxl_driver.c (libxlDomainAttachDeviceDiskLive)
(libxlDomainDetachDeviceDiskLive, libxlDomainAttachDeviceConfig)
(libxlDomainUpdateDeviceConfig): Likewise.
* src/uml/uml_driver.c (umlDomainBlockPeek): Likewise.
* src/xen/xend_internal.c (xenDaemonDomainBlockPeek): Likewise.
* docs/formatsnapshot.html.in: Update documentation.
* tools/virsh.pod (domblkstat, domblkinfo): Likewise.
* docs/schemas/domaincommon.rng (diskTarget): Tighten pattern on
disk targets.
* docs/schemas/domainsnapshot.rng (disksnapshot): Update to match.
* tests/domainsnapshotxml2xmlin/disk_snapshot.xml: Update test.

docs/formatsnapshot.html.in

@@ -129,8 +129,9 @@
           <dt><code>disk</code></dt>
           <dd>This sub-element describes the snapshot properties of a
             specific disk.  The attribute <code>name</code> is
-            mandatory, and must match the <code>&lt;target
+            mandatory, and must match either the <code>&lt;target
-            dev='name'/&gt;</code> of one of
+            dev='name'/&gt;</code> or an unambiguous <code>&lt;source
+            file='name'/&gt;</code> of one of
             the <a href="formatdomain.html#elementsDisks">disk
             devices</a> specified for the domain at the time of the
             snapshot.  The attribute <code>snapshot</code> is
@@ -203,7 +204,7 @@
 &lt;domainsnapshot&gt;
   &lt;description&gt;Snapshot of OS install and updates&lt;/description&gt;
   &lt;disks&gt;
-    &lt;disk name='vda'&gt;
+    &lt;disk name='/path/to/old'&gt;
       &lt;source file='/path/to/new'/&gt;
     &lt;/disk&gt;
     &lt;disk name='vdb' snapshot='no'/&gt;

docs/schemas/domaincommon.rng

@@ -770,10 +770,15 @@
       </choice>
     </element>
   </define>
+  <define name="diskTarget">
+    <data type="string">
+      <param name="pattern">(ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+</param>
+    </data>
+  </define>
   <define name="target">
     <element name="target">
       <attribute name="dev">
-        <ref name="deviceName"/>
+        <ref name="diskTarget"/>
       </attribute>
       <optional>
         <attribute name="bus">

docs/schemas/domainsnapshot.rng

@@ -82,7 +82,10 @@
   <define name='disksnapshot'>
     <element name='disk'>
       <attribute name='name'>
-        <ref name='deviceName'/>
+        <choice>
+          <ref name='diskTarget'/>
+          <ref name='absFilePath'/>
+        </choice>
       </attribute>
       <choice>
         <attribute name='snapshot'>

src/conf/domain_conf.c

@@ -5811,17 +5811,44 @@ virDomainChrTargetTypeToString(int deviceType,
     return type;
 }
 
-int virDomainDiskIndexByName(virDomainDefPtr def, const char *name)
+int
+virDomainDiskIndexByName(virDomainDefPtr def, const char *name,
+                         bool allow_ambiguous)
 {
     virDomainDiskDefPtr vdisk;
     int i;
+    int candidate = -1;
 
+    /* We prefer the <target dev='name'/> name (it's shorter, required
+     * for all disks, and should be unambiguous), but also support
+     * <source file='name'/> (if unambiguous).  Assume dst if there is
+     * no leading slash, source name otherwise.  */
     for (i = 0; i < def->ndisks; i++) {
         vdisk = def->disks[i];
-        if (STREQ(vdisk->dst, name))
+        if (*name != '/') {
-            return i;
+            if (STREQ(vdisk->dst, name))
+                return i;
+        } else if (vdisk->src &&
+                   STREQ(vdisk->src, name)) {
+            if (allow_ambiguous)
+                return i;
+            if (candidate >= 0)
+                return -1;
+            candidate = i;
+        }
     }
-    return -1;
+    return candidate;
+}
+
+/* Return the path to a disk image if a string identifies at least one
+ * disk belonging to the domain (both device strings 'vda' and paths
+ * '/path/to/file' are converted into '/path/to/file').  */
+const char *
+virDomainDiskPathByName(virDomainDefPtr def, const char *name)
+{
+    int i = virDomainDiskIndexByName(def, name, true);
+
+    return i < 0 ? NULL : def->disks[i]->src;
 }
 
 int virDomainDiskInsert(virDomainDefPtr def,
@@ -5897,7 +5924,7 @@ void virDomainDiskRemove(virDomainDefPtr def, size_t i)
 
 int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name)
 {
-    int i = virDomainDiskIndexByName(def, name);
+    int i = virDomainDiskIndexByName(def, name, false);
     if (i < 0)
         return -1;
     virDomainDiskRemove(def, i);
@@ -11651,11 +11678,12 @@ disksorter(const void *a, const void *b)
 
 /* Align def->disks to def->domain.  Sort the list of def->disks,
  * filling in any missing disks or snapshot state defaults given by
- * the domain, with a fallback to a passed in default.  Issue an error
+ * the domain, with a fallback to a passed in default.  Convert paths
- * and return -1 if any def->disks[n]->name appears more than once or
+ * to disk targets for uniformity.  Issue an error and return -1 if
- * does not map to dom->disks.  If require_match, also require that
+ * any def->disks[n]->name appears more than once or does not map to
- * existing def->disks snapshot states do not override explicit
+ * dom->disks.  If require_match, also require that existing
- * def->dom settings.  */
+ * def->disks snapshot states do not override explicit def->dom
+ * settings.  */
 int
 virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
                             int default_snapshot,
@@ -11693,7 +11721,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
     /* Double check requested disks.  */
     for (i = 0; i < def->ndisks; i++) {
         virDomainSnapshotDiskDefPtr disk = &def->disks[i];
-        int idx = virDomainDiskIndexByName(def->dom, disk->name);
+        int idx = virDomainDiskIndexByName(def->dom, disk->name, false);
         int disk_snapshot;
 
         if (idx < 0) {
@@ -11731,6 +11759,13 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
                                  disk->file, disk->name);
             goto cleanup;
         }
+        if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) {
+            VIR_FREE(disk->name);
+            if (!(disk->name = strdup(def->dom->disks[idx]->dst))) {
+                virReportOOMError();
+                goto cleanup;
+            }
+        }
     }
 
     /* Provide defaults for all remaining disks.  */

src/conf/domain_conf.h

@@ -1648,7 +1648,9 @@ int virDomainVcpuPinAdd(virDomainDefPtr def,
 
 int virDomainVcpuPinDel(virDomainDefPtr def, int vcpu);
 
-int virDomainDiskIndexByName(virDomainDefPtr def, const char *name);
+int virDomainDiskIndexByName(virDomainDefPtr def, const char *name,
+                             bool allow_ambiguous);
+const char *virDomainDiskPathByName(virDomainDefPtr, const char *name);
 int virDomainDiskInsert(virDomainDefPtr def,
                         virDomainDiskDefPtr disk);
 void virDomainDiskInsertPreAlloced(virDomainDefPtr def,

src/libvirt_private.syms

@@ -287,6 +287,7 @@ virDomainDiskInsert;
 virDomainDiskInsertPreAlloced;
 virDomainDiskIoTypeFromString;
 virDomainDiskIoTypeToString;
+virDomainDiskPathByName;
 virDomainDiskRemove;
 virDomainDiskRemoveByName;
 virDomainDiskSnapshotTypeFromString;

src/libxl/libxl_driver.c

@@ -2929,7 +2929,7 @@ libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv,
             break;
         case VIR_DOMAIN_DISK_DEVICE_DISK:
             if (l_disk->bus == VIR_DOMAIN_DISK_BUS_XEN) {
-                if (virDomainDiskIndexByName(vm->def, l_disk->dst) >= 0) {
+                if (virDomainDiskIndexByName(vm->def, l_disk->dst, true) >= 0) {
                     libxlError(VIR_ERR_OPERATION_FAILED,
                             _("target %s already exists"), l_disk->dst);
                     goto cleanup;
@@ -2991,7 +2991,8 @@ libxlDomainDetachDeviceDiskLive(libxlDomainObjPrivatePtr priv,
             if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_XEN) {
 
                 if ((i = virDomainDiskIndexByName(vm->def,
-                                                  dev->data.disk->dst)) < 0) {
+                                                  dev->data.disk->dst,
+                                                  false)) < 0) {
                     libxlError(VIR_ERR_OPERATION_FAILED,
                                _("disk %s not found"), dev->data.disk->dst);
                     goto cleanup;
@@ -3061,7 +3062,7 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
     switch (dev->type) {
         case VIR_DOMAIN_DEVICE_DISK:
             disk = dev->data.disk;
-            if (virDomainDiskIndexByName(vmdef, disk->dst) >= 0) {
+            if (virDomainDiskIndexByName(vmdef, disk->dst, true) >= 0) {
                 libxlError(VIR_ERR_INVALID_ARG,
                         _("target %s already exists."), disk->dst);
                 return -1;
@@ -3172,9 +3173,9 @@ libxlDomainUpdateDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
     switch (dev->type) {
         case VIR_DOMAIN_DEVICE_DISK:
             disk = dev->data.disk;
-            if ((i = virDomainDiskIndexByName(vmdef, disk->dst)) < 0) {
+            if ((i = virDomainDiskIndexByName(vmdef, disk->dst, false)) < 0) {
                 libxlError(VIR_ERR_INVALID_ARG,
-                           _("target %s doesn't exists."), disk->dst);
+                           _("target %s doesn't exist."), disk->dst);
                 goto cleanup;
             }
             orig = vmdef->disks[i];

src/qemu/qemu_driver.c

@@ -5495,7 +5495,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
     switch (dev->type) {
     case VIR_DOMAIN_DEVICE_DISK:
         disk = dev->data.disk;
-        if (virDomainDiskIndexByName(vmdef, disk->dst) >= 0) {
+        if (virDomainDiskIndexByName(vmdef, disk->dst, true) >= 0) {
             qemuReportError(VIR_ERR_INVALID_ARG,
                             _("target %s already exists."), disk->dst);
             return -1;
@@ -5613,10 +5613,10 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
     switch (dev->type) {
     case VIR_DOMAIN_DEVICE_DISK:
         disk = dev->data.disk;
-        pos = virDomainDiskIndexByName(vmdef, disk->dst);
+        pos = virDomainDiskIndexByName(vmdef, disk->dst, false);
         if (pos < 0) {
             qemuReportError(VIR_ERR_INVALID_ARG,
-                            _("target %s doesn't exists."), disk->dst);
+                            _("target %s doesn't exist."), disk->dst);
             return -1;
         }
         orig = vmdef->disks[pos];
@@ -7355,7 +7355,8 @@ qemudDomainBlockPeek (virDomainPtr dom,
 {
     struct qemud_driver *driver = dom->conn->privateData;
     virDomainObjPtr vm;
-    int fd = -1, ret = -1, i;
+    int fd = -1, ret = -1;
+    const char *actual;
 
     virCheckFlags(0, -1);
 
@@ -7377,41 +7378,34 @@ qemudDomainBlockPeek (virDomainPtr dom,
         goto cleanup;
     }
 
-    /* Check the path belongs to this domain. */
+    /* Check the path belongs to this domain.  */
-    for (i = 0 ; i < vm->def->ndisks ; i++) {
+    if (!(actual = virDomainDiskPathByName(vm->def, path))) {
-        if (vm->def->disks[i]->src != NULL &&
-            STREQ (vm->def->disks[i]->src, path)) {
-            ret = 0;
-            break;
-        }
-    }
-
-    if (ret == 0) {
-        ret = -1;
-        /* The path is correct, now try to open it and get its size. */
-        fd = open (path, O_RDONLY);
-        if (fd == -1) {
-            virReportSystemError(errno,
-                                 _("%s: failed to open"), path);
-            goto cleanup;
-        }
-
-        /* Seek and read. */
-        /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
-         * be 64 bits on all platforms.
-         */
-        if (lseek (fd, offset, SEEK_SET) == (off_t) -1 ||
-            saferead (fd, buffer, size) == (ssize_t) -1) {
-            virReportSystemError(errno,
-                                 _("%s: failed to seek or read"), path);
-            goto cleanup;
-        }
-
-        ret = 0;
-    } else {
         qemuReportError(VIR_ERR_INVALID_ARG,
-                        "%s", _("invalid path"));
+                        _("invalid path '%s'"), path);
+        goto cleanup;
     }
+    path = actual;
+
+    /* The path is correct, now try to open it and get its size. */
+    fd = open(path, O_RDONLY);
+    if (fd == -1) {
+        virReportSystemError(errno,
+                             _("%s: failed to open"), path);
+        goto cleanup;
+    }
+
+    /* Seek and read. */
+    /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
+     * be 64 bits on all platforms.
+     */
+    if (lseek(fd, offset, SEEK_SET) == (off_t) -1 ||
+        saferead(fd, buffer, size) == (ssize_t) -1) {
+        virReportSystemError(errno,
+                             _("%s: failed to seek or read"), path);
+        goto cleanup;
+    }
+
+    ret = 0;
 
 cleanup:
     VIR_FORCE_CLOSE(fd);
@@ -7491,7 +7485,7 @@ qemudDomainMemoryPeek (virDomainPtr dom,
     qemuDomainObjExitMonitor(driver, vm);
 
     /* Read the memory file into buffer. */
-    if (saferead (fd, buffer, size) == (ssize_t) -1) {
+    if (saferead(fd, buffer, size) == (ssize_t) -1) {
         virReportSystemError(errno,
                              _("failed to read temporary file "
                                "created with template %s"), tmp);
@@ -7527,8 +7521,8 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
     virStorageFileMetadata *meta = NULL;
     virDomainDiskDefPtr disk = NULL;
     struct stat sb;
-    int i;
     int format;
+    const char *actual;
 
     virCheckFlags(0, -1);
 
@@ -7550,22 +7544,15 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
     }
 
     /* Check the path belongs to this domain. */
-    for (i = 0 ; i < vm->def->ndisks ; i++) {
+    if (!(actual = virDomainDiskPathByName(vm->def, path))) {
-        if (vm->def->disks[i]->src != NULL &&
-            STREQ (vm->def->disks[i]->src, path)) {
-            disk = vm->def->disks[i];
-            break;
-        }
-    }
-
-    if (!disk) {
         qemuReportError(VIR_ERR_INVALID_ARG,
                         _("invalid path %s not assigned to domain"), path);
         goto cleanup;
     }
+    path = actual;
 
     /* The path is correct, now try to open it and get its size. */
-    fd = open (path, O_RDONLY);
+    fd = open(path, O_RDONLY);
     if (fd == -1) {
         virReportSystemError(errno,
                              _("failed to open path '%s'"), path);
@@ -7624,7 +7611,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
         /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
          * be 64 bits on all platforms.
          */
-        end = lseek (fd, 0, SEEK_END);
+        end = lseek(fd, 0, SEEK_END);
         if (end == (off_t)-1) {
             virReportSystemError(errno,
                                  _("failed to seek to end of %s"), path);
@@ -9831,23 +9818,26 @@ static const char *
 qemuDiskPathToAlias(virDomainObjPtr vm, const char *path) {
     int i;
     char *ret = NULL;
+    virDomainDiskDefPtr disk;
 
-    for (i = 0 ; i < vm->def->ndisks ; i++) {
+    i = virDomainDiskIndexByName(vm->def, path, true);
-        virDomainDiskDefPtr disk = vm->def->disks[i];
+    if (i < 0)
+        goto cleanup;
 
-        if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK &&
+    disk = vm->def->disks[i];
-            disk->type != VIR_DOMAIN_DISK_TYPE_FILE)
-            continue;
 
-        if (disk->src != NULL && STREQ(disk->src, path)) {
+    if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK &&
-            if (virAsprintf(&ret, "drive-%s", disk->info.alias) < 0) {
+        disk->type != VIR_DOMAIN_DISK_TYPE_FILE)
-                virReportOOMError();
+        goto cleanup;
-                return NULL;
+
-            }
+    if (disk->src) {
-            break;
+        if (virAsprintf(&ret, "drive-%s", disk->info.alias) < 0) {
+            virReportOOMError();
+            return NULL;
         }
     }
 
+cleanup:
     if (!ret) {
         qemuReportError(VIR_ERR_INVALID_ARG,
                         "%s", _("No device found for specified path"));

src/qemu/qemu_process.c

@@ -192,15 +192,10 @@ static virDomainDiskDefPtr
 qemuProcessFindDomainDiskByPath(virDomainObjPtr vm,
                                 const char *path)
 {
-    int i;
+    int i = virDomainDiskIndexByName(vm->def, path, true);
 
-    for (i = 0; i < vm->def->ndisks; i++) {
+    if (i >= 0)
-        virDomainDiskDefPtr disk;
+        return vm->def->disks[i];
-
-        disk = vm->def->disks[i];
-        if (disk->src != NULL && STREQ(disk->src, path))
-            return disk;
-    }
 
     qemuReportError(VIR_ERR_INTERNAL_ERROR,
                     _("no disk found with path %s"),

src/uml/uml_driver.c

@@ -2175,7 +2175,8 @@ umlDomainBlockPeek(virDomainPtr dom,
 {
     struct uml_driver *driver = dom->conn->privateData;
     virDomainObjPtr vm;
-    int fd = -1, ret = -1, i;
+    int fd = -1, ret = -1;
+    const char *actual;
 
     virCheckFlags(0, -1);
 
@@ -2196,41 +2197,34 @@ umlDomainBlockPeek(virDomainPtr dom,
     }
 
     /* Check the path belongs to this domain. */
-    for (i = 0 ; i < vm->def->ndisks ; i++) {
+    if (!(actual = virDomainDiskPathByName(vm->def, path))) {
-        if (vm->def->disks[i]->src != NULL &&
+        umlReportError(VIR_ERR_INVALID_ARG,
-            STREQ (vm->def->disks[i]->src, path)) {
+                       _("invalid path '%s'"), path);
-            ret = 0;
+        goto cleanup;
-            break;
+    }
-        }
+    path = actual;
+
+    /* The path is correct, now try to open it and get its size. */
+    fd = open(path, O_RDONLY);
+    if (fd == -1) {
+        virReportSystemError(errno,
+                             _("cannot open %s"), path);
+        goto cleanup;
     }
 
-    if (ret == 0) {
+    /* Seek and read. */
-        ret = -1;
+    /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
-        /* The path is correct, now try to open it and get its size. */
+     * be 64 bits on all platforms.
-        fd = open (path, O_RDONLY);
+     */
-        if (fd == -1) {
+    if (lseek(fd, offset, SEEK_SET) == (off_t) -1 ||
-            virReportSystemError(errno,
+        saferead(fd, buffer, size) == (ssize_t) -1) {
-                                 _("cannot open %s"), path);
+        virReportSystemError(errno,
-            goto cleanup;
+                             _("cannot read %s"), path);
-        }
+        goto cleanup;
-
-        /* Seek and read. */
-        /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
-         * be 64 bits on all platforms.
-         */
-        if (lseek (fd, offset, SEEK_SET) == (off_t) -1 ||
-            saferead (fd, buffer, size) == (ssize_t) -1) {
-            virReportSystemError(errno,
-                                 _("cannot read %s"), path);
-            goto cleanup;
-        }
-
-        ret = 0;
-    } else {
-        umlReportError(VIR_ERR_INVALID_ARG, "%s",
-                       _("invalid path"));
     }
 
+    ret = 0;
+
 cleanup:
     VIR_FORCE_CLOSE(fd);
     if (vm)

src/xen/xend_internal.c

@@ -3849,11 +3849,11 @@ xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path,
     xenUnifiedPrivatePtr priv;
     struct sexpr *root = NULL;
     int fd = -1, ret = -1;
-    int found = 0, i;
     virDomainDefPtr def;
     int id;
     char * tty;
     int vncport;
+    const char *actual;
 
     priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
 
@@ -3889,18 +3889,12 @@ xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path,
                              vncport)))
         goto cleanup;
 
-    for (i = 0 ; i < def->ndisks ; i++) {
+    if (!(actual = virDomainDiskPathByName(def, path))) {
-        if (def->disks[i]->src &&
-            STREQ(def->disks[i]->src, path)) {
-            found = 1;
-            break;
-        }
-    }
-    if (!found) {
         virXendError(VIR_ERR_INVALID_ARG,
                       _("%s: invalid path"), path);
         goto cleanup;
     }
+    path = actual;
 
     /* The path is correct, now try to open it and get its size. */
     fd = open (path, O_RDONLY);

tests/domainsnapshotxml2xmlin/disk_snapshot.xml

@@ -2,7 +2,7 @@
   <name>my snap name</name>
   <description>!@#$%^</description>
   <disks>
-    <disk name='hda'/>
+    <disk name='/dev/HostVG/QEMUGuest1'/>
     <disk name='hdb' snapshot='no'/>
     <disk name='hdc' snapshot='internal'/>
     <disk name='hdd' snapshot='external'>

tools/virsh.pod

@@ -503,7 +503,9 @@ snapshot metadata with B<snapshot-create>.
 
 =item B<domblkstat> I<domain> I<block-device>
 
-Get device block stats for a running domain.
+Get device block stats for a running domain.  A I<block-device> corresponds
+to a unique target name (<target dev='name'/>) or source file (<source
+file='name'/>) for one of the disk devices attached to I<domain>.
 
 =item B<domifstat> I<domain> I<interface-device>
 
@@ -515,7 +517,9 @@ Get memory stats for a running domain.
 
 =item B<domblkinfo> I<domain> I<block-device>
 
-Get block device size info for a domain.
+Get block device size info for a domain.  A I<block-device> corresponds
+to a unique target name (<target dev='name'/>) or source file (<source
+file='name'/>) for one of the disk devices attached to I<domain>.
 
 =item B<blockpull> I<domain> I<path> [I<bandwidth>]