[libvirt] [PATCH 0/5] virstoragefile refactoring, part 3
Part 2 gave us a common virStorageSource struct, now it's time to start using that struct when crawling backing file chains. I posted some RFCs about my full conversion plan, here's the patches I have working so far. Still plenty more to come, but today's batch of patches took me a lot longer than I had planned due to having to refactor the testsuite to avoid a compiler error about over-large stack allocation. Eric Blake (5): tests: use C99 initialization for storage test tests: refactor virstoragetest for less stack space conf: track when storage type is still undetermined conf: track more fields in backing chain metadata conf: start testing contents of the new backing chain fields src/conf/domain_conf.c | 10 +- src/conf/snapshot_conf.c | 2 +- src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 11 +- src/util/virstoragefile.c | 33 +++- src/util/virstoragefile.h | 43 ++++- tests/virstoragetest.c | 405 ++++++++++++++++++++++++++++++++-------------- 7 files changed, 366 insertions(+), 139 deletions(-) -- 1.9.0
Writing this test with C99 initializers will make it easier to test additions and deletions to struct members as I refactor the code. * tests/virstoragetest.c (mymain): Rewrite initializers. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 105 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 80 insertions(+), 25 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 9ec39c7..9a3b3a3 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -339,60 +339,115 @@ mymain(void) /* Expected details about files in chains */ const testFileData raw = { - NULL, NULL, NULL, VIR_STORAGE_FILE_NONE, false, 0, false, + .expFormat = VIR_STORAGE_FILE_NONE, }; const testFileData qcow2_relback_relstart = { - canonraw, "raw", ".", VIR_STORAGE_FILE_RAW, true, 1024, false, + .expBackingStore = canonraw, + .expBackingStoreRaw = "raw", + .expDirectory = ".", + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData qcow2_relback_absstart = { - canonraw, "raw", datadir, VIR_STORAGE_FILE_RAW, true, 1024, false, + .expBackingStore = canonraw, + .expBackingStoreRaw = "raw", + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData qcow2_absback = { - canonraw, absraw, datadir, VIR_STORAGE_FILE_RAW, true, 1024, false, + .expBackingStore = canonraw, + .expBackingStoreRaw = absraw, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData qcow2_as_probe = { - canonraw, absraw, datadir, VIR_STORAGE_FILE_AUTO, true, 1024, false, + .expBackingStore = canonraw, + .expBackingStoreRaw = absraw, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_AUTO, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData qcow2_bogus = { - NULL, datadir "/bogus", datadir, VIR_STORAGE_FILE_NONE, - false, 1024, false, + .expBackingStoreRaw = datadir "/bogus", + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_NONE, + .expCapacity = 1024, }; const testFileData qcow2_protocol = { - "nbd:example.org:6000", NULL, NULL, VIR_STORAGE_FILE_RAW, - false, 1024, false, + .expBackingStore = "nbd:example.org:6000", + .expFormat = VIR_STORAGE_FILE_RAW, + .expCapacity = 1024, }; const testFileData wrap = { - canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_QCOW2, - true, 1024, false, + .expBackingStore = canonqcow2, + .expBackingStoreRaw = absqcow2, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_QCOW2, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData wrap_as_raw = { - canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_RAW, - true, 1024, false, + .expBackingStore = canonqcow2, + .expBackingStoreRaw = absqcow2, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData wrap_as_probe = { - canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_AUTO, - true, 1024, false, + .expBackingStore = canonqcow2, + .expBackingStoreRaw = absqcow2, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_AUTO, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData qed = { - canonraw, absraw, datadir, VIR_STORAGE_FILE_RAW, - true, 1024, false, + .expBackingStore = canonraw, + .expBackingStoreRaw = absraw, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, }; #if HAVE_SYMLINK const testFileData link1_rel = { - canonraw, "../raw", "sub/../sub/..", VIR_STORAGE_FILE_RAW, - true, 1024, false, + .expBackingStore = canonraw, + .expBackingStoreRaw = "../raw", + .expDirectory = "sub/../sub/..", + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData link1_abs = { - canonraw, "../raw", datadir "/sub/../sub/..", VIR_STORAGE_FILE_RAW, - true, 1024, false, + .expBackingStore = canonraw, + .expBackingStoreRaw = "../raw", + .expDirectory = datadir "/sub/../sub/..", + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData link2_rel = { - canonqcow2, "../sub/link1", "sub/../sub", VIR_STORAGE_FILE_QCOW2, - true, 1024, false, + .expBackingStore = canonqcow2, + .expBackingStoreRaw = "../sub/link1", + .expDirectory = "sub/../sub", + .expFormat = VIR_STORAGE_FILE_QCOW2, + .expIsFile = true, + .expCapacity = 1024, }; const testFileData link2_abs = { - canonqcow2, "../sub/link1", datadir "/sub/../sub", - VIR_STORAGE_FILE_QCOW2, true, 1024, false, + .expBackingStore = canonqcow2, + .expBackingStoreRaw = "../sub/link1", + .expDirectory = datadir "/sub/../sub", + .expFormat = VIR_STORAGE_FILE_QCOW2, + .expIsFile = true, + .expCapacity = 1024, }; #endif -- 1.9.0
On 04/04/14 06:32, Eric Blake wrote:
Writing this test with C99 initializers will make it easier to test additions and deletions to struct members as I refactor the code.
* tests/virstoragetest.c (mymain): Rewrite initializers.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 105 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 80 insertions(+), 25 deletions(-)
ACK, Peter
I'm about to add fields to virStorageFileMetadata, which means also adding fields to the testFileData struct in virstoragetest. Alas, adding even one pointer on an x86_64 machine gave me a dreaded compiler error: virstoragetest.c:712:1: error: the frame size of 4208 bytes is larger than 4096 bytes [-Werror=frame-larger-than=] After some experimentation, I realized that each test was creating yet another testChainData (which contains testFileData) on the stack; forcing the reuse of one of these structures instead of creating a fresh one each time drastically reduces the size requirements. While at it, I also got rid of a lot of intermediate structs, with some macro magic that lets me directly build up the destination chains inline. * tests/virstoragetest.c (mymain): Rewrite TEST_ONE_CHAIN to reuse the same struct for each test, and to take the data inline rather than via intermediate variables. (testChainData): Use bounded array of pointers instead of unlimited array of struct. (testStorageChain): Reflect struct change. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 167 ++++++++++++++++++++++++------------------------- 1 file changed, 81 insertions(+), 86 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 9a3b3a3..efd920a 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -218,7 +218,7 @@ struct testChainData { const char *start; enum virStorageFileFormat format; - const testFileData *files; + const testFileData *files[5]; int nfiles; unsigned int flags; }; @@ -267,13 +267,13 @@ testStorageChain(const void *args) if (virAsprintf(&expect, "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d", - NULLSTR(data->files[i].expBackingStore), - NULLSTR(data->files[i].expBackingStoreRaw), - NULLSTR(data->files[i].expDirectory), - data->files[i].expFormat, - data->files[i].expIsFile, - data->files[i].expCapacity, - data->files[i].expEncrypted) < 0 || + NULLSTR(data->files[i]->expBackingStore), + NULLSTR(data->files[i]->expBackingStoreRaw), + NULLSTR(data->files[i]->expDirectory), + data->files[i]->expFormat, + data->files[i]->expIsFile, + data->files[i]->expCapacity, + data->files[i]->expEncrypted) < 0 || virAsprintf(&actual, "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d", NULLSTR(elt->backingStore), @@ -312,29 +312,42 @@ mymain(void) { int ret; virCommandPtr cmd = NULL; + struct testChainData data; /* 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. */ if ((ret = testPrepImages()) != 0) return ret; -#define TEST_ONE_CHAIN(id, start, format, chain, flags) \ +#define TEST_ONE_CHAIN(id, start, format, flags, ...) \ do { \ - struct testChainData data = { \ - start, format, chain, ARRAY_CARDINALITY(chain), flags, \ + size_t i; \ + memset(&data, 0, sizeof(data)); \ + data = (struct testChainData){ \ + start, format, { __VA_ARGS__ }, 0, flags, \ }; \ + for (i = 0; i < ARRAY_CARDINALITY(data.files); i++) \ + if (data.files[i]) \ + data.nfiles++; \ if (virtTestRun("Storage backing chain " id, \ testStorageChain, &data) < 0) \ ret = -1; \ } while (0) +#define VIR_FLATTEN_2(...) __VA_ARGS__ +#define VIR_FLATTEN_1(_1) VIR_FLATTEN_2 _1 + #define TEST_CHAIN(id, relstart, absstart, format, chain1, flags1, \ chain2, flags2, chain3, flags3, chain4, flags4) \ do { \ - TEST_ONE_CHAIN(#id "a", relstart, format, chain1, flags1); \ - TEST_ONE_CHAIN(#id "b", relstart, format, chain2, flags2); \ - TEST_ONE_CHAIN(#id "c", absstart, format, chain3, flags3); \ - TEST_ONE_CHAIN(#id "d", absstart, format, chain4, flags4); \ + TEST_ONE_CHAIN(#id "a", relstart, format, flags1, \ + VIR_FLATTEN_1(chain1)); \ + TEST_ONE_CHAIN(#id "b", relstart, format, flags2, \ + VIR_FLATTEN_1(chain2)); \ + TEST_ONE_CHAIN(#id "c", absstart, format, flags3, \ + VIR_FLATTEN_1(chain3)); \ + TEST_ONE_CHAIN(#id "d", absstart, format, flags4, \ + VIR_FLATTEN_1(chain4)); \ } while (0) /* Expected details about files in chains */ @@ -454,36 +467,31 @@ mymain(void) /* The actual tests, in several groups. */ /* Missing file */ - const testFileData chain0[] = { }; - TEST_ONE_CHAIN("0", "bogus", VIR_STORAGE_FILE_RAW, chain0, EXP_FAIL); + TEST_ONE_CHAIN("0", "bogus", VIR_STORAGE_FILE_RAW, EXP_FAIL); /* Raw image, whether with right format or no specified format */ - const testFileData chain1[] = { raw }; TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW, - chain1, EXP_PASS, - chain1, ALLOW_PROBE | EXP_PASS, - chain1, EXP_PASS, - chain1, ALLOW_PROBE | EXP_PASS); + (&raw), EXP_PASS, + (&raw), ALLOW_PROBE | EXP_PASS, + (&raw), EXP_PASS, + (&raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(2, "raw", absraw, VIR_STORAGE_FILE_AUTO, - chain1, EXP_PASS, - chain1, ALLOW_PROBE | EXP_PASS, - chain1, EXP_PASS, - chain1, ALLOW_PROBE | EXP_PASS); + (&raw), EXP_PASS, + (&raw), ALLOW_PROBE | EXP_PASS, + (&raw), EXP_PASS, + (&raw), ALLOW_PROBE | EXP_PASS); /* Qcow2 file with relative raw backing, format provided */ - const testFileData chain3a[] = { qcow2_relback_relstart, raw }; - const testFileData chain3c[] = { qcow2_relback_absstart, raw }; - const testFileData chain4a[] = { raw }; TEST_CHAIN(3, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - chain3a, EXP_PASS, - chain3a, ALLOW_PROBE | EXP_PASS, - chain3c, EXP_PASS, - chain3c, ALLOW_PROBE | EXP_PASS); + (&qcow2_relback_relstart, &raw), EXP_PASS, + (&qcow2_relback_relstart, &raw), ALLOW_PROBE | EXP_PASS, + (&qcow2_relback_absstart, &raw), EXP_PASS, + (&qcow2_relback_absstart, &raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(4, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - chain4a, EXP_PASS, - chain3a, ALLOW_PROBE | EXP_PASS, - chain4a, EXP_PASS, - chain3c, ALLOW_PROBE | EXP_PASS); + (&raw), EXP_PASS, + (&qcow2_relback_relstart, &raw), ALLOW_PROBE | EXP_PASS, + (&raw), EXP_PASS, + (&qcow2_relback_absstart, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 file to use absolute backing name */ virCommandFree(cmd); @@ -493,26 +501,23 @@ mymain(void) ret = -1; /* Qcow2 file with raw as absolute backing, backing format provided */ - const testFileData chain5[] = { qcow2_absback, raw }; - const testFileData chain6[] = { raw }; TEST_CHAIN(5, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - chain5, EXP_PASS, - chain5, ALLOW_PROBE | EXP_PASS, - chain5, EXP_PASS, - chain5, ALLOW_PROBE | EXP_PASS); + (&qcow2_absback, &raw), EXP_PASS, + (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS, + (&qcow2_absback, &raw), EXP_PASS, + (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(6, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - chain6, EXP_PASS, - chain5, ALLOW_PROBE | EXP_PASS, - chain6, EXP_PASS, - chain5, ALLOW_PROBE | EXP_PASS); + (&raw), EXP_PASS, + (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS, + (&raw), EXP_PASS, + (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS); /* Wrapped file access */ - const testFileData chain7[] = { wrap, qcow2_absback, raw }; TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - chain7, EXP_PASS, - chain7, ALLOW_PROBE | EXP_PASS, - chain7, EXP_PASS, - chain7, ALLOW_PROBE | EXP_PASS); + (&wrap, &qcow2_absback, &raw), EXP_PASS, + (&wrap, &qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS, + (&wrap, &qcow2_absback, &raw), EXP_PASS, + (&wrap, &qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 and wrap file to omit backing file type */ virCommandFree(cmd); @@ -528,13 +533,11 @@ mymain(void) ret = -1; /* Qcow2 file with raw as absolute backing, backing format omitted */ - const testFileData chain8a[] = { wrap_as_raw, raw }; - const testFileData chain8b[] = { wrap_as_probe, qcow2_as_probe, raw }; TEST_CHAIN(8, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - chain8a, EXP_PASS, - chain8b, ALLOW_PROBE | EXP_PASS, - chain8a, EXP_PASS, - chain8b, ALLOW_PROBE | EXP_PASS); + (&wrap_as_raw, &raw), EXP_PASS, + (&wrap_as_probe, &qcow2_as_probe, &raw), ALLOW_PROBE | EXP_PASS, + (&wrap_as_raw, &raw), EXP_PASS, + (&wrap_as_probe, &qcow2_as_probe, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 to a missing backing file, with backing type */ virCommandFree(cmd); @@ -545,12 +548,11 @@ mymain(void) ret = -1; /* Qcow2 file with missing backing file but specified type */ - const testFileData chain9[] = { qcow2_bogus }; TEST_CHAIN(9, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - chain9, EXP_WARN, - chain9, ALLOW_PROBE | EXP_WARN, - chain9, EXP_WARN, - chain9, ALLOW_PROBE | EXP_WARN); + (&qcow2_bogus), EXP_WARN, + (&qcow2_bogus), ALLOW_PROBE | EXP_WARN, + (&qcow2_bogus), EXP_WARN, + (&qcow2_bogus), ALLOW_PROBE | EXP_WARN); /* Rewrite qcow2 to a missing backing file, without backing type */ virCommandFree(cmd); @@ -560,12 +562,11 @@ mymain(void) ret = -1; /* Qcow2 file with missing backing file and no specified type */ - const testFileData chain10[] = { qcow2_bogus }; TEST_CHAIN(10, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - chain10, EXP_WARN, - chain10, ALLOW_PROBE | EXP_WARN, - chain10, EXP_WARN, - chain10, ALLOW_PROBE | EXP_WARN); + (&qcow2_bogus), EXP_WARN, + (&qcow2_bogus), ALLOW_PROBE | EXP_WARN, + (&qcow2_bogus), EXP_WARN, + (&qcow2_bogus), ALLOW_PROBE | EXP_WARN); /* Rewrite qcow2 to use an nbd: protocol as backend */ virCommandFree(cmd); @@ -576,21 +577,18 @@ mymain(void) ret = -1; /* Qcow2 file with backing protocol instead of file */ - const testFileData chain11[] = { qcow2_protocol }; TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - chain11, EXP_PASS, - chain11, ALLOW_PROBE | EXP_PASS, - chain11, EXP_PASS, - chain11, ALLOW_PROBE | EXP_PASS); + (&qcow2_protocol), EXP_PASS, + (&qcow2_protocol), ALLOW_PROBE | EXP_PASS, + (&qcow2_protocol), EXP_PASS, + (&qcow2_protocol), ALLOW_PROBE | EXP_PASS); /* qed file */ - const testFileData chain12a[] = { raw }; - const testFileData chain12b[] = { qed, raw }; TEST_CHAIN(12, "qed", absqed, VIR_STORAGE_FILE_AUTO, - chain12a, EXP_PASS, - chain12b, ALLOW_PROBE | EXP_PASS, - chain12a, EXP_PASS, - chain12b, ALLOW_PROBE | EXP_PASS); + (&raw), EXP_PASS, + (&qed, &raw), ALLOW_PROBE | EXP_PASS, + (&raw), EXP_PASS, + (&qed, &raw), ALLOW_PROBE | EXP_PASS); #ifdef HAVE_SYMLINK /* Rewrite qcow2 and wrap file to use backing names relative to a @@ -609,15 +607,12 @@ mymain(void) ret = -1; /* Behavior of symlinks to qcow2 with relative backing files */ - const testFileData chain13a[] = { link2_rel, link1_rel, raw }; - const testFileData chain13c[] = { link2_abs, link1_abs, raw }; TEST_CHAIN(13, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, - chain13a, EXP_PASS, - chain13a, ALLOW_PROBE | EXP_PASS, - chain13c, EXP_PASS, - chain13c, ALLOW_PROBE | EXP_PASS); + (&link2_rel, &link1_rel, &raw), EXP_PASS, + (&link2_rel, &link1_rel, &raw), ALLOW_PROBE | EXP_PASS, + (&link2_abs, &link1_abs, &raw), EXP_PASS, + (&link2_abs, &link1_abs, &raw), ALLOW_PROBE | EXP_PASS); #endif - /* Final cleanup */ testCleanupImages(); virCommandFree(cmd); -- 1.9.0
On 04/04/14 06:32, Eric Blake wrote:
I'm about to add fields to virStorageFileMetadata, which means also adding fields to the testFileData struct in virstoragetest. Alas, adding even one pointer on an x86_64 machine gave me a dreaded compiler error:
virstoragetest.c:712:1: error: the frame size of 4208 bytes is larger than 4096 bytes [-Werror=frame-larger-than=]
After some experimentation, I realized that each test was creating yet another testChainData (which contains testFileData) on the stack; forcing the reuse of one of these structures instead of creating a fresh one each time drastically reduces the size requirements. While at it, I also got rid of a lot of intermediate structs, with some macro magic that lets me directly build up the destination chains inline.
Unfortunately it's not completely trivial to understand what's happening on the first glance.
* tests/virstoragetest.c (mymain): Rewrite TEST_ONE_CHAIN to reuse the same struct for each test, and to take the data inline rather than via intermediate variables. (testChainData): Use bounded array of pointers instead of unlimited array of struct. (testStorageChain): Reflect struct change.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 167 ++++++++++++++++++++++++------------------------- 1 file changed, 81 insertions(+), 86 deletions(-)
As it's test code: ACK Peter
On 04/04/2014 02:54 AM, Peter Krempa wrote:
On 04/04/14 06:32, Eric Blake wrote:
I'm about to add fields to virStorageFileMetadata, which means also adding fields to the testFileData struct in virstoragetest. Alas, adding even one pointer on an x86_64 machine gave me a dreaded compiler error:
virstoragetest.c:712:1: error: the frame size of 4208 bytes is larger than 4096 bytes [-Werror=frame-larger-than=]
After some experimentation, I realized that each test was creating yet another testChainData (which contains testFileData) on the stack; forcing the reuse of one of these structures instead of creating a fresh one each time drastically reduces the size requirements. While at it, I also got rid of a lot of intermediate structs, with some macro magic that lets me directly build up the destination chains inline.
Unfortunately it's not completely trivial to understand what's happening on the first glance.
I'll expand the commit message before pushing. Basically,
-#define TEST_ONE_CHAIN(id, start, format, chain, flags) \ +#define TEST_ONE_CHAIN(id, start, format, flags, ...) \ do { \ - struct testChainData data = { \ - start, format, chain, ARRAY_CARDINALITY(chain), flags, \
the old code was populating data.files = chain, where 'chain' was an intermediate variable and files is a testFileData*
+ size_t i; \ + memset(&data, 0, sizeof(data)); \ + data = (struct testChainData){ \ + start, format, { __VA_ARGS__ }, 0, flags, \ }; \
while the new code is populating data.files = { ptr, ptr, ... }, with no intermediate chain variable, and where files is now an array of testFileData*.
+ for (i = 0; i < ARRAY_CARDINALITY(data.files); i++) \ + if (data.files[i]) \ + data.nfiles++; \
Pre-patch, counting the number of files was done by the size of the 'chain' intermediate variable. Post-patch, it is done by counting the number of non-NULL pointers in the 'files' array.
+ TEST_ONE_CHAIN(#id "a", relstart, format, flags1, \ + VIR_FLATTEN_1(chain1)); \
Here, VIR_FLATTEN_1 is macro magic that takes a single macro argument in the form '(a, b, c)' and turns it into multiple arguments 'a, b, c' to TEST_ONE_CHAIN's use of '...'
/* Raw image, whether with right format or no specified format */ - const testFileData chain1[] = { raw }; TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW, - chain1, EXP_PASS, - chain1, ALLOW_PROBE | EXP_PASS, - chain1, EXP_PASS, - chain1, ALLOW_PROBE | EXP_PASS); + (&raw), EXP_PASS, + (&raw), ALLOW_PROBE | EXP_PASS, + (&raw), EXP_PASS, + (&raw), ALLOW_PROBE | EXP_PASS);
Then for each test, I replaced uses of 'chain1' (the intermediate variable)' with '(contents of chain1)' as a direct argument to TEST_CHAIN.
As it's test code: ACK
Yes, we have that going for us - if it compiles and still passes the testsuite, it was probably correct :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Right now, virStorageFileMetadata tracks bool backingStoreIsFile for whether the backing string specified in metadata can be resolved as a file (covering both block and regular file resources) or is treated as a network protocol. But when merging this struct with virStorageSource, it will be easier to just actually track which type of resource it is, as well as have a reserved value for the case where the resource type is unknown (or had an error during probing). * src/util/virstoragefile.h (virStorageType): Add a placeholder value, swap order to match similar public enum. * src/util/virstoragefile.c (virStorage): Update string mapping. * src/conf/domain_conf.c (virDomainDiskSourceParse) (virDomainDiskDefParseXML, virDomainDiskDefFormat) (virDomainDiskSourceFormat): Adjust clients. * src/conf/snapshot_conf.c (virDomainSnapshotDiskDefParseXML): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotPrepareDiskExternalBackingInactive) (qemuDomainSnapshotPrepareDiskExternalOverlayActive) (qemuDomainSnapshotPrepareDiskExternalOverlayInactive) (qemuDomainSnapshotPrepareDiskInternal) (qemuDomainSnapshotCreateSingleDiskActive): Likewise. * src/qemu/qemu_command.c (qemuGetDriveSourceString): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 10 ++++++---- src/conf/snapshot_conf.c | 2 +- src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 11 +++++++++-- src/util/virstoragefile.c | 3 ++- src/util/virstoragefile.h | 8 ++++++-- 6 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 465bf84..0c5c7ab 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4965,7 +4965,7 @@ virDomainDiskSourceParse(xmlNodePtr node, memset(&host, 0, sizeof(host)); - switch (src->type) { + switch ((enum virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: src->path = virXMLPropString(node, "file"); break; @@ -5053,7 +5053,8 @@ virDomainDiskSourceParse(xmlNodePtr node, if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) goto cleanup; break; - default: + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %s"), virStorageTypeToString(src->type)); @@ -5150,7 +5151,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, type = virXMLPropString(node, "type"); if (type) { - if ((def->src.type = virStorageTypeFromString(type)) < 0) { + if ((def->src.type = virStorageTypeFromString(type)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk type '%s'"), type); goto error; @@ -14836,6 +14837,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, src->seclabels, flags); break; + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %d"), src->type); @@ -14867,7 +14869,7 @@ virDomainDiskDefFormat(virBufferPtr buf, char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!type) { + if (!type || !def->src.type) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %d"), def->src.type); return -1; diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 374a104..852a286 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -132,7 +132,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, } if ((type = virXMLPropString(node, "type"))) { - if ((def->src.type = virStorageTypeFromString(type)) < 0 || + if ((def->src.type = virStorageTypeFromString(type)) <= 0 || def->src.type == VIR_STORAGE_TYPE_VOLUME || def->src.type == VIR_STORAGE_TYPE_DIR) { virReportError(VIR_ERR_XML_ERROR, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 099a777..37841d1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3856,6 +3856,7 @@ qemuGetDriveSourceString(int type, break; case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: break; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1d08951..4bb4819 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12356,6 +12356,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("external inactive snapshots are not supported on " @@ -12420,6 +12421,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("external active snapshots are not supported on " @@ -12444,6 +12446,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayInactive(virDomainSnapshotDiskDefPtr case VIR_STORAGE_TYPE_NETWORK: case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("external inactive snapshots are not supported on " @@ -12561,6 +12564,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("internal inactive snapshots are not supported on " @@ -12766,7 +12770,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, VIR_STRDUP(persistSource, snap->src.path) < 0) goto cleanup; - switch (snap->src.type) { + switch ((enum virStorageType)snap->src.type) { case VIR_STORAGE_TYPE_BLOCK: reuse = true; /* fallthrough */ @@ -12813,7 +12817,10 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } break; - default: + case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("snapshots are not supported on '%s' volumes"), virStorageTypeToString(snap->src.type)); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e6a985d..0e1461d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -48,8 +48,9 @@ VIR_LOG_INIT("util.storagefile"); VIR_ENUM_IMPL(virStorage, VIR_STORAGE_TYPE_LAST, - "block", + "none", "file", + "block", "dir", "network", "volume") diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 56105d0..3807285 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -38,10 +38,14 @@ # define VIR_STORAGE_MAX_HEADER 0x8200 -/* Types of disk backends (host resource) */ +/* Types of disk backends (host resource). Comparable to the public + * virStorageVolType, except we have an undetermined state, don't have + * a netdir type, and add a volume type for reference through a + * storage pool. */ enum virStorageType { - VIR_STORAGE_TYPE_BLOCK, + VIR_STORAGE_TYPE_NONE, VIR_STORAGE_TYPE_FILE, + VIR_STORAGE_TYPE_BLOCK, VIR_STORAGE_TYPE_DIR, VIR_STORAGE_TYPE_NETWORK, VIR_STORAGE_TYPE_VOLUME, -- 1.9.0
On 04/04/14 06:32, Eric Blake wrote:
Right now, virStorageFileMetadata tracks bool backingStoreIsFile for whether the backing string specified in metadata can be resolved as a file (covering both block and regular file resources) or is treated as a network protocol. But when merging this struct with virStorageSource, it will be easier to just actually track which type of resource it is, as well as have a reserved value for the case where the resource type is unknown (or had an error during probing).
* src/util/virstoragefile.h (virStorageType): Add a placeholder value, swap order to match similar public enum. * src/util/virstoragefile.c (virStorage): Update string mapping. * src/conf/domain_conf.c (virDomainDiskSourceParse) (virDomainDiskDefParseXML, virDomainDiskDefFormat) (virDomainDiskSourceFormat): Adjust clients. * src/conf/snapshot_conf.c (virDomainSnapshotDiskDefParseXML): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotPrepareDiskExternalBackingInactive) (qemuDomainSnapshotPrepareDiskExternalOverlayActive) (qemuDomainSnapshotPrepareDiskExternalOverlayInactive) (qemuDomainSnapshotPrepareDiskInternal) (qemuDomainSnapshotCreateSingleDiskActive): Likewise. * src/qemu/qemu_command.c (qemuGetDriveSourceString): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 10 ++++++---- src/conf/snapshot_conf.c | 2 +- src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 11 +++++++++-- src/util/virstoragefile.c | 3 ++- src/util/virstoragefile.h | 8 ++++++-- 6 files changed, 25 insertions(+), 10 deletions(-)
ACK, having _TYPE_BLOCK as index 0 caused grief a lot of times for me. Peter
The current use of virStorageFileMetadata is awkward; to learn some of the information about a child node, you have to read fields in the parent node. This does not lend itself well to modifying backing chains (whether inserting a new node in the chain, or consolidating existing nodes); better would be to learn about a child node directly in that node. This patch sets up some new fields which contain redundant information, although not necessarily in the final desired state for the new fields (see the next patch for actual tests of what is there now). Then later patches will do any refactoring necessary to get the fields to their desired states, and update clients to get the information from the new fields, so we can finally delete the fields that are tracking information about the wrong node. * src/util/virstoragefile.h (_virStorageFileMetadata): Add path, canonName, relDir, type, and format fields. Reorder existing fields, and add lots of comments. * src/util/virstoragefile.c (virStorageFileFreeMetadata): Clean new fields. (virStorageFileGetMetadataInternal) (virStorageFileGetMetadataFromFDInternal): Start populating new fields. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 30 ++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 0e1461d..d741fb7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -798,6 +798,10 @@ virStorageFileGetMetadataInternal(const char *path, if (VIR_ALLOC(meta) < 0) return NULL; + if (VIR_STRDUP(meta->path, path) < 0) + goto cleanup; + if (VIR_STRDUP(meta->relDir, directory) < 0) + goto cleanup; if (format == VIR_STORAGE_FILE_AUTO) format = virStorageFileProbeFormatFromBuf(path, buf, len); @@ -808,6 +812,7 @@ virStorageFileGetMetadataInternal(const char *path, format); goto cleanup; } + meta->format = format; /* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files @@ -1013,9 +1018,20 @@ virStorageFileGetMetadataFromFDInternal(const char *path, return NULL; } - /* No header to probe for directories, but also no backing file */ if (S_ISDIR(sb.st_mode)) { - ignore_value(VIR_ALLOC(ret)); + /* No header to probe for directories, but also no backing + * file; therefore, no inclusion loop is possible, and we + * don't need canonName or relDir. */ + if (VIR_ALLOC(ret) < 0) + goto cleanup; + if (VIR_STRDUP(ret->path, path) < 0) { + virStorageFileFreeMetadata(ret); + ret = NULL; + goto cleanup; + } + ret->type = VIR_STORAGE_TYPE_DIR; + ret->format = format > VIR_STORAGE_FILE_NONE ? format + : VIR_STORAGE_FILE_DIR; goto cleanup; } @@ -1030,6 +1046,12 @@ virStorageFileGetMetadataFromFDInternal(const char *path, } ret = virStorageFileGetMetadataInternal(path, buf, len, directory, format); + if (ret) { + if (S_ISREG(sb.st_mode)) + ret->type = VIR_STORAGE_TYPE_FILE; + else if (S_ISBLK(sb.st_mode)) + ret->type = VIR_STORAGE_TYPE_BLOCK; + } cleanup: VIR_FREE(buf); return ret; @@ -1205,6 +1227,10 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) if (!meta) return; + VIR_FREE(meta->path); + VIR_FREE(meta->canonName); + VIR_FREE(meta->relDir); + virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); VIR_FREE(meta->backingStoreRaw); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3807285..a284e37 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -112,17 +112,42 @@ struct _virStorageTimestamps { typedef struct _virStorageFileMetadata virStorageFileMetadata; typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { - char *backingStore; /* Canonical name (absolute file, or protocol) */ - char *backingStoreRaw; /* If file, original name, possibly relative */ - char *directory; /* The directory containing basename of backingStoreRaw */ - int backingStoreFormat; /* enum virStorageFileFormat */ - bool backingStoreIsFile; + /* Name of this file as spelled by the user (top level) or + * metadata of the parent (if this is a backing store). */ + char *path; + /* Canonical name of this file, used to detect loops in the + * backing store chain. */ + char *canonName; + /* Directory to start from if backingStoreRaw is a relative file + * name */ + char *relDir; + /* Name of the backing store recorded in metadata of the parent */ + char *backingStoreRaw; + + /* Backing chain. backingMeta->origName should match + * backingStoreRaw; but the fields are duplicated in the common + * case to make it possible to detect missing backing files, as + * follows: if backingStoreRaw is NULL, this should be NULL. If + * this is NULL and backingStoreRaw is non-NULL, there was an + * error following the chain (such as missing file). Otherwise, + * information about the backing store is here. */ virStorageFileMetadataPtr backingMeta; + /* Details about the current image */ + int type; /* enum virStorageType */ + int format; /* enum virStorageFileFormat */ virStorageEncryptionPtr encryption; unsigned long long capacity; virBitmapPtr features; /* bits described by enum virStorageFileFeature */ char *compat; + + /* Fields I'm trying to delete, because it is confusing to have to + * query the parent metadata for details about the backing + * store. */ + char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as backingMeta->canon */ + char *directory; /* The directory containing basename of backingStoreRaw. Should be same as backingMeta->relDir */ + int backingStoreFormat; /* enum virStorageFileFormat. Should be same as backingMeta->format */ + bool backingStoreIsFile; /* Should be same as backingMeta->type < VIR_STORAGE_TYPE_NETWORK */ }; -- 1.9.0
On 04/04/14 06:32, Eric Blake wrote:
The current use of virStorageFileMetadata is awkward; to learn some of the information about a child node, you have to read fields in the parent node. This does not lend itself well to modifying backing chains (whether inserting a new node in the chain, or consolidating existing nodes); better would be to learn about a child node directly in that node. This patch sets up some new fields which contain redundant information, although not necessarily in the final desired state for the new fields (see the next patch for actual tests of what is there now). Then later patches will do any refactoring necessary to get the fields to their desired states, and update clients to get the information from the new fields, so we can finally delete the fields that are tracking information about the wrong node.
* src/util/virstoragefile.h (_virStorageFileMetadata): Add path, canonName, relDir, type, and format fields. Reorder existing fields, and add lots of comments. * src/util/virstoragefile.c (virStorageFileFreeMetadata): Clean new fields. (virStorageFileGetMetadataInternal) (virStorageFileGetMetadataFromFDInternal): Start populating new fields.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 30 ++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 58 insertions(+), 7 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 0e1461d..d741fb7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c
...
return ret; @@ -1205,6 +1227,10 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) if (!meta) return;
+ VIR_FREE(meta->path); + VIR_FREE(meta->canonName); + VIR_FREE(meta->relDir); + virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); VIR_FREE(meta->backingStoreRaw); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3807285..a284e37 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -112,17 +112,42 @@ struct _virStorageTimestamps { typedef struct _virStorageFileMetadata virStorageFileMetadata; typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { - char *backingStore; /* Canonical name (absolute file, or protocol) */ - char *backingStoreRaw; /* If file, original name, possibly relative */ - char *directory; /* The directory containing basename of backingStoreRaw */ - int backingStoreFormat; /* enum virStorageFileFormat */ - bool backingStoreIsFile; + /* Name of this file as spelled by the user (top level) or + * metadata of the parent (if this is a backing store). */ + char *path; + /* Canonical name of this file, used to detect loops in the + * backing store chain. */ + char *canonName; + /* Directory to start from if backingStoreRaw is a relative file + * name */ + char *relDir; + /* Name of the backing store recorded in metadata of the parent */ + char *backingStoreRaw;
Hmm, this field seems pretty redundant to me, IIUC it's the same data as in "path". OTOH, the "path" field should contain the canonical path as the target is to convert it to the new storage file struct. In that case the "canonName" will be redundant and backingStoreRaw needs to be kept to track the original name. In any case ... one of them seems to be duplicate.
+ + /* Backing chain. backingMeta->origName should match + * backingStoreRaw; but the fields are duplicated in the common + * case to make it possible to detect missing backing files, as + * follows: if backingStoreRaw is NULL, this should be NULL. If + * this is NULL and backingStoreRaw is non-NULL, there was an + * error following the chain (such as missing file). Otherwise, + * information about the backing store is here. */ virStorageFileMetadataPtr backingMeta;
+ /* Details about the current image */ + int type; /* enum virStorageType */ + int format; /* enum virStorageFileFormat */ virStorageEncryptionPtr encryption; unsigned long long capacity; virBitmapPtr features; /* bits described by enum virStorageFileFeature */ char *compat; + + /* Fields I'm trying to delete, because it is confusing to have to + * query the parent metadata for details about the backing + * store. */ + char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as backingMeta->canon */ + char *directory; /* The directory containing basename of backingStoreRaw. Should be same as backingMeta->relDir */ + int backingStoreFormat; /* enum virStorageFileFormat. Should be same as backingMeta->format */ + bool backingStoreIsFile; /* Should be same as backingMeta->type < VIR_STORAGE_TYPE_NETWORK */ };
Peter
On 04/04/2014 03:31 AM, Peter Krempa wrote:
struct _virStorageFileMetadata { - char *backingStore; /* Canonical name (absolute file, or protocol) */ - char *backingStoreRaw; /* If file, original name, possibly relative */ - char *directory; /* The directory containing basename of backingStoreRaw */ - int backingStoreFormat; /* enum virStorageFileFormat */ - bool backingStoreIsFile; + /* Name of this file as spelled by the user (top level) or + * metadata of the parent (if this is a backing store). */ + char *path; + /* Canonical name of this file, used to detect loops in the + * backing store chain. */ + char *canonName; + /* Directory to start from if backingStoreRaw is a relative file + * name */ + char *relDir; + /* Name of the backing store recorded in metadata of the parent */ + char *backingStoreRaw;
Hmm, this field seems pretty redundant to me, IIUC it's the same data as in "path".
No, it's not. Given the chain: base <- top my goal is to have: { .path = "top", .canonName = "/path/to/top", .relDir = ".", .backingStoreRaw = "base", .backingMeta = { .path = "base", .canonName = "/path/to/base", .relDir = ".", .backingStoreRaw = NULL, .backingMeta = NULL, } }
OTOH, the "path" field should contain the canonical path as the target is to convert it to the new storage file struct. In that case the "canonName" will be redundant and backingStoreRaw needs to be kept to track the original name.
Rather, "path" should be (but is not yet) the name as specified by the user or the metadata of the parent file, unmodified (for both local files and for network elements like gluster://server/vol/img); while canonName should be the possibly munged name (munged to canonical absolute name for local files, unmunged for network elements). Loop detection is done by registering canonName in the hash table while following the backing chain.
In any case ... one of them seems to be duplicate.
No, path is about the current element, while backingStoreRaw is about the child element. My other goal in this refactor is to make detection of missing backing chains much saner. Right now, for the chain 'missing <- foo', we have this for 'foo': { .backingStoreRaw = "missing", .backingStore = NULL, .backingStoreIsFile = false, .backingMeta = NULL, } while for the chain 'gluster://.../base <- foo', we have { .backingStoreRaw = NULL, .backingStore = "gluster://.../base", .backingStoreIsFile = false, .backingMeta = NULL, } where code that doesn't care about whether foo exists (such as storage volume dumpxml) vs. code that DOES care (sVirt labeling before starting a qemu domain) has to pay attention to multiple fields in the parent to decide whether to raise an error; furthermore, since there is never any chain emitted for a non-file backing, we can't support any network file of anything other than raw. After the conversion, I envision: { .path = "foo", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, .backingStoreRaw = "missing", .backingMeta = NULL, } or maybe: { .path = "foo", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, .backingStoreRaw = "missing", .backingMeta = { .path = "missing", .type = VIR_STORAGE_TYPE_NONE, .format = VIR_STORAGE_FILE_NONE, } } as the indication of a missing backing file, and: { .path = "foo", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, .backingStoreRaw = "gluster://.../base", .backingMeta = { .path = "gluster://.../base", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, .backingStoreRaw = NULL, .backingMeta = NULL, } } as indication of a chain involving a network backing file, and where we have a much easier task of identifying a network type resource that can be something other than raw, so that we can actually have a chain of multiple network devices if we know how to probe that network file (the way we do for gluster). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/04/14 14:54, Eric Blake wrote:
On 04/04/2014 03:31 AM, Peter Krempa wrote:
struct _virStorageFileMetadata { - char *backingStore; /* Canonical name (absolute file, or protocol) */ - char *backingStoreRaw; /* If file, original name, possibly relative */ - char *directory; /* The directory containing basename of backingStoreRaw */ - int backingStoreFormat; /* enum virStorageFileFormat */ - bool backingStoreIsFile; + /* Name of this file as spelled by the user (top level) or + * metadata of the parent (if this is a backing store). */ + char *path; + /* Canonical name of this file, used to detect loops in the + * backing store chain. */ + char *canonName; + /* Directory to start from if backingStoreRaw is a relative file + * name */ + char *relDir; + /* Name of the backing store recorded in metadata of the parent */
Maybe then change "parent" to "this image" to un-confuse me :)
+ char *backingStoreRaw;
Hmm, this field seems pretty redundant to me, IIUC it's the same data as in "path".
No, it's not.
Given the chain:
base <- top
my goal is to have:
{ .path = "top", .canonName = "/path/to/top", .relDir = ".", .backingStoreRaw = "base", .backingMeta = { .path = "base", .canonName = "/path/to/base", .relDir = ".", .backingStoreRaw = NULL, .backingMeta = NULL, } }
.... my mind was poisoned with the current way the code is filling the fields and a little bit with the comment. ACK the refactor makes sense. Maybe it's worth changing the wording a bit though. Peter
The testsuite is absolutely essential to feeling comfortable about swapping the backing chain structure over to a new format. This patch tests the path settings, and demonstrates that right now the code is passing only the canonical name to the child struct, which means more work is necessary in virstoragefile to pass the user spelling alongside the canonical name down to the child. * tests/virstoragetest.c (testStorageChain): Test path. (mymain): Update expected data. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 239 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 175 insertions(+), 64 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index efd920a..9d6e344 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -205,6 +205,7 @@ struct _testFileData bool expIsFile; unsigned long long expCapacity; bool expEncrypted; + const char *expPath; }; enum { @@ -266,21 +267,25 @@ testStorageChain(const void *args) } if (virAsprintf(&expect, - "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d", + "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d\n" + "path:%s\n", NULLSTR(data->files[i]->expBackingStore), NULLSTR(data->files[i]->expBackingStoreRaw), NULLSTR(data->files[i]->expDirectory), data->files[i]->expFormat, data->files[i]->expIsFile, data->files[i]->expCapacity, - data->files[i]->expEncrypted) < 0 || + data->files[i]->expEncrypted, + NULLSTR(data->files[i]->expPath)) < 0 || virAsprintf(&actual, - "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d", + "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d\n" + "path:%s\n", NULLSTR(elt->backingStore), NULLSTR(elt->backingStoreRaw), NULLSTR(elt->directory), elt->backingStoreFormat, elt->backingStoreIsFile, - elt->capacity, !!elt->encryption) < 0) { + elt->capacity, !!elt->encryption, + NULLSTR(elt->path)) < 0) { VIR_FREE(expect); VIR_FREE(actual); goto cleanup; @@ -351,9 +356,24 @@ mymain(void) } while (0) /* Expected details about files in chains */ - const testFileData raw = { + const testFileData rel_raw = { .expFormat = VIR_STORAGE_FILE_NONE, + .expPath = "raw", }; + const testFileData abs_raw = { + .expFormat = VIR_STORAGE_FILE_NONE, + .expPath = canonraw, + }; + + const testFileData rel_qcow2_as_raw = { + .expFormat = VIR_STORAGE_FILE_NONE, + .expPath = "qcow2", + }; + const testFileData abs_qcow2_as_raw = { + .expFormat = VIR_STORAGE_FILE_NONE, + .expPath = canonqcow2, + }; + const testFileData qcow2_relback_relstart = { .expBackingStore = canonraw, .expBackingStoreRaw = "raw", @@ -361,6 +381,7 @@ mymain(void) .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .expPath = "qcow2", }; const testFileData qcow2_relback_absstart = { .expBackingStore = canonraw, @@ -369,15 +390,28 @@ mymain(void) .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .expPath = canonqcow2, }; - const testFileData qcow2_absback = { + + const testFileData rel_qcow2_absback = { .expBackingStore = canonraw, .expBackingStoreRaw = absraw, .expDirectory = datadir, .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .expPath = "qcow2", }; + const testFileData abs_qcow2_absback = { + .expBackingStore = canonraw, + .expBackingStoreRaw = absraw, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, + .expPath = canonqcow2, + }; + const testFileData qcow2_as_probe = { .expBackingStore = canonraw, .expBackingStoreRaw = absraw, @@ -385,50 +419,122 @@ mymain(void) .expFormat = VIR_STORAGE_FILE_AUTO, .expIsFile = true, .expCapacity = 1024, + .expPath = canonqcow2, }; - const testFileData qcow2_bogus = { + + const testFileData rel_qcow2_bogus = { .expBackingStoreRaw = datadir "/bogus", .expDirectory = datadir, .expFormat = VIR_STORAGE_FILE_NONE, .expCapacity = 1024, + .expPath = "qcow2", }; - const testFileData qcow2_protocol = { + const testFileData abs_qcow2_bogus = { + .expBackingStoreRaw = datadir "/bogus", + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_NONE, + .expCapacity = 1024, + .expPath = canonqcow2, + }; + + const testFileData rel_qcow2_protocol = { .expBackingStore = "nbd:example.org:6000", .expFormat = VIR_STORAGE_FILE_RAW, .expCapacity = 1024, + .expPath = "qcow2", }; - const testFileData wrap = { + const testFileData abs_qcow2_protocol = { + .expBackingStore = "nbd:example.org:6000", + .expFormat = VIR_STORAGE_FILE_RAW, + .expCapacity = 1024, + .expPath = canonqcow2, + }; + + const testFileData rel_wrap = { .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, .expDirectory = datadir, .expFormat = VIR_STORAGE_FILE_QCOW2, .expIsFile = true, .expCapacity = 1024, + .expPath = "wrap", }; - const testFileData wrap_as_raw = { + const testFileData abs_wrap = { + .expBackingStore = canonqcow2, + .expBackingStoreRaw = absqcow2, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_QCOW2, + .expIsFile = true, + .expCapacity = 1024, + .expPath = abswrap, + }; + + const testFileData rel_wrap_as_raw = { .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, .expDirectory = datadir, .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .expPath = "wrap", }; - const testFileData wrap_as_probe = { + const testFileData abs_wrap_as_raw = { + .expBackingStore = canonqcow2, + .expBackingStoreRaw = absqcow2, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, + .expPath = abswrap, + }; + + const testFileData rel_wrap_as_probe = { .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, .expDirectory = datadir, .expFormat = VIR_STORAGE_FILE_AUTO, .expIsFile = true, .expCapacity = 1024, + .expPath = "wrap", }; - const testFileData qed = { + const testFileData abs_wrap_as_probe = { + .expBackingStore = canonqcow2, + .expBackingStoreRaw = absqcow2, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_AUTO, + .expIsFile = true, + .expCapacity = 1024, + .expPath = abswrap, + }; + + const testFileData rel_qed_as_raw = { + .expFormat = VIR_STORAGE_FILE_NONE, + .expPath = "qed", + }; + const testFileData abs_qed_as_raw = { + .expFormat = VIR_STORAGE_FILE_NONE, + .expPath = absqed, + }; + + const testFileData rel_qed = { + .expBackingStore = canonraw, + .expBackingStoreRaw = absraw, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, + .expPath = "qed", + }; + const testFileData abs_qed = { .expBackingStore = canonraw, .expBackingStoreRaw = absraw, .expDirectory = datadir, .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .expPath = absqed, }; + #if HAVE_SYMLINK const testFileData link1_rel = { .expBackingStore = canonraw, @@ -437,6 +543,7 @@ mymain(void) .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .expPath = canonqcow2, }; const testFileData link1_abs = { .expBackingStore = canonraw, @@ -445,7 +552,9 @@ mymain(void) .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .expPath = canonqcow2, }; + const testFileData link2_rel = { .expBackingStore = canonqcow2, .expBackingStoreRaw = "../sub/link1", @@ -453,6 +562,7 @@ mymain(void) .expFormat = VIR_STORAGE_FILE_QCOW2, .expIsFile = true, .expCapacity = 1024, + .expPath = "sub/link2", }; const testFileData link2_abs = { .expBackingStore = canonqcow2, @@ -461,6 +571,7 @@ mymain(void) .expFormat = VIR_STORAGE_FILE_QCOW2, .expIsFile = true, .expCapacity = 1024, + .expPath = abslink2, }; #endif @@ -471,27 +582,27 @@ mymain(void) /* Raw image, whether with right format or no specified format */ TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW, - (&raw), EXP_PASS, - (&raw), ALLOW_PROBE | EXP_PASS, - (&raw), EXP_PASS, - (&raw), ALLOW_PROBE | EXP_PASS); + (&rel_raw), EXP_PASS, + (&rel_raw), ALLOW_PROBE | EXP_PASS, + (&abs_raw), EXP_PASS, + (&abs_raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(2, "raw", absraw, VIR_STORAGE_FILE_AUTO, - (&raw), EXP_PASS, - (&raw), ALLOW_PROBE | EXP_PASS, - (&raw), EXP_PASS, - (&raw), ALLOW_PROBE | EXP_PASS); + (&rel_raw), EXP_PASS, + (&rel_raw), ALLOW_PROBE | EXP_PASS, + (&abs_raw), EXP_PASS, + (&abs_raw), ALLOW_PROBE | EXP_PASS); /* Qcow2 file with relative raw backing, format provided */ TEST_CHAIN(3, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_relback_relstart, &raw), EXP_PASS, - (&qcow2_relback_relstart, &raw), ALLOW_PROBE | EXP_PASS, - (&qcow2_relback_absstart, &raw), EXP_PASS, - (&qcow2_relback_absstart, &raw), ALLOW_PROBE | EXP_PASS); + (&qcow2_relback_relstart, &abs_raw), EXP_PASS, + (&qcow2_relback_relstart, &abs_raw), ALLOW_PROBE | EXP_PASS, + (&qcow2_relback_absstart, &abs_raw), EXP_PASS, + (&qcow2_relback_absstart, &abs_raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(4, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - (&raw), EXP_PASS, - (&qcow2_relback_relstart, &raw), ALLOW_PROBE | EXP_PASS, - (&raw), EXP_PASS, - (&qcow2_relback_absstart, &raw), ALLOW_PROBE | EXP_PASS); + (&rel_qcow2_as_raw), EXP_PASS, + (&qcow2_relback_relstart, &abs_raw), ALLOW_PROBE | EXP_PASS, + (&abs_qcow2_as_raw), EXP_PASS, + (&qcow2_relback_absstart, &abs_raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 file to use absolute backing name */ virCommandFree(cmd); @@ -502,22 +613,22 @@ mymain(void) /* Qcow2 file with raw as absolute backing, backing format provided */ TEST_CHAIN(5, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_absback, &raw), EXP_PASS, - (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS, - (&qcow2_absback, &raw), EXP_PASS, - (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS); + (&rel_qcow2_absback, &abs_raw), EXP_PASS, + (&rel_qcow2_absback, &abs_raw), ALLOW_PROBE | EXP_PASS, + (&abs_qcow2_absback, &abs_raw), EXP_PASS, + (&abs_qcow2_absback, &abs_raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(6, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - (&raw), EXP_PASS, - (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS, - (&raw), EXP_PASS, - (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS); + (&rel_qcow2_as_raw), EXP_PASS, + (&rel_qcow2_absback, &abs_raw), ALLOW_PROBE | EXP_PASS, + (&abs_qcow2_as_raw), EXP_PASS, + (&abs_qcow2_absback, &abs_raw), ALLOW_PROBE | EXP_PASS); /* Wrapped file access */ TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap, &qcow2_absback, &raw), EXP_PASS, - (&wrap, &qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS, - (&wrap, &qcow2_absback, &raw), EXP_PASS, - (&wrap, &qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS); + (&rel_wrap, &abs_qcow2_absback, &abs_raw), EXP_PASS, + (&rel_wrap, &abs_qcow2_absback, &abs_raw), ALLOW_PROBE | EXP_PASS, + (&abs_wrap, &abs_qcow2_absback, &abs_raw), EXP_PASS, + (&abs_wrap, &abs_qcow2_absback, &abs_raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 and wrap file to omit backing file type */ virCommandFree(cmd); @@ -534,10 +645,10 @@ mymain(void) /* Qcow2 file with raw as absolute backing, backing format omitted */ TEST_CHAIN(8, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap_as_raw, &raw), EXP_PASS, - (&wrap_as_probe, &qcow2_as_probe, &raw), ALLOW_PROBE | EXP_PASS, - (&wrap_as_raw, &raw), EXP_PASS, - (&wrap_as_probe, &qcow2_as_probe, &raw), ALLOW_PROBE | EXP_PASS); + (&rel_wrap_as_raw, &abs_qcow2_as_raw), EXP_PASS, + (&rel_wrap_as_probe, &qcow2_as_probe, &abs_raw), ALLOW_PROBE | EXP_PASS, + (&abs_wrap_as_raw, &abs_qcow2_as_raw), EXP_PASS, + (&abs_wrap_as_probe, &qcow2_as_probe, &abs_raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 to a missing backing file, with backing type */ virCommandFree(cmd); @@ -549,10 +660,10 @@ mymain(void) /* Qcow2 file with missing backing file but specified type */ TEST_CHAIN(9, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_bogus), EXP_WARN, - (&qcow2_bogus), ALLOW_PROBE | EXP_WARN, - (&qcow2_bogus), EXP_WARN, - (&qcow2_bogus), ALLOW_PROBE | EXP_WARN); + (&rel_qcow2_bogus), EXP_WARN, + (&rel_qcow2_bogus), ALLOW_PROBE | EXP_WARN, + (&abs_qcow2_bogus), EXP_WARN, + (&abs_qcow2_bogus), ALLOW_PROBE | EXP_WARN); /* Rewrite qcow2 to a missing backing file, without backing type */ virCommandFree(cmd); @@ -563,10 +674,10 @@ mymain(void) /* Qcow2 file with missing backing file and no specified type */ TEST_CHAIN(10, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_bogus), EXP_WARN, - (&qcow2_bogus), ALLOW_PROBE | EXP_WARN, - (&qcow2_bogus), EXP_WARN, - (&qcow2_bogus), ALLOW_PROBE | EXP_WARN); + (&rel_qcow2_bogus), EXP_WARN, + (&rel_qcow2_bogus), ALLOW_PROBE | EXP_WARN, + (&abs_qcow2_bogus), EXP_WARN, + (&abs_qcow2_bogus), ALLOW_PROBE | EXP_WARN); /* Rewrite qcow2 to use an nbd: protocol as backend */ virCommandFree(cmd); @@ -578,17 +689,17 @@ mymain(void) /* Qcow2 file with backing protocol instead of file */ TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_protocol), EXP_PASS, - (&qcow2_protocol), ALLOW_PROBE | EXP_PASS, - (&qcow2_protocol), EXP_PASS, - (&qcow2_protocol), ALLOW_PROBE | EXP_PASS); + (&rel_qcow2_protocol), EXP_PASS, + (&rel_qcow2_protocol), ALLOW_PROBE | EXP_PASS, + (&abs_qcow2_protocol), EXP_PASS, + (&abs_qcow2_protocol), ALLOW_PROBE | EXP_PASS); /* qed file */ TEST_CHAIN(12, "qed", absqed, VIR_STORAGE_FILE_AUTO, - (&raw), EXP_PASS, - (&qed, &raw), ALLOW_PROBE | EXP_PASS, - (&raw), EXP_PASS, - (&qed, &raw), ALLOW_PROBE | EXP_PASS); + (&rel_qed_as_raw), EXP_PASS, + (&rel_qed, &abs_raw), ALLOW_PROBE | EXP_PASS, + (&abs_qed_as_raw), EXP_PASS, + (&abs_qed, &abs_raw), ALLOW_PROBE | EXP_PASS); #ifdef HAVE_SYMLINK /* Rewrite qcow2 and wrap file to use backing names relative to a @@ -608,10 +719,10 @@ mymain(void) /* Behavior of symlinks to qcow2 with relative backing files */ TEST_CHAIN(13, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, - (&link2_rel, &link1_rel, &raw), EXP_PASS, - (&link2_rel, &link1_rel, &raw), ALLOW_PROBE | EXP_PASS, - (&link2_abs, &link1_abs, &raw), EXP_PASS, - (&link2_abs, &link1_abs, &raw), ALLOW_PROBE | EXP_PASS); + (&link2_rel, &link1_rel, &abs_raw), EXP_PASS, + (&link2_rel, &link1_rel, &abs_raw), ALLOW_PROBE | EXP_PASS, + (&link2_abs, &link1_abs, &abs_raw), EXP_PASS, + (&link2_abs, &link1_abs, &abs_raw), ALLOW_PROBE | EXP_PASS); #endif /* Final cleanup */ testCleanupImages(); -- 1.9.0
On 04/04/14 06:32, Eric Blake wrote:
The testsuite is absolutely essential to feeling comfortable about swapping the backing chain structure over to a new format. This patch tests the path settings, and demonstrates that right now the code is passing only the canonical name to the child struct, which means more work is necessary in virstoragefile to pass the user spelling alongside the canonical name down to the child.
* tests/virstoragetest.c (testStorageChain): Test path. (mymain): Update expected data.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 239 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 175 insertions(+), 64 deletions(-)
Looks reasonable. ACK. Peter
participants (2)
-
Eric Blake -
Peter Krempa