On 04/20/2014 04:13 PM, Peter Krempa wrote: 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). 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/14 22:53, Eric Blake wrote: 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. Peter

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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