blockcommit: require base below top

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@redhat.com>

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7ac7aa2d723a12f8d469bb4549c26d8b25222c6e..4ab5a7bbc4843820e79b113902920b0e7eae7be8 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15094,8 +15094,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 0792dd8f8a390d6b02802e54b4c478bc2bd40545..09b5d10e1ef3c5fb9fb70b0411f4a021a30b9462 100644 (file)
--- 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 7c64f18de73c0813284e72d53a54c20a55ee6038..e15578cbf0664cbbd50376669480f773884f66a2 100644 (file)
--- 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: