[libvirt] [PATCH 0/4] catch bogus 'blockcommit --base=a --top=a'
This patch series was first hinted at here: https://www.redhat.com/archives/libvir-list/2014-June/msg00589.html It fixes a minor bug that has existed since blockcommit was first added, where we had an off-by-one that caused us to generate different error messages in some cases where a user was passing an impossible request to blockcommit (basically, we tried to filter bogus cases up front to give a nice message, but missed one case that resulted in reporting a different message from qemu instead). I apologize in advance for the conflicts this creates with Peter's pending series, but think that the testsuite is both more legible and more thorough with the changes added in this series. It's split into several patches to make review easier. And can I just add that it is annoying dealing with gcc's: virstoragetest.c:1076:1: error: the frame size of 6608 bytes is larger than 4096 bytes [-Werror=frame-larger-than=] (although I agree with the need for keeping that warning) Eric Blake (4): storage: add alias for less typing storage: renumber lookup tests storage: better tests of lookup blockcommit: require base below top src/qemu/qemu_driver.c | 3 +- src/util/virstoragefile.c | 33 +++++---- tests/virstoragetest.c | 184 ++++++++++++++++++++++++++++------------------ 3 files changed, 135 insertions(+), 85 deletions(-) -- 1.9.3
Typing chain->backingStore->backingStore gets old after a while; introduce some alias variables to make the test more compact. * tests/virstoragetest.c (mymain): Introduce some shorthand. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 63 +++++++++++++++++++++----------------------------- 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index cdd4794..cffb961 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -531,6 +531,8 @@ mymain(void) virCommandPtr cmd = NULL; struct testChainData data; virStorageSourcePtr chain = NULL; + virStorageSourcePtr chain2; /* short for chain->backingStore */ + virStorageSourcePtr chain3; /* short for chain2->backingStore */ /* Prep some files with qemu-img; if that is not found on PATH, or * if it lacks support for qcow2 and qed, skip this test. */ @@ -919,6 +921,8 @@ mymain(void) ret = -1; goto cleanup; } + chain2 = chain->backingStore; + chain3 = chain2->backingStore; #define TEST_LOOKUP_TARGET(id, target, name, index, result, meta, parent) \ do { \ @@ -934,16 +938,11 @@ mymain(void) TEST_LOOKUP(0, "bogus", NULL, NULL, NULL); TEST_LOOKUP(1, "wrap", chain->path, chain, NULL); TEST_LOOKUP(2, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(3, "qcow2", chain->backingStore->path, chain->backingStore, - chain->path); - TEST_LOOKUP(4, absqcow2, chain->backingStore->path, chain->backingStore, - chain->path); - TEST_LOOKUP(5, "raw", chain->backingStore->backingStore->path, - chain->backingStore->backingStore, chain->backingStore->path); - TEST_LOOKUP(6, absraw, chain->backingStore->backingStore->path, - chain->backingStore->backingStore, chain->backingStore->path); - TEST_LOOKUP(7, NULL, chain->backingStore->backingStore->path, - chain->backingStore->backingStore, chain->backingStore->path); + TEST_LOOKUP(3, "qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(4, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(5, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(6, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(7, NULL, chain3->path, chain3, chain2->path); /* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */ virCommandFree(cmd); @@ -966,20 +965,17 @@ mymain(void) ret = -1; goto cleanup; } + chain2 = chain->backingStore; + chain3 = chain2->backingStore; TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); TEST_LOOKUP(9, "wrap", chain->path, chain, NULL); TEST_LOOKUP(10, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(11, "qcow2", chain->backingStore->path, chain->backingStore, - chain->path); - TEST_LOOKUP(12, absqcow2, chain->backingStore->path, chain->backingStore, - chain->path); - TEST_LOOKUP(13, "raw", chain->backingStore->backingStore->path, - chain->backingStore->backingStore, chain->backingStore->path); - TEST_LOOKUP(14, absraw, chain->backingStore->backingStore->path, - chain->backingStore->backingStore, chain->backingStore->path); - TEST_LOOKUP(15, NULL, chain->backingStore->backingStore->path, - chain->backingStore->backingStore, chain->backingStore->path); + TEST_LOOKUP(11, "qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(12, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(13, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(14, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(15, NULL, chain3->path, chain3, chain2->path); /* Use link to wrap with cross-directory relative backing */ virCommandFree(cmd); @@ -996,36 +992,29 @@ mymain(void) ret = -1; goto cleanup; } + chain2 = chain->backingStore; + chain3 = chain2->backingStore; TEST_LOOKUP(16, "bogus", NULL, NULL, NULL); TEST_LOOKUP(17, "sub/link2", chain->path, chain, NULL); TEST_LOOKUP(18, "wrap", chain->path, chain, NULL); TEST_LOOKUP(19, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(20, "../qcow2", chain->backingStore->path, chain->backingStore, - chain->path); + TEST_LOOKUP(20, "../qcow2", chain2->path, chain2, chain->path); TEST_LOOKUP(21, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(22, absqcow2, chain->backingStore->path, chain->backingStore, - chain->path); - TEST_LOOKUP(23, "raw", chain->backingStore->backingStore->path, - chain->backingStore->backingStore, chain->backingStore->path); - TEST_LOOKUP(24, absraw, chain->backingStore->backingStore->path, - chain->backingStore->backingStore, chain->backingStore->path); - TEST_LOOKUP(25, NULL, chain->backingStore->backingStore->path, - chain->backingStore->backingStore, chain->backingStore->path); + TEST_LOOKUP(22, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(23, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(24, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(25, NULL, chain3->path, chain3, chain2->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, + TEST_LOOKUP_TARGET(31, "vda", "vda[1]", 1, chain2->path, chain2, chain->path); - TEST_LOOKUP_TARGET(32, "vda", "vda[2]", 2, - chain->backingStore->backingStore->path, - chain->backingStore->backingStore, - chain->backingStore->path); + TEST_LOOKUP_TARGET(32, "vda", "vda[2]", 2, chain3->path, chain3, + chain2->path); TEST_LOOKUP_TARGET(33, "vda", "vda[3]", 3, NULL, NULL, NULL); cleanup: -- 1.9.3
On 06/13/14 18:31, Eric Blake wrote:
Typing chain->backingStore->backingStore gets old after a while; introduce some alias variables to make the test more compact.
* tests/virstoragetest.c (mymain): Introduce some shorthand.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 63 +++++++++++++++++++++----------------------------- 1 file changed, 26 insertions(+), 37 deletions(-)
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index cdd4794..cffb961 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c
ACK, Peter
The next patch will be adding tests; for ease of review of that patch, I want to create common context lines that don't change when the new tests are added (it's easier to visually review additions than it is to review an entire chunk of tests rewritten into another larger chunk of tests). * tests/virstoragetest.c (mymain): Add a parameter and renumber the lookup tests. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 79 +++++++++++++++++++++++++------------------------- 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index cffb961..abfeb36 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -924,7 +924,8 @@ mymain(void) chain2 = chain->backingStore; chain3 = chain2->backingStore; -#define TEST_LOOKUP_TARGET(id, target, name, index, result, meta, parent) \ +#define TEST_LOOKUP_TARGET(id, target, ignored, name, index, result, \ + meta, parent) \ do { \ struct testLookupData data2 = { chain, target, name, index, \ result, meta, parent, }; \ @@ -932,17 +933,17 @@ mymain(void) 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); - TEST_LOOKUP(2, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(3, "qcow2", chain2->path, chain2, chain->path); - TEST_LOOKUP(4, absqcow2, chain2->path, chain2, chain->path); - TEST_LOOKUP(5, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(6, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(7, NULL, chain3->path, chain3, chain2->path); +#define TEST_LOOKUP(id, from, name, result, meta, parent) \ + TEST_LOOKUP_TARGET(id, NULL, from, name, 0, result, meta, parent) + + TEST_LOOKUP(0, NULL, "bogus", NULL, NULL, NULL); + TEST_LOOKUP(2, NULL, "wrap", chain->path, chain, NULL); + TEST_LOOKUP(5, NULL, abswrap, chain->path, chain, NULL); + TEST_LOOKUP(8, NULL, "qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(12, NULL, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(16, NULL, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(20, NULL, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(24, NULL, NULL, chain3->path, chain3, chain2->path); /* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */ virCommandFree(cmd); @@ -968,14 +969,14 @@ mymain(void) chain2 = chain->backingStore; chain3 = chain2->backingStore; - TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(9, "wrap", chain->path, chain, NULL); - TEST_LOOKUP(10, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(11, "qcow2", chain2->path, chain2, chain->path); - TEST_LOOKUP(12, absqcow2, chain2->path, chain2, chain->path); - TEST_LOOKUP(13, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(14, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(15, NULL, chain3->path, chain3, chain2->path); + TEST_LOOKUP(28, NULL, "bogus", NULL, NULL, NULL); + TEST_LOOKUP(30, NULL, "wrap", chain->path, chain, NULL); + TEST_LOOKUP(33, NULL, abswrap, chain->path, chain, NULL); + TEST_LOOKUP(36, NULL, "qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(40, NULL, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(44, NULL, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(48, NULL, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(52, NULL, NULL, chain3->path, chain3, chain2->path); /* Use link to wrap with cross-directory relative backing */ virCommandFree(cmd); @@ -995,27 +996,27 @@ mymain(void) chain2 = chain->backingStore; chain3 = chain2->backingStore; - TEST_LOOKUP(16, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(17, "sub/link2", chain->path, chain, NULL); - TEST_LOOKUP(18, "wrap", chain->path, chain, NULL); - TEST_LOOKUP(19, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(20, "../qcow2", chain2->path, chain2, chain->path); - TEST_LOOKUP(21, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(22, absqcow2, chain2->path, chain2, chain->path); - TEST_LOOKUP(23, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(24, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(25, NULL, chain3->path, chain3, chain2->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, chain2->path, chain2, + TEST_LOOKUP(56, NULL, "bogus", NULL, NULL, NULL); + TEST_LOOKUP(57, NULL, "sub/link2", chain->path, chain, NULL); + TEST_LOOKUP(58, NULL, "wrap", chain->path, chain, NULL); + TEST_LOOKUP(59, NULL, abswrap, chain->path, chain, NULL); + TEST_LOOKUP(60, NULL, "../qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(61, NULL, "qcow2", NULL, NULL, NULL); + TEST_LOOKUP(62, NULL, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(63, NULL, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(64, NULL, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(65, NULL, NULL, chain3->path, chain3, chain2->path); + + TEST_LOOKUP_TARGET(66, "vda", NULL, "bogus[1]", 0, NULL, NULL, NULL); + 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(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(32, "vda", "vda[2]", 2, chain3->path, chain3, + TEST_LOOKUP_TARGET(77, "vda", NULL, "vda[2]", 2, chain3->path, chain3, chain2->path); - TEST_LOOKUP_TARGET(33, "vda", "vda[3]", 3, NULL, NULL, NULL); + TEST_LOOKUP_TARGET(81, "vda", NULL, "vda[3]", 3, NULL, NULL, NULL); cleanup: /* Final cleanup */ -- 1.9.3
On 06/13/14 18:31, Eric Blake wrote:
The next patch will be adding tests; for ease of review of that patch, I want to create common context lines that don't change when the new tests are added (it's easier to visually review additions than it is to review an entire chunk of tests rewritten into another larger chunk of tests).
* tests/virstoragetest.c (mymain): Add a parameter and renumber the lookup tests.
Not entirely clear why the new parameter is needed, but gets clarified in the next patch.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 79 +++++++++++++++++++++++++------------------------- 1 file changed, 40 insertions(+), 39 deletions(-)
ACK, Peter
On 06/16/2014 01:36 AM, Peter Krempa wrote:
On 06/13/14 18:31, Eric Blake wrote:
The next patch will be adding tests; for ease of review of that patch, I want to create common context lines that don't change when the new tests are added (it's easier to visually review additions than it is to review an entire chunk of tests rewritten into another larger chunk of tests).
* tests/virstoragetest.c (mymain): Add a parameter and renumber the lookup tests.
Not entirely clear why the new parameter is needed, but gets clarified in the next patch.
I amended the commit message to mention the purpose of the new parameter...
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 79 +++++++++++++++++++++++++------------------------- 1 file changed, 40 insertions(+), 39 deletions(-)
ACK,
and pushed the series. Thanks for the review. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add some more tests of what happens when we restrict a lookup to begin at a point in the middle of a chain. In particular, we want to ensure that a parent is not found when starting at the child. This commit also demonstrates that we have a slight difference in behavior on what parent we report when filtering is in effect; as the determination of the parent affects the code in block commit, exposing this in the testsuite will help justify changes in future patches that tweak the semantics of what lookups are allowed. * tests/virstoragetest.c (testStorageLookup): Test user input. (TEST_LOOKUP_TARGET): Add parameter. (mymain): Add lookup tests. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 74 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 65 insertions(+), 9 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index abfeb36..7c64f18 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -438,6 +438,7 @@ struct testLookupData { virStorageSourcePtr chain; const char *target; + virStorageSourcePtr from; const char *name; unsigned int expIndex; const char *expResult; @@ -465,7 +466,7 @@ testStorageLookup(const void *args) } /* Test twice to ensure optional parameter doesn't cause NULL deref. */ - result = virStorageFileChainLookup(data->chain, NULL, + result = virStorageFileChainLookup(data->chain, data->from, idx ? NULL : data->name, idx, NULL); @@ -494,7 +495,7 @@ testStorageLookup(const void *args) ret = -1; } - result = virStorageFileChainLookup(data->chain, data->chain, + result = virStorageFileChainLookup(data->chain, data->from, data->name, idx, &actualParent); if (!data->expResult) virResetLastError(); @@ -530,6 +531,7 @@ mymain(void) int ret; virCommandPtr cmd = NULL; struct testChainData data; + struct testLookupData data2; virStorageSourcePtr chain = NULL; virStorageSourcePtr chain2; /* short for chain->backingStore */ virStorageSourcePtr chain3; /* short for chain2->backingStore */ @@ -924,26 +926,47 @@ mymain(void) chain2 = chain->backingStore; chain3 = chain2->backingStore; -#define TEST_LOOKUP_TARGET(id, target, ignored, name, index, result, \ +#define TEST_LOOKUP_TARGET(id, target, from, 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; \ + do { \ + data2 = (struct testLookupData){ \ + chain, target, from, name, index, \ + result, meta, parent, }; \ + if (virtTestRun("Chain lookup " #id, \ + testStorageLookup, &data2) < 0) \ + ret = -1; \ } while (0) #define TEST_LOOKUP(id, from, name, result, meta, parent) \ TEST_LOOKUP_TARGET(id, NULL, from, name, 0, result, meta, parent) 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(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(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(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(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(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(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); /* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */ virCommandFree(cmd); @@ -970,13 +993,33 @@ mymain(void) chain3 = chain2->backingStore; 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(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(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(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(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(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(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); /* Use link to wrap with cross-directory relative backing */ virCommandFree(cmd); @@ -1011,11 +1054,24 @@ 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(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(76, "vda", chain3, "vda[1]", 1, NULL, NULL, NULL); TEST_LOOKUP_TARGET(77, "vda", NULL, "vda[2]", 2, chain3->path, chain3, chain2->path); + TEST_LOOKUP_TARGET(78, "vda", chain, "vda[2]", 2, chain3->path, chain3, + 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(81, "vda", NULL, "vda[3]", 3, NULL, NULL, NULL); cleanup: -- 1.9.3
On 06/13/14 18:31, Eric Blake wrote:
Add some more tests of what happens when we restrict a lookup to begin at a point in the middle of a chain. In particular, we want to ensure that a parent is not found when starting at the child. This commit also demonstrates that we have a slight difference in behavior on what parent we report when filtering is in effect; as the determination of the parent affects the code in block commit, exposing this in the testsuite will help justify changes in future patches that tweak the semantics of what lookups are allowed.
* tests/virstoragetest.c (testStorageLookup): Test user input. (TEST_LOOKUP_TARGET): Add parameter. (mymain): Add lookup tests.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 74 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 65 insertions(+), 9 deletions(-)
ACK, Peter
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> --- 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
On 06/13/14 18:31, Eric Blake wrote:
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> --- src/qemu/qemu_driver.c | 3 +-- src/util/virstoragefile.c | 33 ++++++++++++++++++++------------- tests/virstoragetest.c | 36 +++++++++++++++++------------------- 3 files changed, 38 insertions(+), 34 deletions(-)
ACK, Peter
participants (2)
-
Eric Blake -
Peter Krempa