From f22b7899a87466d45589572a5da17b860da2d6af Mon Sep 17 00:00:00 2001 From: Jiri Denemark Date: Fri, 18 Apr 2014 14:35:33 +0200 Subject: [PATCH] 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 --- src/libvirt.c | 24 ++++++++++-- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 46 +++++++++++++++------- src/util/virstoragefile.c | 82 +++++++++++++++++++++++++++++++++------ src/util/virstoragefile.h | 7 ++++ tests/virstoragetest.c | 51 +++++++++++++++++++----- 6 files changed, 173 insertions(+), 38 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 5d54cb595b..b6c99c59c2 100644 --- a/src/libvirt.c +++ b/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 + * 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 + * 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 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 53dbdb4e8f..4cee2eb06d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1829,6 +1829,7 @@ virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; virStorageFileGetSCSIKey; virStorageFileIsClusterFS; +virStorageFileParseChainIndex; virStorageFileProbeFormat; virStorageFileProbeFormatFromBuf; virStorageFileResize; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 700310013c..67ba48734a 100644 --- a/src/qemu/qemu_driver.c +++ b/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 - * itself, and validating that base is on the chain, rather than - * relying on qemu to do this. */ - ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode, - async); + ret = qemuMonitorBlockJob(priv->mon, device, + baseIndex ? baseSource->path : 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, - const char *top, unsigned long bandwidth, +qemuDomainBlockCommit(virDomainPtr dom, + 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, - top, &top_parent))) { + else if (virStorageFileParseChainIndex(disk->dst, top, &topIndex) < 0 || + !(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, - base ? base : baseSource->path, bandwidth); + top && !topIndex ? top : topSource->path, + base && !baseIndex ? base : baseSource->path, + bandwidth); qemuDomainObjExitMonitor(driver, vm); endjob: diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7ceb8ff695..1ce0fa1bc5 100644 --- a/src/util/virstoragefile.c +++ b/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 - * return its canonical name. Pass NULL for NAME to find the base of - * the chain. If META is not NULL, set *META to the point in the - * chain that describes NAME (or to NULL if the backing element is not - * 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). - * Since the results point within CHAIN, they must not be - * independently freed. Reports an error and returns NULL if NAME is - * not found. */ +int +virStorageFileParseChainIndex(const char *diskTarget, + const char *name, + unsigned int *chainIndex) +{ + char **strings = NULL; + unsigned int idx = 0; + char *suffix; + 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); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 82198e5edb..148776ea8a 100644 --- a/src/util/virstoragefile.h +++ b/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); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index e730b8e4d7..018469a68a 100644 --- a/tests/virstoragetest.c +++ b/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) \ - do { \ - struct testLookupData data2 = { chain, name, \ - result, meta, parent, }; \ - if (virtTestRun("Chain lookup " #id, \ - testStorageLookup, &data2) < 0) \ - ret = -1; \ +#define TEST_LOOKUP_TARGET(id, target, name, index, result, meta, parent) \ + do { \ + struct testLookupData data2 = { chain, target, name, index, \ + result, meta, parent, }; \ + if (virtTestRun("Chain lookup " #id, \ + testStorageLookup, &data2) < 0) \ + 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);