11:31 a.m.
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
11:31 a.m.
New subject: [libvirt] [PATCH 1/4] storage: add alias for less typing
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(a)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
2:10 a.m.
New subject: [libvirt] [PATCH 1/4] storage: add alias for less typing
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(a)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
11:31 a.m.
New subject: [libvirt] [PATCH 2/4] storage: renumber lookup tests
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(a)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
2:36 a.m.
New subject: [libvirt] [PATCH 2/4] storage: renumber lookup tests
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(a)redhat.com>
---
tests/virstoragetest.c | 79 +++++++++++++++++++++++++-------------------------
1 file changed, 40 insertions(+), 39 deletions(-)
ACK,
Peter
10:35 a.m.
New subject: [libvirt] [PATCH 2/4] storage: renumber lookup tests
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(a)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
11:31 a.m.
New subject: [libvirt] [PATCH 3/4] storage: better tests of lookup
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(a)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
2:46 a.m.
New subject: [libvirt] [PATCH 3/4] storage: better tests of lookup
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(a)redhat.com>
---
tests/virstoragetest.c | 74 ++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 65 insertions(+), 9 deletions(-)
ACK,
Peter
11:31 a.m.
New subject: [libvirt] [PATCH 4/4] 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(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
2:51 a.m.
New subject: [libvirt] [PATCH 4/4] blockcommit: require base below top
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(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(-)
ACK,
Peter
3812
days inactive
3815
days old
9 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa