Add support for addressing backing stores by index

Each backing store of a given disk is associated with a unique index
(which is also formatted in domain XML) for easier addressing of any
particular backing store. With this patch, any backing store can be
addressed by its disk target and the index. For example, "vdc[4]"
addresses the backing store with index equal to 4 of the disk identified
by "vdc" target. Such shorthand can be used in any API in place for a
backing file path:

    virsh blockcommit domain vda --base vda[3] --top vda[2]

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

src/libvirt.c

@@ -19669,7 +19669,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk,
  * virDomainBlockRebase:
  * @dom: pointer to domain object
  * @disk: path to the block device, or device shorthand
- * @base: path to backing file to keep, or NULL for no backing file
+ * @base: path to backing file to keep, or device shorthand,
+ *        or NULL for no backing file
  * @bandwidth: (optional) specify copy bandwidth limit in MiB/s
  * @flags: bitwise-OR of virDomainBlockRebaseFlags
  *
@@ -19731,6 +19732,14 @@ virDomainBlockPull(virDomainPtr dom, const char *disk,
  * can be found by calling virDomainGetXMLDesc() and inspecting
  * elements within //domain/devices/disk.
  *
+ * The @base parameter can be either a path to a file within the backing
+ * chain, or the device target shorthand (the <target dev='...'/>
+ * sub-element, such as "vda") followed by an index to the backing chain
+ * enclosed in square brackets. Backing chain indexes can be found by
+ * inspecting //disk//backingStore/@index in the domain XML. Thus, for
+ * example, "vda[3]" refers to the backing store with index equal to "3"
+ * in the chain of disk "vda".
+ *
  * The maximum bandwidth (in MiB/s) that will be used to do the copy can be
  * specified with the bandwidth parameter.  If set to 0, libvirt will choose a
  * suitable default.  Some hypervisors do not support this feature and will
@@ -19793,9 +19802,10 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk,
  * virDomainBlockCommit:
  * @dom: pointer to domain object
  * @disk: path to the block device, or device shorthand
- * @base: path to backing file to merge into, or NULL for default
+ * @base: path to backing file to merge into, or device shorthand,
+ *        or NULL for default
  * @top: path to file within backing chain that contains data to be merged,
- *       or NULL to merge all possible data
+ *       or device shorthand, or NULL to merge all possible data
  * @bandwidth: (optional) specify commit bandwidth limit in MiB/s
  * @flags: bitwise-OR of virDomainBlockCommitFlags
  *
@@ -19845,6 +19855,14 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk,
  * can be found by calling virDomainGetXMLDesc() and inspecting
  * elements within //domain/devices/disk.
  *
+ * The @base and @top parameters can be either paths to files within the
+ * backing chain, or the device target shorthand (the <target dev='...'/>
+ * sub-element, such as "vda") followed by an index to the backing chain
+ * enclosed in square brackets. Backing chain indexes can be found by
+ * inspecting //disk//backingStore/@index in the domain XML. Thus, for
+ * example, "vda[3]" refers to the backing store with index equal to "3"
+ * in the chain of disk "vda".
+ *
  * The maximum bandwidth (in MiB/s) that will be used to do the commit can be
  * specified with the bandwidth parameter.  If set to 0, libvirt will choose a
  * suitable default.  Some hypervisors do not support this feature and will

src/libvirt_private.syms

@@ -1829,6 +1829,7 @@ virStorageFileGetMetadataFromBuf;
 virStorageFileGetMetadataFromFD;
 virStorageFileGetSCSIKey;
 virStorageFileIsClusterFS;
+virStorageFileParseChainIndex;
 virStorageFileProbeFormat;
 virStorageFileProbeFormatFromBuf;
 virStorageFileResize;

src/qemu/qemu_driver.c

@@ -14854,6 +14854,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
     virObjectEventPtr event = NULL;
     int idx;
     virDomainDiskDefPtr disk;
+    virStorageSourcePtr baseSource = NULL;
+    unsigned int baseIndex = 0;
 
     if (!virDomainObjIsActive(vm)) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -14915,12 +14917,17 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
         goto endjob;
     }
 
+    if (base &&
+        (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 ||
+         !(baseSource = virStorageFileChainLookup(&disk->src,
+                                                  disk->src.backingStore,
+                                                  base, baseIndex, NULL))))
+        goto endjob;
+
     qemuDomainObjEnterMonitor(driver, vm);
-    /* XXX - libvirt should really be tracking the backing file chain
+    ret = qemuMonitorBlockJob(priv->mon, device,
-     * itself, and validating that base is on the chain, rather than
+                              baseIndex ? baseSource->path : base,
-     * relying on qemu to do this.  */
+                              bandwidth, info, mode, async);
-    ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode,
-                              async);
     qemuDomainObjExitMonitor(driver, vm);
     if (ret < 0)
         goto endjob;
@@ -15274,8 +15281,11 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth,
 
 
 static int
-qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
+qemuDomainBlockCommit(virDomainPtr dom,
-                      const char *top, unsigned long bandwidth,
+                      const char *path,
+                      const char *base,
+                      const char *top,
+                      unsigned long bandwidth,
                       unsigned int flags)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
@@ -15286,7 +15296,9 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
     int idx;
     virDomainDiskDefPtr disk = NULL;
     virStorageSourcePtr topSource;
+    unsigned int topIndex = 0;
     virStorageSourcePtr baseSource;
+    unsigned int baseIndex = 0;
     const char *top_parent = NULL;
     bool clean_access = false;
 
@@ -15327,12 +15339,15 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
     if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
         goto endjob;
 
-    if (!top) {
+    if (!top)
         topSource = &disk->src;
-    } else if (!(topSource = virStorageFileChainLookup(disk->src.backingStore,
+    else if (virStorageFileParseChainIndex(disk->dst, top, &topIndex) < 0 ||
-                                                       top, &top_parent))) {
+             !(topSource = virStorageFileChainLookup(&disk->src,
+                                                     disk->src.backingStore,
+                                                     top, topIndex,
+                                                     &top_parent)))
         goto endjob;
-    }
+
     if (!topSource->backingStore) {
         virReportError(VIR_ERR_INVALID_ARG,
                        _("top '%s' in chain for '%s' has no backing file"),
@@ -15342,7 +15357,9 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
 
     if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW))
         baseSource = topSource->backingStore;
-    else if (!(baseSource = virStorageFileChainLookup(topSource, base, NULL)))
+    else if (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 ||
+             !(baseSource = virStorageFileChainLookup(&disk->src, topSource,
+                                                      base, baseIndex, NULL)))
         goto endjob;
 
     if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) &&
@@ -15377,8 +15394,9 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
      * thing if the user specified a relative name). */
     qemuDomainObjEnterMonitor(driver, vm);
     ret = qemuMonitorBlockCommit(priv->mon, device,
-                                 top ? top : topSource->path,
+                                 top && !topIndex ? top : topSource->path,
-                                 base ? base : baseSource->path, bandwidth);
+                                 base && !baseIndex ? base : baseSource->path,
+                                 bandwidth);
     qemuDomainObjExitMonitor(driver, vm);
 
  endjob:

src/util/virstoragefile.c

@@ -1507,33 +1507,86 @@ int virStorageFileGetSCSIKey(const char *path,
 }
 #endif
 
-/* Given a CHAIN, look for the backing file NAME within the chain and
+int
- * return its canonical name.  Pass NULL for NAME to find the base of
+virStorageFileParseChainIndex(const char *diskTarget,
- * the chain.  If META is not NULL, set *META to the point in the
+                              const char *name,
- * chain that describes NAME (or to NULL if the backing element is not
+                              unsigned int *chainIndex)
- * a file).  If PARENT is not NULL, set *PARENT to the preferred name
+{
- * of the parent (or to NULL if NAME matches the start of the chain).
+    char **strings = NULL;
- * Since the results point within CHAIN, they must not be
+    unsigned int idx = 0;
- * independently freed.  Reports an error and returns NULL if NAME is
+    char *suffix;
- * not found.  */
+    int ret = 0;
+
+    *chainIndex = 0;
+
+    if (name && diskTarget)
+        strings = virStringSplit(name, "[", 2);
+
+    if (virStringListLength(strings) != 2)
+        goto cleanup;
+
+    if (virStrToLong_ui(strings[1], &suffix, 10, &idx) < 0 ||
+        STRNEQ(suffix, "]"))
+        goto cleanup;
+
+    if (STRNEQ(diskTarget, strings[0])) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("requested target '%s' does not match target '%s'"),
+                       strings[0], diskTarget);
+        ret = -1;
+        goto cleanup;
+    }
+
+    *chainIndex = idx;
+
+ cleanup:
+    virStringFreeList(strings);
+    return ret;
+}
+
+/* Given a @chain, look for the backing store @name within the chain starting
+ * from @startFrom or @chain if @startFrom is NULL and return that location
+ * within the chain.  @chain must always point to the top of the chain.  Pass
+ * NULL for @name and 0 for @idx to find the base of the chain.  Pass nonzero
+ * @idx to find the backing source according to its position in the backing
+ * chain.  If @parent is not NULL, set *@parent to the preferred name of the
+ * parent (or to NULL if @name matches the start of the chain).  Since the
+ * results point within @chain, they must not be independently freed.
+ * Reports an error and returns NULL if @name is not found.
+ */
 virStorageSourcePtr
 virStorageFileChainLookup(virStorageSourcePtr chain,
+                          virStorageSourcePtr startFrom,
                           const char *name,
+                          unsigned int idx,
                           const char **parent)
 {
     const char *start = chain->path;
     const char *tmp;
     const char *parentDir = ".";
     bool nameIsFile = virStorageIsFile(name);
+    size_t i;
 
     if (!parent)
         parent = &tmp;
-
     *parent = NULL;
+
+    i = 0;
+    if (startFrom) {
+        while (chain && chain != startFrom) {
+            chain = chain->backingStore;
+            i++;
+        }
+    }
+
     while (chain) {
-        if (!name) {
+        if (!name && !idx) {
             if (!chain->backingStore)
                 break;
+        } else if (idx) {
+            VIR_DEBUG("%zu: %s", i, chain->path);
+            if (idx == i)
+                break;
         } else {
             if (STREQ(name, chain->relPath))
                 break;
@@ -1550,6 +1603,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
         *parent = chain->path;
         parentDir = chain->relDir;
         chain = chain->backingStore;
+        i++;
     }
 
     if (!chain)
@@ -1557,7 +1611,11 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
     return chain;
 
  error:
-    if (name) {
+    if (idx) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("could not find backing store %u in chain for '%s'"),
+                       idx, start);
+    } else if (name) {
         virReportError(VIR_ERR_INVALID_ARG,
                        _("could not find image '%s' in chain for '%s'"),
                        name, start);

src/util/virstoragefile.h

@@ -281,8 +281,15 @@ virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path,
 int virStorageFileChainGetBroken(virStorageSourcePtr chain,
                                  char **broken_file);
 
+int virStorageFileParseChainIndex(const char *diskTarget,
+                                  const char *name,
+                                  unsigned int *chainIndex)
+    ATTRIBUTE_NONNULL(3);
+
 virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr chain,
+                                              virStorageSourcePtr startFrom,
                                               const char *name,
+                                              unsigned int idx,
                                               const char **parent)
     ATTRIBUTE_NONNULL(1);
 

tests/virstoragetest.c

@@ -413,7 +413,9 @@ testStorageChain(const void *args)
 struct testLookupData
 {
     virStorageSourcePtr chain;
+    const char *target;
     const char *name;
+    unsigned int expIndex;
     const char *expResult;
     virStorageSourcePtr expMeta;
     const char *expParent;
@@ -426,9 +428,22 @@ testStorageLookup(const void *args)
     int ret = 0;
     virStorageSourcePtr result;
     const char *actualParent;
+    unsigned int idx;
+
+    if (virStorageFileParseChainIndex(data->target, data->name, &idx) < 0 &&
+        data->expIndex) {
+        fprintf(stderr, "call should not have failed\n");
+        ret = -1;
+    }
+    if (idx != data->expIndex) {
+        fprintf(stderr, "index: expected %u, got %u\n", data->expIndex, idx);
+        ret = -1;
+    }
 
      /* Test twice to ensure optional parameter doesn't cause NULL deref. */
-    result = virStorageFileChainLookup(data->chain, data->name, NULL);
+    result = virStorageFileChainLookup(data->chain, NULL,
+                                       idx ? NULL : data->name,
+                                       idx, NULL);
 
     if (!data->expResult) {
         if (!virGetLastError()) {
@@ -455,7 +470,8 @@ testStorageLookup(const void *args)
         ret = -1;
     }
 
-    result = virStorageFileChainLookup(data->chain, data->name, &actualParent);
+    result = virStorageFileChainLookup(data->chain, data->chain,
+                                       data->name, idx, &actualParent);
     if (!data->expResult)
         virResetLastError();
 
@@ -878,14 +894,16 @@ mymain(void)
         goto cleanup;
     }
 
-#define TEST_LOOKUP(id, name, result, meta, parent)                  \
+#define TEST_LOOKUP_TARGET(id, target, name, index, result, meta, parent)   \
-    do {                                                             \
+    do {                                                                    \
-        struct testLookupData data2 = { chain, name,                 \
+        struct testLookupData data2 = { chain, target, name, index,         \
-                                        result, meta, parent, };     \
+                                        result, meta, parent, };            \
-        if (virtTestRun("Chain lookup " #id,                         \
+        if (virtTestRun("Chain lookup " #id,                                \
-                        testStorageLookup, &data2) < 0)              \
+                        testStorageLookup, &data2) < 0)                     \
-            ret = -1;                                                \
+            ret = -1;                                                       \
     } while (0)
+#define TEST_LOOKUP(id, name, result, meta, parent)                         \
+    TEST_LOOKUP_TARGET(id, NULL, name, 0, result, meta, parent)
 
     TEST_LOOKUP(0, "bogus", NULL, NULL, NULL);
     TEST_LOOKUP(1, "wrap", chain->path, chain, NULL);
@@ -969,6 +987,21 @@ mymain(void)
     TEST_LOOKUP(25, NULL, chain->backingStore->backingStore->path,
                 chain->backingStore->backingStore, chain->backingStore->path);
 
+    TEST_LOOKUP_TARGET(26, "vda", "bogus[1]", 0, NULL, NULL, NULL);
+    TEST_LOOKUP_TARGET(27, "vda", "vda[-1]", 0, NULL, NULL, NULL);
+    TEST_LOOKUP_TARGET(28, "vda", "vda[1][1]", 0, NULL, NULL, NULL);
+    TEST_LOOKUP_TARGET(29, "vda", "wrap", 0, chain->path, chain, NULL);
+    TEST_LOOKUP_TARGET(30, "vda", "vda[0]", 0, NULL, NULL, NULL);
+    TEST_LOOKUP_TARGET(31, "vda", "vda[1]", 1,
+                       chain->backingStore->path,
+                       chain->backingStore,
+                       chain->path);
+    TEST_LOOKUP_TARGET(32, "vda", "vda[2]", 2,
+                       chain->backingStore->backingStore->path,
+                       chain->backingStore->backingStore,
+                       chain->backingStore->path);
+    TEST_LOOKUP_TARGET(33, "vda", "vda[3]", 3, NULL, NULL, NULL);
+
  cleanup:
     /* Final cleanup */
     virStorageSourceFree(chain);