11:31 a.m.
The block commit code looks for an explicit base file relative
to the discovered top file; so for a chain of:
base <- snap1 <- snap2 <- snap3
and a command of:
virsh blockcommit $dom vda --base snap2 --top snap1
we got a sane message (here from libvirt 1.0.5):
error: invalid argument: could not find base 'snap2' below 'snap1' in
chain for 'vda'
Meanwhile, recent refactoring has slightly reduced the quality of the
libvirt error messages, by losing the phrase 'below xyz':
error: invalid argument: could not find image 'snap2' in chain for
'snap3'
But we had a one-off, where we were not excluding the top file
itself in searching for the base; thankfully qemu still reports
the error, but the quality is worse:
virsh blockcommit $dom vda --base snap2 --top snap2
error: internal error unable to execute QEMU command 'block-commit': Base
'/snap2' not found
Fix the one-off in blockcommit by changing the semantics of name
lookup - if a starting point is specified, then the result must
be below that point, rather than including that point. The only
other call to chain lookup was blockpull code, which was already
forcing the lookup to omit the active layer and only needs a
tweak to use the new semantics.
This also fixes the bug exposed in the testsuite, where when doing
a lookup pinned to an intermediate point in the chain, we were
unable to return the name of the parent also in the chain.
* src/util/virstoragefile.c (virStorageFileChainLookup): Change
semantics for non-NULL startFrom.
* src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Adjust caller,
to keep existing semantics.
* tests/virstoragetest.c (mymain): Adjust to expose new semantics.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 3 +--
src/util/virstoragefile.c | 33 ++++++++++++++++++++-------------
tests/virstoragetest.c | 36 +++++++++++++++++-------------------
3 files changed, 38 insertions(+), 34 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7bf2020..b4bf561 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15096,8 +15096,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
if (base &&
(virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 ||
- !(baseSource = virStorageFileChainLookup(disk->src,
- disk->src->backingStore,
+ !(baseSource = virStorageFileChainLookup(disk->src, disk->src,
base, baseIndex, NULL))))
goto endjob;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 0792dd8..09b5d10 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1331,15 +1331,16 @@ virStorageFileParseChainIndex(const char *diskTarget,
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.
+/* Given a @chain, look for the backing store @name that is a backing file
+ * of @startFrom (or any member of @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,
@@ -1360,10 +1361,11 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
i = 0;
if (startFrom) {
- while (chain && chain != startFrom) {
+ while (chain && chain != startFrom->backingStore) {
chain = chain->backingStore;
i++;
}
+ *parent = startFrom->path;
}
while (chain) {
@@ -1403,9 +1405,14 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
_("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);
+ if (startFrom)
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("could not find image '%s' beneath '%s'
in "
+ "chain for '%s'"), name,
startFrom->path, start);
+ else
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("could not find image '%s' in chain for
'%s'"),
+ name, start);
} else {
virReportError(VIR_ERR_INVALID_ARG,
_("could not find base image in chain for
'%s'"),
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 7c64f18..e15578c 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -942,31 +942,31 @@ mymain(void)
TEST_LOOKUP(0, NULL, "bogus", NULL, NULL, NULL);
TEST_LOOKUP(1, chain, "bogus", NULL, NULL, NULL);
TEST_LOOKUP(2, NULL, "wrap", chain->path, chain, NULL);
- TEST_LOOKUP(3, chain, "wrap", chain->path, chain, NULL);
+ TEST_LOOKUP(3, chain, "wrap", NULL, NULL, NULL);
TEST_LOOKUP(4, chain2, "wrap", NULL, NULL, NULL);
TEST_LOOKUP(5, NULL, abswrap, chain->path, chain, NULL);
- TEST_LOOKUP(6, chain, abswrap, chain->path, chain, NULL);
+ TEST_LOOKUP(6, chain, abswrap, NULL, NULL, NULL);
TEST_LOOKUP(7, chain2, abswrap, NULL, NULL, NULL);
TEST_LOOKUP(8, NULL, "qcow2", chain2->path, chain2, chain->path);
TEST_LOOKUP(9, chain, "qcow2", chain2->path, chain2, chain->path);
- TEST_LOOKUP(10, chain2, "qcow2", chain2->path, chain2, NULL);
+ TEST_LOOKUP(10, chain2, "qcow2", NULL, NULL, NULL);
TEST_LOOKUP(11, chain3, "qcow2", NULL, NULL, NULL);
TEST_LOOKUP(12, NULL, absqcow2, chain2->path, chain2, chain->path);
TEST_LOOKUP(13, chain, absqcow2, chain2->path, chain2, chain->path);
- TEST_LOOKUP(14, chain2, absqcow2, chain2->path, chain2, NULL);
+ TEST_LOOKUP(14, chain2, absqcow2, NULL, NULL, NULL);
TEST_LOOKUP(15, chain3, absqcow2, NULL, NULL, NULL);
TEST_LOOKUP(16, NULL, "raw", chain3->path, chain3, chain2->path);
TEST_LOOKUP(17, chain, "raw", chain3->path, chain3, chain2->path);
TEST_LOOKUP(18, chain2, "raw", chain3->path, chain3, chain2->path);
- TEST_LOOKUP(19, chain3, "raw", chain3->path, chain3, NULL);
+ TEST_LOOKUP(19, chain3, "raw", NULL, NULL, NULL);
TEST_LOOKUP(20, NULL, absraw, chain3->path, chain3, chain2->path);
TEST_LOOKUP(21, chain, absraw, chain3->path, chain3, chain2->path);
TEST_LOOKUP(22, chain2, absraw, chain3->path, chain3, chain2->path);
- TEST_LOOKUP(23, chain3, absraw, chain3->path, chain3, NULL);
+ TEST_LOOKUP(23, chain3, absraw, NULL, NULL, NULL);
TEST_LOOKUP(24, NULL, NULL, chain3->path, chain3, chain2->path);
TEST_LOOKUP(25, chain, NULL, chain3->path, chain3, chain2->path);
TEST_LOOKUP(26, chain2, NULL, chain3->path, chain3, chain2->path);
- TEST_LOOKUP(27, chain3, NULL, chain3->path, chain3, NULL);
+ TEST_LOOKUP(27, chain3, NULL, NULL, NULL, NULL);
/* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */
virCommandFree(cmd);
@@ -995,31 +995,31 @@ mymain(void)
TEST_LOOKUP(28, NULL, "bogus", NULL, NULL, NULL);
TEST_LOOKUP(29, chain, "bogus", NULL, NULL, NULL);
TEST_LOOKUP(30, NULL, "wrap", chain->path, chain, NULL);
- TEST_LOOKUP(31, chain, "wrap", chain->path, chain, NULL);
+ TEST_LOOKUP(31, chain, "wrap", NULL, NULL, NULL);
TEST_LOOKUP(32, chain2, "wrap", NULL, NULL, NULL);
TEST_LOOKUP(33, NULL, abswrap, chain->path, chain, NULL);
- TEST_LOOKUP(34, chain, abswrap, chain->path, chain, NULL);
+ TEST_LOOKUP(34, chain, abswrap, NULL, NULL, NULL);
TEST_LOOKUP(35, chain2, abswrap, NULL, NULL, NULL);
TEST_LOOKUP(36, NULL, "qcow2", chain2->path, chain2, chain->path);
TEST_LOOKUP(37, chain, "qcow2", chain2->path, chain2, chain->path);
- TEST_LOOKUP(38, chain2, "qcow2", chain2->path, chain2, NULL);
+ TEST_LOOKUP(38, chain2, "qcow2", NULL, NULL, NULL);
TEST_LOOKUP(39, chain3, "qcow2", NULL, NULL, NULL);
TEST_LOOKUP(40, NULL, absqcow2, chain2->path, chain2, chain->path);
TEST_LOOKUP(41, chain, absqcow2, chain2->path, chain2, chain->path);
- TEST_LOOKUP(42, chain2, absqcow2, chain2->path, chain2, NULL);
+ TEST_LOOKUP(42, chain2, absqcow2, NULL, NULL, NULL);
TEST_LOOKUP(43, chain3, absqcow2, NULL, NULL, NULL);
TEST_LOOKUP(44, NULL, "raw", chain3->path, chain3, chain2->path);
TEST_LOOKUP(45, chain, "raw", chain3->path, chain3, chain2->path);
TEST_LOOKUP(46, chain2, "raw", chain3->path, chain3, chain2->path);
- TEST_LOOKUP(47, chain3, "raw", chain3->path, chain3, NULL);
+ TEST_LOOKUP(47, chain3, "raw", NULL, NULL, NULL);
TEST_LOOKUP(48, NULL, absraw, chain3->path, chain3, chain2->path);
TEST_LOOKUP(49, chain, absraw, chain3->path, chain3, chain2->path);
TEST_LOOKUP(50, chain2, absraw, chain3->path, chain3, chain2->path);
- TEST_LOOKUP(51, chain3, absraw, chain3->path, chain3, NULL);
+ TEST_LOOKUP(51, chain3, absraw, NULL, NULL, NULL);
TEST_LOOKUP(52, NULL, NULL, chain3->path, chain3, chain2->path);
TEST_LOOKUP(53, chain, NULL, chain3->path, chain3, chain2->path);
TEST_LOOKUP(54, chain2, NULL, chain3->path, chain3, chain2->path);
- TEST_LOOKUP(55, chain3, NULL, chain3->path, chain3, NULL);
+ TEST_LOOKUP(55, chain3, NULL, NULL, NULL, NULL);
/* Use link to wrap with cross-directory relative backing */
virCommandFree(cmd);
@@ -1054,15 +1054,14 @@ mymain(void)
TEST_LOOKUP_TARGET(67, "vda", NULL, "vda[-1]", 0, NULL, NULL,
NULL);
TEST_LOOKUP_TARGET(68, "vda", NULL, "vda[1][1]", 0, NULL, NULL,
NULL);
TEST_LOOKUP_TARGET(69, "vda", NULL, "wrap", 0, chain->path,
chain, NULL);
- TEST_LOOKUP_TARGET(70, "vda", chain, "wrap", 0, chain->path,
chain, NULL);
+ TEST_LOOKUP_TARGET(70, "vda", chain, "wrap", 0, NULL, NULL,
NULL);
TEST_LOOKUP_TARGET(71, "vda", chain2, "wrap", 0, NULL, NULL,
NULL);
TEST_LOOKUP_TARGET(72, "vda", NULL, "vda[0]", 0, NULL, NULL,
NULL);
TEST_LOOKUP_TARGET(73, "vda", NULL, "vda[1]", 1, chain2->path,
chain2,
chain->path);
TEST_LOOKUP_TARGET(74, "vda", chain, "vda[1]", 1,
chain2->path, chain2,
chain->path);
- TEST_LOOKUP_TARGET(75, "vda", chain2, "vda[1]", 1,
chain2->path, chain2,
- NULL);
+ TEST_LOOKUP_TARGET(75, "vda", chain2, "vda[1]", 1, NULL, NULL,
NULL);
TEST_LOOKUP_TARGET(76, "vda", chain3, "vda[1]", 1, NULL, NULL,
NULL);
TEST_LOOKUP_TARGET(77, "vda", NULL, "vda[2]", 2, chain3->path,
chain3,
chain2->path);
@@ -1070,8 +1069,7 @@ mymain(void)
chain2->path);
TEST_LOOKUP_TARGET(79, "vda", chain2, "vda[2]", 2,
chain3->path, chain3,
chain2->path);
- TEST_LOOKUP_TARGET(80, "vda", chain3, "vda[2]", 2,
chain3->path, chain3,
- NULL);
+ TEST_LOOKUP_TARGET(80, "vda", chain3, "vda[2]", 2, NULL, NULL,
NULL);
TEST_LOOKUP_TARGET(81, "vda", NULL, "vda[3]", 3, NULL, NULL,
NULL);
cleanup:
--
1.9.3