On 04/22/2014 06:49 AM, Jiri Denemark wrote: ACK - it's nice to see the sort of cleanups we can do now that the structs aren't as complicated. Given the testsuite rework I did, I'm a bit more worried about pointer equality here, and we may need to use STREQ after all. But we can double-check that after finishing the rest of the cleanups in your series. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

virStorageFileChainLookup is able to give use virStorageSourcePtr which contains the pointer to its canonical path. Let's use a more general virStorageSourcePtr instead of just canonical path. Former base_canon maps to baseSource->path. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a557d75..35ab2f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15297,8 +15297,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, int idx; virDomainDiskDefPtr disk = NULL; virStorageSourcePtr topSource; + virStorageSourcePtr baseSource; const char *top_parent = NULL; - const char *base_canon = NULL; bool clean_access = false; virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); @@ -15353,16 +15353,12 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, } if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) - base_canon = topSource->backingStore->path; - else if (!(base_canon = virStorageFileChainLookup(topSource, - base, NULL, NULL))) + baseSource = topSource->backingStore; + else if (!(virStorageFileChainLookup(topSource, base, &baseSource, NULL))) goto endjob; - /* Note that this code exploits the fact that - * virStorageFileChainLookup guarantees a simple pointer - * comparison will work, rather than needing full-blown STREQ. */ if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && - base_canon != topSource->backingStore->path) { + baseSource != topSource->backingStore) { virReportError(VIR_ERR_INVALID_ARG, _("base '%s' is not immediately below '%s' in chain " "for '%s'"), @@ -15378,7 +15374,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, * operation succeeds, but doing that requires tracking the * operation in XML across libvirtd restarts. */ clean_access = true; - if (qemuDomainPrepareDiskChainElement(driver, vm, disk, base_canon, + if (qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource->path, VIR_DISK_CHAIN_READ_WRITE) < 0 || (top_parent && top_parent != disk->src.path && qemuDomainPrepareDiskChainElement(driver, vm, disk, @@ -15394,13 +15390,13 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockCommit(priv->mon, device, top ? top : topSource->path, - base ? base : base_canon, bandwidth); + base ? base : baseSource->path, bandwidth); qemuDomainObjExitMonitor(driver, vm); endjob: if (ret < 0 && clean_access) { /* Revert access to read-only, if possible. */ - qemuDomainPrepareDiskChainElement(driver, vm, disk, base_canon, + qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource->path, VIR_DISK_CHAIN_READ_ONLY); if (top_parent && top_parent != disk->src.path) qemuDomainPrepareDiskChainElement(driver, vm, disk, -- 1.9.2

Returning both virStorageSourcePtr and its path member does not make a lot of sense. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 7 +++---- src/util/virstoragefile.c | 16 +++++++--------- src/util/virstoragefile.h | 7 +++---- tests/virstoragetest.c | 39 +++++++++++++++++++++++---------------- 4 files changed, 36 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 35ab2f0..0283d99 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15340,9 +15340,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, if (!top) { topSource = &disk->src; - } else if (!(virStorageFileChainLookup(&disk->src, - top, &topSource, - &top_parent))) { + } else if (!(topSource = virStorageFileChainLookup(disk->src.backingStore, + top, &top_parent))) { goto endjob; } if (!topSource->backingStore) { @@ -15354,7 +15353,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) baseSource = topSource->backingStore; - else if (!(virStorageFileChainLookup(topSource, base, &baseSource, NULL))) + else if (!(baseSource = virStorageFileChainLookup(topSource, base, NULL))) goto endjob; if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b2d8f62..4fdbb8b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1510,9 +1510,9 @@ int virStorageFileGetSCSIKey(const char *path, * Since the results point within CHAIN, they must not be * independently freed. Reports an error and returns NULL if NAME is * not found. */ -const char * +virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr chain, - const char *name, virStorageSourcePtr *meta, + const char *name, const char **parent) { const char *start = chain->path; @@ -1545,24 +1545,22 @@ virStorageFileChainLookup(virStorageSourcePtr chain, parentDir = chain->relDir; chain = chain->backingStore; } + if (!chain) goto error; - if (meta) - *meta = chain; - return chain->path; + return chain; error: - if (name) + if (name) { virReportError(VIR_ERR_INVALID_ARG, _("could not find image '%s' in chain for '%s'"), name, start); - else + } else { virReportError(VIR_ERR_INVALID_ARG, _("could not find base image in chain for '%s'"), start); + } *parent = NULL; - if (meta) - *meta = NULL; return NULL; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index a0adb9b..82198e5 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -281,10 +281,9 @@ virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, int virStorageFileChainGetBroken(virStorageSourcePtr chain, char **broken_file); -const char *virStorageFileChainLookup(virStorageSourcePtr chain, - const char *name, - virStorageSourcePtr *meta, - const char **parent) +virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr chain, + const char *name, + const char **parent) ATTRIBUTE_NONNULL(1); int virStorageFileResize(const char *path, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 5320c78..edab03a 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -424,16 +424,11 @@ testStorageLookup(const void *args) { const struct testLookupData *data = args; int ret = 0; - const char *actualResult; - virStorageSourcePtr actualMeta; + virStorageSourcePtr result; const char *actualParent; - /* This function is documented as giving results within chain, but - * as the same string may be duplicated into more than one field, - * we rely on STREQ rather than pointer equality. Test twice to - * ensure optional parameters don't cause NULL deref. */ - actualResult = virStorageFileChainLookup(data->chain, data->name, - NULL, NULL); + /* Test twice to ensure optional parameter doesn't cause NULL deref. */ + result = virStorageFileChainLookup(data->chain, data->name, NULL); if (!data->expResult) { if (!virGetLastError()) { @@ -448,24 +443,36 @@ testStorageLookup(const void *args) } } - if (STRNEQ_NULLABLE(data->expResult, actualResult)) { + if (!result) { + if (data->expResult) { + fprintf(stderr, "result 1: expected %s, got NULL\n", + data->expResult); + ret = -1; + } + } else if (STRNEQ_NULLABLE(data->expResult, result->path)) { fprintf(stderr, "result 1: expected %s, got %s\n", - NULLSTR(data->expResult), NULLSTR(actualResult)); + NULLSTR(data->expResult), NULLSTR(result->path)); ret = -1; } - actualResult = virStorageFileChainLookup(data->chain, data->name, - &actualMeta, &actualParent); + result = virStorageFileChainLookup(data->chain, data->name, &actualParent); if (!data->expResult) virResetLastError(); - if (STRNEQ_NULLABLE(data->expResult, actualResult)) { + + if (!result) { + if (data->expResult) { + fprintf(stderr, "result 2: expected %s, got NULL\n", + data->expResult); + ret = -1; + } + } else if (STRNEQ_NULLABLE(data->expResult, result->path)) { fprintf(stderr, "result 2: expected %s, got %s\n", - NULLSTR(data->expResult), NULLSTR(actualResult)); + NULLSTR(data->expResult), NULLSTR(result->path)); ret = -1; } - if (data->expMeta != actualMeta) { + if (data->expMeta != result) { fprintf(stderr, "meta: expected %p, got %p\n", - data->expMeta, actualMeta); + data->expMeta, result); ret = -1; } if (STRNEQ_NULLABLE(data->expParent, actualParent)) { -- 1.9.2

Each backing store of a given disk is associated with a unique index (which is also formated 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(a)redhat.com&gt; --- src/libvirt.c | 13 ++++++-- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 29 ++++++++++++----- src/util/virstoragefile.c | 83 ++++++++++++++++++++++++++++++++++++++++------- src/util/virstoragefile.h | 7 ++++ tests/virstoragetest.c | 51 ++++++++++++++++++++++++----- 6 files changed, 152 insertions(+), 32 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 4454829..a9f56c3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19787,9 +19787,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 * @@ -19839,6 +19840,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 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fe8a810..b77050d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1830,6 +1830,7 @@ virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; virStorageFileGetSCSIKey; virStorageFileIsClusterFS; +virStorageFileParseChainIndex; virStorageFileProbeFormat; virStorageFileProbeFormatFromBuf; virStorageFileResize; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0283d99..4ed1123 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15285,8 +15285,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; @@ -15297,7 +15300,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; @@ -15338,12 +15343,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"), @@ -15353,7 +15361,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) && @@ -15388,8 +15398,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 4fdbb8b..f4387e0 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1501,33 +1501,87 @@ 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 *index) +{ + char **strings = NULL; + unsigned int idx = 0; + char *suffix; + int ret = 0; + + *index = 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; + } + + *index = idx; + + cleanup: + virStringFreeList(strings); + return ret; +} + +/* Given a CHAIN, look for the backing file NAME within the chain starting + * from @startFrom or @chain if @startFrom is NULL and return its canonical + * name. @chain must always point to the top of the chain. Pass NULL for + * NAME and 0 for INDEX to find the base of the chain. Pass nonzero INDEX + * to find the backing source according to its position in the backing 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, + * independently freed. Reports an error and returns NULL if NAME is not + * found. */ virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr chain, + virStorageSourcePtr startFrom, const char *name, + unsigned int index, 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 && !index) { if (!chain->backingStore) break; + } else if (index) { + VIR_DEBUG("%zu: %s", i, chain->path); + if (index == i) + break; } else { if (STREQ(name, chain->relPath)) break; @@ -1544,6 +1598,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, *parent = chain->path; parentDir = chain->relDir; chain = chain->backingStore; + i++; } if (!chain) @@ -1551,7 +1606,11 @@ virStorageFileChainLookup(virStorageSourcePtr chain, return chain; error: - if (name) { + if (index) { + virReportError(VIR_ERR_INVALID_ARG, + _("could not find backing store %u in chain for '%s'"), + index, 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 82198e5..043896d 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 *index) + ATTRIBUTE_NONNULL(3); + virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr chain, + virStorageSourcePtr startFrom, const char *name, + unsigned int index, const char **parent) ATTRIBUTE_NONNULL(1); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index edab03a..7802577 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 index; + + if (virStorageFileParseChainIndex(data->target, data->name, &index) < 0 && + data->expIndex) { + fprintf(stderr, "call should not have failed\n"); + ret = -1; + } + if (index != data->expIndex) { + fprintf(stderr, "index: expected %u, got %u\n", data->expIndex, index); + 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, + index ? NULL : data->name, + index, 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, index, &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); -- 1.9.2

On Thu, Apr 24, 2014 at 13:43:48 -0600, Eric Blake wrote: I think we are OK with this notation :-) ... No, NULL is explicitly allowed for both diskTarget and name to make the function simpler to call. And the test suite already checks that passing NULL for either of the two parameters works and returns the expected result. I fixed the thing you pointed out and also got rid of "index" to avoid breaking the build again :-) and pushed the series. Thanks. Jirka