[libvirt] [PATCH 0/5] progress towards removing virStorageFileMetadata
I'm almost to the point where virStorageSource can track everything that virStorageFileMetadata was used for, so that we can use one struct instead of two. But while working on this today, I noticed that virStorageFileChainLookup() does not have any unit tests, so I'll be writing those before removing the two remaining redundant fields. Eric Blake (5): conf: provide details on network backing store conf: expose probe for non-local storage conf: delete useless backingStoreIsFile field conf: return backing information separately from metadata conf: delete useless backingStoreFormat field src/conf/domain_conf.c | 2 +- src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 34 ++++---- src/storage/storage_backend_gluster.c | 14 ++-- src/util/virstoragefile.c | 146 +++++++++++++++++++++------------- src/util/virstoragefile.h | 9 ++- tests/virstoragetest.c | 49 ++++-------- 7 files changed, 135 insertions(+), 120 deletions(-) -- 1.9.0
So far, my work has been merely preserving the status quo of backing file analysis. But this patch starts to tread in the territory of making the backing chain code more powerful - we will eventually support network storage containing non-raw formats. Here, we expose metadata information about a network backing store, even if that information is still hardcoded to a raw format for now. * src/util/virstoragefile.c (virStorageFileGetMetadataRecurse): Also populate struct for non-file backing. (virStorageFileGetMetadata, virStorageFileGetMetadatainternal): Recognize non-file top image. (virFindBackingFile): Add comment. * tests/virstoragetest.c (mymain): Update test to reflect it. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 62 +++++++++++++++++++++++++++++++++++------------ tests/virstoragetest.c | 17 +++++++++---- 2 files changed, 59 insertions(+), 20 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f5fe8ad..7a91a01 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -569,6 +569,14 @@ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) virFindBackingFile(const char *start, const char *path, char **directory, char **canonical) { + /* FIXME - when we eventually allow non-raw network devices, we + * must ensure that we handle backing files the same way as qemu. + * For a qcow2 top file of gluster://server/vol/img, qemu treats + * the relative backing file 'rel' as meaning + * 'gluster://server/vol/rel', while the backing file '/abs' is + * used as a local file. But we cannot canonicalize network + * devices via canonicalize_file_name(), because they are not part + * of the local file system. */ char *combined = NULL; int ret = -1; @@ -874,6 +882,12 @@ virStorageFileGetMetadataInternal(const char *path, meta->backingStoreRaw, path); } + } else { + if (VIR_STRDUP(meta->backingStoreRaw, backing) < 0) { + VIR_FREE(backing); + goto cleanup; + } + backingFormat = VIR_STORAGE_FILE_RAW; } VIR_FREE(backing); meta->backingStoreFormat = backingFormat; @@ -1132,18 +1146,32 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) return -1; - if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) { - virReportSystemError(-fd, _("Failed to open file '%s'"), path); - return -1; - } + if (virBackingStoreIsFile(path)) { + if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) { + virReportSystemError(-fd, _("Failed to open file '%s'"), path); + return -1; + } - ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, directory, - fd, format, meta); + ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, + directory, + fd, format, meta); - if (VIR_CLOSE(fd) < 0) - VIR_WARN("could not close file %s", path); + if (VIR_CLOSE(fd) < 0) + VIR_WARN("could not close file %s", path); + } else { + /* FIXME: when the proper storage drivers are compiled in, it + * would be nice to read metadata from the network storage to + * allow for non-raw images. */ + if (VIR_STRDUP(meta->path, path) < 0) + return -1; + if (VIR_STRDUP(meta->canonPath, path) < 0) + return -1; + meta->type = VIR_STORAGE_TYPE_NETWORK; + meta->format = VIR_STORAGE_FILE_RAW; + ret = 0; + } - if (ret == 0 && meta->backingStoreIsFile) { + if (ret == 0 && meta->backingStore) { virStorageFileMetadataPtr backing; if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) @@ -1207,12 +1235,16 @@ virStorageFileGetMetadata(const char *path, int format, if (!cycle) return NULL; - if (!(canonPath = canonicalize_file_name(path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); - goto cleanup; - } - if (!(directory = mdir_name(path))) { - virReportOOMError(); + if (virBackingStoreIsFile(path)) { + if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + goto cleanup; + } + if (!(directory = mdir_name(path))) { + virReportOOMError(); + goto cleanup; + } + } else if (VIR_STRDUP(canonPath, path) < 0) { goto cleanup; } if (VIR_ALLOC(meta) < 0) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 9a9b0d8..e8409f6 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -621,17 +621,24 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStore = "nbd:example.org:6000"; - qcow2.expBackingStoreRaw = NULL; + qcow2.expBackingStoreRaw = "nbd:example.org:6000"; qcow2.expBackingDirRel = NULL; qcow2.expBackingDirAbs = NULL; qcow2.expBackingFormat = VIR_STORAGE_FILE_RAW; /* Qcow2 file with backing protocol instead of file */ + testFileData nbd = { + .pathRel = "nbd:example.org:6000", + .pathAbs = "nbd:example.org:6000", + .canonPath = "nbd:example.org:6000", + .type = VIR_STORAGE_TYPE_NETWORK, + .format = VIR_STORAGE_FILE_RAW, + }; TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2), EXP_PASS, - (&qcow2), ALLOW_PROBE | EXP_PASS, - (&qcow2), EXP_PASS, - (&qcow2), ALLOW_PROBE | EXP_PASS); + (&qcow2, &nbd), EXP_PASS, + (&qcow2, &nbd), ALLOW_PROBE | EXP_PASS, + (&qcow2, &nbd), EXP_PASS, + (&qcow2, &nbd), ALLOW_PROBE | EXP_PASS); /* qed file */ testFileData qed = { -- 1.9.0
On 04/10/14 05:41, Eric Blake wrote:
So far, my work has been merely preserving the status quo of backing file analysis. But this patch starts to tread in the territory of making the backing chain code more powerful - we will eventually support network storage containing non-raw formats. Here, we expose metadata information about a network backing store, even if that information is still hardcoded to a raw format for now.
* src/util/virstoragefile.c (virStorageFileGetMetadataRecurse): Also populate struct for non-file backing. (virStorageFileGetMetadata, virStorageFileGetMetadatainternal): Recognize non-file top image. (virFindBackingFile): Add comment. * tests/virstoragetest.c (mymain): Update test to reflect it.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 62 +++++++++++++++++++++++++++++++++++------------ tests/virstoragetest.c | 17 +++++++++---- 2 files changed, 59 insertions(+), 20 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f5fe8ad..7a91a01 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c
@@ -874,6 +882,12 @@ virStorageFileGetMetadataInternal(const char *path, meta->backingStoreRaw, path);
} + } else { + if (VIR_STRDUP(meta->backingStoreRaw, backing) < 0) { + VIR_FREE(backing); + goto cleanup; + } + backingFormat = VIR_STORAGE_FILE_RAW; } VIR_FREE(backing); meta->backingStoreFormat = backingFormat;
This hunk invalidates the comment in virStorageFileChainCheckBroken, as currently backing store raw will be set always. ACK with the comment updated. Peter
On 04/10/2014 03:58 AM, Peter Krempa wrote:
On 04/10/14 05:41, Eric Blake wrote:
So far, my work has been merely preserving the status quo of backing file analysis. But this patch starts to tread in the territory of making the backing chain code more powerful - we will eventually support network storage containing non-raw formats. Here, we expose metadata information about a network backing store, even if that information is still hardcoded to a raw format for now.
+ } else { + if (VIR_STRDUP(meta->backingStoreRaw, backing) < 0) { + VIR_FREE(backing); + goto cleanup; + } + backingFormat = VIR_STORAGE_FILE_RAW; } VIR_FREE(backing); meta->backingStoreFormat = backingFormat;
This hunk invalidates the comment in virStorageFileChainCheckBroken, as currently backing store raw will be set always.
ACK with the comment updated.
I squashed this in and pushed, along with the other ACKed patches in the series: diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index 7a91a01..8222528 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -1272,7 +1272,8 @@ virStorageFileGetMetadata(const char *path, int format, * * If CHAIN is broken, set *brokenFile to the broken file name, * otherwise set it to NULL. Caller MUST free *brokenFile after use. - * Return 0 on success, negative on error. + * Return 0 on success (including when brokenFile is set), negative on + * error (allocation failure). */ int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, @@ -1288,8 +1289,8 @@ virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, tmp = chain; while (tmp) { - /* Break if no backing store, backing store is not file, or - * other problem such as infinite loop */ + /* Break when we hit end of chain; report error if we detected + * a missing backing file, infinite loop, or other error */ if (!tmp->backingStoreRaw) break; if (!tmp->backingStore) { -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Deciding if a user string represents a local file instead of a network path is an operation worth exposing directly, particularly since the next patch will be removing a redundant variable that was caching the information. * src/util/virstoragefile.h (virStorageIsFile): New declaration. * src/util/virstoragefile.c (virBackingStoreIsFile): Rename... (virStorageIsFile): ...export, and allow NULL input. (virStorageFileGetMetadataInternal) (virStorageFileGetMetadataRecurse, virStorageFileGetMetadata): Update callers. * src/conf/domain_conf.c (virDomainDiskDefForeachPath): Use it. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. * src/libvirt_private.syms (virstoragefile.h): Export function. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 2 +- src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 2 +- src/util/virstoragefile.c | 20 +++++++++++++------- src/util/virstoragefile.h | 1 + 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1c4f9d2..726c8ba 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18565,7 +18565,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, goto cleanup; tmp = disk->backingChain; - while (tmp && tmp->backingStoreIsFile) { + while (tmp && virStorageIsFile(tmp->backingStore)) { if (!ignoreOpenFailure && !tmp->backingMeta) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to visit backing chain file %s"), diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9c189dc..cd43335 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1833,6 +1833,7 @@ virStorageFileIsClusterFS; virStorageFileProbeFormat; virStorageFileProbeFormatFromBuf; virStorageFileResize; +virStorageIsFile; virStorageNetHostDefClear; virStorageNetHostDefCopy; virStorageNetHostDefFree; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index bb20385..5730e3b 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -118,7 +118,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, *backingStore = meta->backingStore; meta->backingStore = NULL; if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO && - meta->backingStoreIsFile) { + virStorageIsFile(*backingStore)) { if ((ret = virStorageFileProbeFormat(*backingStore, -1, -1)) < 0) { /* If the backing file is currently unavailable, only log an error, * but continue. Returning -1 here would disable the whole storage diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7a91a01..cb66da4 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -688,11 +688,17 @@ virStorageFileMatchesVersion(int format, return false; } -static bool -virBackingStoreIsFile(const char *backing) +bool +virStorageIsFile(const char *backing) { - char *colon = strchr(backing, ':'); - char *slash = strchr(backing, '/'); + char *colon; + char *slash; + + if (!backing) + return false; + + colon = strchr(backing, ':'); + slash = strchr(backing, '/'); /* Reject anything that looks like a protocol (such as nbd: or * rbd:); if someone really does want a relative file name that @@ -866,7 +872,7 @@ virStorageFileGetMetadataInternal(const char *path, VIR_FREE(backing); goto cleanup; } - if (virBackingStoreIsFile(backing)) { + if (virStorageIsFile(backing)) { meta->backingStoreIsFile = true; meta->backingStoreRaw = meta->backingStore; meta->backingStore = NULL; @@ -1146,7 +1152,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) return -1; - if (virBackingStoreIsFile(path)) { + if (virStorageIsFile(path)) { if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), path); return -1; @@ -1235,7 +1241,7 @@ virStorageFileGetMetadata(const char *path, int format, if (!cycle) return NULL; - if (virBackingStoreIsFile(path)) { + if (virStorageIsFile(path)) { if (!(canonPath = canonicalize_file_name(path))) { virReportSystemError(errno, _("unable to resolve '%s'"), path); goto cleanup; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 4192140..6c08c31 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -315,6 +315,7 @@ int virStorageFileResize(const char *path, bool pre_allocate); int virStorageFileIsClusterFS(const char *path); +bool virStorageIsFile(const char *path); int virStorageFileGetLVMKey(const char *path, char **key); -- 1.9.0
On 04/10/14 05:41, Eric Blake wrote:
Deciding if a user string represents a local file instead of a network path is an operation worth exposing directly, particularly since the next patch will be removing a redundant variable that was caching the information.
* src/util/virstoragefile.h (virStorageIsFile): New declaration. * src/util/virstoragefile.c (virBackingStoreIsFile): Rename... (virStorageIsFile): ...export, and allow NULL input. (virStorageFileGetMetadataInternal) (virStorageFileGetMetadataRecurse, virStorageFileGetMetadata): Update callers. * src/conf/domain_conf.c (virDomainDiskDefForeachPath): Use it. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. * src/libvirt_private.syms (virstoragefile.h): Export function.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 2 +- src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 2 +- src/util/virstoragefile.c | 20 +++++++++++++------- src/util/virstoragefile.h | 1 + 5 files changed, 17 insertions(+), 9 deletions(-)
ACK, although we should switch to determining whether the file is local by inspecting other metadata than the backing string in the future. Peter
Finally starting to prune away some of the old fields that have been made redundant by the new fields, on my way towards directly reusing virStorageSource. * src/util/virstoragefile.h (_virStorageFileMetadata): Drop field. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal) (virStorageFileChainLookup): Adjust callers. * tests/virstoragetest.c (_testFileData, testStorageChain) (mymain): Simplify test. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 5 +---- src/util/virstoragefile.h | 1 - tests/virstoragetest.c | 16 +++------------- 3 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cb66da4..fe8144a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -866,14 +866,12 @@ virStorageFileGetMetadataInternal(const char *path, if (store == BACKING_STORE_ERROR) goto cleanup; - meta->backingStoreIsFile = false; if (backing != NULL) { if (VIR_STRDUP(meta->backingStore, backing) < 0) { VIR_FREE(backing); goto cleanup; } if (virStorageIsFile(backing)) { - meta->backingStoreIsFile = true; meta->backingStoreRaw = meta->backingStore; meta->backingStore = NULL; if (virFindBackingFile(directory, backing, @@ -882,7 +880,6 @@ virStorageFileGetMetadataInternal(const char *path, /* the backing file is (currently) unavailable, treat this * file as standalone: * backingStoreRaw is kept to mark broken image chains */ - meta->backingStoreIsFile = false; backingFormat = VIR_STORAGE_FILE_NONE; VIR_WARN("Backing file '%s' of image '%s' is missing.", meta->backingStoreRaw, path); @@ -1569,7 +1566,7 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, } else if (STREQ_NULLABLE(name, owner->backingStoreRaw) || STREQ(name, owner->backingStore)) { break; - } else if (owner->backingStoreIsFile) { + } else if (virStorageIsFile(owner->backingStore)) { char *absName = NULL; if (virFindBackingFile(owner->directory, name, NULL, &absName) < 0) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 6c08c31..4da160b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -148,7 +148,6 @@ struct _virStorageFileMetadata { char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as backingMeta->canonPath */ 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 */ }; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index e8409f6..846d130 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -233,7 +233,6 @@ struct _testFileData const char *expBackingDirRel; const char *expBackingDirAbs; enum virStorageFileFormat expBackingFormat; - bool expIsFile; unsigned long long expCapacity; bool expEncrypted; const char *pathRel; @@ -326,13 +325,12 @@ testStorageChain(const void *args) expRelDir = isAbs ? data->files[i]->relDirAbs : data->files[i]->relDirRel; if (virAsprintf(&expect, - "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d\n" + "store:%s\nraw:%s\ndirectory:%s\nother:%d %lld %d\n" "path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(data->files[i]->expBackingStore), NULLSTR(data->files[i]->expBackingStoreRaw), NULLSTR(expBackingDirectory), data->files[i]->expBackingFormat, - data->files[i]->expIsFile, data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(expPath), @@ -341,12 +339,12 @@ testStorageChain(const void *args) data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, - "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d\n" + "store:%s\nraw:%s\ndirectory:%s\nother:%d %lld %d\n" "path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(elt->backingStore), NULLSTR(elt->backingStoreRaw), NULLSTR(elt->directory), - elt->backingStoreFormat, elt->backingStoreIsFile, + elt->backingStoreFormat, elt->capacity, !!elt->encryption, NULLSTR(elt->path), NULLSTR(elt->canonPath), @@ -458,7 +456,6 @@ mymain(void) .expBackingDirRel = ".", .expBackingDirAbs = datadir, .expBackingFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, .expCapacity = 1024, .pathRel = "qcow2", .pathAbs = canonqcow2, @@ -520,7 +517,6 @@ mymain(void) .expBackingDirRel = datadir, .expBackingDirAbs = datadir, .expBackingFormat = VIR_STORAGE_FILE_QCOW2, - .expIsFile = true, .expCapacity = 1024, .pathRel = "wrap", .pathAbs = abswrap, @@ -562,7 +558,6 @@ mymain(void) .expBackingDirRel = datadir, .expBackingDirAbs = datadir, .expBackingFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, .expCapacity = 1024, .pathRel = "wrap", .pathAbs = abswrap, @@ -588,7 +583,6 @@ mymain(void) qcow2.expBackingStore = NULL; qcow2.expBackingStoreRaw = datadir "/bogus"; qcow2.expBackingFormat = VIR_STORAGE_FILE_NONE; - qcow2.expIsFile = false; qcow2.pathRel = "qcow2"; qcow2.relDirRel = "."; @@ -647,7 +641,6 @@ mymain(void) .expBackingDirRel = datadir, .expBackingDirAbs = datadir, .expBackingFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, .expCapacity = 1024, .pathRel = "qed", .pathAbs = absqed, @@ -714,7 +707,6 @@ mymain(void) .expBackingDirRel = "sub/../sub/..", .expBackingDirAbs = datadir "/sub/../sub/..", .expBackingFormat = VIR_STORAGE_FILE_RAW, - .expIsFile = true, .expCapacity = 1024, .pathRel = "../sub/link1", .pathAbs = "../sub/link1", @@ -730,7 +722,6 @@ mymain(void) .expBackingDirRel = "sub/../sub", .expBackingDirAbs = datadir "/sub/../sub", .expBackingFormat = VIR_STORAGE_FILE_QCOW2, - .expIsFile = true, .expCapacity = 1024, .pathRel = "sub/link2", .pathAbs = abslink2, @@ -762,7 +753,6 @@ mymain(void) qcow2.expBackingDirRel = "."; qcow2.expBackingDirAbs = datadir; qcow2.expBackingFormat= VIR_STORAGE_FILE_NONE; - qcow2.expIsFile = true; /* Behavior of an infinite loop chain */ TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, -- 1.9.0
On 04/10/14 05:41, Eric Blake wrote:
Finally starting to prune away some of the old fields that have been made redundant by the new fields, on my way towards directly reusing virStorageSource.
* src/util/virstoragefile.h (_virStorageFileMetadata): Drop field. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal) (virStorageFileChainLookup): Adjust callers. * tests/virstoragetest.c (_testFileData, testStorageChain) (mymain): Simplify test.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 5 +---- src/util/virstoragefile.h | 1 - tests/virstoragetest.c | 16 +++------------- 3 files changed, 4 insertions(+), 18 deletions(-)
ACK, Peter
A couple pieces of virStorageFileMetadata are used only while collecting information about the chain, and don't need to live permanently in the struct. This patch refactors external callers to collect the information separately, so that the next patch can remove the fields. * src/util/virstoragefile.h (virStorageFileGetMetadataFromBuf): Alter signature. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal): Likewise. (virStorageFileGetMetadataFromFDInternal): Adjust callers. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/storage/storage_backend_fs.c | 34 ++++++++++++++----------------- src/storage/storage_backend_gluster.c | 14 ++++++------- src/util/virstoragefile.c | 38 ++++++++++++++++++++--------------- src/util/virstoragefile.h | 6 +++++- 4 files changed, 48 insertions(+), 44 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 5730e3b..be963d4 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -106,7 +106,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (!(meta = virStorageFileGetMetadataFromBuf(target->path, header, len, - target->format))) { + target->format, + backingStore, + backingStoreFormat))) { ret = -1; goto error; } @@ -114,25 +116,19 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, VIR_FORCE_CLOSE(fd); - if (meta && meta->backingStore) { - *backingStore = meta->backingStore; - meta->backingStore = NULL; - if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO && - virStorageIsFile(*backingStore)) { - if ((ret = virStorageFileProbeFormat(*backingStore, -1, -1)) < 0) { - /* If the backing file is currently unavailable, only log an error, - * but continue. Returning -1 here would disable the whole storage - * pool, making it unavailable for even maintenance. */ - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot probe backing volume format: %s"), - *backingStore); - ret = -3; - } else { - *backingStoreFormat = ret; - ret = 0; - } + if (meta && *backingStore && + *backingStoreFormat == VIR_STORAGE_FILE_AUTO && + virStorageIsFile(*backingStore)) { + if ((ret = virStorageFileProbeFormat(*backingStore, -1, -1)) < 0) { + /* If the backing file is currently unavailable, only log an error, + * but continue. Returning -1 here would disable the whole storage + * pool, making it unavailable for even maintenance. */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot probe backing volume format: %s"), + *backingStore); + ret = -3; } else { - *backingStoreFormat = meta->backingStoreFormat; + *backingStoreFormat = ret; ret = 0; } } else { diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 2593c1c..e0a25df 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -298,16 +298,14 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, len)) < 0) goto cleanup; if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, - vol->target.format))) + vol->target.format, + &vol->backingStore.path, + &vol->backingStore.format))) goto cleanup; - if (meta->backingStore) { - vol->backingStore.path = meta->backingStore; - meta->backingStore = NULL; - vol->backingStore.format = meta->backingStoreFormat; - if (vol->backingStore.format < 0) - vol->backingStore.format = VIR_STORAGE_FILE_RAW; - } + if (vol->backingStore.path && + vol->backingStore.format < 0) + vol->backingStore.format = VIR_STORAGE_FILE_RAW; if (meta->capacity) vol->target.capacity = meta->capacity; if (meta->encryption) { diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index fe8144a..c509a9a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -790,13 +790,16 @@ qcow2GetFeatures(virBitmapPtr *features, * information about the file and its backing store. */ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(7) +ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9) virStorageFileGetMetadataInternal(const char *path, const char *canonPath, const char *directory, char *buf, size_t len, int format, - virStorageFileMetadataPtr meta) + virStorageFileMetadataPtr meta, + char **backingStore, + int *backingFormat) { int ret = -1; @@ -855,10 +858,9 @@ virStorageFileGetMetadataInternal(const char *path, } if (fileTypeInfo[format].getBackingStore != NULL) { - char *backing; - int backingFormat; + char *backing = NULL; int store = fileTypeInfo[format].getBackingStore(&backing, - &backingFormat, + backingFormat, buf, len); if (store == BACKING_STORE_INVALID) goto done; @@ -880,23 +882,20 @@ virStorageFileGetMetadataInternal(const char *path, /* the backing file is (currently) unavailable, treat this * file as standalone: * backingStoreRaw is kept to mark broken image chains */ - backingFormat = VIR_STORAGE_FILE_NONE; + *backingFormat = VIR_STORAGE_FILE_NONE; VIR_WARN("Backing file '%s' of image '%s' is missing.", meta->backingStoreRaw, path); } } else { - if (VIR_STRDUP(meta->backingStoreRaw, backing) < 0) { - VIR_FREE(backing); - goto cleanup; - } - backingFormat = VIR_STORAGE_FILE_RAW; + *backingStore = backing; + backing = NULL; + *backingFormat = VIR_STORAGE_FILE_RAW; } VIR_FREE(backing); - meta->backingStoreFormat = backingFormat; } else { meta->backingStore = NULL; - meta->backingStoreFormat = VIR_STORAGE_FILE_NONE; + *backingFormat = VIR_STORAGE_FILE_NONE; } } @@ -980,6 +979,8 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) * @buf: header bytes from @path * @len: length of @buf * @format: expected image format + * @backing: output malloc'd name of backing image, if any + * @backingFormat: format of @backing * * Extract metadata about the storage volume with the specified * image format. If image format is VIR_STORAGE_FILE_AUTO, it @@ -989,7 +990,7 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) * format, since a malicious guest can turn a raw file into any * other non-raw format at will. * - * If the returned meta.backingStoreFormat is VIR_STORAGE_FILE_AUTO + * If the returned @backingFormat is VIR_STORAGE_FILE_AUTO * it indicates the image didn't specify an explicit format for its * backing store. Callers are advised against probing for the * backing store format in this case. @@ -1000,7 +1001,9 @@ virStorageFileMetadataPtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, - int format) + int format, + char **backing, + int *backingFormat) { virStorageFileMetadataPtr ret = NULL; char *canonPath; @@ -1013,7 +1016,8 @@ virStorageFileGetMetadataFromBuf(const char *path, goto cleanup; if (virStorageFileGetMetadataInternal(path, canonPath, ".", buf, len, - format, ret) < 0) { + format, ret, backing, + backingFormat) < 0) { virStorageFileFreeMetadata(ret); ret = NULL; } @@ -1068,7 +1072,9 @@ virStorageFileGetMetadataFromFDInternal(const char *path, } ret = virStorageFileGetMetadataInternal(path, canonPath, directory, - buf, len, format, meta); + buf, len, format, meta, + &meta->backingStoreRaw, + &meta->backingStoreFormat); if (ret == 0) { if (S_ISREG(sb.st_mode)) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 4da160b..edfe9eb 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -295,7 +295,11 @@ virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, virStorageFileMetadataPtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, - int format); + int format, + char **backing, + int *backingFormat) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5) + ATTRIBUTE_NONNULL(6); int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, char **broken_file); -- 1.9.0
On 04/10/14 05:41, Eric Blake wrote:
A couple pieces of virStorageFileMetadata are used only while collecting information about the chain, and don't need to live permanently in the struct. This patch refactors external callers to collect the information separately, so that the next patch can remove the fields.
* src/util/virstoragefile.h (virStorageFileGetMetadataFromBuf): Alter signature. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal): Likewise. (virStorageFileGetMetadataFromFDInternal): Adjust callers. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/storage/storage_backend_fs.c | 34 ++++++++++++++----------------- src/storage/storage_backend_gluster.c | 14 ++++++------- src/util/virstoragefile.c | 38 ++++++++++++++++++++--------------- src/util/virstoragefile.h | 6 +++++- 4 files changed, 48 insertions(+), 44 deletions(-)
ACK, Peter
Drop another redundant field from virStorageFileMetadata. * src/util/virstoragefile.h (_virStorageFileMetadata): Drop field. * src/util/virstoragefile.c (virStorageFileGetMetadataFromFDInternal) (virStorageFileGetMetadataFromFD) (virStorageFileGetMetadataRecurse): Adjust callers. * tests/virstoragetest.c (_testFileData, testStorageChain) (mymain): Simplify test. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 39 ++++++++++++++++++--------------------- src/util/virstoragefile.h | 1 - tests/virstoragetest.c | 22 ++-------------------- 3 files changed, 20 insertions(+), 42 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c509a9a..3645abb 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1035,13 +1035,19 @@ virStorageFileGetMetadataFromFDInternal(const char *path, const char *directory, int fd, int format, - virStorageFileMetadataPtr meta) + virStorageFileMetadataPtr meta, + int *backingFormat) { char *buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; struct stat sb; int ret = -1; + int dummy; + if (backingFormat) + *backingFormat = VIR_STORAGE_FILE_NONE; + else + backingFormat = &dummy; if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), @@ -1074,7 +1080,7 @@ virStorageFileGetMetadataFromFDInternal(const char *path, ret = virStorageFileGetMetadataInternal(path, canonPath, directory, buf, len, format, meta, &meta->backingStoreRaw, - &meta->backingStoreFormat); + backingFormat); if (ret == 0) { if (S_ISREG(sb.st_mode)) @@ -1099,11 +1105,6 @@ virStorageFileGetMetadataFromFDInternal(const char *path, * format, since a malicious guest can turn a raw file into any * other non-raw format at will. * - * If the returned meta.backingStoreFormat is VIR_STORAGE_FILE_AUTO - * it indicates the image didn't specify an explicit format for its - * backing store. Callers are advised against probing for the - * backing store format in this case. - * * Caller MUST free the result after use via virStorageFileFreeMetadata. */ virStorageFileMetadataPtr @@ -1121,7 +1122,7 @@ virStorageFileGetMetadataFromFD(const char *path, if (VIR_ALLOC(ret) < 0) goto cleanup; if (virStorageFileGetMetadataFromFDInternal(path, canonPath, ".", - fd, format, ret) < 0) { + fd, format, ret, NULL) < 0) { virStorageFileFreeMetadata(ret); ret = NULL; } @@ -1142,6 +1143,8 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, { int fd; int ret = -1; + int backingFormat; + VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", path, canonPath, NULLSTR(directory), format, (int)uid, (int)gid, allow_probe); @@ -1163,7 +1166,8 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, directory, - fd, format, meta); + fd, format, meta, + &backingFormat); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); @@ -1183,19 +1187,17 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, if (ret == 0 && meta->backingStore) { virStorageFileMetadataPtr backing; - if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) - meta->backingStoreFormat = VIR_STORAGE_FILE_RAW; - else if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) - meta->backingStoreFormat = VIR_STORAGE_FILE_AUTO; - format = meta->backingStoreFormat; + if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) + backingFormat = VIR_STORAGE_FILE_RAW; + else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) + backingFormat = VIR_STORAGE_FILE_AUTO; if (VIR_ALLOC(backing) < 0 || virStorageFileGetMetadataRecurse(meta->backingStoreRaw, meta->backingStore, - meta->directory, format, + meta->directory, backingFormat, uid, gid, allow_probe, cycle, backing) < 0) { /* If we failed to get backing data, mark the chain broken */ - meta->backingStoreFormat = VIR_STORAGE_FILE_NONE; VIR_FREE(meta->backingStore); virStorageFileFreeMetadata(backing); } else { @@ -1220,11 +1222,6 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, * format, since a malicious guest can turn a raw file into any * other non-raw format at will. * - * If the returned meta.backingStoreFormat is VIR_STORAGE_FILE_AUTO - * it indicates the image didn't specify an explicit format for its - * backing store. Callers are advised against using ALLOW_PROBE, as - * it would probe the backing store format in this case. - * * Caller MUST free result after use via virStorageFileFreeMetadata. */ virStorageFileMetadataPtr diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index edfe9eb..c747f20 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -147,7 +147,6 @@ struct _virStorageFileMetadata { * store. */ char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as backingMeta->canonPath */ char *directory; /* The directory containing basename of backingStoreRaw. Should be same as backingMeta->relDir */ - int backingStoreFormat; /* enum virStorageFileFormat. Should be same as backingMeta->format */ }; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 846d130..13b5032 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -232,7 +232,6 @@ struct _testFileData const char *expBackingStoreRaw; const char *expBackingDirRel; const char *expBackingDirAbs; - enum virStorageFileFormat expBackingFormat; unsigned long long expCapacity; bool expEncrypted; const char *pathRel; @@ -325,12 +324,11 @@ testStorageChain(const void *args) expRelDir = isAbs ? data->files[i]->relDirAbs : data->files[i]->relDirRel; if (virAsprintf(&expect, - "store:%s\nraw:%s\ndirectory:%s\nother:%d %lld %d\n" + "store:%s\nraw:%s\ndirectory:%s\nother:%lld %d\n" "path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(data->files[i]->expBackingStore), NULLSTR(data->files[i]->expBackingStoreRaw), NULLSTR(expBackingDirectory), - data->files[i]->expBackingFormat, data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(expPath), @@ -339,12 +337,11 @@ testStorageChain(const void *args) data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, - "store:%s\nraw:%s\ndirectory:%s\nother:%d %lld %d\n" + "store:%s\nraw:%s\ndirectory:%s\nother:%lld %d\n" "path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(elt->backingStore), NULLSTR(elt->backingStoreRaw), NULLSTR(elt->directory), - elt->backingStoreFormat, elt->capacity, !!elt->encryption, NULLSTR(elt->path), NULLSTR(elt->canonPath), @@ -428,7 +425,6 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { - .expBackingFormat = VIR_STORAGE_FILE_NONE, .pathRel = "raw", .pathAbs = canonraw, .canonPath = canonraw, @@ -455,7 +451,6 @@ mymain(void) .expBackingStoreRaw = "raw", .expBackingDirRel = ".", .expBackingDirAbs = datadir, - .expBackingFormat = VIR_STORAGE_FILE_RAW, .expCapacity = 1024, .pathRel = "qcow2", .pathAbs = canonqcow2, @@ -466,7 +461,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; testFileData qcow2_as_raw = { - .expBackingFormat = VIR_STORAGE_FILE_NONE, .pathRel = "qcow2", .pathAbs = canonqcow2, .canonPath = canonqcow2, @@ -516,7 +510,6 @@ mymain(void) .expBackingStoreRaw = absqcow2, .expBackingDirRel = datadir, .expBackingDirAbs = datadir, - .expBackingFormat = VIR_STORAGE_FILE_QCOW2, .expCapacity = 1024, .pathRel = "wrap", .pathAbs = abswrap, @@ -546,8 +539,6 @@ mymain(void) "-b", absqcow2, "wrap", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - wrap.expBackingFormat = VIR_STORAGE_FILE_AUTO; - qcow2.expBackingFormat = VIR_STORAGE_FILE_AUTO; qcow2_as_raw.pathRel = absqcow2; qcow2_as_raw.relDirRel = datadir; @@ -557,7 +548,6 @@ mymain(void) .expBackingStoreRaw = absqcow2, .expBackingDirRel = datadir, .expBackingDirAbs = datadir, - .expBackingFormat = VIR_STORAGE_FILE_RAW, .expCapacity = 1024, .pathRel = "wrap", .pathAbs = abswrap, @@ -582,7 +572,6 @@ mymain(void) ret = -1; qcow2.expBackingStore = NULL; qcow2.expBackingStoreRaw = datadir "/bogus"; - qcow2.expBackingFormat = VIR_STORAGE_FILE_NONE; qcow2.pathRel = "qcow2"; qcow2.relDirRel = "."; @@ -618,7 +607,6 @@ mymain(void) qcow2.expBackingStoreRaw = "nbd:example.org:6000"; qcow2.expBackingDirRel = NULL; qcow2.expBackingDirAbs = NULL; - qcow2.expBackingFormat = VIR_STORAGE_FILE_RAW; /* Qcow2 file with backing protocol instead of file */ testFileData nbd = { @@ -640,7 +628,6 @@ mymain(void) .expBackingStoreRaw = absraw, .expBackingDirRel = datadir, .expBackingDirAbs = datadir, - .expBackingFormat = VIR_STORAGE_FILE_RAW, .expCapacity = 1024, .pathRel = "qed", .pathAbs = absqed, @@ -651,7 +638,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QED, }; testFileData qed_as_raw = { - .expBackingFormat = VIR_STORAGE_FILE_NONE, .pathRel = "qed", .pathAbs = absqed, .canonPath = canonqed, @@ -706,7 +692,6 @@ mymain(void) .expBackingStoreRaw = "../raw", .expBackingDirRel = "sub/../sub/..", .expBackingDirAbs = datadir "/sub/../sub/..", - .expBackingFormat = VIR_STORAGE_FILE_RAW, .expCapacity = 1024, .pathRel = "../sub/link1", .pathAbs = "../sub/link1", @@ -721,7 +706,6 @@ mymain(void) .expBackingStoreRaw = "../sub/link1", .expBackingDirRel = "sub/../sub", .expBackingDirAbs = datadir "/sub/../sub", - .expBackingFormat = VIR_STORAGE_FILE_QCOW2, .expCapacity = 1024, .pathRel = "sub/link2", .pathAbs = abslink2, @@ -752,7 +736,6 @@ mymain(void) qcow2.expBackingStoreRaw = "qcow2"; qcow2.expBackingDirRel = "."; qcow2.expBackingDirAbs = datadir; - qcow2.expBackingFormat= VIR_STORAGE_FILE_NONE; /* Behavior of an infinite loop chain */ TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -777,7 +760,6 @@ mymain(void) qcow2.expBackingDirRel = datadir; qcow2.pathRel = absqcow2; qcow2.relDirRel = datadir; - wrap.expBackingFormat = VIR_STORAGE_FILE_QCOW2; /* Behavior of an infinite loop chain */ TEST_CHAIN(17, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, -- 1.9.0
On 04/10/14 05:41, Eric Blake wrote:
Drop another redundant field from virStorageFileMetadata.
* src/util/virstoragefile.h (_virStorageFileMetadata): Drop field. * src/util/virstoragefile.c (virStorageFileGetMetadataFromFDInternal) (virStorageFileGetMetadataFromFD) (virStorageFileGetMetadataRecurse): Adjust callers. * tests/virstoragetest.c (_testFileData, testStorageChain) (mymain): Simplify test.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 39 ++++++++++++++++++--------------------- src/util/virstoragefile.h | 1 - tests/virstoragetest.c | 22 ++-------------------- 3 files changed, 20 insertions(+), 42 deletions(-)
(Re) ACK, the previous attempt didn't go through apparently. Peter
On 04/09/2014 09:41 PM, Eric Blake wrote:
I'm almost to the point where virStorageSource can track everything that virStorageFileMetadata was used for, so that we can use one struct instead of two. But while working on this today, I noticed that virStorageFileChainLookup() does not have any unit tests, so I'll be writing those before removing the two remaining redundant fields.
It will look something like this (although I still need more test cases before turning it into a formal commit): diff --git i/tests/virstoragetest.c w/tests/virstoragetest.c index 13b5032..225d18d 100644 --- i/tests/virstoragetest.c +++ w/tests/virstoragetest.c @@ -375,12 +375,69 @@ testStorageChain(const void *args) return ret; } +struct testLookupData +{ + virStorageFileMetadataPtr chain; + const char *start; + const char *name; + const char *expResult; + virStorageFileMetadataPtr expMeta; + const char *expParent; +}; + +static int +testStorageLookup(const void *args) +{ + const struct testLookupData *data = args; + int ret = 0; + const char *actualResult; + virStorageFileMetadataPtr actualMeta; + const char *actualParent; + + /* This function is documented as giving results within chain, + * which means we can use pointer equality instead of STREQ. Test + * twice to ensure optional parameters don't cause NULL deref. */ + actualResult = virStorageFileChainLookup(data->chain, data->start, + data->name, NULL, NULL); + if (data->expResult != actualResult) { + fprintf(stderr, "result 1: expected %s(%p), got %s(%p)\n", + NULLSTR(data->expResult), data->expResult, + NULLSTR(actualResult), actualResult); + ret = -1; + } + + actualResult = virStorageFileChainLookup(data->chain, data->start, + data->name, &actualMeta, + &actualParent); + if (data->expResult != actualResult) { + fprintf(stderr, "result 2: expected %s(%p), got %s(%p)\n", + NULLSTR(data->expResult), data->expResult, + NULLSTR(actualResult), actualResult); + ret = -1; + } + if (data->expMeta != actualMeta) { + fprintf(stderr, "meta: expected %p, got %p\n", + data->expMeta, actualMeta); + ret = -1; + } + if (data->expParent != actualParent) { + fprintf(stderr, "parent: expected %s(%p), got %s(%p)\n", + NULLSTR(data->expParent), data->expParent, + NULLSTR(actualParent), actualParent); + ret = -1; + } + + return ret; +} + static int mymain(void) { int ret; virCommandPtr cmd = NULL; struct testChainData data; + virStorageFileMetadataPtr chain = NULL; + const char *start = NULL; /* 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. */ @@ -768,6 +825,26 @@ mymain(void) (&wrap, &qcow2), EXP_WARN, (&wrap, &qcow2), ALLOW_PROBE | EXP_WARN); + /* Test behavior of chain lookups */ + start = "wrap"; + chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, + -1, -1, false); + if (!chain) + return EXIT_FAILURE; + +#define TEST_LOOKUP(id, name, result, meta, parent) \ + do { \ + struct testLookupData data2 = { chain, start, name, \ + result, meta, parent, }; \ + if (virtTestRun("Chain lookup " #id, \ + testStorageLookup, &data2) < 0) \ + ret = -1; \ + } while (0) + + TEST_LOOKUP(1, "wrap", start, chain, NULL); + TEST_LOOKUP(2, abswrap, start, chain, NULL); + TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta, start); + /* Final cleanup */ testCleanupImages(); virCommandFree(cmd); -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa