[libvirt] [PATCH 00/18] Get rid of virStorageFileMetadata
Peter Krempa (18): qemu: unexport qemuDiskChainCheckBroken util: storagefile: Always store raw backing name in the metadata util: storage: Don't force path canonicalization when loading metadata util: storage: Remove obsolete argument virStorageFileGetMetadataInternal util: storage: Move checking of the actual backing image to the worker storage: util: Clean up arguments of virStorageFileGetMetadataInternal util: storage: Rename "path" to "relPath" in virStorageFileMetadata util: storagefile: Rename "canonPath" to "path" in virStorageFileMetadata util: virstoragefile: Don't use "backingStore" directly virstoragefile: Kill "backingStore" field from virStorageFileMetadata util: storagefile: Add function to free a virStorageSourcePrt util: storagefile: Add fields from virStorageMetadata to virStorageSource maint: Switch over from struct virStorageFileMetadata to virStorageSource util: virstorage: Kill struct virStorageFileMetadata util: virstoragefile: Rename backingMeta to backingStore storage: Move disk->backingChain to the recursive disk->src.backingStore util: virstoragefile: Don't mangle data stored about directories util: storage: Invert the way recursive metadata retrieval works src/conf/domain_conf.c | 17 +- src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 58 ++--- src/qemu/qemu_domain.h | 2 - src/qemu/qemu_driver.c | 44 ++-- src/security/security_selinux.c | 2 +- src/security/virt-aa-helper.c | 9 +- src/storage/storage_backend_fs.c | 4 +- src/storage/storage_backend_gluster.c | 4 +- src/util/virstoragefile.c | 476 ++++++++++++++++------------------ src/util/virstoragefile.h | 88 +++---- tests/virstoragetest.c | 178 ++++++++----- 13 files changed, 435 insertions(+), 450 deletions(-) -- 1.9.1
The function isn't used in any other source file. Move it so that it doesn't need a declaration. --- src/qemu/qemu_domain.c | 44 ++++++++++++++++++++++---------------------- src/qemu/qemu_domain.h | 2 -- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index cdd4601..1641eed 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2234,6 +2234,28 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, return -1; } +static int +qemuDiskChainCheckBroken(virDomainDiskDefPtr disk) +{ + char *brokenFile = NULL; + + if (!virDomainDiskGetSource(disk) || !disk->backingChain) + return 0; + + if (virStorageFileChainGetBroken(disk->backingChain, &brokenFile) < 0) + return -1; + + if (brokenFile) { + virReportError(VIR_ERR_INVALID_ARG, + _("Backing file '%s' of image '%s' is missing."), + brokenFile, virDomainDiskGetSource(disk)); + VIR_FREE(brokenFile); + return -1; + } + + return 0; +} + int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -2337,28 +2359,6 @@ qemuDomainCleanupRun(virQEMUDriverPtr driver, priv->ncleanupCallbacks_max = 0; } -int -qemuDiskChainCheckBroken(virDomainDiskDefPtr disk) -{ - char *brokenFile = NULL; - - if (!virDomainDiskGetSource(disk) || !disk->backingChain) - return 0; - - if (virStorageFileChainGetBroken(disk->backingChain, &brokenFile) < 0) - return -1; - - if (brokenFile) { - virReportError(VIR_ERR_INVALID_ARG, - _("Backing file '%s' of image '%s' is missing."), - brokenFile, virDomainDiskGetSource(disk)); - VIR_FREE(brokenFile); - return -1; - } - - return 0; -} - static void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b2830c4..31611b5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -352,8 +352,6 @@ int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, bool start_with_state); -int qemuDiskChainCheckBroken(virDomainDiskDefPtr disk); - int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
The function isn't used in any other source file. Move it so that it doesn't need a declaration. --- src/qemu/qemu_domain.c | 44 ++++++++++++++++++++++---------------------- src/qemu/qemu_domain.h | 2 -- 2 files changed, 22 insertions(+), 24 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Don't use the backingStoreRaw as a indication of broken chains. Fill it always and tweak the broken image chain detector to avoid changing the semantics. The new semantics to detect a broken chain is the presence of string in backingStoreRaw but the lack of the backing chain metadata structure in the chain. Now that the raw backing store name is always filled there's no need to pass the raw name variable separately to fill in case the backing is not a file. Tweak the function so that it can handle a NULL in that case. --- src/util/virstoragefile.c | 51 +++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 73cfef0..5fbb6e7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -864,8 +864,7 @@ virStorageFileGetMetadataInternal(const char *path, } if (fileTypeInfo[format].getBackingStore != NULL) { - char *backing = NULL; - int store = fileTypeInfo[format].getBackingStore(&backing, + int store = fileTypeInfo[format].getBackingStore(&meta->backingStoreRaw, backingFormat, buf, len); if (store == BACKING_STORE_INVALID) @@ -874,15 +873,10 @@ virStorageFileGetMetadataInternal(const char *path, if (store == BACKING_STORE_ERROR) goto cleanup; - if (backing != NULL) { - if (VIR_STRDUP(meta->backingStore, backing) < 0) { - VIR_FREE(backing); - goto cleanup; - } - if (virStorageIsFile(backing)) { - meta->backingStoreRaw = meta->backingStore; - meta->backingStore = NULL; - if (virFindBackingFile(directory, backing, + if (meta->backingStoreRaw) { + if (virStorageIsFile(meta->backingStoreRaw)) { + if (virFindBackingFile(directory, + meta->backingStoreRaw, backingDirectory, &meta->backingStore) < 0) { /* the backing file is (currently) unavailable, treat this @@ -894,11 +888,15 @@ virStorageFileGetMetadataInternal(const char *path, } } else { - *backingStore = backing; - backing = NULL; + if (VIR_STRDUP(meta->backingStore, meta->backingStoreRaw) < 0) + goto cleanup; + + if (backingStore && + VIR_STRDUP(*backingStore, meta->backingStoreRaw) < 0) + goto cleanup; + *backingFormat = VIR_STORAGE_FILE_RAW; } - VIR_FREE(backing); } else { meta->backingStore = NULL; *backingFormat = VIR_STORAGE_FILE_NONE; @@ -1085,7 +1083,7 @@ virStorageFileGetMetadataFromFDInternal(const char *path, ret = virStorageFileGetMetadataInternal(path, canonPath, directory, buf, len, format, meta, - &meta->backingStoreRaw, + NULL, backingFormat, backingDirectory); if (ret == 0) { @@ -1296,31 +1294,24 @@ virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, char **brokenFile) { virStorageFileMetadataPtr tmp; - int ret = -1; *brokenFile = NULL; if (!chain) return 0; - tmp = chain; - while (tmp) { + for (tmp = chain; tmp; tmp = tmp->backingMeta) { /* 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) { - if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0) - goto error; - break; - } - tmp = tmp->backingMeta; - } + if (!tmp->backingMeta && tmp->backingStoreRaw) { + if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0) + return -1; - ret = 0; + return 0; + } + } - error: - return ret; + return 0; } -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Don't use the backingStoreRaw as a indication of broken chains. Fill it always and tweak the broken image chain detector to avoid changing the semantics.
The new semantics to detect a broken chain is the presence of string in backingStoreRaw but the lack of the backing chain metadata structure in the chain.
Yay - you were able to take this where I had been aiming to go before I got interrupted by real life (TM) last week :)
Now that the raw backing store name is always filled there's no need to pass the raw name variable separately to fill in case the backing is not a file. Tweak the function so that it can handle a NULL in that case. --- src/util/virstoragefile.c | 51 +++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 30 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Avoid breaking gluster volumes that don't have local representation. Use the provided name when canonicalization fails. Broken by commit 79f11b35 Fixes: $ virsh pool-start glusterpool error: Failed to start pool glusterpool error: unable to resolve 'asdf': No such file or directory --- src/util/virstoragefile.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5fbb6e7..c707200 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1012,10 +1012,12 @@ virStorageFileGetMetadataFromBuf(const char *path, virStorageFileMetadataPtr ret = NULL; char *canonPath; - if (!(canonPath = canonicalize_file_name(path))) { + if (!(canonPath = canonicalize_file_name(path)) && + VIR_STRDUP(canonPath, path) < 0) { virReportSystemError(errno, _("unable to resolve '%s'"), path); return NULL; } + if (VIR_ALLOC(ret) < 0) goto cleanup; -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Avoid breaking gluster volumes that don't have local representation. Use the provided name when canonicalization fails. Broken by commit 79f11b35
Fixes: $ virsh pool-start glusterpool error: Failed to start pool glusterpool error: unable to resolve 'asdf': No such file or directory --- src/util/virstoragefile.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
I'd feel a bit better if we had testsuite coverage for this (since my broken commit still managed to pass 'make check', it shows our testsuite has a hole).
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5fbb6e7..c707200 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1012,10 +1012,12 @@ virStorageFileGetMetadataFromBuf(const char *path, virStorageFileMetadataPtr ret = NULL; char *canonPath;
- if (!(canonPath = canonicalize_file_name(path))) { + if (!(canonPath = canonicalize_file_name(path)) && + VIR_STRDUP(canonPath, path) < 0) { virReportSystemError(errno, _("unable to resolve '%s'"), path); return NULL; }
I'm not sure if I like this. We are blindly trying to resolve 'path' as if it were a local file name, and then if it failed, use 'path' as-is in case it was a remote name. I think what we should really be doing is: if (virStorageIsFile(path)) { if (!(canonPath = canonicalize_file_name(path))) { error; } } else { if (VIR_STRDUP(canonPath, path) < 0) { error; } } In other words, the bug is that we are trying to canonicalize a non-file name (which has a chance for false positives if the file system contains an odd file name), rather than reserving the canonicalization to happen ONLY when we know it is not a network protocol name. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/22/2014 07:34 AM, Eric Blake wrote:
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Avoid breaking gluster volumes that don't have local representation. Use the provided name when canonicalization fails. Broken by commit 79f11b35
Fixes: $ virsh pool-start glusterpool error: Failed to start pool glusterpool error: unable to resolve 'asdf': No such file or directory --- src/util/virstoragefile.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
I'd feel a bit better if we had testsuite coverage for this (since my broken commit still managed to pass 'make check', it shows our testsuite has a hole).
- if (!(canonPath = canonicalize_file_name(path))) { + if (!(canonPath = canonicalize_file_name(path)) && + VIR_STRDUP(canonPath, path) < 0) { virReportSystemError(errno, _("unable to resolve '%s'"), path); return NULL; }
I'm not sure if I like this. We are blindly trying to resolve 'path' as if it were a local file name, and then if it failed, use 'path' as-is in case it was a remote name. I think what we should really be doing is:
I'd still like the test improvements, but those can be separate patches as long as they are in by the time gluster interaction is fully integrated. So for this patch, I can live with ACK if you squash this in: diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index c707200..6ea45a4 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -1012,9 +1012,12 @@ virStorageFileGetMetadataFromBuf(const char *path, virStorageFileMetadataPtr ret = NULL; char *canonPath; - if (!(canonPath = canonicalize_file_name(path)) && - VIR_STRDUP(canonPath, path) < 0) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (virStorageIsFile(path)) { + if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + return NULL; + } + } else if (VIR_STRDUP(canonPath, path) < 0) { return NULL; } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/22/14 22:53, Eric Blake wrote:
On 04/22/2014 07:34 AM, Eric Blake wrote:
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Avoid breaking gluster volumes that don't have local representation. Use the provided name when canonicalization fails. Broken by commit 79f11b35
Fixes: $ virsh pool-start glusterpool error: Failed to start pool glusterpool error: unable to resolve 'asdf': No such file or directory --- src/util/virstoragefile.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
I'd feel a bit better if we had testsuite coverage for this (since my broken commit still managed to pass 'make check', it shows our testsuite has a hole).
- if (!(canonPath = canonicalize_file_name(path))) { + if (!(canonPath = canonicalize_file_name(path)) && + VIR_STRDUP(canonPath, path) < 0) { virReportSystemError(errno, _("unable to resolve '%s'"), path); return NULL; }
I'm not sure if I like this. We are blindly trying to resolve 'path' as if it were a local file name, and then if it failed, use 'path' as-is in case it was a remote name. I think what we should really be doing is:
I'd still like the test improvements, but those can be separate patches as long as they are in by the time gluster interaction is fully integrated. So for this patch, I can live with ACK if you squash this in:
diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index c707200..6ea45a4 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -1012,9 +1012,12 @@ virStorageFileGetMetadataFromBuf(const char *path, virStorageFileMetadataPtr ret = NULL; char *canonPath;
- if (!(canonPath = canonicalize_file_name(path)) && - VIR_STRDUP(canonPath, path) < 0) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (virStorageIsFile(path)) {
You are suggesting this same change in patch 6/18 ... as that patch actually removes this code I'll leave this patch as-is and fix the code in the new helper introduced in 6/18.
+ if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + return NULL; + } + } else if (VIR_STRDUP(canonPath, path) < 0) { return NULL; }
Peter
On 04/23/14 09:56, Peter Krempa wrote:
On 04/22/14 22:53, Eric Blake wrote:
On 04/22/2014 07:34 AM, Eric Blake wrote:
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Avoid breaking gluster volumes that don't have local representation. Use the provided name when canonicalization fails. Broken by commit 79f11b35
...
diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index c707200..6ea45a4 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -1012,9 +1012,12 @@ virStorageFileGetMetadataFromBuf(const char *path, virStorageFileMetadataPtr ret = NULL; char *canonPath;
- if (!(canonPath = canonicalize_file_name(path)) && - VIR_STRDUP(canonPath, path) < 0) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (virStorageIsFile(path)) {
You are suggesting this same change in patch 6/18 ... as that patch actually removes this code I'll leave this patch as-is and fix the code in the new helper introduced in 6/18.
Or I can squash those two commits together (effectively just mention the fix in the commit message).
+ if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + return NULL; + } + } else if (VIR_STRDUP(canonPath, path) < 0) { return NULL; }
Peter
On 04/23/2014 01:59 AM, Peter Krempa wrote:
- if (!(canonPath = canonicalize_file_name(path)) && - VIR_STRDUP(canonPath, path) < 0) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (virStorageIsFile(path)) {
You are suggesting this same change in patch 6/18 ... as that patch actually removes this code I'll leave this patch as-is and fix the code in the new helper introduced in 6/18.
Or I can squash those two commits together (effectively just mention the fix in the commit message).
Squashing into one works for me. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
As we already pass the whole structure down the call path there's no need to return some stuff in a separate argument. Remove the argument and call tweakers to avoid breaking semantics. virStorageFileGetMetadataFromBuf will be refactored later along with the storage driver. --- src/util/virstoragefile.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c707200..513f15d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -803,7 +803,6 @@ virStorageFileGetMetadataInternal(const char *path, size_t len, int format, virStorageFileMetadataPtr meta, - char **backingStore, int *backingFormat, char **backingDirectory) { @@ -891,10 +890,6 @@ virStorageFileGetMetadataInternal(const char *path, if (VIR_STRDUP(meta->backingStore, meta->backingStoreRaw) < 0) goto cleanup; - if (backingStore && - VIR_STRDUP(*backingStore, meta->backingStoreRaw) < 0) - goto cleanup; - *backingFormat = VIR_STORAGE_FILE_RAW; } } else { @@ -1022,12 +1017,17 @@ virStorageFileGetMetadataFromBuf(const char *path, goto cleanup; if (virStorageFileGetMetadataInternal(path, canonPath, ".", buf, len, - format, ret, backing, + format, ret, backingFormat, NULL) < 0) { virStorageFileFreeMetadata(ret); ret = NULL; } + if (VIR_STRDUP(*backing, ret->backingStoreRaw) < 0) { + virStorageFileFreeMetadata(ret); + ret = NULL; + } + cleanup: VIR_FREE(canonPath); return ret; @@ -1085,7 +1085,6 @@ virStorageFileGetMetadataFromFDInternal(const char *path, ret = virStorageFileGetMetadataInternal(path, canonPath, directory, buf, len, format, meta, - NULL, backingFormat, backingDirectory); if (ret == 0) { -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
As we already pass the whole structure down the call path there's no need to return some stuff in a separate argument. Remove the argument and call tweakers to avoid breaking semantics.
s/call tweakers/tweak callers/
virStorageFileGetMetadataFromBuf will be refactored later along with the storage driver. --- src/util/virstoragefile.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/22/2014 02:42 PM, Eric Blake wrote:
On 04/20/2014 04:13 PM, Peter Krempa wrote:
As we already pass the whole structure down the call path there's no need to return some stuff in a separate argument. Remove the argument and call tweakers to avoid breaking semantics.
s/call tweakers/tweak callers/
virStorageFileGetMetadataFromBuf will be refactored later along with the storage driver. --- src/util/virstoragefile.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
ACK
Oops, spoke too early: please squash this in: diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index 8a5bb01..cb22255 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -795,7 +795,7 @@ 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) +ATTRIBUTE_NONNULL(8) virStorageFileGetMetadataInternal(const char *path, const char *canonPath, const char *directory, -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Move the code checking the presence of the backing file to the recursive worker function instead of the metadata parser. The recursive worker will later be changed to parse more than just local files and this change will help the separation. --- src/util/virstoragefile.c | 63 ++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 513f15d..a005e00 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -795,7 +795,7 @@ 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) +ATTRIBUTE_NONNULL(8) virStorageFileGetMetadataInternal(const char *path, const char *canonPath, const char *directory, @@ -803,8 +803,7 @@ virStorageFileGetMetadataInternal(const char *path, size_t len, int format, virStorageFileMetadataPtr meta, - int *backingFormat, - char **backingDirectory) + int *backingFormat) { int ret = -1; @@ -871,31 +870,6 @@ virStorageFileGetMetadataInternal(const char *path, if (store == BACKING_STORE_ERROR) goto cleanup; - - if (meta->backingStoreRaw) { - if (virStorageIsFile(meta->backingStoreRaw)) { - if (virFindBackingFile(directory, - meta->backingStoreRaw, - backingDirectory, - &meta->backingStore) < 0) { - /* the backing file is (currently) unavailable, treat this - * file as standalone: - * backingStoreRaw is kept to mark broken image chains */ - *backingFormat = VIR_STORAGE_FILE_NONE; - VIR_WARN("Backing file '%s' of image '%s' is missing.", - meta->backingStoreRaw, path); - - } - } else { - if (VIR_STRDUP(meta->backingStore, meta->backingStoreRaw) < 0) - goto cleanup; - - *backingFormat = VIR_STORAGE_FILE_RAW; - } - } else { - meta->backingStore = NULL; - *backingFormat = VIR_STORAGE_FILE_NONE; - } } if (fileTypeInfo[format].getFeatures != NULL && @@ -1018,7 +992,7 @@ virStorageFileGetMetadataFromBuf(const char *path, if (virStorageFileGetMetadataInternal(path, canonPath, ".", buf, len, format, ret, - backingFormat, NULL) < 0) { + backingFormat) < 0) { virStorageFileFreeMetadata(ret); ret = NULL; } @@ -1042,8 +1016,7 @@ virStorageFileGetMetadataFromFDInternal(const char *path, int fd, int format, virStorageFileMetadataPtr meta, - int *backingFormat, - char **backingDirectory) + int *backingFormat) { char *buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; @@ -1085,7 +1058,7 @@ virStorageFileGetMetadataFromFDInternal(const char *path, ret = virStorageFileGetMetadataInternal(path, canonPath, directory, buf, len, format, meta, - backingFormat, backingDirectory); + backingFormat); if (ret == 0) { if (S_ISREG(sb.st_mode)) @@ -1128,7 +1101,7 @@ virStorageFileGetMetadataFromFD(const char *path, goto cleanup; if (virStorageFileGetMetadataFromFDInternal(path, canonPath, ".", fd, format, ret, - NULL, NULL) < 0) { + NULL) < 0) { virStorageFileFreeMetadata(ret); ret = NULL; } @@ -1174,8 +1147,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, directory, fd, format, meta, - &backingFormat, - &backingDirectory); + &backingFormat); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); @@ -1192,7 +1164,26 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, ret = 0; } - if (ret == 0 && meta->backingStore) { + if (ret == 0 && meta->backingStoreRaw) { + if (virStorageIsFile(meta->backingStoreRaw)) { + if (virFindBackingFile(directory, + meta->backingStoreRaw, + &backingDirectory, + &meta->backingStore) < 0) { + /* the backing file is (currently) unavailable, treat this + * file as standalone: + * backingStoreRaw is kept to mark broken image chains */ + VIR_WARN("Backing file '%s' of image '%s' is missing.", + meta->backingStoreRaw, path); + + return 0; + } + } else { + if (VIR_STRDUP(meta->backingStore, meta->backingStoreRaw) < 0) + return -1; + } + + virStorageFileMetadataPtr backing; if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Move the code checking the presence of the backing file to the recursive worker function instead of the metadata parser. The recursive worker will later be changed to parse more than just local files and this change will help the separation. --- src/util/virstoragefile.c | 63 ++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 36 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 513f15d..a005e00 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -795,7 +795,7 @@ 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) +ATTRIBUTE_NONNULL(8)
This hunk belongs in 4/18.
@@ -1192,7 +1164,26 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, ret = 0; }
- if (ret == 0 && meta->backingStore) { + if (ret == 0 && meta->backingStoreRaw) { + if (virStorageIsFile(meta->backingStoreRaw)) { + if (virFindBackingFile(directory, + meta->backingStoreRaw, + &backingDirectory, + &meta->backingStore) < 0) { + /* the backing file is (currently) unavailable, treat this + * file as standalone: + * backingStoreRaw is kept to mark broken image chains */ + VIR_WARN("Backing file '%s' of image '%s' is missing.", + meta->backingStoreRaw, path); + + return 0; + } + } else { + if (VIR_STRDUP(meta->backingStore, meta->backingStoreRaw) < 0) + return -1; + } + + virStorageFileMetadataPtr backing;
This puts a declaration after statement; if you want, you could squash this in. Either way, ACK. diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index 6f0fb79..82b5c65 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -1168,6 +1168,8 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, } if (ret == 0 && meta->backingStoreRaw) { + virStorageFileMetadataPtr backing; + if (virStorageIsFile(meta->backingStoreRaw)) { if (virFindBackingFile(directory, meta->backingStoreRaw, @@ -1186,9 +1188,6 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, return -1; } - - virStorageFileMetadataPtr backing; - if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) backingFormat = VIR_STORAGE_FILE_RAW; else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Avoid passing lot of arguments into guts of metadata retrieval to fill the actual structure. Temporarily fill the structure before passing it down to the actual metadata extractor. This will later help the inversion of the steps taken to extract the metadata so that this function can be fully converted to virStorageSource as the data struct. --- src/util/virstoragefile.c | 164 +++++++++++++++++++++++----------------------- 1 file changed, 81 insertions(+), 83 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a005e00..e37be06 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -788,81 +788,68 @@ qcow2GetFeatures(virBitmapPtr *features, } -/* Given a header in BUF with length LEN, as parsed from the file with - * user-provided name PATH and opened from CANONPATH, and where any - * relative backing file will be opened from DIRECTORY, and assuming - * it has the given FORMAT, populate the newly-allocated META with - * information about the file and its backing store. */ +/* Given a header in BUF with length LEN, as parsed from the storage file + * assuming it has the given FORMAT, populate information into META + * with information about the file and its backing store. Return format + * of the backing store as BACKING_FORMAT. PATH and FORMAT have to be + * pre-populated in META*/ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(7) -ATTRIBUTE_NONNULL(8) -virStorageFileGetMetadataInternal(const char *path, - const char *canonPath, - const char *directory, +ATTRIBUTE_NONNULL(4) +virStorageFileGetMetadataInternal(virStorageFileMetadataPtr meta, char *buf, size_t len, - int format, - virStorageFileMetadataPtr meta, int *backingFormat) { int ret = -1; - VIR_DEBUG("path=%s, canonPath=%s, dir=%s, buf=%p, len=%zu, format=%d", - path, canonPath, directory, buf, len, format); + VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d", + meta->path, buf, len, meta->format); - if (VIR_STRDUP(meta->path, path) < 0) - goto cleanup; - if (VIR_STRDUP(meta->canonPath, canonPath) < 0) - goto cleanup; - if (VIR_STRDUP(meta->relDir, directory) < 0) - goto cleanup; + if (meta->format == VIR_STORAGE_FILE_AUTO) + meta->format = virStorageFileProbeFormatFromBuf(meta->path, buf, len); - if (format == VIR_STORAGE_FILE_AUTO) - format = virStorageFileProbeFormatFromBuf(path, buf, len); - - if (format <= VIR_STORAGE_FILE_NONE || - format >= VIR_STORAGE_FILE_LAST) { - virReportSystemError(EINVAL, _("unknown storage file format %d"), - format); + if (meta->format <= VIR_STORAGE_FILE_NONE || + meta->format >= VIR_STORAGE_FILE_LAST) { + virReportSystemError(EINVAL, _("unknown storage file meta->format %d"), + meta->format); goto cleanup; } - meta->format = format; /* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files */ - if (!fileTypeInfo[format].magic) + if (!fileTypeInfo[meta->format].magic) goto done; /* Optionally extract capacity from file */ - if (fileTypeInfo[format].sizeOffset != -1) { - if ((fileTypeInfo[format].sizeOffset + 8) > len) + if (fileTypeInfo[meta->format].sizeOffset != -1) { + if ((fileTypeInfo[meta->format].sizeOffset + 8) > len) goto done; - if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) + if (fileTypeInfo[meta->format].endian == LV_LITTLE_ENDIAN) meta->capacity = virReadBufInt64LE(buf + - fileTypeInfo[format].sizeOffset); + fileTypeInfo[meta->format].sizeOffset); else meta->capacity = virReadBufInt64BE(buf + - fileTypeInfo[format].sizeOffset); + fileTypeInfo[meta->format].sizeOffset); /* Avoid unlikely, but theoretically possible overflow */ if (meta->capacity > (ULLONG_MAX / - fileTypeInfo[format].sizeMultiplier)) + fileTypeInfo[meta->format].sizeMultiplier)) goto done; - meta->capacity *= fileTypeInfo[format].sizeMultiplier; + meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier; } - if (fileTypeInfo[format].qcowCryptOffset != -1) { + if (fileTypeInfo[meta->format].qcowCryptOffset != -1) { int crypt_format; crypt_format = virReadBufInt32BE(buf + - fileTypeInfo[format].qcowCryptOffset); + fileTypeInfo[meta->format].qcowCryptOffset); if (crypt_format && VIR_ALLOC(meta->encryption) < 0) goto cleanup; } - if (fileTypeInfo[format].getBackingStore != NULL) { - int store = fileTypeInfo[format].getBackingStore(&meta->backingStoreRaw, + if (fileTypeInfo[meta->format].getBackingStore != NULL) { + int store = fileTypeInfo[meta->format].getBackingStore(&meta->backingStoreRaw, backingFormat, buf, len); if (store == BACKING_STORE_INVALID) @@ -872,11 +859,11 @@ virStorageFileGetMetadataInternal(const char *path, goto cleanup; } - if (fileTypeInfo[format].getFeatures != NULL && - fileTypeInfo[format].getFeatures(&meta->features, format, buf, len) < 0) + if (fileTypeInfo[meta->format].getFeatures != NULL && + fileTypeInfo[meta->format].getFeatures(&meta->features, meta->format, buf, len) < 0) goto cleanup; - if (format == VIR_STORAGE_FILE_QCOW2 && meta->features && + if (meta->format == VIR_STORAGE_FILE_QCOW2 && meta->features && VIR_STRDUP(meta->compat, "1.1") < 0) goto cleanup; @@ -946,6 +933,34 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) } +static virStorageFileMetadataPtr +virStorageFileMetadataNew(const char *path, + int format) +{ + virStorageFileMetadataPtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->format = format; + ret->type = VIR_STORAGE_TYPE_FILE; + + if (VIR_STRDUP(ret->path, path) < 0) + goto error; + + if (!(ret->canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + goto error; + } + + return ret; + + error: + virStorageFileFreeMetadata(ret); + return NULL; +} + + /** * virStorageFileGetMetadataFromBuf: * @path: name of file, for error messages @@ -979,19 +994,11 @@ virStorageFileGetMetadataFromBuf(const char *path, int *backingFormat) { virStorageFileMetadataPtr ret = NULL; - char *canonPath; - if (!(canonPath = canonicalize_file_name(path)) && - VIR_STRDUP(canonPath, path) < 0) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (!(ret = virStorageFileMetadataNew(path, format))) return NULL; - } - - if (VIR_ALLOC(ret) < 0) - goto cleanup; - if (virStorageFileGetMetadataInternal(path, canonPath, ".", buf, len, - format, ret, + if (virStorageFileGetMetadataInternal(ret, buf, len, backingFormat) < 0) { virStorageFileFreeMetadata(ret); ret = NULL; @@ -1002,20 +1009,14 @@ virStorageFileGetMetadataFromBuf(const char *path, ret = NULL; } - cleanup: - VIR_FREE(canonPath); return ret; } /* Internal version that also supports a containing directory name. */ static int -virStorageFileGetMetadataFromFDInternal(const char *path, - const char *canonPath, - const char *directory, +virStorageFileGetMetadataFromFDInternal(virStorageFileMetadataPtr meta, int fd, - int format, - virStorageFileMetadataPtr meta, int *backingFormat) { char *buf = NULL; @@ -1026,11 +1027,13 @@ virStorageFileGetMetadataFromFDInternal(const char *path, if (!backingFormat) backingFormat = &dummy; + *backingFormat = VIR_STORAGE_FILE_NONE; + if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), - path); + meta->path); return -1; } @@ -1038,8 +1041,8 @@ virStorageFileGetMetadataFromFDInternal(const char *path, /* No header to probe for directories, but also no backing * file; therefore, no inclusion loop is possible, and we * don't need canonName or relDir. */ - if (VIR_STRDUP(meta->path, path) < 0) - goto cleanup; + VIR_FREE(meta->relDir); + VIR_FREE(meta->canonPath); meta->type = VIR_STORAGE_TYPE_DIR; meta->format = VIR_STORAGE_FILE_DIR; ret = 0; @@ -1047,18 +1050,16 @@ virStorageFileGetMetadataFromFDInternal(const char *path, } if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { - virReportSystemError(errno, _("cannot seek to start of '%s'"), path); + virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->path); goto cleanup; } if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), path); + virReportSystemError(errno, _("cannot read header '%s'"), meta->path); goto cleanup; } - ret = virStorageFileGetMetadataInternal(path, canonPath, directory, - buf, len, format, meta, - backingFormat); + ret = virStorageFileGetMetadataInternal(meta, buf, len, backingFormat); if (ret == 0) { if (S_ISREG(sb.st_mode)) @@ -1071,7 +1072,6 @@ virStorageFileGetMetadataFromFDInternal(const char *path, return ret; } - /** * virStorageFileGetMetadataFromFD: * @@ -1091,23 +1091,16 @@ virStorageFileGetMetadataFromFD(const char *path, int format) { virStorageFileMetadataPtr ret = NULL; - char *canonPath; - if (!(canonPath = canonicalize_file_name(path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); - return NULL; - } - if (VIR_ALLOC(ret) < 0) + if (!(ret = virStorageFileMetadataNew(path, format))) goto cleanup; - if (virStorageFileGetMetadataFromFDInternal(path, canonPath, ".", - fd, format, ret, - NULL) < 0) { + + if (virStorageFileGetMetadataFromFDInternal(ret, fd, NULL) < 0) { virStorageFileFreeMetadata(ret); ret = NULL; } cleanup: - VIR_FREE(canonPath); return ret; } @@ -1139,15 +1132,20 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, return -1; if (virStorageIsFile(path)) { + if (VIR_STRDUP(meta->path, path) < 0) + return -1; + if (VIR_STRDUP(meta->canonPath, canonPath) < 0) + return -1; + if (VIR_STRDUP(meta->relDir, directory) < 0) + return -1; + meta->format = format; + 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, - &backingFormat); + ret = virStorageFileGetMetadataFromFDInternal(meta, fd, &backingFormat); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Avoid passing lot of arguments into guts of metadata retrieval to fill the actual structure. Temporarily fill the structure before passing it down to the actual metadata extractor.
This will later help the inversion of the steps taken to extract the metadata so that this function can be fully converted to virStorageSource as the data struct. --- src/util/virstoragefile.c | 164 +++++++++++++++++++++++----------------------- 1 file changed, 81 insertions(+), 83 deletions(-)
-/* Given a header in BUF with length LEN, as parsed from the file with - * user-provided name PATH and opened from CANONPATH, and where any - * relative backing file will be opened from DIRECTORY, and assuming - * it has the given FORMAT, populate the newly-allocated META with - * information about the file and its backing store. */ +/* Given a header in BUF with length LEN, as parsed from the storage file + * assuming it has the given FORMAT, populate information into META + * with information about the file and its backing store. Return format + * of the backing store as BACKING_FORMAT. PATH and FORMAT have to be + * pre-populated in META*/
Cosmetic - I usually stick space before */
{ int ret = -1;
- VIR_DEBUG("path=%s, canonPath=%s, dir=%s, buf=%p, len=%zu, format=%d", - path, canonPath, directory, buf, len, format); + VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d", + meta->path, buf, len, meta->format);
The diff would be slightly smaller if you do: int format = meta->format; up front, then keep the original use of format everywhere else...
- if (VIR_STRDUP(meta->path, path) < 0) - goto cleanup; - if (VIR_STRDUP(meta->canonPath, canonPath) < 0) - goto cleanup; - if (VIR_STRDUP(meta->relDir, directory) < 0) - goto cleanup; + if (meta->format == VIR_STORAGE_FILE_AUTO) + meta->format = virStorageFileProbeFormatFromBuf(meta->path, buf, len);
...well, right here, you'd have to do 'format = meta->format = virStorage...();', but the rest of the function has fewer changes. But it doesn't matter to me; I'm fine keeping it as you wrote it.
+static virStorageFileMetadataPtr +virStorageFileMetadataNew(const char *path, + int format) +{ + virStorageFileMetadataPtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->format = format; + ret->type = VIR_STORAGE_TYPE_FILE; + + if (VIR_STRDUP(ret->path, path) < 0) + goto error; + + if (!(ret->canonPath = canonicalize_file_name(path))) {
Same complaint as in 3/18 - you should only call canonicalize_file_name() if you KNOW that path should be interpreted as a file name.
@@ -979,19 +994,11 @@ virStorageFileGetMetadataFromBuf(const char *path, int *backingFormat) { virStorageFileMetadataPtr ret = NULL; - char *canonPath;
- if (!(canonPath = canonicalize_file_name(path)) && - VIR_STRDUP(canonPath, path) < 0) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (!(ret = virStorageFileMetadataNew(path, format))) return NULL; - }
Minor merge conflicts with my suggested resolution to 3/18 - but should be obvious resolution.
@@ -1071,7 +1072,6 @@ virStorageFileGetMetadataFromFDInternal(const char *path, return ret; }
- /**
Spurious whitespace change. ACK with this squashed in (and any additional trivial cleanups you want to optionally do based on my comments above): diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index b0fe2ae..ff8de43 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -948,8 +948,12 @@ virStorageFileMetadataNew(const char *path, if (VIR_STRDUP(ret->path, path) < 0) goto error; - if (!(ret->canonPath = canonicalize_file_name(path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (virStorageIsFile(path)) { + if (!(ret->canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + goto error; + } + } else if (VIR_STRDUP(ret->canonPath, path) < 0) { goto error; } @@ -1072,6 +1076,7 @@ virStorageFileGetMetadataFromFDInternal(virStorageFileMetadataPtr meta, return ret; } + /** * virStorageFileGetMetadataFromFD: * -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To allow future change of virStorageFileMetadata to virStorageSource we need to store a full path in the "path" variable as rest of the code expects it to be a full path. Rename the "path" field to "relPath" to keep tracking the info but allowing a real "path" field. --- src/util/virstoragefile.c | 22 +++++++++++----------- src/util/virstoragefile.h | 2 +- tests/virstoragetest.c | 6 +++--- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e37be06..f11dd36 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -802,11 +802,11 @@ virStorageFileGetMetadataInternal(virStorageFileMetadataPtr meta, { int ret = -1; - VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d", - meta->path, buf, len, meta->format); + VIR_DEBUG("relPath=%s, buf=%p, len=%zu, meta->format=%d", + meta->relPath, buf, len, meta->format); if (meta->format == VIR_STORAGE_FILE_AUTO) - meta->format = virStorageFileProbeFormatFromBuf(meta->path, buf, len); + meta->format = virStorageFileProbeFormatFromBuf(meta->relPath, buf, len); if (meta->format <= VIR_STORAGE_FILE_NONE || meta->format >= VIR_STORAGE_FILE_LAST) { @@ -945,7 +945,7 @@ virStorageFileMetadataNew(const char *path, ret->format = format; ret->type = VIR_STORAGE_TYPE_FILE; - if (VIR_STRDUP(ret->path, path) < 0) + if (VIR_STRDUP(ret->relPath, path) < 0) goto error; if (!(ret->canonPath = canonicalize_file_name(path))) { @@ -1033,7 +1033,7 @@ virStorageFileGetMetadataFromFDInternal(virStorageFileMetadataPtr meta, if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), - meta->path); + meta->relPath); return -1; } @@ -1050,12 +1050,12 @@ virStorageFileGetMetadataFromFDInternal(virStorageFileMetadataPtr meta, } if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { - virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->path); + virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->relPath); goto cleanup; } if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), meta->path); + virReportSystemError(errno, _("cannot read header '%s'"), meta->relPath); goto cleanup; } @@ -1132,7 +1132,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, return -1; if (virStorageIsFile(path)) { - if (VIR_STRDUP(meta->path, path) < 0) + if (VIR_STRDUP(meta->relPath, path) < 0) return -1; if (VIR_STRDUP(meta->canonPath, canonPath) < 0) return -1; @@ -1153,7 +1153,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, /* 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) + if (VIR_STRDUP(meta->relPath, path) < 0) return -1; if (VIR_STRDUP(meta->canonPath, path) < 0) return -1; @@ -1316,7 +1316,7 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) if (!meta) return; - VIR_FREE(meta->path); + VIR_FREE(meta->relPath); VIR_FREE(meta->canonPath); VIR_FREE(meta->relDir); @@ -1548,7 +1548,7 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, if (!chain->backingMeta) break; } else { - if (STREQ(name, chain->path)) + if (STREQ(name, chain->relPath)) break; if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || chain->type == VIR_STORAGE_TYPE_BLOCK)) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1b8b14f..867214e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -121,7 +121,7 @@ typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { /* Name of the current file as spelled by the user (top level) or * metadata of the overlay (if this is a backing store). */ - char *path; + char *relPath; /* Canonical name of the current file, used to detect loops in the * backing store chain. */ char *canonPath; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 820e53f..b0de438 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -323,7 +323,7 @@ testStorageChain(const void *args) : data->files[i]->relDirRel; if (virAsprintf(&expect, "store:%s\nraw:%s\nother:%lld %d\n" - "path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", + "relPath:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(data->files[i]->expBackingStore), NULLSTR(data->files[i]->expBackingStoreRaw), data->files[i]->expCapacity, @@ -335,11 +335,11 @@ testStorageChain(const void *args) data->files[i]->format) < 0 || virAsprintf(&actual, "store:%s\nraw:%s\nother:%lld %d\n" - "path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", + "relPath:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(elt->backingStore), NULLSTR(elt->backingStoreRaw), elt->capacity, !!elt->encryption, - NULLSTR(elt->path), + NULLSTR(elt->relPath), NULLSTR(elt->canonPath), NULLSTR(elt->relDir), elt->type, elt->format) < 0) { -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
To allow future change of virStorageFileMetadata to virStorageSource we need to store a full path in the "path" variable as rest of the code expects it to be a full path. Rename the "path" field to "relPath" to keep tracking the info but allowing a real "path" field. --- src/util/virstoragefile.c | 22 +++++++++++----------- src/util/virstoragefile.h | 2 +- tests/virstoragetest.c | 6 +++--- 3 files changed, 15 insertions(+), 15 deletions(-)
@@ -945,7 +945,7 @@ virStorageFileMetadataNew(const char *path, ret->format = format; ret->type = VIR_STORAGE_TYPE_FILE;
- if (VIR_STRDUP(ret->path, path) < 0) + if (VIR_STRDUP(ret->relPath, path) < 0)
Obvious merge conflict resolution here with my proposed fixup to 6/18. Mechanical; ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
As for the previous patch, this change is needed to achieve compatibility with all the existing code, where we expect a fully qualified path of local files to be present. --- src/util/virstoragefile.c | 18 +++++++-------- src/util/virstoragefile.h | 2 +- tests/virstoragetest.c | 56 +++++++++++++++++++++++------------------------ 3 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f11dd36..d4d8427 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -948,7 +948,7 @@ virStorageFileMetadataNew(const char *path, if (VIR_STRDUP(ret->relPath, path) < 0) goto error; - if (!(ret->canonPath = canonicalize_file_name(path))) { + if (!(ret->path = canonicalize_file_name(path))) { virReportSystemError(errno, _("unable to resolve '%s'"), path); goto error; } @@ -1042,7 +1042,7 @@ virStorageFileGetMetadataFromFDInternal(virStorageFileMetadataPtr meta, * file; therefore, no inclusion loop is possible, and we * don't need canonName or relDir. */ VIR_FREE(meta->relDir); - VIR_FREE(meta->canonPath); + VIR_FREE(meta->path); meta->type = VIR_STORAGE_TYPE_DIR; meta->format = VIR_STORAGE_FILE_DIR; ret = 0; @@ -1134,7 +1134,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, if (virStorageIsFile(path)) { if (VIR_STRDUP(meta->relPath, path) < 0) return -1; - if (VIR_STRDUP(meta->canonPath, canonPath) < 0) + if (VIR_STRDUP(meta->path, canonPath) < 0) return -1; if (VIR_STRDUP(meta->relDir, directory) < 0) return -1; @@ -1155,7 +1155,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, * allow for non-raw images. */ if (VIR_STRDUP(meta->relPath, path) < 0) return -1; - if (VIR_STRDUP(meta->canonPath, path) < 0) + if (VIR_STRDUP(meta->path, path) < 0) return -1; meta->type = VIR_STORAGE_TYPE_NETWORK; meta->format = VIR_STORAGE_FILE_RAW; @@ -1317,7 +1317,7 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) return; VIR_FREE(meta->relPath); - VIR_FREE(meta->canonPath); + VIR_FREE(meta->path); VIR_FREE(meta->relDir); virStorageFileFreeMetadata(meta->backingMeta); @@ -1534,7 +1534,7 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *name, virStorageFileMetadataPtr *meta, const char **parent) { - const char *start = chain->canonPath; + const char *start = chain->path; const char *tmp; const char *parentDir = "."; bool nameIsFile = virStorageIsFile(name); @@ -1553,14 +1553,14 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || chain->type == VIR_STORAGE_TYPE_BLOCK)) { int result = virFileRelLinkPointsTo(parentDir, name, - chain->canonPath); + chain->path); if (result < 0) goto error; if (result > 0) break; } } - *parent = chain->canonPath; + *parent = chain->path; parentDir = chain->relDir; chain = chain->backingMeta; } @@ -1568,7 +1568,7 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, goto error; if (meta) *meta = chain; - return chain->canonPath; + return chain->path; error: if (name) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 867214e..3f072b6 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -124,7 +124,7 @@ struct _virStorageFileMetadata { char *relPath; /* Canonical name of the current file, used to detect loops in the * backing store chain. */ - char *canonPath; + char *path; /* Directory to start from if backingStoreRaw is a relative file * name. */ char *relDir; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index b0de438..dabaa99 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -237,7 +237,7 @@ struct _testFileData bool expEncrypted; const char *pathRel; const char *pathAbs; - const char *canonPath; + const char *path; const char *relDirRel; const char *relDirAbs; int type; @@ -323,24 +323,24 @@ testStorageChain(const void *args) : data->files[i]->relDirRel; if (virAsprintf(&expect, "store:%s\nraw:%s\nother:%lld %d\n" - "relPath:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", + "relPath:%s\npath:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(data->files[i]->expBackingStore), NULLSTR(data->files[i]->expBackingStoreRaw), data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(expPath), - NULLSTR(data->files[i]->canonPath), + NULLSTR(data->files[i]->path), NULLSTR(expRelDir), data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, "store:%s\nraw:%s\nother:%lld %d\n" - "relPath:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", + "relPath:%s\npath:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(elt->backingStore), NULLSTR(elt->backingStoreRaw), elt->capacity, !!elt->encryption, NULLSTR(elt->relPath), - NULLSTR(elt->canonPath), + NULLSTR(elt->path), NULLSTR(elt->relDir), elt->type, elt->format) < 0) { VIR_FREE(expect); @@ -491,7 +491,7 @@ mymain(void) testFileData raw = { .pathRel = "raw", .pathAbs = canonraw, - .canonPath = canonraw, + .path = canonraw, .relDirRel = ".", .relDirAbs = datadir, .type = VIR_STORAGE_TYPE_FILE, @@ -516,7 +516,7 @@ mymain(void) .expCapacity = 1024, .pathRel = "qcow2", .pathAbs = canonqcow2, - .canonPath = canonqcow2, + .path = canonqcow2, .relDirRel = ".", .relDirAbs = datadir, .type = VIR_STORAGE_TYPE_FILE, @@ -525,7 +525,7 @@ mymain(void) testFileData qcow2_as_raw = { .pathRel = "qcow2", .pathAbs = canonqcow2, - .canonPath = canonqcow2, + .path = canonqcow2, .relDirRel = ".", .relDirAbs = datadir, .type = VIR_STORAGE_TYPE_FILE, @@ -572,7 +572,7 @@ mymain(void) .expCapacity = 1024, .pathRel = "wrap", .pathAbs = abswrap, - .canonPath = canonwrap, + .path = canonwrap, .relDirRel = ".", .relDirAbs = datadir, .type = VIR_STORAGE_TYPE_FILE, @@ -608,7 +608,7 @@ mymain(void) .expCapacity = 1024, .pathRel = "wrap", .pathAbs = abswrap, - .canonPath = canonwrap, + .path = canonwrap, .relDirRel = ".", .relDirAbs = datadir, .type = VIR_STORAGE_TYPE_FILE, @@ -667,7 +667,7 @@ mymain(void) testFileData nbd = { .pathRel = "nbd:example.org:6000", .pathAbs = "nbd:example.org:6000", - .canonPath = "nbd:example.org:6000", + .path = "nbd:example.org:6000", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, }; @@ -684,7 +684,7 @@ mymain(void) .expCapacity = 1024, .pathRel = "qed", .pathAbs = absqed, - .canonPath = canonqed, + .path = canonqed, .relDirRel = ".", .relDirAbs = datadir, .type = VIR_STORAGE_TYPE_FILE, @@ -693,7 +693,7 @@ mymain(void) testFileData qed_as_raw = { .pathRel = "qed", .pathAbs = absqed, - .canonPath = canonqed, + .path = canonqed, .relDirRel = ".", .relDirAbs = datadir, .type = VIR_STORAGE_TYPE_FILE, @@ -746,7 +746,7 @@ mymain(void) .expCapacity = 1024, .pathRel = "../sub/link1", .pathAbs = "../sub/link1", - .canonPath = canonqcow2, + .path = canonqcow2, .relDirRel = "sub/../sub", .relDirAbs = datadir "/sub/../sub", .type = VIR_STORAGE_TYPE_FILE, @@ -758,7 +758,7 @@ mymain(void) .expCapacity = 1024, .pathRel = "sub/link2", .pathAbs = abslink2, - .canonPath = canonwrap, + .path = canonwrap, .relDirRel = "sub", .relDirAbs = datadir "/sub", .type = VIR_STORAGE_TYPE_FILE, @@ -839,12 +839,12 @@ mymain(void) } while (0) TEST_LOOKUP(0, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(1, "wrap", chain->canonPath, chain, NULL); - TEST_LOOKUP(2, abswrap, chain->canonPath, chain, NULL); + TEST_LOOKUP(1, "wrap", chain->path, chain, NULL); + TEST_LOOKUP(2, abswrap, chain->path, chain, NULL); TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta, - chain->canonPath); + chain->path); TEST_LOOKUP(4, absqcow2, chain->backingStore, chain->backingMeta, - chain->canonPath); + chain->path); TEST_LOOKUP(5, "raw", chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); TEST_LOOKUP(6, absraw, chain->backingMeta->backingStore, @@ -875,12 +875,12 @@ mymain(void) } TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(9, "wrap", chain->canonPath, chain, NULL); - TEST_LOOKUP(10, abswrap, chain->canonPath, chain, NULL); + TEST_LOOKUP(9, "wrap", chain->path, chain, NULL); + TEST_LOOKUP(10, abswrap, chain->path, chain, NULL); TEST_LOOKUP(11, "qcow2", chain->backingStore, chain->backingMeta, - chain->canonPath); + chain->path); TEST_LOOKUP(12, absqcow2, chain->backingStore, chain->backingMeta, - chain->canonPath); + chain->path); TEST_LOOKUP(13, "raw", chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); TEST_LOOKUP(14, absraw, chain->backingMeta->backingStore, @@ -905,14 +905,14 @@ mymain(void) } TEST_LOOKUP(16, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(17, "sub/link2", chain->canonPath, chain, NULL); - TEST_LOOKUP(18, "wrap", chain->canonPath, chain, NULL); - TEST_LOOKUP(19, abswrap, chain->canonPath, chain, NULL); + TEST_LOOKUP(17, "sub/link2", chain->path, chain, NULL); + TEST_LOOKUP(18, "wrap", chain->path, chain, NULL); + TEST_LOOKUP(19, abswrap, chain->path, chain, NULL); TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, - chain->canonPath); + chain->path); TEST_LOOKUP(21, "qcow2", NULL, NULL, NULL); TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, - chain->canonPath); + chain->path); TEST_LOOKUP(23, "raw", chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); TEST_LOOKUP(24, absraw, chain->backingMeta->backingStore, -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
As for the previous patch, this change is needed to achieve compatibility with all the existing code, where we expect a fully qualified path of local files to be present. --- src/util/virstoragefile.c | 18 +++++++-------- src/util/virstoragefile.h | 2 +- tests/virstoragetest.c | 56 +++++++++++++++++++++++------------------------ 3 files changed, 38 insertions(+), 38 deletions(-)
+++ b/src/util/virstoragefile.c @@ -948,7 +948,7 @@ virStorageFileMetadataNew(const char *path, if (VIR_STRDUP(ret->relPath, path) < 0) goto error;
- if (!(ret->canonPath = canonicalize_file_name(path))) { + if (!(ret->path = canonicalize_file_name(path))) {
Another merge conflict with my earlier squash-in requests. Should be an obvious fix. Mechanical; ACK.
+++ b/tests/virstoragetest.c @@ -237,7 +237,7 @@ struct _testFileData bool expEncrypted; const char *pathRel; const char *pathAbs; - const char *canonPath; + const char *path;
I'm not sure I would have renamed this field. But I guess it doesn't hurt. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
As a temporary step to allow killing of the "backingStore" field of struct virStorageFileMetadata the recursive metadata retrieval function will be converted not to use the field in the lookup process. --- src/util/virstoragefile.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d4d8427..2077839 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1116,6 +1116,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, int fd; int ret = -1; int backingFormat; + char *backingPath = NULL; char *backingDirectory = NULL; VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", @@ -1167,7 +1168,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, if (virFindBackingFile(directory, meta->backingStoreRaw, &backingDirectory, - &meta->backingStore) < 0) { + &backingPath) < 0) { /* the backing file is (currently) unavailable, treat this * file as standalone: * backingStoreRaw is kept to mark broken image chains */ @@ -1177,10 +1178,12 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, return 0; } } else { - if (VIR_STRDUP(meta->backingStore, meta->backingStoreRaw) < 0) + if (VIR_STRDUP(backingPath, meta->backingStoreRaw) < 0) return -1; } + if (VIR_STRDUP(meta->backingStore, backingPath) < 0) + return -1; virStorageFileMetadataPtr backing; @@ -1190,7 +1193,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, backingFormat = VIR_STORAGE_FILE_AUTO; if (VIR_ALLOC(backing) < 0 || virStorageFileGetMetadataRecurse(meta->backingStoreRaw, - meta->backingStore, + backingPath, backingDirectory, backingFormat, uid, gid, allow_probe, cycle, backing) < 0) { @@ -1201,7 +1204,9 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, meta->backingMeta = backing; } } + VIR_FREE(backingDirectory); + VIR_FREE(backingPath); return ret; } -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
As a temporary step to allow killing of the "backingStore" field of struct virStorageFileMetadata the recursive metadata retrieval function will be converted not to use the field in the lookup process. --- src/util/virstoragefile.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
@@ -1177,10 +1178,12 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, return 0; } } else { - if (VIR_STRDUP(meta->backingStore, meta->backingStoreRaw) < 0) + if (VIR_STRDUP(backingPath, meta->backingStoreRaw) < 0) return -1; }
+ if (VIR_STRDUP(meta->backingStore, backingPath) < 0) + return -1;
virStorageFileMetadataPtr backing;
Merge conflict with my suggested changes to 4/18, should be easy enough to fix. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Remove the obsolete field replaced by data in "path". The testsuite requires tweaking as the name of the backing file is now stored one layer deeper in the backking chain linked list. --- src/conf/domain_conf.c | 13 ++++++------ src/qemu/qemu_driver.c | 8 ++++---- src/util/virstoragefile.c | 5 ----- src/util/virstoragefile.h | 5 ----- tests/virstoragetest.c | 50 +++++++++++++++++++++++------------------------ 5 files changed, 36 insertions(+), 45 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 05fa3f9..006aa96 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18565,16 +18565,17 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (iter(disk, path, 0, opaque) < 0) goto cleanup; - - tmp = disk->backingChain; - while (tmp && virStorageIsFile(tmp->backingStore)) { - if (!ignoreOpenFailure && !tmp->backingMeta) { + /* XXX: temporarily we need to select the second element of the backing + * chain to start as the first is the copy of the disk itself. */ + tmp = disk->backingChain ? disk->backingChain->backingMeta : NULL; + while (tmp && virStorageIsFile(tmp->path)) { + if (!ignoreOpenFailure && tmp->backingStoreRaw && !tmp->backingMeta) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to visit backing chain file %s"), - tmp->backingStore); + tmp->backingStoreRaw); goto cleanup; } - if (iter(disk, tmp->backingStore, ++depth, opaque) < 0) + if (iter(disk, tmp->path, ++depth, opaque) < 0) goto cleanup; tmp = tmp->backingMeta; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e58fa3f..c242104 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15134,7 +15134,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) && STREQ_NULLABLE(format, "raw") && - disk->backingChain->backingStore) { + disk->backingChain->backingMeta->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' has backing file, so raw shallow copy " "is not possible"), @@ -15347,14 +15347,14 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, &top_parent))) { goto endjob; } - if (!top_meta || !top_meta->backingStore) { + if (!top_meta || !top_meta->backingMeta) { virReportError(VIR_ERR_INVALID_ARG, _("top '%s' in chain for '%s' has no backing file"), top_canon, path); goto endjob; } if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) - base_canon = top_meta->backingStore; + base_canon = top_meta->backingMeta->path; else if (!(base_canon = virStorageFileChainLookup(top_meta, base, NULL, NULL))) goto endjob; @@ -15363,7 +15363,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, * virStorageFileChainLookup guarantees a simple pointer * comparison will work, rather than needing full-blown STREQ. */ if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && - base_canon != top_meta->backingStore) { + base_canon != top_meta->backingMeta->path) { virReportError(VIR_ERR_INVALID_ARG, _("base '%s' is not immediately below '%s' in chain " "for '%s'"), diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2077839..2d6be17 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1182,9 +1182,6 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, return -1; } - if (VIR_STRDUP(meta->backingStore, backingPath) < 0) - return -1; - virStorageFileMetadataPtr backing; if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) @@ -1198,7 +1195,6 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, uid, gid, allow_probe, cycle, backing) < 0) { /* If we failed to get backing data, mark the chain broken */ - VIR_FREE(meta->backingStore); virStorageFileFreeMetadata(backing); } else { meta->backingMeta = backing; @@ -1326,7 +1322,6 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) VIR_FREE(meta->relDir); virStorageFileFreeMetadata(meta->backingMeta); - VIR_FREE(meta->backingStore); VIR_FREE(meta->backingStoreRaw); VIR_FREE(meta->compat); virBitmapFree(meta->features); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3f072b6..2e9312f 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -148,11 +148,6 @@ struct _virStorageFileMetadata { unsigned long long capacity; virBitmapPtr features; /* bits described by enum virStorageFileFeature */ char *compat; - - /* Fields I'm trying to delete, because it is confusing to have to - * query the parent metadata for details about the backing - * store. */ - char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as backingMeta->canonPath */ }; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index dabaa99..646c15e 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -336,7 +336,7 @@ testStorageChain(const void *args) virAsprintf(&actual, "store:%s\nraw:%s\nother:%lld %d\n" "relPath:%s\npath:%s\nrelDir:%s\ntype:%d %d\n", - NULLSTR(elt->backingStore), + NULLSTR(elt->backingMeta ? elt->backingMeta->path : NULL), NULLSTR(elt->backingStoreRaw), elt->capacity, !!elt->encryption, NULLSTR(elt->relPath), @@ -841,16 +841,16 @@ mymain(void) TEST_LOOKUP(0, "bogus", NULL, NULL, NULL); TEST_LOOKUP(1, "wrap", chain->path, chain, NULL); TEST_LOOKUP(2, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta, + TEST_LOOKUP(3, "qcow2", chain->backingMeta->path, chain->backingMeta, chain->path); - TEST_LOOKUP(4, absqcow2, chain->backingStore, chain->backingMeta, + TEST_LOOKUP(4, absqcow2, chain->backingMeta->path, chain->backingMeta, chain->path); - TEST_LOOKUP(5, "raw", chain->backingMeta->backingStore, - chain->backingMeta->backingMeta, chain->backingStore); - TEST_LOOKUP(6, absraw, chain->backingMeta->backingStore, - chain->backingMeta->backingMeta, chain->backingStore); - TEST_LOOKUP(7, NULL, chain->backingMeta->backingStore, - chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(5, "raw", chain->backingMeta->backingMeta->path, + chain->backingMeta->backingMeta, chain->backingMeta->path); + TEST_LOOKUP(6, absraw, chain->backingMeta->backingMeta->path, + chain->backingMeta->backingMeta, chain->backingMeta->path); + TEST_LOOKUP(7, NULL, chain->backingMeta->backingMeta->path, + chain->backingMeta->backingMeta, chain->backingMeta->path); /* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */ virCommandFree(cmd); @@ -877,16 +877,16 @@ mymain(void) TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); TEST_LOOKUP(9, "wrap", chain->path, chain, NULL); TEST_LOOKUP(10, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(11, "qcow2", chain->backingStore, chain->backingMeta, + TEST_LOOKUP(11, "qcow2", chain->backingMeta->path, chain->backingMeta, chain->path); - TEST_LOOKUP(12, absqcow2, chain->backingStore, chain->backingMeta, + TEST_LOOKUP(12, absqcow2, chain->backingMeta->path, chain->backingMeta, chain->path); - TEST_LOOKUP(13, "raw", chain->backingMeta->backingStore, - chain->backingMeta->backingMeta, chain->backingStore); - TEST_LOOKUP(14, absraw, chain->backingMeta->backingStore, - chain->backingMeta->backingMeta, chain->backingStore); - TEST_LOOKUP(15, NULL, chain->backingMeta->backingStore, - chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(13, "raw", chain->backingMeta->backingMeta->path, + chain->backingMeta->backingMeta, chain->backingMeta->path); + TEST_LOOKUP(14, absraw, chain->backingMeta->backingMeta->path, + chain->backingMeta->backingMeta, chain->backingMeta->path); + TEST_LOOKUP(15, NULL, chain->backingMeta->backingMeta->path, + chain->backingMeta->backingMeta, chain->backingMeta->path); /* Use link to wrap with cross-directory relative backing */ virCommandFree(cmd); @@ -908,17 +908,17 @@ mymain(void) TEST_LOOKUP(17, "sub/link2", chain->path, chain, NULL); TEST_LOOKUP(18, "wrap", chain->path, chain, NULL); TEST_LOOKUP(19, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, + TEST_LOOKUP(20, "../qcow2", chain->backingMeta->path, chain->backingMeta, chain->path); TEST_LOOKUP(21, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, + TEST_LOOKUP(22, absqcow2, chain->backingMeta->path, chain->backingMeta, chain->path); - TEST_LOOKUP(23, "raw", chain->backingMeta->backingStore, - chain->backingMeta->backingMeta, chain->backingStore); - TEST_LOOKUP(24, absraw, chain->backingMeta->backingStore, - chain->backingMeta->backingMeta, chain->backingStore); - TEST_LOOKUP(25, NULL, chain->backingMeta->backingStore, - chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(23, "raw", chain->backingMeta->backingMeta->path, + chain->backingMeta->backingMeta, chain->backingMeta->path); + TEST_LOOKUP(24, absraw, chain->backingMeta->backingMeta->path, + chain->backingMeta->backingMeta, chain->backingMeta->path); + TEST_LOOKUP(25, NULL, chain->backingMeta->backingMeta->path, + chain->backingMeta->backingMeta, chain->backingMeta->path); cleanup: /* Final cleanup */ -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Remove the obsolete field replaced by data in "path".
The testsuite requires tweaking as the name of the backing file is now stored one layer deeper in the backking chain linked list.
s/backking/backing/
--- src/conf/domain_conf.c | 13 ++++++------ src/qemu/qemu_driver.c | 8 ++++---- src/util/virstoragefile.c | 5 ----- src/util/virstoragefile.h | 5 ----- tests/virstoragetest.c | 50 +++++++++++++++++++++++------------------------ 5 files changed, 36 insertions(+), 45 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 05fa3f9..006aa96 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18565,16 +18565,17 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
if (iter(disk, path, 0, opaque) < 0) goto cleanup; - - tmp = disk->backingChain; - while (tmp && virStorageIsFile(tmp->backingStore)) { - if (!ignoreOpenFailure && !tmp->backingMeta) { + /* XXX: temporarily we need to select the second element of the backing + * chain to start as the first is the copy of the disk itself. */ + tmp = disk->backingChain ? disk->backingChain->backingMeta : NULL; + while (tmp && virStorageIsFile(tmp->path)) {
For that matter, if a future patch allows a file to be the backing store of a network path, we don't want to stop the iteration just because there was nothing to label on the intermediate network resource. So this isn't the last time we will be touching this function :)
+++ b/src/util/virstoragefile.h @@ -148,11 +148,6 @@ struct _virStorageFileMetadata { unsigned long long capacity; virBitmapPtr features; /* bits described by enum virStorageFileFeature */ char *compat; - - /* Fields I'm trying to delete, because it is confusing to have to - * query the parent metadata for details about the backing - * store. */ - char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as backingMeta->canonPath */ };
Yay - you finished the efforts I started. The fact that you were able to pick up where I left off is reassuring - it means the design is indeed progressing to something a bit more reasonable to understand, and that I explained my goals fairly well. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add a free function as some parts of the code will allocate the structure. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 11 +++++++++++ src/util/virstoragefile.h | 1 + 3 files changed, 13 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0e81f2f..69c986f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1843,6 +1843,7 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceClear; +virStorageSourceFree; virStorageSourceGetActualType; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2d6be17..7f853a4 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1712,3 +1712,14 @@ virStorageSourceClear(virStorageSourcePtr def) virStorageNetHostDefFree(def->nhosts, def->hosts); virStorageSourceAuthClear(def); } + + +void +virStorageSourceFree(virStorageSourcePtr def) +{ + if (!def) + return; + + virStorageSourceClear(def); + VIR_FREE(def); +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 2e9312f..0e5136e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -333,5 +333,6 @@ void virStorageSourceAuthClear(virStorageSourcePtr def); void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); +void virStorageSourceFree(virStorageSourcePtr def); #endif /* __VIR_STORAGE_FILE_H__ */ -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote: s/Prt/Ptr/ in the subject
Add a free function as some parts of the code will allocate the structure. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 11 +++++++++++ src/util/virstoragefile.h | 1 + 3 files changed, 13 insertions(+)
ACK with that fix. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add the required fields that are missing from the new structure that will allow us to switch the storage file metadata code entirely to the new structure. Add "relPath" and "relDir" and the raw backing store name. Also allow creating linked lists of virStorageSourcePtrs to express backing chains. --- src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7f853a4..f519d82 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1711,6 +1711,13 @@ virStorageSourceClear(virStorageSourcePtr def) virStorageNetHostDefFree(def->nhosts, def->hosts); virStorageSourceAuthClear(def); + + VIR_FREE(def->relPath); + VIR_FREE(def->relDir); + VIR_FREE(def->backingStoreRaw); + + /* recursively free backing chain */ + virStorageSourceFree(def->backingMeta); } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 0e5136e..11e25eb 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -271,8 +271,22 @@ struct _virStorageSource { size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; + /* backing chain of the storage source */ + virStorageSourcePtr backingMeta; + /* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv; + + /* metadata about storage image which need separate fields */ + /* Name of the current file as spelled by the user (top level) or + * metadata of the overlay (if this is a backing store). */ + char *relPath; + /* Directory to start from if backingStoreRaw is a relative file + * name. */ + char *relDir; + /* Name of the child backing store recorded in metadata of the + * current file. */ + char *backingStoreRaw; }; -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Add the required fields that are missing from the new structure that will allow us to switch the storage file metadata code entirely to the new structure.
Add "relPath" and "relDir" and the raw backing store name. Also allow creating linked lists of virStorageSourcePtrs to express backing chains. --- src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 14 ++++++++++++++ 2 files changed, 21 insertions(+)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Replace the old structure with the new one. This change is a trivial name change operation (along with change of the freeing function). --- src/conf/domain_conf.c | 4 +-- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 16 +++++------ src/storage/storage_backend_fs.c | 4 +-- src/storage/storage_backend_gluster.c | 4 +-- src/util/virstoragefile.c | 54 +++++++++++++++++------------------ src/util/virstoragefile.h | 32 ++++++++++----------- tests/virstoragetest.c | 20 ++++++------- 9 files changed, 69 insertions(+), 69 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 006aa96..695d7ec 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1190,7 +1190,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) virStorageSourceClear(&def->src); VIR_FREE(def->serial); VIR_FREE(def->dst); - virStorageFileFreeMetadata(def->backingChain); + virStorageSourceFree(def->backingChain); VIR_FREE(def->mirror); VIR_FREE(def->wwn); VIR_FREE(def->vendor); @@ -18553,7 +18553,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, { int ret = -1; size_t depth = 0; - virStorageFileMetadata *tmp; + virStorageSourcePtr tmp; const char *path = virDomainDiskGetSource(disk); int type = virDomainDiskGetType(disk); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2ff0bc4..56bb8bd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -605,7 +605,7 @@ struct _virDomainDiskDef { char *dst; int tray_status; /* enum virDomainDiskTray */ int removable; /* enum virDomainFeatureState */ - virStorageFileMetadataPtr backingChain; + virStorageSourcePtr backingChain; char *mirror; int mirrorFormat; /* enum virStorageFileFormat */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1641eed..b6f9e6a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2410,7 +2410,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, if (disk->backingChain) { if (force) { - virStorageFileFreeMetadata(disk->backingChain); + virStorageSourceFree(disk->backingChain); disk->backingChain = NULL; } else { goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c242104..8005213 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10303,7 +10303,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, int ret = -1; int fd = -1; off_t end; - virStorageFileMetadata *meta = NULL; + virStorageSourcePtr meta = NULL; virDomainDiskDefPtr disk = NULL; struct stat sb; int idx; @@ -10450,7 +10450,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, cleanup: VIR_FREE(alias); - virStorageFileFreeMetadata(meta); + virStorageSourceFree(meta); VIR_FORCE_CLOSE(fd); /* If we failed to get data from a domain because it's inactive and @@ -12022,7 +12022,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, * temporarily modify the disk in place. */ char *origsrc = disk->src.path; int origformat = disk->src.format; - virStorageFileMetadataPtr origchain = disk->backingChain; + virStorageSourcePtr origchain = disk->backingChain; bool origreadonly = disk->readonly; int ret = -1; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -12735,7 +12735,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, * recompute it. Better would be storing the chain ourselves rather than * reprobing, but this requires modifying domain_conf and our XML to fully * track the chain across libvirtd restarts. */ - virStorageFileFreeMetadata(disk->backingChain); + virStorageSourceFree(disk->backingChain); disk->backingChain = NULL; if (virStorageFileInit(&snap->src) < 0) @@ -14715,7 +14715,7 @@ qemuDomainBlockPivot(virConnectPtr conn, bool resume = false; char *oldsrc = NULL; int oldformat; - virStorageFileMetadataPtr oldchain = NULL; + virStorageSourcePtr oldchain = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); /* Probe the status, if needed. */ @@ -14810,7 +14810,7 @@ qemuDomainBlockPivot(virConnectPtr conn, * fact that we aren't tracking the full chain ourselves; so * for now, we leak the access to the original. */ VIR_FREE(oldsrc); - virStorageFileFreeMetadata(oldchain); + virStorageSourceFree(oldchain); disk->mirror = NULL; } else { /* On failure, qemu abandons the mirror, and reverts back to @@ -14823,7 +14823,7 @@ qemuDomainBlockPivot(virConnectPtr conn, * success case, there's security labeling to worry about. */ disk->src.path = oldsrc; disk->src.format = oldformat; - virStorageFileFreeMetadata(disk->backingChain); + virStorageSourceFree(disk->backingChain); disk->backingChain = oldchain; VIR_FREE(disk->mirror); } @@ -15297,7 +15297,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, int idx; virDomainDiskDefPtr disk = NULL; const char *top_canon = NULL; - virStorageFileMetadataPtr top_meta = NULL; + virStorageSourcePtr top_meta = NULL; const char *top_parent = NULL; const char *base_canon = NULL; bool clean_access = false; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index fa5eba1..36666f4 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -69,7 +69,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, { int fd = -1; int ret = -1; - virStorageFileMetadata *meta = NULL; + virStorageSourcePtr meta = NULL; struct stat sb; char *header = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; @@ -176,7 +176,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, VIR_FORCE_CLOSE(fd); cleanup: - virStorageFileFreeMetadata(meta); + virStorageSourceFree(meta); VIR_FREE(header); return ret; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e0a25df..5ab1e7e 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -243,7 +243,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, int ret = -1; virStorageVolDefPtr vol = NULL; glfs_fd_t *fd = NULL; - virStorageFileMetadata *meta = NULL; + virStorageSourcePtr meta = NULL; char *header = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; @@ -324,7 +324,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, vol = NULL; ret = 0; cleanup: - virStorageFileFreeMetadata(meta); + virStorageSourceFree(meta); virStorageVolDefFree(vol); if (fd) glfs_close(fd); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f519d82..e7dc6ae 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -795,7 +795,7 @@ qcow2GetFeatures(virBitmapPtr *features, * pre-populated in META*/ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) -virStorageFileGetMetadataInternal(virStorageFileMetadataPtr meta, +virStorageFileGetMetadataInternal(virStorageSourcePtr meta, char *buf, size_t len, int *backingFormat) @@ -933,11 +933,11 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) } -static virStorageFileMetadataPtr +static virStorageSourcePtr virStorageFileMetadataNew(const char *path, int format) { - virStorageFileMetadataPtr ret = NULL; + virStorageSourcePtr ret = NULL; if (VIR_ALLOC(ret) < 0) return NULL; @@ -956,7 +956,7 @@ virStorageFileMetadataNew(const char *path, return ret; error: - virStorageFileFreeMetadata(ret); + virStorageSourceFree(ret); return NULL; } @@ -983,9 +983,9 @@ virStorageFileMetadataNew(const char *path, * backing store. Callers are advised against probing for the * backing store format in this case. * - * Caller MUST free the result after use via virStorageFileFreeMetadata. + * Caller MUST free the result after use via virStorageSourceFree. */ -virStorageFileMetadataPtr +virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, @@ -993,19 +993,19 @@ virStorageFileGetMetadataFromBuf(const char *path, char **backing, int *backingFormat) { - virStorageFileMetadataPtr ret = NULL; + virStorageSourcePtr ret = NULL; if (!(ret = virStorageFileMetadataNew(path, format))) return NULL; if (virStorageFileGetMetadataInternal(ret, buf, len, backingFormat) < 0) { - virStorageFileFreeMetadata(ret); + virStorageSourceFree(ret); ret = NULL; } if (VIR_STRDUP(*backing, ret->backingStoreRaw) < 0) { - virStorageFileFreeMetadata(ret); + virStorageSourceFree(ret); ret = NULL; } @@ -1015,7 +1015,7 @@ virStorageFileGetMetadataFromBuf(const char *path, /* Internal version that also supports a containing directory name. */ static int -virStorageFileGetMetadataFromFDInternal(virStorageFileMetadataPtr meta, +virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, int fd, int *backingFormat) { @@ -1083,20 +1083,20 @@ virStorageFileGetMetadataFromFDInternal(virStorageFileMetadataPtr meta, * format, since a malicious guest can turn a raw file into any * other non-raw format at will. * - * Caller MUST free the result after use via virStorageFileFreeMetadata. + * Caller MUST free the result after use via virStorageSourceFree. */ -virStorageFileMetadataPtr +virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format) { - virStorageFileMetadataPtr ret = NULL; + virStorageSourcePtr ret = NULL; if (!(ret = virStorageFileMetadataNew(path, format))) goto cleanup; if (virStorageFileGetMetadataFromFDInternal(ret, fd, NULL) < 0) { - virStorageFileFreeMetadata(ret); + virStorageSourceFree(ret); ret = NULL; } @@ -1111,7 +1111,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, const char *directory, int format, uid_t uid, gid_t gid, bool allow_probe, virHashTablePtr cycle, - virStorageFileMetadataPtr meta) + virStorageSourcePtr meta) { int fd; int ret = -1; @@ -1182,7 +1182,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, return -1; } - virStorageFileMetadataPtr backing; + virStorageSourcePtr backing; if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) backingFormat = VIR_STORAGE_FILE_RAW; @@ -1195,7 +1195,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, uid, gid, allow_probe, cycle, backing) < 0) { /* If we failed to get backing data, mark the chain broken */ - virStorageFileFreeMetadata(backing); + virStorageSourceFree(backing); } else { meta->backingMeta = backing; } @@ -1221,9 +1221,9 @@ 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. * - * Caller MUST free result after use via virStorageFileFreeMetadata. + * Caller MUST free result after use via virStorageSourceFree. */ -virStorageFileMetadataPtr +virStorageSourcePtr virStorageFileGetMetadata(const char *path, int format, uid_t uid, gid_t gid, bool allow_probe) @@ -1232,8 +1232,8 @@ virStorageFileGetMetadata(const char *path, int format, path, format, (int)uid, (int)gid, allow_probe); virHashTablePtr cycle = virHashCreate(5, NULL); - virStorageFileMetadataPtr meta = NULL; - virStorageFileMetadataPtr ret = NULL; + virStorageSourcePtr meta = NULL; + virStorageSourcePtr ret = NULL; char *canonPath = NULL; char *directory = NULL; @@ -1265,7 +1265,7 @@ virStorageFileGetMetadata(const char *path, int format, meta = NULL; cleanup: - virStorageFileFreeMetadata(meta); + virStorageSourceFree(meta); VIR_FREE(canonPath); VIR_FREE(directory); virHashFree(cycle); @@ -1281,10 +1281,10 @@ virStorageFileGetMetadata(const char *path, int format, * error (allocation failure). */ int -virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, +virStorageFileChainGetBroken(virStorageSourcePtr chain, char **brokenFile) { - virStorageFileMetadataPtr tmp; + virStorageSourcePtr tmp; *brokenFile = NULL; @@ -1307,7 +1307,7 @@ virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, /** - * virStorageFileFreeMetadata: + * virStorageFileMetadataFree: * * Free pointers in passed structure and structure itself. */ @@ -1530,8 +1530,8 @@ int virStorageFileGetSCSIKey(const char *path, * independently freed. Reports an error and returns NULL if NAME is * not found. */ const char * -virStorageFileChainLookup(virStorageFileMetadataPtr chain, - const char *name, virStorageFileMetadataPtr *meta, +virStorageFileChainLookup(virStorageSourcePtr chain, + const char *name, virStorageSourcePtr *meta, const char **parent) { const char *start = chain->path; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 11e25eb..89fa1ee 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -298,28 +298,28 @@ int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); int virStorageFileProbeFormatFromBuf(const char *path, char *buf, size_t buflen); -virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, - int format, - uid_t uid, gid_t gid, - bool allow_probe) +virStorageSourcePtr virStorageFileGetMetadata(const char *path, + int format, + uid_t uid, gid_t gid, + bool allow_probe) ATTRIBUTE_NONNULL(1); -virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, - int fd, - int format); -virStorageFileMetadataPtr virStorageFileGetMetadataFromBuf(const char *path, - char *buf, - size_t len, - int format, - char **backing, - int *backingFormat) +virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, + int fd, + int format); +virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, + char *buf, + size_t len, + int format, + char **backing, + int *backingFormat) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6); -int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, +int virStorageFileChainGetBroken(virStorageSourcePtr chain, char **broken_file); -const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain, +const char *virStorageFileChainLookup(virStorageSourcePtr chain, const char *name, - virStorageFileMetadataPtr *meta, + virStorageSourcePtr *meta, const char **parent) ATTRIBUTE_NONNULL(1); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 646c15e..90311af 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -266,8 +266,8 @@ testStorageChain(const void *args) { const struct testChainData *data = args; int ret = -1; - virStorageFileMetadataPtr meta; - virStorageFileMetadataPtr elt; + virStorageSourcePtr meta; + virStorageSourcePtr elt; size_t i = 0; char *broken = NULL; bool isAbs = !!(data->flags & ABS_START); @@ -367,16 +367,16 @@ testStorageChain(const void *args) ret = 0; cleanup: VIR_FREE(broken); - virStorageFileFreeMetadata(meta); + virStorageSourceFree(meta); return ret; } struct testLookupData { - virStorageFileMetadataPtr chain; + virStorageSourcePtr chain; const char *name; const char *expResult; - virStorageFileMetadataPtr expMeta; + virStorageSourcePtr expMeta; const char *expParent; }; @@ -386,7 +386,7 @@ testStorageLookup(const void *args) const struct testLookupData *data = args; int ret = 0; const char *actualResult; - virStorageFileMetadataPtr actualMeta; + virStorageSourcePtr actualMeta; const char *actualParent; /* This function is documented as giving results within chain, but @@ -444,7 +444,7 @@ mymain(void) int ret; virCommandPtr cmd = NULL; struct testChainData data; - virStorageFileMetadataPtr chain = NULL; + virStorageSourcePtr chain = 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. */ @@ -866,7 +866,7 @@ mymain(void) ret = -1; /* Test behavior of chain lookups, relative backing from absolute start */ - virStorageFileFreeMetadata(chain); + virStorageSourceFree(chain); chain = virStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, -1, -1, false); if (!chain) { @@ -896,7 +896,7 @@ mymain(void) ret = -1; /* Test behavior of chain lookups, relative backing */ - virStorageFileFreeMetadata(chain); + virStorageSourceFree(chain); chain = virStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2, -1, -1, false); if (!chain) { @@ -922,7 +922,7 @@ mymain(void) cleanup: /* Final cleanup */ - virStorageFileFreeMetadata(chain); + virStorageSourceFree(chain); testCleanupImages(); virCommandFree(cmd); -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Replace the old structure with the new one. This change is a trivial name change operation (along with change of the freeing function). --- src/conf/domain_conf.c | 4 +-- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 16 +++++------ src/storage/storage_backend_fs.c | 4 +-- src/storage/storage_backend_gluster.c | 4 +-- src/util/virstoragefile.c | 54 +++++++++++++++++------------------ src/util/virstoragefile.h | 32 ++++++++++----------- tests/virstoragetest.c | 20 ++++++------- 9 files changed, 69 insertions(+), 69 deletions(-)
@@ -1182,7 +1182,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, return -1; }
- virStorageFileMetadataPtr backing; + virStorageSourcePtr backing;
Merge conflict with my suggestions for 5/18; obvious resolution. Mechanical; ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Remove the now unused pieces of the structure. --- src/libvirt_private.syms | 1 - src/util/virstoragefile.c | 23 ----------------------- src/util/virstoragefile.h | 37 ------------------------------------- 3 files changed, 61 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 69c986f..fe8a810 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1824,7 +1824,6 @@ virStorageFileFeatureTypeFromString; virStorageFileFeatureTypeToString; virStorageFileFormatTypeFromString; virStorageFileFormatTypeToString; -virStorageFileFreeMetadata; virStorageFileGetLVMKey; virStorageFileGetMetadata; virStorageFileGetMetadataFromBuf; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e7dc6ae..db6b02c 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1307,29 +1307,6 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain, /** - * virStorageFileMetadataFree: - * - * Free pointers in passed structure and structure itself. - */ -void -virStorageFileFreeMetadata(virStorageFileMetadata *meta) -{ - if (!meta) - return; - - VIR_FREE(meta->relPath); - VIR_FREE(meta->path); - VIR_FREE(meta->relDir); - - virStorageFileFreeMetadata(meta->backingMeta); - VIR_FREE(meta->backingStoreRaw); - VIR_FREE(meta->compat); - virBitmapFree(meta->features); - virStorageEncryptionFree(meta->encryption); - VIR_FREE(meta); -} - -/** * virStorageFileResize: * * Change the capacity of the raw storage file at 'path'. diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 89fa1ee..fa9ae44 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -116,41 +116,6 @@ struct _virStorageTimestamps { }; -typedef struct _virStorageFileMetadata virStorageFileMetadata; -typedef virStorageFileMetadata *virStorageFileMetadataPtr; -struct _virStorageFileMetadata { - /* Name of the current file as spelled by the user (top level) or - * metadata of the overlay (if this is a backing store). */ - char *relPath; - /* Canonical name of the current file, used to detect loops in the - * backing store chain. */ - char *path; - /* Directory to start from if backingStoreRaw is a relative file - * name. */ - char *relDir; - /* Name of the child backing store recorded in metadata of the - * current file. */ - char *backingStoreRaw; - - /* Backing chain. In the common case, the child's - * backingMeta->path will be a duplicate of this file's - * backingStoreRaw; this setup makes it possible to detect missing - * backing files: if backingStoreRaw is NULL, this field should be - * NULL. If this field is NULL and backingStoreRaw is non-NULL, - * there was an error following the chain (such as a missing - * file). Otherwise, information about the child is here. */ - virStorageFileMetadataPtr backingMeta; - - /* Details about the current image */ - int type; /* enum virStorageType */ - int format; /* enum virStorageFileFormat */ - virStorageEncryptionPtr encryption; - unsigned long long capacity; - virBitmapPtr features; /* bits described by enum virStorageFileFeature */ - char *compat; -}; - - /* Information related to network storage */ enum virStorageNetProtocol { VIR_STORAGE_NET_PROTOCOL_NBD, @@ -323,8 +288,6 @@ const char *virStorageFileChainLookup(virStorageSourcePtr chain, const char **parent) ATTRIBUTE_NONNULL(1); -void virStorageFileFreeMetadata(virStorageFileMetadataPtr meta); - int virStorageFileResize(const char *path, unsigned long long capacity, unsigned long long orig_capacity, -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Remove the now unused pieces of the structure. --- src/libvirt_private.syms | 1 - src/util/virstoragefile.c | 23 ----------------------- src/util/virstoragefile.h | 37 ------------------------------------- 3 files changed, 61 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To conform with the naming of the planed XML output rename the metadata variable name. s/backingMeta/backingStore/g --- src/conf/domain_conf.c | 6 +++--- src/qemu/qemu_driver.c | 8 ++++---- src/util/virstoragefile.c | 12 +++++------ src/util/virstoragefile.h | 2 +- tests/virstoragetest.c | 52 +++++++++++++++++++++++------------------------ 5 files changed, 40 insertions(+), 40 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 695d7ec..d9c4485 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18567,9 +18567,9 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, goto cleanup; /* XXX: temporarily we need to select the second element of the backing * chain to start as the first is the copy of the disk itself. */ - tmp = disk->backingChain ? disk->backingChain->backingMeta : NULL; + tmp = disk->backingChain ? disk->backingChain->backingStore : NULL; while (tmp && virStorageIsFile(tmp->path)) { - if (!ignoreOpenFailure && tmp->backingStoreRaw && !tmp->backingMeta) { + if (!ignoreOpenFailure && tmp->backingStoreRaw && !tmp->backingStore) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to visit backing chain file %s"), tmp->backingStoreRaw); @@ -18577,7 +18577,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, } if (iter(disk, tmp->path, ++depth, opaque) < 0) goto cleanup; - tmp = tmp->backingMeta; + tmp = tmp->backingStore; } ret = 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8005213..339b231 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15134,7 +15134,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) && STREQ_NULLABLE(format, "raw") && - disk->backingChain->backingMeta->path) { + disk->backingChain->backingStore->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' has backing file, so raw shallow copy " "is not possible"), @@ -15347,14 +15347,14 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, &top_parent))) { goto endjob; } - if (!top_meta || !top_meta->backingMeta) { + if (!top_meta || !top_meta->backingStore) { virReportError(VIR_ERR_INVALID_ARG, _("top '%s' in chain for '%s' has no backing file"), top_canon, path); goto endjob; } if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) - base_canon = top_meta->backingMeta->path; + base_canon = top_meta->backingStore->path; else if (!(base_canon = virStorageFileChainLookup(top_meta, base, NULL, NULL))) goto endjob; @@ -15363,7 +15363,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, * virStorageFileChainLookup guarantees a simple pointer * comparison will work, rather than needing full-blown STREQ. */ if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && - base_canon != top_meta->backingMeta->path) { + base_canon != top_meta->backingStore->path) { virReportError(VIR_ERR_INVALID_ARG, _("base '%s' is not immediately below '%s' in chain " "for '%s'"), diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index db6b02c..37cda8e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1197,7 +1197,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, /* If we failed to get backing data, mark the chain broken */ virStorageSourceFree(backing); } else { - meta->backingMeta = backing; + meta->backingStore = backing; } } @@ -1291,10 +1291,10 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain, if (!chain) return 0; - for (tmp = chain; tmp; tmp = tmp->backingMeta) { + for (tmp = chain; tmp; tmp = tmp->backingStore) { /* Break when we hit end of chain; report error if we detected * a missing backing file, infinite loop, or other error */ - if (!tmp->backingMeta && tmp->backingStoreRaw) { + if (!tmp->backingStore && tmp->backingStoreRaw) { if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0) return -1; @@ -1522,7 +1522,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, *parent = NULL; while (chain) { if (!name) { - if (!chain->backingMeta) + if (!chain->backingStore) break; } else { if (STREQ(name, chain->relPath)) @@ -1539,7 +1539,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, } *parent = chain->path; parentDir = chain->relDir; - chain = chain->backingMeta; + chain = chain->backingStore; } if (!chain) goto error; @@ -1694,7 +1694,7 @@ virStorageSourceClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw); /* recursively free backing chain */ - virStorageSourceFree(def->backingMeta); + virStorageSourceFree(def->backingStore); } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index fa9ae44..e60befe 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -237,7 +237,7 @@ struct _virStorageSource { virSecurityDeviceLabelDefPtr *seclabels; /* backing chain of the storage source */ - virStorageSourcePtr backingMeta; + virStorageSourcePtr backingStore; /* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 90311af..19f3f58 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -336,7 +336,7 @@ testStorageChain(const void *args) virAsprintf(&actual, "store:%s\nraw:%s\nother:%lld %d\n" "relPath:%s\npath:%s\nrelDir:%s\ntype:%d %d\n", - NULLSTR(elt->backingMeta ? elt->backingMeta->path : NULL), + NULLSTR(elt->backingStore ? elt->backingStore->path : NULL), NULLSTR(elt->backingStoreRaw), elt->capacity, !!elt->encryption, NULLSTR(elt->relPath), @@ -356,7 +356,7 @@ testStorageChain(const void *args) } VIR_FREE(expect); VIR_FREE(actual); - elt = elt->backingMeta; + elt = elt->backingStore; i++; } if (i != data->nfiles) { @@ -841,16 +841,16 @@ mymain(void) TEST_LOOKUP(0, "bogus", NULL, NULL, NULL); TEST_LOOKUP(1, "wrap", chain->path, chain, NULL); TEST_LOOKUP(2, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(3, "qcow2", chain->backingMeta->path, chain->backingMeta, + TEST_LOOKUP(3, "qcow2", chain->backingStore->path, chain->backingStore, chain->path); - TEST_LOOKUP(4, absqcow2, chain->backingMeta->path, chain->backingMeta, + TEST_LOOKUP(4, absqcow2, chain->backingStore->path, chain->backingStore, chain->path); - TEST_LOOKUP(5, "raw", chain->backingMeta->backingMeta->path, - chain->backingMeta->backingMeta, chain->backingMeta->path); - TEST_LOOKUP(6, absraw, chain->backingMeta->backingMeta->path, - chain->backingMeta->backingMeta, chain->backingMeta->path); - TEST_LOOKUP(7, NULL, chain->backingMeta->backingMeta->path, - chain->backingMeta->backingMeta, chain->backingMeta->path); + TEST_LOOKUP(5, "raw", chain->backingStore->backingStore->path, + chain->backingStore->backingStore, chain->backingStore->path); + TEST_LOOKUP(6, absraw, chain->backingStore->backingStore->path, + chain->backingStore->backingStore, chain->backingStore->path); + TEST_LOOKUP(7, NULL, chain->backingStore->backingStore->path, + chain->backingStore->backingStore, chain->backingStore->path); /* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */ virCommandFree(cmd); @@ -877,16 +877,16 @@ mymain(void) TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); TEST_LOOKUP(9, "wrap", chain->path, chain, NULL); TEST_LOOKUP(10, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(11, "qcow2", chain->backingMeta->path, chain->backingMeta, + TEST_LOOKUP(11, "qcow2", chain->backingStore->path, chain->backingStore, chain->path); - TEST_LOOKUP(12, absqcow2, chain->backingMeta->path, chain->backingMeta, + TEST_LOOKUP(12, absqcow2, chain->backingStore->path, chain->backingStore, chain->path); - TEST_LOOKUP(13, "raw", chain->backingMeta->backingMeta->path, - chain->backingMeta->backingMeta, chain->backingMeta->path); - TEST_LOOKUP(14, absraw, chain->backingMeta->backingMeta->path, - chain->backingMeta->backingMeta, chain->backingMeta->path); - TEST_LOOKUP(15, NULL, chain->backingMeta->backingMeta->path, - chain->backingMeta->backingMeta, chain->backingMeta->path); + TEST_LOOKUP(13, "raw", chain->backingStore->backingStore->path, + chain->backingStore->backingStore, chain->backingStore->path); + TEST_LOOKUP(14, absraw, chain->backingStore->backingStore->path, + chain->backingStore->backingStore, chain->backingStore->path); + TEST_LOOKUP(15, NULL, chain->backingStore->backingStore->path, + chain->backingStore->backingStore, chain->backingStore->path); /* Use link to wrap with cross-directory relative backing */ virCommandFree(cmd); @@ -908,17 +908,17 @@ mymain(void) TEST_LOOKUP(17, "sub/link2", chain->path, chain, NULL); TEST_LOOKUP(18, "wrap", chain->path, chain, NULL); TEST_LOOKUP(19, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(20, "../qcow2", chain->backingMeta->path, chain->backingMeta, + TEST_LOOKUP(20, "../qcow2", chain->backingStore->path, chain->backingStore, chain->path); TEST_LOOKUP(21, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(22, absqcow2, chain->backingMeta->path, chain->backingMeta, + TEST_LOOKUP(22, absqcow2, chain->backingStore->path, chain->backingStore, chain->path); - TEST_LOOKUP(23, "raw", chain->backingMeta->backingMeta->path, - chain->backingMeta->backingMeta, chain->backingMeta->path); - TEST_LOOKUP(24, absraw, chain->backingMeta->backingMeta->path, - chain->backingMeta->backingMeta, chain->backingMeta->path); - TEST_LOOKUP(25, NULL, chain->backingMeta->backingMeta->path, - chain->backingMeta->backingMeta, chain->backingMeta->path); + TEST_LOOKUP(23, "raw", chain->backingStore->backingStore->path, + chain->backingStore->backingStore, chain->backingStore->path); + TEST_LOOKUP(24, absraw, chain->backingStore->backingStore->path, + chain->backingStore->backingStore, chain->backingStore->path); + TEST_LOOKUP(25, NULL, chain->backingStore->backingStore->path, + chain->backingStore->backingStore, chain->backingStore->path); cleanup: /* Final cleanup */ -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
To conform with the naming of the planed XML output rename the metadata
s/planed/planned/
variable name.
s/backingMeta/backingStore/g --- src/conf/domain_conf.c | 6 +++--- src/qemu/qemu_driver.c | 8 ++++---- src/util/virstoragefile.c | 12 +++++------ src/util/virstoragefile.h | 2 +- tests/virstoragetest.c | 52 +++++++++++++++++++++++------------------------ 5 files changed, 40 insertions(+), 40 deletions(-)
Mechanical; ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Switch over to storing of the backing chain as a recursive virStorageSource structure. This is a string based move. Currently the first element will be present twice in the backing chain as currently the retrieval function stores the parent in the newly detected chain. This will be fixed later. --- src/conf/domain_conf.c | 5 ++--- src/conf/domain_conf.h | 1 - src/qemu/qemu_domain.c | 20 ++++++++++---------- src/qemu/qemu_driver.c | 30 +++++++++++++++--------------- src/security/security_selinux.c | 2 +- src/security/virt-aa-helper.c | 8 ++++---- 6 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d9c4485..69ca5e7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1190,7 +1190,6 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) virStorageSourceClear(&def->src); VIR_FREE(def->serial); VIR_FREE(def->dst); - virStorageSourceFree(def->backingChain); VIR_FREE(def->mirror); VIR_FREE(def->wwn); VIR_FREE(def->vendor); @@ -18541,7 +18540,7 @@ virDomainSmartcardDefForeach(virDomainDefPtr def, /* Call iter(disk, name, depth, opaque) for each element of disk and - * its backing chain in the pre-populated disk->backingChain. + * its backing chain in the pre-populated disk->src.backingStore. * ignoreOpenFailure determines whether to warn about a chain that * mentions a backing file without also having metadata on that * file. */ @@ -18567,7 +18566,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, goto cleanup; /* XXX: temporarily we need to select the second element of the backing * chain to start as the first is the copy of the disk itself. */ - tmp = disk->backingChain ? disk->backingChain->backingStore : NULL; + tmp = disk->src.backingStore ? disk->src.backingStore->backingStore : NULL; while (tmp && virStorageIsFile(tmp->path)) { if (!ignoreOpenFailure && tmp->backingStoreRaw && !tmp->backingStore) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 56bb8bd..93c5bb3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -605,7 +605,6 @@ struct _virDomainDiskDef { char *dst; int tray_status; /* enum virDomainDiskTray */ int removable; /* enum virDomainFeatureState */ - virStorageSourcePtr backingChain; char *mirror; int mirrorFormat; /* enum virStorageFileFormat */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b6f9e6a..45ed872 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2239,10 +2239,10 @@ qemuDiskChainCheckBroken(virDomainDiskDefPtr disk) { char *brokenFile = NULL; - if (!virDomainDiskGetSource(disk) || !disk->backingChain) + if (!virDomainDiskGetSource(disk) || !disk->src.backingStore) return 0; - if (virStorageFileChainGetBroken(disk->backingChain, &brokenFile) < 0) + if (virStorageFileChainGetBroken(disk->src.backingStore, &brokenFile) < 0) return -1; if (brokenFile) { @@ -2408,10 +2408,10 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, type == VIR_STORAGE_TYPE_VOLUME) goto cleanup; - if (disk->backingChain) { + if (disk->src.backingStore) { if (force) { - virStorageSourceFree(disk->backingChain); - disk->backingChain = NULL; + virStorageSourceFree(disk->src.backingStore); + disk->src.backingStore = NULL; } else { goto cleanup; } @@ -2419,11 +2419,11 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, qemuDomainGetImageIds(cfg, vm, disk, &uid, &gid); - disk->backingChain = virStorageFileGetMetadata(src, - virDomainDiskGetFormat(disk), - uid, gid, - cfg->allowDiskFormatProbing); - if (!disk->backingChain) + disk->src.backingStore = virStorageFileGetMetadata(src, + virDomainDiskGetFormat(disk), + uid, gid, + cfg->allowDiskFormatProbing); + if (!disk->src.backingStore) ret = -1; cleanup: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 339b231..c0094c1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12022,14 +12022,14 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, * temporarily modify the disk in place. */ char *origsrc = disk->src.path; int origformat = disk->src.format; - virStorageSourcePtr origchain = disk->backingChain; + virStorageSourcePtr origchain = disk->src.backingStore; bool origreadonly = disk->readonly; int ret = -1; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); disk->src.path = (char *) file; /* casting away const is safe here */ disk->src.format = VIR_STORAGE_FILE_RAW; - disk->backingChain = NULL; + disk->src.backingStore = NULL; disk->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; if (mode == VIR_DISK_CHAIN_NO_ACCESS) { @@ -12054,7 +12054,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, cleanup: disk->src.path = origsrc; disk->src.format = origformat; - disk->backingChain = origchain; + disk->src.backingStore = origchain; disk->readonly = origreadonly; virObjectUnref(cfg); return ret; @@ -12730,13 +12730,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0) goto cleanup; - /* XXX Here, we know we are about to alter disk->backingChain if + /* XXX Here, we know we are about to alter disk->src.backingStore if * successful, so we nuke the existing chain so that future commands will * recompute it. Better would be storing the chain ourselves rather than * reprobing, but this requires modifying domain_conf and our XML to fully * track the chain across libvirtd restarts. */ - virStorageSourceFree(disk->backingChain); - disk->backingChain = NULL; + virStorageSourceFree(disk->src.backingStore); + disk->src.backingStore = NULL; if (virStorageFileInit(&snap->src) < 0) goto cleanup; @@ -14774,14 +14774,14 @@ qemuDomainBlockPivot(virConnectPtr conn, * we know for sure that there is a backing chain. */ oldsrc = disk->src.path; oldformat = disk->src.format; - oldchain = disk->backingChain; + oldchain = disk->src.backingStore; disk->src.path = disk->mirror; disk->src.format = disk->mirrorFormat; - disk->backingChain = NULL; + disk->src.backingStore = NULL; if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) { disk->src.path = oldsrc; disk->src.format = oldformat; - disk->backingChain = oldchain; + disk->src.backingStore = oldchain; goto cleanup; } if (disk->mirrorFormat && disk->mirrorFormat != VIR_STORAGE_FILE_RAW && @@ -14792,7 +14792,7 @@ qemuDomainBlockPivot(virConnectPtr conn, disk) < 0)) { disk->src.path = oldsrc; disk->src.format = oldformat; - disk->backingChain = oldchain; + disk->src.backingStore = oldchain; goto cleanup; } @@ -14823,8 +14823,8 @@ qemuDomainBlockPivot(virConnectPtr conn, * success case, there's security labeling to worry about. */ disk->src.path = oldsrc; disk->src.format = oldformat; - virStorageSourceFree(disk->backingChain); - disk->backingChain = oldchain; + virStorageSourceFree(disk->src.backingStore); + disk->src.backingStore = oldchain; VIR_FREE(disk->mirror); } disk->mirrorFormat = VIR_STORAGE_FILE_NONE; @@ -15134,7 +15134,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) && STREQ_NULLABLE(format, "raw") && - disk->backingChain->backingStore->path) { + disk->src.backingStore->backingStore->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' has backing file, so raw shallow copy " "is not possible"), @@ -15341,8 +15341,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, if (!top) { top_canon = disk->src.path; - top_meta = disk->backingChain; - } else if (!(top_canon = virStorageFileChainLookup(disk->backingChain, + top_meta = disk->src.backingStore; + } else if (!(top_canon = virStorageFileChainLookup(disk->src.backingStore, top, &top_meta, &top_parent))) { goto endjob; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index bfac11c..19d76fd 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1148,7 +1148,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, * be tracked in domain XML, at which point labelskip should be a * per-file attribute instead of a disk attribute. */ if (disk_seclabel && disk_seclabel->labelskip && - !disk->backingChain) + !disk->src.backingStore) return 0; /* Don't restore labels on readoly/shared disks, because diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index da66583..1c89815 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -950,11 +950,11 @@ get_files(vahControl * ctl) /* 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) { + if (!disk->src.backingStore) { bool probe = ctl->allowDiskFormatProbing; - disk->backingChain = virStorageFileGetMetadata(src, - virDomainDiskGetFormat(disk), - -1, -1, probe); + disk->src.backingStore = virStorageFileGetMetadata(src, + virDomainDiskGetFormat(disk), + -1, -1, probe); } /* XXX passing ignoreOpenFailure = true to get back to the behavior -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Switch over to storing of the backing chain as a recursive virStorageSource structure.
This is a string based move. Currently the first element will be present twice in the backing chain as currently the retrieval function stores the parent in the newly detected chain. This will be fixed later. --- src/conf/domain_conf.c | 5 ++--- src/conf/domain_conf.h | 1 - src/qemu/qemu_domain.c | 20 ++++++++++---------- src/qemu/qemu_driver.c | 30 +++++++++++++++--------------- src/security/security_selinux.c | 2 +- src/security/virt-aa-helper.c | 8 ++++---- 6 files changed, 32 insertions(+), 34 deletions(-)
@@ -12730,13 +12730,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0) goto cleanup;
- /* XXX Here, we know we are about to alter disk->backingChain if + /* XXX Here, we know we are about to alter disk->src.backingStore if * successful, so we nuke the existing chain so that future commands will * recompute it. Better would be storing the chain ourselves rather than * reprobing, but this requires modifying domain_conf and our XML to fully * track the chain across libvirtd restarts. */
Nice to know we are getting closer to fixing this comment. Indeed mechanical, and I agree with your choice for the awkward double-storage of the top of the chain until a later patch. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Don't remove detected metadata about directory based storage volumes. --- src/util/virstoragefile.c | 7 ++----- tests/virstoragetest.c | 3 +++ 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 37cda8e..8d5ef2f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1038,11 +1038,8 @@ virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, } if (S_ISDIR(sb.st_mode)) { - /* No header to probe for directories, but also no backing - * file; therefore, no inclusion loop is possible, and we - * don't need canonName or relDir. */ - VIR_FREE(meta->relDir); - VIR_FREE(meta->path); + /* No header to probe for directories, but also no backing file. Just + * update the metadata.*/ meta->type = VIR_STORAGE_TYPE_DIR; meta->format = VIR_STORAGE_FILE_DIR; ret = 0; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 19f3f58..370b8c2 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -709,6 +709,9 @@ mymain(void) testFileData dir = { .pathRel = "dir", .pathAbs = absdir, + .path = canondir, + .relDirRel = ".", + .relDirAbs = datadir, .type = VIR_STORAGE_TYPE_DIR, .format = VIR_STORAGE_FILE_DIR, }; -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Don't remove detected metadata about directory based storage volumes. --- src/util/virstoragefile.c | 7 ++----- tests/virstoragetest.c | 3 +++ 2 files changed, 5 insertions(+), 5 deletions(-)
ACK.
+++ b/tests/virstoragetest.c @@ -709,6 +709,9 @@ mymain(void) testFileData dir = { .pathRel = "dir", .pathAbs = absdir, + .path = canondir, + .relDirRel = ".", + .relDirAbs = datadir, .type = VIR_STORAGE_TYPE_DIR, .format = VIR_STORAGE_FILE_DIR, };
Ah, I'm glad I spent time improving the testsuite. This entire series has been much easier to justify and easier to work with because of the tests. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To avoid having the root of a backing chain present twice in the list we need to invert the working of virStorageFileGetMetadataRecurse. Until now the recursive worker created a new backing chain element from the name and other information passed as arguments. This required us to pass the data of the parend in a deconstructed way and the worker created a new entry for the parent. This patch converts this function so that it just fills in metadata about the parent and creates a backing chain element from those. This removes the duplication of the first element. To avoid breaking the test suite, virstoragetest now calls a wrapper that creates the parent structure explicitly and pre-fills it with the test data with same function signature as previously used. --- src/conf/domain_conf.c | 5 +- src/qemu/qemu_domain.c | 12 ++- src/qemu/qemu_driver.c | 6 +- src/security/virt-aa-helper.c | 7 +- src/util/virstoragefile.c | 193 ++++++++++++++++++++++-------------------- src/util/virstoragefile.h | 7 +- tests/virstoragetest.c | 47 +++++++++- 7 files changed, 158 insertions(+), 119 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 69ca5e7..99b57ee 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18564,9 +18564,8 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (iter(disk, path, 0, opaque) < 0) goto cleanup; - /* XXX: temporarily we need to select the second element of the backing - * chain to start as the first is the copy of the disk itself. */ - tmp = disk->src.backingStore ? disk->src.backingStore->backingStore : NULL; + + tmp = disk->src.backingStore; while (tmp && virStorageIsFile(tmp->path)) { if (!ignoreOpenFailure && tmp->backingStoreRaw && !tmp->backingStore) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 45ed872..bb9cb6b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2239,10 +2239,10 @@ qemuDiskChainCheckBroken(virDomainDiskDefPtr disk) { char *brokenFile = NULL; - if (!virDomainDiskGetSource(disk) || !disk->src.backingStore) + if (!virDomainDiskGetSource(disk)) return 0; - if (virStorageFileChainGetBroken(disk->src.backingStore, &brokenFile) < 0) + if (virStorageFileChainGetBroken(&disk->src, &brokenFile) < 0) return -1; if (brokenFile) { @@ -2419,11 +2419,9 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, qemuDomainGetImageIds(cfg, vm, disk, &uid, &gid); - disk->src.backingStore = virStorageFileGetMetadata(src, - virDomainDiskGetFormat(disk), - uid, gid, - cfg->allowDiskFormatProbing); - if (!disk->src.backingStore) + if (virStorageFileGetMetadata(&disk->src, + uid, gid, + cfg->allowDiskFormatProbing) < 0) ret = -1; cleanup: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c0094c1..7dd2b86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15134,7 +15134,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) && STREQ_NULLABLE(format, "raw") && - disk->src.backingStore->backingStore->path) { + disk->src.backingStore->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' has backing file, so raw shallow copy " "is not possible"), @@ -15341,8 +15341,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, if (!top) { top_canon = disk->src.path; - top_meta = disk->src.backingStore; - } else if (!(top_canon = virStorageFileChainLookup(disk->src.backingStore, + top_meta = &disk->src; + } else if (!(top_canon = virStorageFileChainLookup(&disk->src, top, &top_meta, &top_parent))) { goto endjob; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 1c89815..32fc04a 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -943,18 +943,15 @@ 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) + if (!virDomainDiskGetSource(disk)) 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->src.backingStore) { bool probe = ctl->allowDiskFormatProbing; - disk->src.backingStore = virStorageFileGetMetadata(src, - virDomainDiskGetFormat(disk), - -1, -1, probe); + virStorageFileGetMetadata(&disk->src, -1, -1, probe); } /* XXX passing ignoreOpenFailure = true to get back to the behavior diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8d5ef2f..b2d8f62 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1104,105 +1104,112 @@ virStorageFileGetMetadataFromFD(const char *path, /* Recursive workhorse for virStorageFileGetMetadata. */ static int -virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, - const char *directory, - int format, uid_t uid, gid_t gid, - bool allow_probe, virHashTablePtr cycle, - virStorageSourcePtr meta) +virStorageFileGetMetadataRecurse(virStorageSourcePtr src, + const char *canonPath, + uid_t uid, gid_t gid, + bool allow_probe, + virHashTablePtr cycle) { int fd; int ret = -1; + virStorageSourcePtr backingStore = NULL; int backingFormat; - char *backingPath = NULL; - char *backingDirectory = NULL; VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - path, canonPath, NULLSTR(directory), format, + src->path, canonPath, NULLSTR(src->relDir), src->format, (int)uid, (int)gid, allow_probe); if (virHashLookup(cycle, canonPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store for %s is self-referential"), - path); + src->path); return -1; } + if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) return -1; - if (virStorageIsFile(path)) { - if (VIR_STRDUP(meta->relPath, path) < 0) - return -1; - if (VIR_STRDUP(meta->path, canonPath) < 0) - return -1; - if (VIR_STRDUP(meta->relDir, directory) < 0) + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { + if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) { + virReportSystemError(-fd, _("Failed to open file '%s'"), + src->path); return -1; - meta->format = format; + } - if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) { - virReportSystemError(-fd, _("Failed to open file '%s'"), path); + if (virStorageFileGetMetadataFromFDInternal(src, fd, + &backingFormat) < 0) { + VIR_FORCE_CLOSE(fd); return -1; } - ret = virStorageFileGetMetadataFromFDInternal(meta, fd, &backingFormat); - if (VIR_CLOSE(fd) < 0) - VIR_WARN("could not close file %s", path); + VIR_WARN("could not close file %s", src->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->relPath, path) < 0) - return -1; - if (VIR_STRDUP(meta->path, path) < 0) - return -1; - meta->type = VIR_STORAGE_TYPE_NETWORK; - meta->format = VIR_STORAGE_FILE_RAW; - ret = 0; + /* TODO: currently we call this only for local storage */ + return 0; } - if (ret == 0 && meta->backingStoreRaw) { - if (virStorageIsFile(meta->backingStoreRaw)) { - if (virFindBackingFile(directory, - meta->backingStoreRaw, - &backingDirectory, - &backingPath) < 0) { - /* the backing file is (currently) unavailable, treat this - * file as standalone: - * backingStoreRaw is kept to mark broken image chains */ - VIR_WARN("Backing file '%s' of image '%s' is missing.", - meta->backingStoreRaw, path); - - return 0; - } - } else { - if (VIR_STRDUP(backingPath, meta->backingStoreRaw) < 0) - return -1; - } + /* check wether we need to go deeper */ + if (!src->backingStoreRaw) + return 0; - virStorageSourcePtr backing; - - 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, - backingPath, - backingDirectory, backingFormat, - uid, gid, allow_probe, - cycle, backing) < 0) { - /* If we failed to get backing data, mark the chain broken */ - virStorageSourceFree(backing); - } else { - meta->backingStore = backing; + if (VIR_ALLOC(backingStore) < 0) + return -1; + + if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0) + goto cleanup; + + if (virStorageIsFile(src->backingStoreRaw)) { + backingStore->type = VIR_STORAGE_TYPE_FILE; + + if (virFindBackingFile(src->relDir, + src->backingStoreRaw, + &backingStore->relDir, + &backingStore->path) < 0) { + /* the backing file is (currently) unavailable, treat this + * file as standalone: + * backingStoreRaw is kept to mark broken image chains */ + VIR_WARN("Backing file '%s' of image '%s' is missing.", + src->backingStoreRaw, src->path); + ret = 0; + goto cleanup; } + } else { + /* TODO: To satisfy the test case, copy the network URI as path. T + * his will be removed later */ + backingStore->type = VIR_STORAGE_TYPE_NETWORK; + + if (VIR_STRDUP(backingStore->path, src->backingStoreRaw) < 0) + goto cleanup; } - VIR_FREE(backingDirectory); - VIR_FREE(backingPath); + if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) + backingStore->format = VIR_STORAGE_FILE_RAW; + else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) + backingStore->format = VIR_STORAGE_FILE_AUTO; + else + backingStore->format = backingFormat; + + if (virStorageFileGetMetadataRecurse(backingStore, + backingStore->path, + uid, gid, allow_probe, + cycle) < 0) { + /* if we fail somewhere midway, just accept the and return a + * broken chain */ + ret = 0; + goto cleanup; + } + + src->backingStore = backingStore; + backingStore = NULL; + ret = 0; + + cleanup: + virStorageSourceFree(backingStore); return ret; } + /** * virStorageFileGetMetadata: * @@ -1220,51 +1227,51 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, * * Caller MUST free result after use via virStorageSourceFree. */ -virStorageSourcePtr -virStorageFileGetMetadata(const char *path, int format, +int +virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, bool allow_probe) { VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", - path, format, (int)uid, (int)gid, allow_probe); + src->path, src->format, (int)uid, (int)gid, allow_probe); - virHashTablePtr cycle = virHashCreate(5, NULL); - virStorageSourcePtr meta = NULL; - virStorageSourcePtr ret = NULL; + virHashTablePtr cycle = NULL; char *canonPath = NULL; - char *directory = NULL; + int ret = -1; - if (!cycle) - return NULL; + if (!(cycle = virHashCreate(5, NULL))) + return -1; - if (virStorageIsFile(path)) { - if (!(canonPath = canonicalize_file_name(path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { + if (!(canonPath = canonicalize_file_name(src->path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), + src->path); goto cleanup; } - if (!(directory = mdir_name(path))) { + + if (!src->relPath && + VIR_STRDUP(src->relPath, src->path) < 0) + goto cleanup; + + if (!src->relDir && + !(src->relDir = mdir_name(src->path))) { virReportOOMError(); goto cleanup; } - } else if (VIR_STRDUP(canonPath, path) < 0) { + } else { + /* TODO: currently unimplemented for non-local storage */ + ret = 0; goto cleanup; } - if (VIR_ALLOC(meta) < 0) - goto cleanup; - if (format <= VIR_STORAGE_FILE_NONE) - format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - if (virStorageFileGetMetadataRecurse(path, canonPath, directory, format, - uid, gid, allow_probe, cycle, - meta) < 0) - goto cleanup; - ret = meta; - meta = NULL; + if (src->format <= VIR_STORAGE_FILE_NONE) + src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; + + ret = virStorageFileGetMetadataRecurse(src, canonPath, uid, gid, + allow_probe, cycle); cleanup: - virStorageSourceFree(meta); VIR_FREE(canonPath); - VIR_FREE(directory); virHashFree(cycle); return ret; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index e60befe..a0adb9b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -263,10 +263,9 @@ int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); int virStorageFileProbeFormatFromBuf(const char *path, char *buf, size_t buflen); -virStorageSourcePtr virStorageFileGetMetadata(const char *path, - int format, - uid_t uid, gid_t gid, - bool allow_probe) +int virStorageFileGetMetadata(virStorageSourcePtr src, + uid_t uid, gid_t gid, + bool allow_probe) ATTRIBUTE_NONNULL(1); virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, int fd, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 370b8c2..5320c78 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -29,6 +29,7 @@ #include "virlog.h" #include "virstoragefile.h" #include "virstring.h" +#include "dirname.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -88,6 +89,44 @@ testCleanupImages(void) virFileDeleteTree(datadir); } + +static virStorageSourcePtr +testStorageFileGetMetadata(const char *path, + int format, + uid_t uid, gid_t gid, + bool allow_probe) +{ + virStorageSourcePtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->type = VIR_STORAGE_TYPE_FILE; + ret->format = format; + + if (VIR_STRDUP(ret->relPath, path) < 0) + goto error; + + if (!(ret->relDir = mdir_name(path))) { + virReportOOMError(); + goto error; + } + + if (!(ret->path = canonicalize_file_name(path))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "failed to resolve '%s'", path); + goto error; + } + + if (virStorageFileGetMetadata(ret, uid, gid, allow_probe) < 0) + goto error; + + return ret; + + error: + virStorageSourceFree(ret); + return NULL; +} + static int testPrepImages(void) { @@ -272,7 +311,7 @@ testStorageChain(const void *args) char *broken = NULL; bool isAbs = !!(data->flags & ABS_START); - meta = virStorageFileGetMetadata(data->start, data->format, -1, -1, + meta = testStorageFileGetMetadata(data->start, data->format, -1, -1, (data->flags & ALLOW_PROBE) != 0); if (!meta) { if (data->flags & EXP_FAIL) { @@ -825,7 +864,7 @@ mymain(void) ret = -1; /* Test behavior of chain lookups, absolute backing from relative start */ - chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, + chain = testStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, -1, -1, false); if (!chain) { ret = -1; @@ -870,7 +909,7 @@ mymain(void) /* Test behavior of chain lookups, relative backing from absolute start */ virStorageSourceFree(chain); - chain = virStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, + chain = testStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, -1, -1, false); if (!chain) { ret = -1; @@ -900,7 +939,7 @@ mymain(void) /* Test behavior of chain lookups, relative backing */ virStorageSourceFree(chain); - chain = virStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2, + chain = testStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2, -1, -1, false); if (!chain) { ret = -1; -- 1.9.1
On 04/20/2014 04:13 PM, Peter Krempa wrote:
To avoid having the root of a backing chain present twice in the list we need to invert the working of virStorageFileGetMetadataRecurse.
Until now the recursive worker created a new backing chain element from the name and other information passed as arguments. This required us to pass the data of the parend in a deconstructed way and the worker
s/parend/parent/
created a new entry for the parent.
This patch converts this function so that it just fills in metadata about the parent and creates a backing chain element from those. This removes the duplication of the first element.
To avoid breaking the test suite, virstoragetest now calls a wrapper that creates the parent structure explicitly and pre-fills it with the test data with same function signature as previously used. --- src/conf/domain_conf.c | 5 +- src/qemu/qemu_domain.c | 12 ++- src/qemu/qemu_driver.c | 6 +- src/security/virt-aa-helper.c | 7 +- src/util/virstoragefile.c | 193 ++++++++++++++++++++++-------------------- src/util/virstoragefile.h | 7 +- tests/virstoragetest.c | 47 +++++++++- 7 files changed, 158 insertions(+), 119 deletions(-)
- } + /* check wether we need to go deeper */
s/wether/whether/
} + } else { + /* TODO: To satisfy the test case, copy the network URI as path. T + * his will be removed later */
s/T.*his/This/
+ + if (virStorageFileGetMetadataRecurse(backingStore, + backingStore->path, + uid, gid, allow_probe, + cycle) < 0) { + /* if we fail somewhere midway, just accept the and return a + * broken chain */
s/the and/and/ ?
@@ -1220,51 +1227,51 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, * * Caller MUST free result after use via virStorageSourceFree. */ -virStorageSourcePtr -virStorageFileGetMetadata(const char *path, int format, +int +virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, bool allow_probe)
Yep, quite an inversion; but looks reasonable.
+ +static virStorageSourcePtr +testStorageFileGetMetadata(const char *path, + int format, + uid_t uid, gid_t gid, + bool allow_probe)
Nice wrapper.
- meta = virStorageFileGetMetadata(data->start, data->format, -1, -1, + meta = testStorageFileGetMetadata(data->start, data->format, -1, -1, (data->flags & ALLOW_PROBE) != 0);
Indentation of second line is now off; here and in many other hunks this file. It's late for me, and this one's big enough that I'd like to revisit it in my morning to make sure I'm not overlooking anything else. It will probably be ack with nits fixed, but can't hurt to be careful... -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/20/2014 04:13 PM, Peter Krempa wrote:
To avoid having the root of a backing chain present twice in the list we need to invert the working of virStorageFileGetMetadataRecurse.
/* Recursive workhorse for virStorageFileGetMetadata. */ static int -virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, - const char *directory, - int format, uid_t uid, gid_t gid, - bool allow_probe, virHashTablePtr cycle, - virStorageSourcePtr meta) +virStorageFileGetMetadataRecurse(virStorageSourcePtr src, + const char *canonPath, + uid_t uid, gid_t gid, + bool allow_probe, + virHashTablePtr cycle) {
In actually trying to apply this patch, I hit merge conflicts from my previous suggested changes. At this point, it would be easier for me if you can rebase the series, apply the acked patches, and post a v2 of this one. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/23/14 18:18, Eric Blake wrote:
On 04/20/2014 04:13 PM, Peter Krempa wrote:
To avoid having the root of a backing chain present twice in the list we need to invert the working of virStorageFileGetMetadataRecurse.
/* Recursive workhorse for virStorageFileGetMetadata. */ static int -virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, - const char *directory, - int format, uid_t uid, gid_t gid, - bool allow_probe, virHashTablePtr cycle, - virStorageSourcePtr meta) +virStorageFileGetMetadataRecurse(virStorageSourcePtr src, + const char *canonPath, + uid_t uid, gid_t gid, + bool allow_probe, + virHashTablePtr cycle) {
In actually trying to apply this patch, I hit merge conflicts from my previous suggested changes. At this point, it would be easier for me if you can rebase the series, apply the acked patches, and post a v2 of this one.
Okay, I've just pushed patches 1-17 (with 3 and 6 squashed together) and I'll post the rebased version of this one soon. Peter
To avoid having the root of a backing chain present twice in the list we need to invert the working of virStorageFileGetMetadataRecurse. Until now the recursive worker created a new backing chain element from the name and other information passed as arguments. This required us to pass the data of the parent in a deconstructed way and the worker created a new entry for the parent. This patch converts this function so that it just fills in metadata about the parent and creates a backing chain element from those. This removes the duplication of the first element. To avoid breaking the test suite, virstoragetest now calls a wrapper that creates the parent structure explicitly and pre-fills it with the test data with same function signature as previously used. --- src/conf/domain_conf.c | 5 +- src/qemu/qemu_domain.c | 12 ++- src/qemu/qemu_driver.c | 6 +- src/security/virt-aa-helper.c | 7 +- src/util/virstoragefile.c | 193 ++++++++++++++++++++++-------------------- src/util/virstoragefile.h | 7 +- tests/virstoragetest.c | 55 ++++++++++-- 7 files changed, 162 insertions(+), 123 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 206f75a..b7781c9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18537,9 +18537,8 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (iter(disk, path, 0, opaque) < 0) goto cleanup; - /* XXX: temporarily we need to select the second element of the backing - * chain to start as the first is the copy of the disk itself. */ - tmp = disk->src.backingStore ? disk->src.backingStore->backingStore : NULL; + + tmp = disk->src.backingStore; while (tmp && virStorageIsFile(tmp->path)) { if (!ignoreOpenFailure && tmp->backingStoreRaw && !tmp->backingStore) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 45ed872..bb9cb6b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2239,10 +2239,10 @@ qemuDiskChainCheckBroken(virDomainDiskDefPtr disk) { char *brokenFile = NULL; - if (!virDomainDiskGetSource(disk) || !disk->src.backingStore) + if (!virDomainDiskGetSource(disk)) return 0; - if (virStorageFileChainGetBroken(disk->src.backingStore, &brokenFile) < 0) + if (virStorageFileChainGetBroken(&disk->src, &brokenFile) < 0) return -1; if (brokenFile) { @@ -2419,11 +2419,9 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, qemuDomainGetImageIds(cfg, vm, disk, &uid, &gid); - disk->src.backingStore = virStorageFileGetMetadata(src, - virDomainDiskGetFormat(disk), - uid, gid, - cfg->allowDiskFormatProbing); - if (!disk->src.backingStore) + if (virStorageFileGetMetadata(&disk->src, + uid, gid, + cfg->allowDiskFormatProbing) < 0) ret = -1; cleanup: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a610ed8..c01da6c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15123,7 +15123,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) && STREQ_NULLABLE(format, "raw") && - disk->src.backingStore->backingStore->path) { + disk->src.backingStore->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' has backing file, so raw shallow copy " "is not possible"), @@ -15330,8 +15330,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, if (!top) { top_canon = disk->src.path; - top_meta = disk->src.backingStore; - } else if (!(top_canon = virStorageFileChainLookup(disk->src.backingStore, + top_meta = &disk->src; + } else if (!(top_canon = virStorageFileChainLookup(&disk->src, top, &top_meta, &top_parent))) { goto endjob; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 1c89815..32fc04a 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -943,18 +943,15 @@ 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) + if (!virDomainDiskGetSource(disk)) 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->src.backingStore) { bool probe = ctl->allowDiskFormatProbing; - disk->src.backingStore = virStorageFileGetMetadata(src, - virDomainDiskGetFormat(disk), - -1, -1, probe); + virStorageFileGetMetadata(&disk->src, -1, -1, probe); } /* XXX passing ignoreOpenFailure = true to get back to the behavior diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a716b5d..e8413d5 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1110,105 +1110,112 @@ virStorageFileGetMetadataFromFD(const char *path, /* Recursive workhorse for virStorageFileGetMetadata. */ static int -virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, - const char *directory, - int format, uid_t uid, gid_t gid, - bool allow_probe, virHashTablePtr cycle, - virStorageSourcePtr meta) +virStorageFileGetMetadataRecurse(virStorageSourcePtr src, + const char *canonPath, + uid_t uid, gid_t gid, + bool allow_probe, + virHashTablePtr cycle) { int fd; int ret = -1; + virStorageSourcePtr backingStore = NULL; int backingFormat; - char *backingPath = NULL; - char *backingDirectory = NULL; VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - path, canonPath, NULLSTR(directory), format, + src->path, canonPath, NULLSTR(src->relDir), src->format, (int)uid, (int)gid, allow_probe); if (virHashLookup(cycle, canonPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store for %s is self-referential"), - path); + src->path); return -1; } + if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) return -1; - if (virStorageIsFile(path)) { - if (VIR_STRDUP(meta->relPath, path) < 0) - return -1; - if (VIR_STRDUP(meta->path, canonPath) < 0) - return -1; - if (VIR_STRDUP(meta->relDir, directory) < 0) + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { + if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) { + virReportSystemError(-fd, _("Failed to open file '%s'"), + src->path); return -1; - meta->format = format; + } - if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) { - virReportSystemError(-fd, _("Failed to open file '%s'"), path); + if (virStorageFileGetMetadataFromFDInternal(src, fd, + &backingFormat) < 0) { + VIR_FORCE_CLOSE(fd); return -1; } - ret = virStorageFileGetMetadataFromFDInternal(meta, fd, &backingFormat); - if (VIR_CLOSE(fd) < 0) - VIR_WARN("could not close file %s", path); + VIR_WARN("could not close file %s", src->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->relPath, path) < 0) - return -1; - if (VIR_STRDUP(meta->path, path) < 0) - return -1; - meta->type = VIR_STORAGE_TYPE_NETWORK; - meta->format = VIR_STORAGE_FILE_RAW; - ret = 0; + /* TODO: currently we call this only for local storage */ + return 0; } - if (ret == 0 && meta->backingStoreRaw) { - virStorageSourcePtr backing; - - if (virStorageIsFile(meta->backingStoreRaw)) { - if (virFindBackingFile(directory, - meta->backingStoreRaw, - &backingDirectory, - &backingPath) < 0) { - /* the backing file is (currently) unavailable, treat this - * file as standalone: - * backingStoreRaw is kept to mark broken image chains */ - VIR_WARN("Backing file '%s' of image '%s' is missing.", - meta->backingStoreRaw, path); - - return 0; - } - } else { - if (VIR_STRDUP(backingPath, meta->backingStoreRaw) < 0) - return -1; - } + /* check whether we need to go deeper */ + if (!src->backingStoreRaw) + return 0; - 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, - backingPath, - backingDirectory, backingFormat, - uid, gid, allow_probe, - cycle, backing) < 0) { - /* If we failed to get backing data, mark the chain broken */ - virStorageSourceFree(backing); - } else { - meta->backingStore = backing; + if (VIR_ALLOC(backingStore) < 0) + return -1; + + if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0) + goto cleanup; + + if (virStorageIsFile(src->backingStoreRaw)) { + backingStore->type = VIR_STORAGE_TYPE_FILE; + + if (virFindBackingFile(src->relDir, + src->backingStoreRaw, + &backingStore->relDir, + &backingStore->path) < 0) { + /* the backing file is (currently) unavailable, treat this + * file as standalone: + * backingStoreRaw is kept to mark broken image chains */ + VIR_WARN("Backing file '%s' of image '%s' is missing.", + src->backingStoreRaw, src->path); + ret = 0; + goto cleanup; } + } else { + /* TODO: To satisfy the test case, copy the network URI as path. This + * will be removed later. */ + backingStore->type = VIR_STORAGE_TYPE_NETWORK; + + if (VIR_STRDUP(backingStore->path, src->backingStoreRaw) < 0) + goto cleanup; } - VIR_FREE(backingDirectory); - VIR_FREE(backingPath); + if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) + backingStore->format = VIR_STORAGE_FILE_RAW; + else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) + backingStore->format = VIR_STORAGE_FILE_AUTO; + else + backingStore->format = backingFormat; + + if (virStorageFileGetMetadataRecurse(backingStore, + backingStore->path, + uid, gid, allow_probe, + cycle) < 0) { + /* if we fail somewhere midway, just accept the and return a + * broken chain */ + ret = 0; + goto cleanup; + } + + src->backingStore = backingStore; + backingStore = NULL; + ret = 0; + + cleanup: + virStorageSourceFree(backingStore); return ret; } + /** * virStorageFileGetMetadata: * @@ -1226,51 +1233,51 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, * * Caller MUST free result after use via virStorageSourceFree. */ -virStorageSourcePtr -virStorageFileGetMetadata(const char *path, int format, +int +virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, bool allow_probe) { VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", - path, format, (int)uid, (int)gid, allow_probe); + src->path, src->format, (int)uid, (int)gid, allow_probe); - virHashTablePtr cycle = virHashCreate(5, NULL); - virStorageSourcePtr meta = NULL; - virStorageSourcePtr ret = NULL; + virHashTablePtr cycle = NULL; char *canonPath = NULL; - char *directory = NULL; + int ret = -1; - if (!cycle) - return NULL; + if (!(cycle = virHashCreate(5, NULL))) + return -1; - if (virStorageIsFile(path)) { - if (!(canonPath = canonicalize_file_name(path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { + if (!(canonPath = canonicalize_file_name(src->path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), + src->path); goto cleanup; } - if (!(directory = mdir_name(path))) { + + if (!src->relPath && + VIR_STRDUP(src->relPath, src->path) < 0) + goto cleanup; + + if (!src->relDir && + !(src->relDir = mdir_name(src->path))) { virReportOOMError(); goto cleanup; } - } else if (VIR_STRDUP(canonPath, path) < 0) { + } else { + /* TODO: currently unimplemented for non-local storage */ + ret = 0; goto cleanup; } - if (VIR_ALLOC(meta) < 0) - goto cleanup; - if (format <= VIR_STORAGE_FILE_NONE) - format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - if (virStorageFileGetMetadataRecurse(path, canonPath, directory, format, - uid, gid, allow_probe, cycle, - meta) < 0) - goto cleanup; - ret = meta; - meta = NULL; + if (src->format <= VIR_STORAGE_FILE_NONE) + src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; + + ret = virStorageFileGetMetadataRecurse(src, canonPath, uid, gid, + allow_probe, cycle); cleanup: - virStorageSourceFree(meta); VIR_FREE(canonPath); - VIR_FREE(directory); virHashFree(cycle); return ret; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index e60befe..a0adb9b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -263,10 +263,9 @@ int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); int virStorageFileProbeFormatFromBuf(const char *path, char *buf, size_t buflen); -virStorageSourcePtr virStorageFileGetMetadata(const char *path, - int format, - uid_t uid, gid_t gid, - bool allow_probe) +int virStorageFileGetMetadata(virStorageSourcePtr src, + uid_t uid, gid_t gid, + bool allow_probe) ATTRIBUTE_NONNULL(1); virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, int fd, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 370b8c2..c02d866 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -29,6 +29,7 @@ #include "virlog.h" #include "virstoragefile.h" #include "virstring.h" +#include "dirname.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -88,6 +89,44 @@ testCleanupImages(void) virFileDeleteTree(datadir); } + +static virStorageSourcePtr +testStorageFileGetMetadata(const char *path, + int format, + uid_t uid, gid_t gid, + bool allow_probe) +{ + virStorageSourcePtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->type = VIR_STORAGE_TYPE_FILE; + ret->format = format; + + if (VIR_STRDUP(ret->relPath, path) < 0) + goto error; + + if (!(ret->relDir = mdir_name(path))) { + virReportOOMError(); + goto error; + } + + if (!(ret->path = canonicalize_file_name(path))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "failed to resolve '%s'", path); + goto error; + } + + if (virStorageFileGetMetadata(ret, uid, gid, allow_probe) < 0) + goto error; + + return ret; + + error: + virStorageSourceFree(ret); + return NULL; +} + static int testPrepImages(void) { @@ -272,8 +311,8 @@ testStorageChain(const void *args) char *broken = NULL; bool isAbs = !!(data->flags & ABS_START); - meta = virStorageFileGetMetadata(data->start, data->format, -1, -1, - (data->flags & ALLOW_PROBE) != 0); + meta = testStorageFileGetMetadata(data->start, data->format, -1, -1, + (data->flags & ALLOW_PROBE) != 0); if (!meta) { if (data->flags & EXP_FAIL) { virResetLastError(); @@ -825,8 +864,8 @@ mymain(void) ret = -1; /* Test behavior of chain lookups, absolute backing from relative start */ - chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, - -1, -1, false); + chain = testStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, + -1, -1, false); if (!chain) { ret = -1; goto cleanup; @@ -870,8 +909,8 @@ mymain(void) /* Test behavior of chain lookups, relative backing from absolute start */ virStorageSourceFree(chain); - chain = virStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, - -1, -1, false); + chain = testStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, + -1, -1, false); if (!chain) { ret = -1; goto cleanup; @@ -900,8 +939,8 @@ mymain(void) /* Test behavior of chain lookups, relative backing */ virStorageSourceFree(chain); - chain = virStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2, - -1, -1, false); + chain = testStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2, + -1, -1, false); if (!chain) { ret = -1; goto cleanup; -- 1.9.1
On 04/23/2014 03:19 PM, Peter Krempa wrote:
To avoid having the root of a backing chain present twice in the list we need to invert the working of virStorageFileGetMetadataRecurse.
Until now the recursive worker created a new backing chain element from the name and other information passed as arguments. This required us to pass the data of the parent in a deconstructed way and the worker created a new entry for the parent.
This patch converts this function so that it just fills in metadata about the parent and creates a backing chain element from those. This removes the duplication of the first element.
To avoid breaking the test suite, virstoragetest now calls a wrapper that creates the parent structure explicitly and pre-fills it with the test data with same function signature as previously used. --- src/conf/domain_conf.c | 5 +- src/qemu/qemu_domain.c | 12 ++- src/qemu/qemu_driver.c | 6 +- src/security/virt-aa-helper.c | 7 +- src/util/virstoragefile.c | 193 ++++++++++++++++++++++-------------------- src/util/virstoragefile.h | 7 +- tests/virstoragetest.c | 55 ++++++++++-- 7 files changed, 162 insertions(+), 123 deletions(-)
+ + if (virStorageFileGetMetadataRecurse(backingStore, + backingStore->path, + uid, gid, allow_probe, + cycle) < 0) { + /* if we fail somewhere midway, just accept the and return a
s/the // ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/24/14 14:19, Eric Blake wrote:
On 04/23/2014 03:19 PM, Peter Krempa wrote:
To avoid having the root of a backing chain present twice in the list we need to invert the working of virStorageFileGetMetadataRecurse.
Until now the recursive worker created a new backing chain element from the name and other information passed as arguments. This required us to pass the data of the parent in a deconstructed way and the worker created a new entry for the parent.
This patch converts this function so that it just fills in metadata about the parent and creates a backing chain element from those. This removes the duplication of the first element.
To avoid breaking the test suite, virstoragetest now calls a wrapper that creates the parent structure explicitly and pre-fills it with the test data with same function signature as previously used. --- src/conf/domain_conf.c | 5 +- src/qemu/qemu_domain.c | 12 ++- src/qemu/qemu_driver.c | 6 +- src/security/virt-aa-helper.c | 7 +- src/util/virstoragefile.c | 193 ++++++++++++++++++++++-------------------- src/util/virstoragefile.h | 7 +- tests/virstoragetest.c | 55 ++++++++++-- 7 files changed, 162 insertions(+), 123 deletions(-)
+ + if (virStorageFileGetMetadataRecurse(backingStore, + backingStore->path, + uid, gid, allow_probe, + cycle) < 0) { + /* if we fail somewhere midway, just accept the and return a
s/the //
ACK
Pushed; Thanks. Peter
participants (2)
-
Eric Blake -
Peter Krempa