11:32 p.m.
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
11:32 p.m.
New subject: [libvirt] [PATCH 1/5] tests: use C99 initialization for storage test
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(a)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
2:59 a.m.
New subject: [libvirt] [PATCH 1/5] tests: use C99 initialization for storage test
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(a)redhat.com>
---
tests/virstoragetest.c | 105 +++++++++++++++++++++++++++++++++++++------------
1 file changed, 80 insertions(+), 25 deletions(-)
ACK,
Peter
11:32 p.m.
New subject: [libvirt] [PATCH 2/5] tests: refactor virstoragetest for less stack space
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(a)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
3:54 a.m.
New subject: [libvirt] [PATCH 2/5] tests: refactor virstoragetest for less stack space
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(a)redhat.com>
---
tests/virstoragetest.c | 167 ++++++++++++++++++++++++-------------------------
1 file changed, 81 insertions(+), 86 deletions(-)
As it's test code: ACK
Peter
7:35 a.m.
New subject: [libvirt] [PATCH 2/5] tests: refactor virstoragetest for less stack space
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
11:32 p.m.
New subject: [libvirt] [PATCH 3/5] conf: track when storage type is still undetermined
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(a)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
3:59 a.m.
New subject: [libvirt] [PATCH 3/5] conf: track when storage type is still undetermined
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(a)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
11:32 p.m.
New subject: [libvirt] [PATCH 4/5] conf: track more fields in backing chain metadata
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(a)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
4:31 a.m.
New subject: [libvirt] [PATCH 4/5] conf: track more fields in backing chain metadata
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(a)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
7:54 a.m.
New subject: [libvirt] [PATCH 4/5] conf: track more fields in backing chain metadata
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
10:07 a.m.
New subject: [libvirt] [PATCH 4/5] conf: track more fields in backing chain metadata
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
11:32 p.m.
New subject: [libvirt] [PATCH 5/5] conf: start testing contents of the new backing chain fields
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(a)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
5:02 a.m.
New subject: [libvirt] [PATCH 5/5] conf: start testing contents of the new backing chain fields
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(a)redhat.com>
---
tests/virstoragetest.c | 239 ++++++++++++++++++++++++++++++++++++-------------
1 file changed, 175 insertions(+), 64 deletions(-)
Looks reasonable. ACK.
Peter
3915
days inactive
3915
days old
13 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa