[libvirt] [PATCH 0/5] more work on virstoragefile
More patches along my way towards exposing backing chains in domain XML. Eric Blake (5): conf: avoid memleak on NULL path conf: fix detection of infinite backing loop conf: test for more scenarios conf: interleave virstoragetest structs conf: another refactor of virstoragetest src/security/virt-aa-helper.c | 5 +- src/util/virstoragefile.c | 28 +++- src/util/virstoragefile.h | 3 +- tests/virstoragetest.c | 360 ++++++++++++++++++++++++------------------ 4 files changed, 230 insertions(+), 166 deletions(-) -- 1.9.0
I noticed that the apparmor code could request metadata even for a cdrom with no media, which would cause a memory leak of the hash table used to look for loops in the backing chain. But even before that, we blindly dereferenced the path for printing a debug statement, so it is just better to enforce that this is only used on non-NULL names. * src/util/virstoragefile.c (virStorageFileGetMetadata): Assume non-NULL path. * src/util/virstoragefile.h: Annotate this. * src/security/virt-aa-helper.c (get_files): Fix caller. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/security/virt-aa-helper.c | 5 ++++- src/util/virstoragefile.c | 4 ++-- src/util/virstoragefile.h | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 64a382c..7bddb2c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -943,13 +943,16 @@ get_files(vahControl * ctl) for (i = 0; i < ctl->def->ndisks; i++) { virDomainDiskDefPtr disk = ctl->def->disks[i]; + const char *src = virDomainDiskGetSource(disk); + if (!src) + continue; /* XXX - if we knew the qemu user:group here we could send it in * so that the open could be re-tried as that user:group. */ if (!disk->backingChain) { bool probe = ctl->allowDiskFormatProbing; - disk->backingChain = virStorageFileGetMetadata(virDomainDiskGetSource(disk), + disk->backingChain = virStorageFileGetMetadata(src, virDomainDiskGetFormat(disk), -1, -1, probe); } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 0e1461d..017717c 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1142,9 +1142,9 @@ virStorageFileGetMetadata(const char *path, int format, path, format, (int)uid, (int)gid, allow_probe); virHashTablePtr cycle = virHashCreate(5, NULL); - virStorageFileMetadataPtr ret; + virStorageFileMetadataPtr ret = NULL; - if (!cycle || !path) + if (!cycle) return NULL; if (format <= VIR_STORAGE_FILE_NONE) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3807285..83ec2bd 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -257,7 +257,8 @@ int virStorageFileProbeFormatFromBuf(const char *path, char *buf, virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, int format, uid_t uid, gid_t gid, - bool allow_probe); + bool allow_probe) + ATTRIBUTE_NONNULL(1); virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format); -- 1.9.0
On 04/06/14 05:32, Eric Blake wrote:
I noticed that the apparmor code could request metadata even for a cdrom with no media, which would cause a memory leak of the hash table used to look for loops in the backing chain. But even before that, we blindly dereferenced the path for printing a debug statement, so it is just better to enforce that this is only used on non-NULL names.
* src/util/virstoragefile.c (virStorageFileGetMetadata): Assume non-NULL path. * src/util/virstoragefile.h: Annotate this. * src/security/virt-aa-helper.c (get_files): Fix caller.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/security/virt-aa-helper.c | 5 ++++- src/util/virstoragefile.c | 4 ++-- src/util/virstoragefile.h | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 64a382c..7bddb2c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -943,13 +943,16 @@ get_files(vahControl * ctl)
for (i = 0; i < ctl->def->ndisks; i++) { virDomainDiskDefPtr disk = ctl->def->disks[i]; + const char *src = virDomainDiskGetSource(disk);
+ if (!src) + continue; /* XXX - if we knew the qemu user:group here we could send it in * so that the open could be re-tried as that user:group. */ if (!disk->backingChain) { bool probe = ctl->allowDiskFormatProbing; - disk->backingChain = virStorageFileGetMetadata(virDomainDiskGetSource(disk), + disk->backingChain = virStorageFileGetMetadata(src, virDomainDiskGetFormat(disk), -1, -1, probe); } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 0e1461d..017717c 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1142,9 +1142,9 @@ virStorageFileGetMetadata(const char *path, int format, path, format, (int)uid, (int)gid, allow_probe);
virHashTablePtr cycle = virHashCreate(5, NULL); - virStorageFileMetadataPtr ret; + virStorageFileMetadataPtr ret = NULL;
This assignment will always be overwritten or unused.
- if (!cycle || !path) + if (!cycle) return NULL;
if (format <= VIR_STORAGE_FILE_NONE)
ACK, Peter
On 04/07/2014 02:22 AM, Peter Krempa wrote:
On 04/06/14 05:32, Eric Blake wrote:
I noticed that the apparmor code could request metadata even for a cdrom with no media, which would cause a memory leak of the hash table used to look for loops in the backing chain. But even before that, we blindly dereferenced the path for printing a debug statement, so it is just better to enforce that this is only used on non-NULL names.
+++ b/src/util/virstoragefile.c @@ -1142,9 +1142,9 @@ virStorageFileGetMetadata(const char *path, int format, path, format, (int)uid, (int)gid, allow_probe);
virHashTablePtr cycle = virHashCreate(5, NULL); - virStorageFileMetadataPtr ret; + virStorageFileMetadataPtr ret = NULL;
This assignment will always be overwritten or unused.
Leftovers from a different approach I had tried. I dropped this change...
- if (!cycle || !path) + if (!cycle) return NULL;
if (format <= VIR_STORAGE_FILE_NONE)
ACK,
and pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
While trying to refactor the backing file chain, I noticed that if you have a self-referential qcow2 file via a relative name: qemu-img create -f qcow2 loop 10M qemu-img rebase -u -f qcow2 -F qcow2 -b loop loop then libvirt was creating a chain 2 deep before realizing it had hit a loop; furthermore, virStorageFileChainCheckBroken was not identifying the chain as broken. With this patch, the loop is detected when the chain is only 1 deep; still enough for storage volume XML to display the file, but now with a proper error report about where the loop was found. * src/util/virstoragefile.c (virStorageFileGetMetadataRecurse): Mark chain broken if OOM or infinite loop occurs. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 017717c..39e4c19 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1074,15 +1074,21 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory, path, format, (int)uid, (int)gid, allow_probe); virStorageFileMetadataPtr ret = NULL; + char *canonical = canonicalize_file_name(path); - if (virHashLookup(cycle, path)) { + /* Hash by canonical name */ + if (!canonical) { + virReportOOMError(); + goto cleanup; + } + if (virHashLookup(cycle, canonical)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store for %s is self-referential"), path); - return NULL; + goto cleanup; } - if (virHashAddEntry(cycle, path, (void *)1) < 0) - return NULL; + if (virHashAddEntry(cycle, canonical, (void *)1) < 0) + goto cleanup; if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), path); @@ -1106,8 +1112,15 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory, uid, gid, allow_probe, cycle); + if (!ret->backingMeta) { + /* If we failed to get backing data, mark the chain broken */ + ret->backingStoreFormat = VIR_STORAGE_FILE_NONE; + VIR_FREE(ret->backingStore); + } } + cleanup: + VIR_FREE(canonical); return ret; } @@ -1176,7 +1189,8 @@ virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, tmp = chain; while (tmp) { - /* Break if no backing store or backing store is not file */ + /* Break if no backing store, backing store is not file, or + * other problem such as infinite loop */ if (!tmp->backingStoreRaw) break; if (!tmp->backingStore) { -- 1.9.0
On 04/06/14 05:32, Eric Blake wrote:
While trying to refactor the backing file chain, I noticed that if you have a self-referential qcow2 file via a relative name:
qemu-img create -f qcow2 loop 10M qemu-img rebase -u -f qcow2 -F qcow2 -b loop loop
then libvirt was creating a chain 2 deep before realizing it had hit a loop; furthermore, virStorageFileChainCheckBroken was not identifying the chain as broken. With this patch, the loop is detected when the chain is only 1 deep; still enough for storage volume XML to display the file, but now with a proper error report about where the loop was found.
* src/util/virstoragefile.c (virStorageFileGetMetadataRecurse): Mark chain broken if OOM or infinite loop occurs.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 017717c..39e4c19 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1074,15 +1074,21 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory,
You are adding this code into the recursive worker. It's possible only in the first iteration here to get a non-canonical "path" here, as all other backing chain elements are prepped with virFindBackingFile.
path, format, (int)uid, (int)gid, allow_probe);
virStorageFileMetadataPtr ret = NULL; + char *canonical = canonicalize_file_name(path);
Thus this call is redundant on all other iterations than the first.
- if (virHashLookup(cycle, path)) { + /* Hash by canonical name */ + if (!canonical) { + virReportOOMError(); + goto cleanup; + } + if (virHashLookup(cycle, canonical)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store for %s is self-referential"), path); - return NULL; + goto cleanup; } - if (virHashAddEntry(cycle, path, (void *)1) < 0) - return NULL; + if (virHashAddEntry(cycle, canonical, (void *)1) < 0) + goto cleanup;
if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), path);
Please move the canonicalization code into virStorageFileGetMetadata so that it doesn't get called multiple times. (And also one of my planed refactors need to change this function to track remote images too and this would definitely be changed afterwards). Peter
While trying to refactor the backing file chain, I noticed that if you have a self-referential qcow2 file via a relative name: qemu-img create -f qcow2 loop 10M qemu-img rebase -u -f qcow2 -F qcow2 -b loop loop then libvirt was creating a chain 2 deep before realizing it had hit a loop; furthermore, virStorageFileChainCheckBroken was not identifying the chain as broken. With this patch, the loop is detected when the chain is only 1 deep; still enough for storage volume XML to display the file, but now with a proper error report about where the loop was found. * src/util/virstoragefile.c (virStorageFileGetMetadataRecurse): Add parameter, require canonical path up front. Mark chain broken on OOM or loop detection. (virStorageFileGetMetadata): Pass in canonical name. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: hoist canonical computation out of virStorageFileGetMetadataRecurse rest of patch series still works as is src/util/virstoragefile.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3cdc89c..413ea5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1065,26 +1065,27 @@ virStorageFileGetMetadataFromFD(const char *path, /* Recursive workhorse for virStorageFileGetMetadata. */ static virStorageFileMetadataPtr -virStorageFileGetMetadataRecurse(const char *path, const char *directory, +virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, + const char *directory, int format, uid_t uid, gid_t gid, bool allow_probe, virHashTablePtr cycle) { int fd; - VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", - path, format, (int)uid, (int)gid, allow_probe); + VIR_DEBUG("path=%s canonPath=%s format=%d uid=%d gid=%d probe=%d", + path, canonPath, format, (int)uid, (int)gid, allow_probe); virStorageFileMetadataPtr ret = NULL; - if (virHashLookup(cycle, path)) { + if (virHashLookup(cycle, canonPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store for %s is self-referential"), path); return NULL; } - if (virHashAddEntry(cycle, path, (void *)1) < 0) + if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) return NULL; - if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { + if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), path); return NULL; } @@ -1100,14 +1101,19 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory, else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO; format = ret->backingStoreFormat; - ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStore, + ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStoreRaw, + ret->backingStore, ret->directory, format, uid, gid, allow_probe, cycle); + if (!ret->backingMeta) { + /* If we failed to get backing data, mark the chain broken */ + ret->backingStoreFormat = VIR_STORAGE_FILE_NONE; + VIR_FREE(ret->backingStore); + } } - return ret; } @@ -1142,15 +1148,23 @@ virStorageFileGetMetadata(const char *path, int format, path, format, (int)uid, (int)gid, allow_probe); virHashTablePtr cycle = virHashCreate(5, NULL); - virStorageFileMetadataPtr ret; + virStorageFileMetadataPtr ret = NULL; + char *canonPath; if (!cycle) return NULL; + if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + goto cleanup; + } + if (format <= VIR_STORAGE_FILE_NONE) format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(path, NULL, format, uid, gid, - allow_probe, cycle); + ret = virStorageFileGetMetadataRecurse(path, canonPath, NULL, format, + uid, gid, allow_probe, cycle); + cleanup: + VIR_FREE(canonPath); virHashFree(cycle); return ret; } @@ -1176,7 +1190,8 @@ virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, tmp = chain; while (tmp) { - /* Break if no backing store or backing store is not file */ + /* Break if no backing store, backing store is not file, or + * other problem such as infinite loop */ if (!tmp->backingStoreRaw) break; if (!tmp->backingStore) { -- 1.9.0
On 04/08/14 06:07, Eric Blake wrote:
While trying to refactor the backing file chain, I noticed that if you have a self-referential qcow2 file via a relative name:
qemu-img create -f qcow2 loop 10M qemu-img rebase -u -f qcow2 -F qcow2 -b loop loop
then libvirt was creating a chain 2 deep before realizing it had hit a loop; furthermore, virStorageFileChainCheckBroken was not identifying the chain as broken. With this patch, the loop is detected when the chain is only 1 deep; still enough for storage volume XML to display the file, but now with a proper error report about where the loop was found.
* src/util/virstoragefile.c (virStorageFileGetMetadataRecurse): Add parameter, require canonical path up front. Mark chain broken on OOM or loop detection. (virStorageFileGetMetadata): Pass in canonical name.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
v2: hoist canonical computation out of virStorageFileGetMetadataRecurse rest of patch series still works as is
src/util/virstoragefile.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3cdc89c..413ea5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1065,26 +1065,27 @@ virStorageFileGetMetadataFromFD(const char *path,
/* Recursive workhorse for virStorageFileGetMetadata. */ static virStorageFileMetadataPtr -virStorageFileGetMetadataRecurse(const char *path, const char *directory, +virStorageFileGetMetadataRecurse(const char *path, const char *canonPath,
Hmm, I think that passing in canonical path isn't necessary here. The path argument is from the second iteration canonicalized by the metadata retrieval function. So to avoid the bug you IMO just need to canonicalize the first call of this function. It's true that it would actually change the path as reported by debug/error messages but I don't think that should be problematic.
+ const char *directory, int format, uid_t uid, gid_t gid, bool allow_probe, virHashTablePtr cycle) { int fd; - VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", - path, format, (int)uid, (int)gid, allow_probe); + VIR_DEBUG("path=%s canonPath=%s format=%d uid=%d gid=%d probe=%d", + path, canonPath, format, (int)uid, (int)gid, allow_probe);
virStorageFileMetadataPtr ret = NULL;
- if (virHashLookup(cycle, path)) { + if (virHashLookup(cycle, canonPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store for %s is self-referential"), path); return NULL; } - if (virHashAddEntry(cycle, path, (void *)1) < 0) + if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) return NULL;
- if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { + if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), path); return NULL; } @@ -1100,14 +1101,19 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory, else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO; format = ret->backingStoreFormat; - ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStore, + ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStoreRaw, + ret->backingStore, ret->directory, format, uid, gid, allow_probe, cycle); + if (!ret->backingMeta) { + /* If we failed to get backing data, mark the chain broken */ + ret->backingStoreFormat = VIR_STORAGE_FILE_NONE; + VIR_FREE(ret->backingStore); + } } - return ret; }
@@ -1142,15 +1148,23 @@ virStorageFileGetMetadata(const char *path, int format, path, format, (int)uid, (int)gid, allow_probe);
virHashTablePtr cycle = virHashCreate(5, NULL); - virStorageFileMetadataPtr ret; + virStorageFileMetadataPtr ret = NULL; + char *canonPath;
if (!cycle) return NULL;
+ if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + goto cleanup; + } + if (format <= VIR_STORAGE_FILE_NONE) format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(path, NULL, format, uid, gid, - allow_probe, cycle); + ret = virStorageFileGetMetadataRecurse(path, canonPath, NULL, format,
... Here I'd change path for canonPath and would not refactor the rest.
+ uid, gid, allow_probe, cycle); + cleanup: + VIR_FREE(canonPath); virHashFree(cycle); return ret; }
Is there any reason you chose to add the parameter? Peter
On 04/08/2014 06:35 AM, Peter Krempa wrote:
On 04/08/14 06:07, Eric Blake wrote:
While trying to refactor the backing file chain, I noticed that if you have a self-referential qcow2 file via a relative name:
qemu-img create -f qcow2 loop 10M qemu-img rebase -u -f qcow2 -F qcow2 -b loop loop
then libvirt was creating a chain 2 deep before realizing it had hit a loop; furthermore, virStorageFileChainCheckBroken was not identifying the chain as broken. With this patch, the loop is detected when the chain is only 1 deep; still enough for storage volume XML to display the file, but now with a proper error report about where the loop was found.
* src/util/virstoragefile.c (virStorageFileGetMetadataRecurse): Add parameter, require canonical path up front. Mark chain broken on OOM or loop detection. (virStorageFileGetMetadata): Pass in canonical name.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
v2: hoist canonical computation out of virStorageFileGetMetadataRecurse rest of patch series still works as is
/* Recursive workhorse for virStorageFileGetMetadata. */ static virStorageFileMetadataPtr -virStorageFileGetMetadataRecurse(const char *path, const char *directory, +virStorageFileGetMetadataRecurse(const char *path, const char *canonPath,
Hmm, I think that passing in canonical path isn't necessary here. The path argument is from the second iteration canonicalized by the metadata retrieval function. So to avoid the bug you IMO just need to canonicalize the first call of this function. It's true that it would actually change the path as reported by debug/error messages but I don't think that should be problematic.
I actually want to pass BOTH names, through the ENTIRE call stack - the short name for error reporting, but the canonical name for open() operations. The short name may be relative, but it is relative to the parent and NOT the current working directory, so it is unsuitable for open(); but the canonical name resolves symlinks and and is therefore unsuitable for easy correlation back to the names specified by the user or the qcow2 metadata. I've got another patch coming that updates the entire call stack to use both names, not just the virStorageFileGetMetadataRecurse() function.
if (format <= VIR_STORAGE_FILE_NONE) format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(path, NULL, format, uid, gid, - allow_probe, cycle); + ret = virStorageFileGetMetadataRecurse(path, canonPath, NULL, format,
... Here I'd change path for canonPath and would not refactor the rest.
+ uid, gid, allow_probe, cycle); + cleanup: + VIR_FREE(canonPath); virHashFree(cycle); return ret; }
Is there any reason you chose to add the parameter?
I could pass just canonPath here, to avoid the signature change in this patch, and save the entire signature change for the coming patch that passes both names down the entire call stack. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/08/14 14:39, Eric Blake wrote:
On 04/08/2014 06:35 AM, Peter Krempa wrote:
On 04/08/14 06:07, Eric Blake wrote:
While trying to refactor the backing file chain, I noticed that if you have a self-referential qcow2 file via a relative name:
qemu-img create -f qcow2 loop 10M qemu-img rebase -u -f qcow2 -F qcow2 -b loop loop
then libvirt was creating a chain 2 deep before realizing it had hit a loop; furthermore, virStorageFileChainCheckBroken was not identifying the chain as broken. With this patch, the loop is detected when the chain is only 1 deep; still enough for storage volume XML to display the file, but now with a proper error report about where the loop was found.
* src/util/virstoragefile.c (virStorageFileGetMetadataRecurse): Add parameter, require canonical path up front. Mark chain broken on OOM or loop detection. (virStorageFileGetMetadata): Pass in canonical name.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
v2: hoist canonical computation out of virStorageFileGetMetadataRecurse rest of patch series still works as is
/* Recursive workhorse for virStorageFileGetMetadata. */ static virStorageFileMetadataPtr -virStorageFileGetMetadataRecurse(const char *path, const char *directory, +virStorageFileGetMetadataRecurse(const char *path, const char *canonPath,
Hmm, I think that passing in canonical path isn't necessary here. The path argument is from the second iteration canonicalized by the metadata retrieval function. So to avoid the bug you IMO just need to canonicalize the first call of this function. It's true that it would actually change the path as reported by debug/error messages but I don't think that should be problematic.
I actually want to pass BOTH names, through the ENTIRE call stack - the short name for error reporting, but the canonical name for open() operations. The short name may be relative, but it is relative to the parent and NOT the current working directory, so it is unsuitable for open(); but the canonical name resolves symlinks and and is therefore unsuitable for easy correlation back to the names specified by the user or the qcow2 metadata. I've got another patch coming that updates the entire call stack to use both names, not just the virStorageFileGetMetadataRecurse() function.
if (format <= VIR_STORAGE_FILE_NONE) format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(path, NULL, format, uid, gid, - allow_probe, cycle); + ret = virStorageFileGetMetadataRecurse(path, canonPath, NULL, format,
... Here I'd change path for canonPath and would not refactor the rest.
+ uid, gid, allow_probe, cycle); + cleanup: + VIR_FREE(canonPath); virHashFree(cycle); return ret; }
Is there any reason you chose to add the parameter?
I could pass just canonPath here, to avoid the signature change in this patch, and save the entire signature change for the coming patch that passes both names down the entire call stack.
Ah, okay then. ACK. Peter
Part of the upcoming refactoring will change how broken chains are detected; it makes sense to test that this works. In particular, test the just-fixed infinite loop detection bug. Also, make sure that detection of directories is sane. * tests/virstoragetest.c (testStorageChain): Enhance test. (mymain): Add more tests. (testCleanupImages, testPrepImages): Populate a directory. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 4 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index efd920a..a25f91f 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -57,6 +57,7 @@ static char *absqcow2; static char *canonqcow2; static char *abswrap; static char *absqed; +static char *absdir; static char *abslink2; static void @@ -69,6 +70,7 @@ testCleanupImages(void) VIR_FREE(canonqcow2); VIR_FREE(abswrap); VIR_FREE(absqed); + VIR_FREE(absdir); VIR_FREE(abslink2); if (chdir(abs_builddir) < 0) { @@ -110,6 +112,7 @@ testPrepImages(void) virAsprintf(&absqcow2, "%s/qcow2", datadir) < 0 || virAsprintf(&abswrap, "%s/wrap", datadir) < 0 || virAsprintf(&absqed, "%s/qed", datadir) < 0 || + virAsprintf(&absdir, "%s/dir", datadir) < 0 || virAsprintf(&abslink2, "%s/sub/link2", datadir) < 0) goto cleanup; @@ -117,6 +120,10 @@ testPrepImages(void) fprintf(stderr, "unable to create directory %s\n", datadir "/sub"); goto cleanup; } + if (virFileMakePath(datadir "/dir") < 0) { + fprintf(stderr, "unable to create directory %s\n", datadir "/dir"); + goto cleanup; + } if (chdir(datadir) < 0) { fprintf(stderr, "unable to test relative backing chains\n"); @@ -231,6 +238,7 @@ testStorageChain(const void *args) virStorageFileMetadataPtr meta; virStorageFileMetadataPtr elt; size_t i = 0; + char *broken = NULL; meta = virStorageFileGetMetadata(data->start, data->format, -1, -1, (data->flags & ALLOW_PROBE) != 0); @@ -250,9 +258,19 @@ testStorageChain(const void *args) goto cleanup; } virResetLastError(); - } else if (virGetLastError()) { - fprintf(stderr, "call should not have warned\n"); - goto cleanup; + if (virStorageFileChainGetBroken(meta, &broken) || !broken) { + fprintf(stderr, "call should identify broken part of chain\n"); + goto cleanup; + } + } else { + if (virGetLastError()) { + fprintf(stderr, "call should not have warned\n"); + goto cleanup; + } + if (virStorageFileChainGetBroken(meta, &broken) || broken) { + fprintf(stderr, "chain should not be identified as broken\n"); + goto cleanup; + } } elt = meta; @@ -303,6 +321,7 @@ testStorageChain(const void *args) ret = 0; cleanup: + VIR_FREE(broken); virStorageFileFreeMetadata(meta); return ret; } @@ -429,6 +448,40 @@ mymain(void) .expIsFile = true, .expCapacity = 1024, }; + + const testFileData dir = { + .expIsFile = false, + }; + + const testFileData qcow2_loop1_rel = { + .expBackingStoreRaw = "qcow2", + .expDirectory = ".", + .expFormat = VIR_STORAGE_FILE_NONE, + .expIsFile = true, + .expCapacity = 1024, + }; + const testFileData qcow2_loop1_abs = { + .expBackingStoreRaw = "qcow2", + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_NONE, + .expIsFile = true, + .expCapacity = 1024, + }; + const testFileData qcow2_loop2_rel = { + .expBackingStoreRaw = "wrap", + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_NONE, + .expIsFile = true, + .expCapacity = 1024, + }; + const testFileData qcow2_loop2_abs = { + .expBackingStoreRaw = "wrap", + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_NONE, + .expIsFile = true, + .expCapacity = 1024, + }; + #if HAVE_SYMLINK const testFileData link1_rel = { .expBackingStore = canonraw, @@ -590,6 +643,18 @@ mymain(void) (&raw), EXP_PASS, (&qed, &raw), ALLOW_PROBE | EXP_PASS); + /* directory */ + TEST_CHAIN(13, "dir", absdir, VIR_STORAGE_FILE_AUTO, + (&dir), EXP_PASS, + (&dir), ALLOW_PROBE | EXP_PASS, + (&dir), EXP_PASS, + (&dir), ALLOW_PROBE | EXP_PASS); + TEST_CHAIN(14, "dir", absdir, VIR_STORAGE_FILE_DIR, + (&dir), EXP_PASS, + (&dir), ALLOW_PROBE | EXP_PASS, + (&dir), EXP_PASS, + (&dir), ALLOW_PROBE | EXP_PASS); + #ifdef HAVE_SYMLINK /* Rewrite qcow2 and wrap file to use backing names relative to a * symlink from a different directory */ @@ -607,12 +672,47 @@ mymain(void) ret = -1; /* Behavior of symlinks to qcow2 with relative backing files */ - TEST_CHAIN(13, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, + TEST_CHAIN(15, "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); #endif + + /* Rewrite qcow2 to be a self-referential loop */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", "qcow2", "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Behavior of an infinite loop chain */ + TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, + (&qcow2_loop1_rel), EXP_WARN, + (&qcow2_loop1_rel), ALLOW_PROBE | EXP_WARN, + (&qcow2_loop1_abs), EXP_WARN, + (&qcow2_loop1_abs), ALLOW_PROBE | EXP_WARN); + + /* Rewrite wrap and qcow2 to be mutually-referential loop */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", "wrap", "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", absqcow2, "wrap", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Behavior of an infinite loop chain */ + TEST_CHAIN(17, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, + (&wrap, &qcow2_loop2_rel), EXP_WARN, + (&wrap, &qcow2_loop2_rel), ALLOW_PROBE | EXP_WARN, + (&wrap, &qcow2_loop2_abs), EXP_WARN, + (&wrap, &qcow2_loop2_abs), ALLOW_PROBE | EXP_WARN); + /* Final cleanup */ testCleanupImages(); virCommandFree(cmd); -- 1.9.0
On 04/06/14 05:32, Eric Blake wrote:
Part of the upcoming refactoring will change how broken chains are detected; it makes sense to test that this works. In particular, test the just-fixed infinite loop detection bug. Also, make sure that detection of directories is sane.
* tests/virstoragetest.c (testStorageChain): Enhance test. (mymain): Add more tests. (testCleanupImages, testPrepImages): Populate a directory.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 4 deletions(-)
ACK, Peter
As I add more tests, it's getting harder to follow the split between a struct in one place and a test using the struct in another. Interleaving the tests makes changes more localized, and also makes debugging easier when a test goes wrong during my refactoring work. * tests/virstoragetest.c (mymain): Modify structs as we go, rather than up-front. (testStorageChain): Make failure debugging easier. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 331 ++++++++++++++++++++++--------------------------- 1 file changed, 145 insertions(+), 186 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index a25f91f..938f878 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -304,6 +304,7 @@ testStorageChain(const void *args) goto cleanup; } if (STRNEQ(expect, actual)) { + fprintf(stderr, "chain member %zu", i); virtTestDifference(stderr, expect, actual); VIR_FREE(expect); VIR_FREE(actual); @@ -369,160 +370,15 @@ mymain(void) VIR_FLATTEN_1(chain4)); \ } while (0) - /* Expected details about files in chains */ - const testFileData raw = { - .expFormat = VIR_STORAGE_FILE_NONE, - }; - const testFileData qcow2_relback_relstart = { - .expBackingStore = canonraw, - .expBackingStoreRaw = "raw", - .expDirectory = ".", - .expFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData qcow2_relback_absstart = { - .expBackingStore = canonraw, - .expBackingStoreRaw = "raw", - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData qcow2_absback = { - .expBackingStore = canonraw, - .expBackingStoreRaw = absraw, - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData qcow2_as_probe = { - .expBackingStore = canonraw, - .expBackingStoreRaw = absraw, - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_AUTO, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData qcow2_bogus = { - .expBackingStoreRaw = datadir "/bogus", - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_NONE, - .expCapacity = 1024, - }; - const testFileData qcow2_protocol = { - .expBackingStore = "nbd:example.org:6000", - .expFormat = VIR_STORAGE_FILE_RAW, - .expCapacity = 1024, - }; - const testFileData wrap = { - .expBackingStore = canonqcow2, - .expBackingStoreRaw = absqcow2, - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_QCOW2, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData wrap_as_raw = { - .expBackingStore = canonqcow2, - .expBackingStoreRaw = absqcow2, - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData wrap_as_probe = { - .expBackingStore = canonqcow2, - .expBackingStoreRaw = absqcow2, - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_AUTO, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData qed = { - .expBackingStore = canonraw, - .expBackingStoreRaw = absraw, - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, - .expCapacity = 1024, - }; - - const testFileData dir = { - .expIsFile = false, - }; - - const testFileData qcow2_loop1_rel = { - .expBackingStoreRaw = "qcow2", - .expDirectory = ".", - .expFormat = VIR_STORAGE_FILE_NONE, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData qcow2_loop1_abs = { - .expBackingStoreRaw = "qcow2", - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_NONE, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData qcow2_loop2_rel = { - .expBackingStoreRaw = "wrap", - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_NONE, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData qcow2_loop2_abs = { - .expBackingStoreRaw = "wrap", - .expDirectory = datadir, - .expFormat = VIR_STORAGE_FILE_NONE, - .expIsFile = true, - .expCapacity = 1024, - }; - -#if HAVE_SYMLINK - const testFileData link1_rel = { - .expBackingStore = canonraw, - .expBackingStoreRaw = "../raw", - .expDirectory = "sub/../sub/..", - .expFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData link1_abs = { - .expBackingStore = canonraw, - .expBackingStoreRaw = "../raw", - .expDirectory = datadir "/sub/../sub/..", - .expFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData link2_rel = { - .expBackingStore = canonqcow2, - .expBackingStoreRaw = "../sub/link1", - .expDirectory = "sub/../sub", - .expFormat = VIR_STORAGE_FILE_QCOW2, - .expIsFile = true, - .expCapacity = 1024, - }; - const testFileData link2_abs = { - .expBackingStore = canonqcow2, - .expBackingStoreRaw = "../sub/link1", - .expDirectory = datadir "/sub/../sub", - .expFormat = VIR_STORAGE_FILE_QCOW2, - .expIsFile = true, - .expCapacity = 1024, - }; -#endif - /* The actual tests, in several groups. */ /* Missing file */ TEST_ONE_CHAIN("0", "bogus", VIR_STORAGE_FILE_RAW, EXP_FAIL); /* Raw image, whether with right format or no specified format */ + testFileData raw = { + .expFormat = VIR_STORAGE_FILE_NONE, + }; TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW, (&raw), EXP_PASS, (&raw), ALLOW_PROBE | EXP_PASS, @@ -535,16 +391,32 @@ mymain(void) (&raw), ALLOW_PROBE | EXP_PASS); /* Qcow2 file with relative raw backing, format provided */ + testFileData qcow2_relstart = { + .expBackingStore = canonraw, + .expBackingStoreRaw = "raw", + .expDirectory = ".", + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, + }; + testFileData qcow2_absstart = { + .expBackingStore = canonraw, + .expBackingStoreRaw = "raw", + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, + }; 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_relstart, &raw), EXP_PASS, + (&qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, + (&qcow2_absstart, &raw), EXP_PASS, + (&qcow2_absstart, &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, + (&qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, (&raw), EXP_PASS, - (&qcow2_relback_absstart, &raw), ALLOW_PROBE | EXP_PASS); + (&qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 file to use absolute backing name */ virCommandFree(cmd); @@ -552,25 +424,36 @@ mymain(void) "-F", "raw", "-b", absraw, "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; + qcow2_relstart.expBackingStoreRaw = absraw; + qcow2_relstart.expDirectory = datadir; + qcow2_absstart.expBackingStoreRaw = absraw; /* 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); + (&qcow2_relstart, &raw), EXP_PASS, + (&qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, + (&qcow2_absstart, &raw), EXP_PASS, + (&qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(6, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, (&raw), EXP_PASS, - (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS, + (&qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, (&raw), EXP_PASS, - (&qcow2_absback, &raw), ALLOW_PROBE | EXP_PASS); + (&qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); /* Wrapped file access */ + testFileData wrap = { + .expBackingStore = canonqcow2, + .expBackingStoreRaw = absqcow2, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_QCOW2, + .expIsFile = true, + .expCapacity = 1024, + }; 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); + (&wrap, &qcow2_relstart, &raw), EXP_PASS, + (&wrap, &qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, + (&wrap, &qcow2_absstart, &raw), EXP_PASS, + (&wrap, &qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 and wrap file to omit backing file type */ virCommandFree(cmd); @@ -584,13 +467,24 @@ mymain(void) "-b", absqcow2, "wrap", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; + wrap.expFormat = VIR_STORAGE_FILE_AUTO; + qcow2_relstart.expFormat = VIR_STORAGE_FILE_AUTO; + qcow2_absstart.expFormat = VIR_STORAGE_FILE_AUTO; /* Qcow2 file with raw as absolute backing, backing format omitted */ + testFileData wrap_as_raw = { + .expBackingStore = canonqcow2, + .expBackingStoreRaw = absqcow2, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, + }; 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, &qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, (&wrap_as_raw, &raw), EXP_PASS, - (&wrap_as_probe, &qcow2_as_probe, &raw), ALLOW_PROBE | EXP_PASS); + (&wrap, &qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 to a missing backing file, with backing type */ virCommandFree(cmd); @@ -599,13 +493,17 @@ mymain(void) "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; + qcow2_absstart.expBackingStore = NULL; + qcow2_absstart.expBackingStoreRaw = datadir "/bogus"; + qcow2_absstart.expFormat = VIR_STORAGE_FILE_NONE; + qcow2_absstart.expIsFile = false; /* 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); + (&qcow2_absstart), EXP_WARN, + (&qcow2_absstart), ALLOW_PROBE | EXP_WARN, + (&qcow2_absstart), EXP_WARN, + (&qcow2_absstart), ALLOW_PROBE | EXP_WARN); /* Rewrite qcow2 to a missing backing file, without backing type */ virCommandFree(cmd); @@ -616,10 +514,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); + (&qcow2_absstart), EXP_WARN, + (&qcow2_absstart), ALLOW_PROBE | EXP_WARN, + (&qcow2_absstart), EXP_WARN, + (&qcow2_absstart), ALLOW_PROBE | EXP_WARN); /* Rewrite qcow2 to use an nbd: protocol as backend */ virCommandFree(cmd); @@ -628,15 +526,27 @@ mymain(void) "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; + qcow2_absstart.expBackingStore = "nbd:example.org:6000"; + qcow2_absstart.expBackingStoreRaw = NULL; + qcow2_absstart.expDirectory = NULL; + qcow2_absstart.expFormat = VIR_STORAGE_FILE_RAW; /* 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); + (&qcow2_absstart), EXP_PASS, + (&qcow2_absstart), ALLOW_PROBE | EXP_PASS, + (&qcow2_absstart), EXP_PASS, + (&qcow2_absstart), ALLOW_PROBE | EXP_PASS); /* qed file */ + testFileData qed = { + .expBackingStore = canonraw, + .expBackingStoreRaw = absraw, + .expDirectory = datadir, + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, + }; TEST_CHAIN(12, "qed", absqed, VIR_STORAGE_FILE_AUTO, (&raw), EXP_PASS, (&qed, &raw), ALLOW_PROBE | EXP_PASS, @@ -644,6 +554,9 @@ mymain(void) (&qed, &raw), ALLOW_PROBE | EXP_PASS); /* directory */ + testFileData dir = { + .expIsFile = false, + }; TEST_CHAIN(13, "dir", absdir, VIR_STORAGE_FILE_AUTO, (&dir), EXP_PASS, (&dir), ALLOW_PROBE | EXP_PASS, @@ -672,6 +585,38 @@ mymain(void) ret = -1; /* Behavior of symlinks to qcow2 with relative backing files */ + testFileData link1_rel = { + .expBackingStore = canonraw, + .expBackingStoreRaw = "../raw", + .expDirectory = "sub/../sub/..", + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, + }; + testFileData link1_abs = { + .expBackingStore = canonraw, + .expBackingStoreRaw = "../raw", + .expDirectory = datadir "/sub/../sub/..", + .expFormat = VIR_STORAGE_FILE_RAW, + .expIsFile = true, + .expCapacity = 1024, + }; + testFileData link2_rel = { + .expBackingStore = canonqcow2, + .expBackingStoreRaw = "../sub/link1", + .expDirectory = "sub/../sub", + .expFormat = VIR_STORAGE_FILE_QCOW2, + .expIsFile = true, + .expCapacity = 1024, + }; + testFileData link2_abs = { + .expBackingStore = canonqcow2, + .expBackingStoreRaw = "../sub/link1", + .expDirectory = datadir "/sub/../sub", + .expFormat = VIR_STORAGE_FILE_QCOW2, + .expIsFile = true, + .expCapacity = 1024, + }; TEST_CHAIN(15, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, (&link2_rel, &link1_rel, &raw), EXP_PASS, (&link2_rel, &link1_rel, &raw), ALLOW_PROBE | EXP_PASS, @@ -685,13 +630,23 @@ mymain(void) "-F", "qcow2", "-b", "qcow2", "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; + qcow2_relstart.expBackingStore = NULL; + qcow2_relstart.expBackingStoreRaw = "qcow2"; + qcow2_relstart.expDirectory = "."; + qcow2_relstart.expFormat= VIR_STORAGE_FILE_NONE; + qcow2_relstart.expIsFile = true; + qcow2_absstart.expBackingStore = NULL; + qcow2_absstart.expBackingStoreRaw = "qcow2"; + qcow2_absstart.expDirectory = datadir; + qcow2_absstart.expFormat= VIR_STORAGE_FILE_NONE; + qcow2_absstart.expIsFile = true; /* Behavior of an infinite loop chain */ TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_loop1_rel), EXP_WARN, - (&qcow2_loop1_rel), ALLOW_PROBE | EXP_WARN, - (&qcow2_loop1_abs), EXP_WARN, - (&qcow2_loop1_abs), ALLOW_PROBE | EXP_WARN); + (&qcow2_relstart), EXP_WARN, + (&qcow2_relstart), ALLOW_PROBE | EXP_WARN, + (&qcow2_absstart), EXP_WARN, + (&qcow2_absstart), ALLOW_PROBE | EXP_WARN); /* Rewrite wrap and qcow2 to be mutually-referential loop */ virCommandFree(cmd); @@ -705,13 +660,17 @@ mymain(void) "-F", "qcow2", "-b", absqcow2, "wrap", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; + qcow2_relstart.expBackingStoreRaw = "wrap"; + qcow2_relstart.expDirectory = datadir; + qcow2_absstart.expBackingStoreRaw = "wrap"; + wrap.expFormat = VIR_STORAGE_FILE_QCOW2; /* Behavior of an infinite loop chain */ TEST_CHAIN(17, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap, &qcow2_loop2_rel), EXP_WARN, - (&wrap, &qcow2_loop2_rel), ALLOW_PROBE | EXP_WARN, - (&wrap, &qcow2_loop2_abs), EXP_WARN, - (&wrap, &qcow2_loop2_abs), ALLOW_PROBE | EXP_WARN); + (&wrap, &qcow2_relstart), EXP_WARN, + (&wrap, &qcow2_relstart), ALLOW_PROBE | EXP_WARN, + (&wrap, &qcow2_absstart), EXP_WARN, + (&wrap, &qcow2_absstart), ALLOW_PROBE | EXP_WARN); /* Final cleanup */ testCleanupImages(); -- 1.9.0
On 04/06/14 05:32, Eric Blake wrote:
As I add more tests, it's getting harder to follow the split between a struct in one place and a test using the struct in another. Interleaving the tests makes changes more localized, and also makes debugging easier when a test goes wrong during my refactoring work.
* tests/virstoragetest.c (mymain): Modify structs as we go, rather than up-front. (testStorageChain): Make failure debugging easier.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 331 ++++++++++++++++++++++--------------------------- 1 file changed, 145 insertions(+), 186 deletions(-)
ACK, the original structs also were locals in mymain, thus this patch doesn't change stack space requirements for that function. Peter
Another reduction in the number of structs I have to modify when I start tracking new fields in virStorageFileMetadata. * tests/virstoragetest.c (_testFileData): Add fields. (testStorageChain): Select between fields based on flag. (mymain): Record both absolute and relative expectations in one struct. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 199 +++++++++++++++++++++++-------------------------- 1 file changed, 93 insertions(+), 106 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 938f878..74ad15c 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -202,12 +202,18 @@ testPrepImages(void) goto cleanup; } +/* Many fields of virStorageFileMetadata have the same content whether + * we access the file relatively or absolutely; but file names differ + * depending on how the chain was opened. For ease of testing, we + * test both relative and absolute starts, and use a flag to say which + * of the two variations to compare against. */ typedef struct _testFileData testFileData; struct _testFileData { const char *expBackingStore; const char *expBackingStoreRaw; - const char *expDirectory; + const char *expDirectoryRel; + const char *expDirectoryAbs; enum virStorageFileFormat expFormat; bool expIsFile; unsigned long long expCapacity; @@ -219,13 +225,14 @@ enum { EXP_FAIL = 1, EXP_WARN = 2, ALLOW_PROBE = 4, + ABS_START = 8, }; struct testChainData { const char *start; enum virStorageFileFormat format; - const testFileData *files[5]; + const testFileData *files[4]; int nfiles; unsigned int flags; }; @@ -239,6 +246,7 @@ testStorageChain(const void *args) virStorageFileMetadataPtr elt; size_t i = 0; char *broken = NULL; + bool abs = !!(data->flags & ABS_START); meta = virStorageFileGetMetadata(data->start, data->format, -1, -1, (data->flags & ALLOW_PROBE) != 0); @@ -277,17 +285,20 @@ testStorageChain(const void *args) while (elt) { char *expect = NULL; char *actual = NULL; + const char *expDirectory; if (i == data->nfiles) { fprintf(stderr, "probed chain was too long\n"); goto cleanup; } + expDirectory = abs ? data->files[i]->expDirectoryAbs + : data->files[i]->expDirectoryRel; 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), + NULLSTR(expDirectory), data->files[i]->expFormat, data->files[i]->expIsFile, data->files[i]->expCapacity, @@ -364,9 +375,9 @@ mymain(void) VIR_FLATTEN_1(chain1)); \ TEST_ONE_CHAIN(#id "b", relstart, format, flags2, \ VIR_FLATTEN_1(chain2)); \ - TEST_ONE_CHAIN(#id "c", absstart, format, flags3, \ + TEST_ONE_CHAIN(#id "c", absstart, format, flags3 | ABS_START,\ VIR_FLATTEN_1(chain3)); \ - TEST_ONE_CHAIN(#id "d", absstart, format, flags4, \ + TEST_ONE_CHAIN(#id "d", absstart, format, flags4 | ABS_START,\ VIR_FLATTEN_1(chain4)); \ } while (0) @@ -391,32 +402,25 @@ mymain(void) (&raw), ALLOW_PROBE | EXP_PASS); /* Qcow2 file with relative raw backing, format provided */ - testFileData qcow2_relstart = { + testFileData qcow2 = { .expBackingStore = canonraw, .expBackingStoreRaw = "raw", - .expDirectory = ".", - .expFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, - .expCapacity = 1024, - }; - testFileData qcow2_absstart = { - .expBackingStore = canonraw, - .expBackingStoreRaw = "raw", - .expDirectory = datadir, + .expDirectoryRel = ".", + .expDirectoryAbs = datadir, .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, }; TEST_CHAIN(3, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_relstart, &raw), EXP_PASS, - (&qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, - (&qcow2_absstart, &raw), EXP_PASS, - (&qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); + (&qcow2, &raw), EXP_PASS, + (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + (&qcow2, &raw), EXP_PASS, + (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(4, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, (&raw), EXP_PASS, - (&qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, + (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, (&raw), EXP_PASS, - (&qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); + (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 file to use absolute backing name */ virCommandFree(cmd); @@ -424,36 +428,36 @@ mymain(void) "-F", "raw", "-b", absraw, "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2_relstart.expBackingStoreRaw = absraw; - qcow2_relstart.expDirectory = datadir; - qcow2_absstart.expBackingStoreRaw = absraw; + qcow2.expBackingStoreRaw = absraw; + qcow2.expDirectoryRel = datadir; /* Qcow2 file with raw as absolute backing, backing format provided */ TEST_CHAIN(5, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_relstart, &raw), EXP_PASS, - (&qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, - (&qcow2_absstart, &raw), EXP_PASS, - (&qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); + (&qcow2, &raw), EXP_PASS, + (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + (&qcow2, &raw), EXP_PASS, + (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(6, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, (&raw), EXP_PASS, - (&qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, + (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, (&raw), EXP_PASS, - (&qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); + (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); /* Wrapped file access */ testFileData wrap = { .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, - .expDirectory = datadir, + .expDirectoryRel = datadir, + .expDirectoryAbs = datadir, .expFormat = VIR_STORAGE_FILE_QCOW2, .expIsFile = true, .expCapacity = 1024, }; TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap, &qcow2_relstart, &raw), EXP_PASS, - (&wrap, &qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, - (&wrap, &qcow2_absstart, &raw), EXP_PASS, - (&wrap, &qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); + (&wrap, &qcow2, &raw), EXP_PASS, + (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS, + (&wrap, &qcow2, &raw), EXP_PASS, + (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 and wrap file to omit backing file type */ virCommandFree(cmd); @@ -468,23 +472,23 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; wrap.expFormat = VIR_STORAGE_FILE_AUTO; - qcow2_relstart.expFormat = VIR_STORAGE_FILE_AUTO; - qcow2_absstart.expFormat = VIR_STORAGE_FILE_AUTO; + qcow2.expFormat = VIR_STORAGE_FILE_AUTO; /* Qcow2 file with raw as absolute backing, backing format omitted */ testFileData wrap_as_raw = { .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, - .expDirectory = datadir, + .expDirectoryRel = datadir, + .expDirectoryAbs = datadir, .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, }; TEST_CHAIN(8, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap_as_raw, &raw), EXP_PASS, - (&wrap, &qcow2_relstart, &raw), ALLOW_PROBE | EXP_PASS, + (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS, (&wrap_as_raw, &raw), EXP_PASS, - (&wrap, &qcow2_absstart, &raw), ALLOW_PROBE | EXP_PASS); + (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 to a missing backing file, with backing type */ virCommandFree(cmd); @@ -493,17 +497,17 @@ mymain(void) "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2_absstart.expBackingStore = NULL; - qcow2_absstart.expBackingStoreRaw = datadir "/bogus"; - qcow2_absstart.expFormat = VIR_STORAGE_FILE_NONE; - qcow2_absstart.expIsFile = false; + qcow2.expBackingStore = NULL; + qcow2.expBackingStoreRaw = datadir "/bogus"; + qcow2.expFormat = VIR_STORAGE_FILE_NONE; + qcow2.expIsFile = false; /* Qcow2 file with missing backing file but specified type */ TEST_CHAIN(9, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_absstart), EXP_WARN, - (&qcow2_absstart), ALLOW_PROBE | EXP_WARN, - (&qcow2_absstart), EXP_WARN, - (&qcow2_absstart), ALLOW_PROBE | EXP_WARN); + (&qcow2), EXP_WARN, + (&qcow2), ALLOW_PROBE | EXP_WARN, + (&qcow2), EXP_WARN, + (&qcow2), ALLOW_PROBE | EXP_WARN); /* Rewrite qcow2 to a missing backing file, without backing type */ virCommandFree(cmd); @@ -514,10 +518,10 @@ mymain(void) /* Qcow2 file with missing backing file and no specified type */ TEST_CHAIN(10, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_absstart), EXP_WARN, - (&qcow2_absstart), ALLOW_PROBE | EXP_WARN, - (&qcow2_absstart), EXP_WARN, - (&qcow2_absstart), ALLOW_PROBE | EXP_WARN); + (&qcow2), EXP_WARN, + (&qcow2), ALLOW_PROBE | EXP_WARN, + (&qcow2), EXP_WARN, + (&qcow2), ALLOW_PROBE | EXP_WARN); /* Rewrite qcow2 to use an nbd: protocol as backend */ virCommandFree(cmd); @@ -526,23 +530,25 @@ mymain(void) "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2_absstart.expBackingStore = "nbd:example.org:6000"; - qcow2_absstart.expBackingStoreRaw = NULL; - qcow2_absstart.expDirectory = NULL; - qcow2_absstart.expFormat = VIR_STORAGE_FILE_RAW; + qcow2.expBackingStore = "nbd:example.org:6000"; + qcow2.expBackingStoreRaw = NULL; + qcow2.expDirectoryRel = NULL; + qcow2.expDirectoryAbs = NULL; + qcow2.expFormat = VIR_STORAGE_FILE_RAW; /* Qcow2 file with backing protocol instead of file */ TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_absstart), EXP_PASS, - (&qcow2_absstart), ALLOW_PROBE | EXP_PASS, - (&qcow2_absstart), EXP_PASS, - (&qcow2_absstart), ALLOW_PROBE | EXP_PASS); + (&qcow2), EXP_PASS, + (&qcow2), ALLOW_PROBE | EXP_PASS, + (&qcow2), EXP_PASS, + (&qcow2), ALLOW_PROBE | EXP_PASS); /* qed file */ testFileData qed = { .expBackingStore = canonraw, .expBackingStoreRaw = absraw, - .expDirectory = datadir, + .expDirectoryRel = datadir, + .expDirectoryAbs = datadir, .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, @@ -585,43 +591,29 @@ mymain(void) ret = -1; /* Behavior of symlinks to qcow2 with relative backing files */ - testFileData link1_rel = { - .expBackingStore = canonraw, - .expBackingStoreRaw = "../raw", - .expDirectory = "sub/../sub/..", - .expFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, - .expCapacity = 1024, - }; - testFileData link1_abs = { + testFileData link1 = { .expBackingStore = canonraw, .expBackingStoreRaw = "../raw", - .expDirectory = datadir "/sub/../sub/..", + .expDirectoryRel = "sub/../sub/..", + .expDirectoryAbs = datadir "/sub/../sub/..", .expFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, }; - testFileData link2_rel = { - .expBackingStore = canonqcow2, - .expBackingStoreRaw = "../sub/link1", - .expDirectory = "sub/../sub", - .expFormat = VIR_STORAGE_FILE_QCOW2, - .expIsFile = true, - .expCapacity = 1024, - }; - testFileData link2_abs = { + testFileData link2 = { .expBackingStore = canonqcow2, .expBackingStoreRaw = "../sub/link1", - .expDirectory = datadir "/sub/../sub", + .expDirectoryRel = "sub/../sub", + .expDirectoryAbs = datadir "/sub/../sub", .expFormat = VIR_STORAGE_FILE_QCOW2, .expIsFile = true, .expCapacity = 1024, }; TEST_CHAIN(15, "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, &link1, &raw), EXP_PASS, + (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS, + (&link2, &link1, &raw), EXP_PASS, + (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS); #endif /* Rewrite qcow2 to be a self-referential loop */ @@ -630,23 +622,19 @@ mymain(void) "-F", "qcow2", "-b", "qcow2", "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2_relstart.expBackingStore = NULL; - qcow2_relstart.expBackingStoreRaw = "qcow2"; - qcow2_relstart.expDirectory = "."; - qcow2_relstart.expFormat= VIR_STORAGE_FILE_NONE; - qcow2_relstart.expIsFile = true; - qcow2_absstart.expBackingStore = NULL; - qcow2_absstart.expBackingStoreRaw = "qcow2"; - qcow2_absstart.expDirectory = datadir; - qcow2_absstart.expFormat= VIR_STORAGE_FILE_NONE; - qcow2_absstart.expIsFile = true; + qcow2.expBackingStore = NULL; + qcow2.expBackingStoreRaw = "qcow2"; + qcow2.expDirectoryRel = "."; + qcow2.expDirectoryAbs = datadir; + qcow2.expFormat= VIR_STORAGE_FILE_NONE; + qcow2.expIsFile = true; /* Behavior of an infinite loop chain */ TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2_relstart), EXP_WARN, - (&qcow2_relstart), ALLOW_PROBE | EXP_WARN, - (&qcow2_absstart), EXP_WARN, - (&qcow2_absstart), ALLOW_PROBE | EXP_WARN); + (&qcow2), EXP_WARN, + (&qcow2), ALLOW_PROBE | EXP_WARN, + (&qcow2), EXP_WARN, + (&qcow2), ALLOW_PROBE | EXP_WARN); /* Rewrite wrap and qcow2 to be mutually-referential loop */ virCommandFree(cmd); @@ -660,17 +648,16 @@ mymain(void) "-F", "qcow2", "-b", absqcow2, "wrap", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2_relstart.expBackingStoreRaw = "wrap"; - qcow2_relstart.expDirectory = datadir; - qcow2_absstart.expBackingStoreRaw = "wrap"; + qcow2.expBackingStoreRaw = "wrap"; + qcow2.expDirectoryRel = datadir; wrap.expFormat = VIR_STORAGE_FILE_QCOW2; /* Behavior of an infinite loop chain */ TEST_CHAIN(17, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap, &qcow2_relstart), EXP_WARN, - (&wrap, &qcow2_relstart), ALLOW_PROBE | EXP_WARN, - (&wrap, &qcow2_absstart), EXP_WARN, - (&wrap, &qcow2_absstart), ALLOW_PROBE | EXP_WARN); + (&wrap, &qcow2), EXP_WARN, + (&wrap, &qcow2), ALLOW_PROBE | EXP_WARN, + (&wrap, &qcow2), EXP_WARN, + (&wrap, &qcow2), ALLOW_PROBE | EXP_WARN); /* Final cleanup */ testCleanupImages(); -- 1.9.0
On 04/06/14 05:32, Eric Blake wrote:
Another reduction in the number of structs I have to modify when I start tracking new fields in virStorageFileMetadata.
* tests/virstoragetest.c (_testFileData): Add fields. (testStorageChain): Select between fields based on flag. (mymain): Record both absolute and relative expectations in one struct.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 199 +++++++++++++++++++++++-------------------------- 1 file changed, 93 insertions(+), 106 deletions(-)
ACK, Peter
On 04/07/2014 03:18 AM, Peter Krempa wrote:
On 04/06/14 05:32, Eric Blake wrote:
Another reduction in the number of structs I have to modify when I start tracking new fields in virStorageFileMetadata.
* tests/virstoragetest.c (_testFileData): Add fields. (testStorageChain): Select between fields based on flag. (mymain): Record both absolute and relative expectations in one struct.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 199 +++++++++++++++++++++++-------------------------- 1 file changed, 93 insertions(+), 106 deletions(-)
ACK,
Thanks; pushed 2-5. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa