The previous patch started a separation of error messages reported against the user-specified name, vs. tracking the canonical path that was actually opened. This patch extends that notion, by hoisting directory detection up front, passing the canonical path through the entire call chain, and simplifying lower-level functions that can now assume that a canonical path and directory have been supplied. * src/util/virstoragefile.c (virStorageFileGetMetadataFromFDInternal) (virStorageFileGetMetadataInternal): Add parameter, require directory. (virFindBackingFile): Require directory. (virStorageFileGetMetadataFromFD): Pass canonical path. (virStorageFileGetMetadataFromBuf): Likewise. (virStorageFileGetMetadata): Determine initial directory. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 97 ++++++++++++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 38 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 9a23ec7..336842c 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -558,14 +558,15 @@ qedGetBackingStore(char **res, } /** - * Given a starting point START (either an original file name, or the - * directory containing the original name, depending on START_IS_DIR) - * and a possibly relative backing file NAME, compute the relative - * DIRECTORY (optional) and CANONICAL (mandatory) location of the - * backing file. Return 0 on success, negative on error. + * Given a starting point START (a directory containing the original + * file, if the original file was opened via a relative path; ignored + * if NAME is absolute), determine the location of the backing file + * NAME (possibly relative), and compute the relative DIRECTORY + * (optional) and CANONICAL (mandatory) location of the backing file. + * Return 0 on success, negative on error. */ -static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5) -virFindBackingFile(const char *start, bool start_is_dir, const char *path, +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) +virFindBackingFile(const char *start, const char *path, char **directory, char **canonical) { char *combined = NULL; @@ -574,19 +575,8 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path, if (*path == '/') { /* Safe to cast away const */ combined = (char *)path; - } else { - size_t d_len = start_is_dir ? strlen(start) : dir_len(start); - - if (d_len > INT_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("name too long: '%s'"), start); - goto cleanup; - } else if (d_len == 0) { - start = "."; - d_len = 1; - } - if (virAsprintf(&combined, "%.*s/%s", (int)d_len, start, path) < 0) - goto cleanup; + } else if (virAsprintf(&combined, "%s/%s", start, path) < 0) { + goto cleanup; } if (directory && !(*directory = mdir_name(combined))) { @@ -779,22 +769,24 @@ qcow2GetFeatures(virBitmapPtr *features, } -/* Given a header in BUF with length LEN, as parsed from the file - * located at PATH, and optionally opened from a given DIRECTORY, - * return metadata about that file, assuming it has the given - * FORMAT. */ -static virStorageFileMetadataPtr +/* 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, return + * metadata about that file, assuming it has the given FORMAT. */ +static virStorageFileMetadataPtr ATTRIBUTE_NONNULL(1) +ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) virStorageFileGetMetadataInternal(const char *path, + const char *canonPath, + const char *directory, char *buf, size_t len, - const char *directory, int format) { virStorageFileMetadata *meta = NULL; virStorageFileMetadata *ret = NULL; - VIR_DEBUG("path=%s, buf=%p, len=%zu, directory=%s, format=%d", - path, buf, len, NULLSTR(directory), format); + VIR_DEBUG("path=%s, canonPath=%s, dir=%s, buf=%p, len=%zu, format=%d", + path, canonPath, directory, buf, len, format); if (VIR_ALLOC(meta) < 0) return NULL; @@ -864,8 +856,7 @@ virStorageFileGetMetadataInternal(const char *path, meta->backingStoreIsFile = true; meta->backingStoreRaw = meta->backingStore; meta->backingStore = NULL; - if (virFindBackingFile(directory ? directory : path, - !!directory, backing, + if (virFindBackingFile(directory, backing, &meta->directory, &meta->backingStore) < 0) { /* the backing file is (currently) unavailable, treat this @@ -990,15 +981,27 @@ virStorageFileGetMetadataFromBuf(const char *path, size_t len, int format) { - return virStorageFileGetMetadataInternal(path, buf, len, NULL, format); + virStorageFileMetadataPtr ret; + char *canonPath; + + if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + return NULL; + } + + ret = virStorageFileGetMetadataInternal(path, canonPath, ".", buf, len, + format); + VIR_FREE(canonPath); + return ret; } /* Internal version that also supports a containing directory name. */ static virStorageFileMetadataPtr virStorageFileGetMetadataFromFDInternal(const char *path, - int fd, + const char *canonPath, const char *directory, + int fd, int format) { char *buf = NULL; @@ -1029,7 +1032,8 @@ virStorageFileGetMetadataFromFDInternal(const char *path, goto cleanup; } - ret = virStorageFileGetMetadataInternal(path, buf, len, directory, format); + ret = virStorageFileGetMetadataInternal(path, canonPath, directory, + buf, len, format); cleanup: VIR_FREE(buf); return ret; @@ -1059,7 +1063,17 @@ virStorageFileGetMetadataFromFD(const char *path, int fd, int format) { - return virStorageFileGetMetadataFromFDInternal(path, fd, NULL, format); + virStorageFileMetadataPtr ret; + char *canonPath; + + if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + return NULL; + } + ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, ".", + fd, format); + VIR_FREE(canonPath); + return ret; } @@ -1091,7 +1105,8 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, return NULL; } - ret = virStorageFileGetMetadataFromFDInternal(path, fd, directory, format); + ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, directory, + fd, format); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); @@ -1150,7 +1165,8 @@ virStorageFileGetMetadata(const char *path, int format, virHashTablePtr cycle = virHashCreate(5, NULL); virStorageFileMetadataPtr ret = NULL; - char *canonPath; + char *canonPath = NULL; + char *directory = NULL; if (!cycle) return NULL; @@ -1159,13 +1175,18 @@ virStorageFileGetMetadata(const char *path, int format, virReportSystemError(errno, _("unable to resolve '%s'"), path); goto cleanup; } + if (!(directory = mdir_name(path))) { + virReportOOMError(); + goto cleanup; + } if (format <= VIR_STORAGE_FILE_NONE) format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(path, canonPath, NULL, format, + ret = virStorageFileGetMetadataRecurse(path, canonPath, directory, format, uid, gid, allow_probe, cycle); cleanup: VIR_FREE(canonPath); + VIR_FREE(directory); virHashFree(cycle); return ret; } @@ -1464,7 +1485,7 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, break; } else if (owner->backingStoreIsFile) { char *absName = NULL; - if (virFindBackingFile(owner->directory, true, name, + if (virFindBackingFile(owner->directory, name, NULL, &absName) < 0) goto error; if (absName && STREQ(absName, owner->backingStore)) { -- 1.9.0

Right now, we are allocating virStorageFileMetadata near the bottom of the callchain, only after we have identified that we are visiting a file (and not a network resource). I'm hoping to eventually support parsing the backing chain from XML, where the backing chain crawl then validates what was parsed rather than allocating a fresh structure. Likewise, I'm working towards a setup where we have a backing element even for networks. Both of these use cases are easier to code if the allocation is hoisted earlier. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal) (virStorageFileGetMetadataFromFDInternal): Change signature. (virStorageFileGetMetadataFromBuf) (virStorageFileGetMetadataRecurse, virStorageFileGetMetadata): Update callers. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 126 +++++++++++++++++++++++++++------------------- 1 file changed, 75 insertions(+), 51 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 336842c..e516acf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -771,26 +771,24 @@ 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, return - * metadata about that file, assuming it has the given FORMAT. */ -static virStorageFileMetadataPtr ATTRIBUTE_NONNULL(1) -ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) + * 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. */ +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) +ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(7) virStorageFileGetMetadataInternal(const char *path, const char *canonPath, const char *directory, char *buf, size_t len, - int format) + int format, + virStorageFileMetadataPtr meta) { - virStorageFileMetadata *meta = NULL; - virStorageFileMetadata *ret = NULL; + int ret = -1; VIR_DEBUG("path=%s, canonPath=%s, dir=%s, buf=%p, len=%zu, format=%d", path, canonPath, directory, buf, len, format); - if (VIR_ALLOC(meta) < 0) - return NULL; - if (format == VIR_STORAGE_FILE_AUTO) format = virStorageFileProbeFormatFromBuf(path, buf, len); @@ -886,11 +884,9 @@ virStorageFileGetMetadataInternal(const char *path, goto cleanup; done: - ret = meta; - meta = NULL; + ret = 0; cleanup: - virStorageFileFreeMetadata(meta); return ret; } @@ -981,44 +977,52 @@ virStorageFileGetMetadataFromBuf(const char *path, size_t len, int format) { - virStorageFileMetadataPtr ret; + 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) + goto cleanup; + + if (virStorageFileGetMetadataInternal(path, canonPath, ".", buf, len, + format, ret) < 0) { + virStorageFileFreeMetadata(ret); + ret = NULL; + } - ret = virStorageFileGetMetadataInternal(path, canonPath, ".", buf, len, - format); + cleanup: VIR_FREE(canonPath); return ret; } /* Internal version that also supports a containing directory name. */ -static virStorageFileMetadataPtr +static int virStorageFileGetMetadataFromFDInternal(const char *path, const char *canonPath, const char *directory, int fd, - int format) + int format, + virStorageFileMetadataPtr meta) { char *buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; struct stat sb; - virStorageFileMetadataPtr ret = NULL; + int ret = -1; if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), path); - return NULL; + return -1; } /* No header to probe for directories, but also no backing file */ if (S_ISDIR(sb.st_mode)) { - ignore_value(VIR_ALLOC(ret)); + ret = 0; goto cleanup; } @@ -1033,7 +1037,8 @@ virStorageFileGetMetadataFromFDInternal(const char *path, } ret = virStorageFileGetMetadataInternal(path, canonPath, directory, - buf, len, format); + buf, len, format, meta); + cleanup: VIR_FREE(buf); return ret; @@ -1063,71 +1068,81 @@ virStorageFileGetMetadataFromFD(const char *path, int fd, int format) { - virStorageFileMetadataPtr ret; + virStorageFileMetadataPtr ret = NULL; char *canonPath; if (!(canonPath = canonicalize_file_name(path))) { virReportSystemError(errno, _("unable to resolve '%s'"), path); return NULL; } - ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, ".", - fd, format); + if (VIR_ALLOC(ret) < 0) + goto cleanup; + if (virStorageFileGetMetadataFromFDInternal(path, canonPath, ".", + fd, format, ret) < 0) { + virStorageFileFreeMetadata(ret); + ret = NULL; + } + + cleanup: VIR_FREE(canonPath); return ret; } /* Recursive workhorse for virStorageFileGetMetadata. */ -static virStorageFileMetadataPtr +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) + bool allow_probe, virHashTablePtr cycle, + virStorageFileMetadataPtr meta) { int fd; + int ret = -1; VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", path, canonPath, NULLSTR(directory), format, (int)uid, (int)gid, allow_probe); - virStorageFileMetadataPtr ret = NULL; - if (virHashLookup(cycle, canonPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store for %s is self-referential"), path); - return NULL; + return -1; } if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) - return NULL; + return -1; if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), path); - return NULL; + return -1; } ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, directory, - fd, format); + fd, format, meta); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); - if (ret && ret->backingStoreIsFile) { - if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) - ret->backingStoreFormat = VIR_STORAGE_FILE_RAW; - else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) - ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO; - format = ret->backingStoreFormat; - ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStoreRaw, - ret->backingStore, - ret->directory, - format, - uid, gid, - allow_probe, - cycle); - if (!ret->backingMeta) { + if (ret == 0 && meta->backingStoreIsFile) { + virStorageFileMetadataPtr backing; + + if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) + meta->backingStoreFormat = VIR_STORAGE_FILE_RAW; + else if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) + meta->backingStoreFormat = VIR_STORAGE_FILE_AUTO; + format = meta->backingStoreFormat; + if (VIR_ALLOC(backing) < 0 || + virStorageFileGetMetadataRecurse(meta->backingStoreRaw, + meta->backingStore, + meta->directory, format, + uid, gid, allow_probe, + cycle, backing) < 0) { /* If we failed to get backing data, mark the chain broken */ - ret->backingStoreFormat = VIR_STORAGE_FILE_NONE; - VIR_FREE(ret->backingStore); + meta->backingStoreFormat = VIR_STORAGE_FILE_NONE; + VIR_FREE(meta->backingStore); + virStorageFileFreeMetadata(backing); + } else { + meta->backingMeta = backing; } } return ret; @@ -1164,6 +1179,7 @@ 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; char *canonPath = NULL; char *directory = NULL; @@ -1179,12 +1195,20 @@ virStorageFileGetMetadata(const char *path, int format, virReportOOMError(); 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; - ret = virStorageFileGetMetadataRecurse(path, canonPath, directory, format, - uid, gid, allow_probe, cycle); + if (virStorageFileGetMetadataRecurse(path, canonPath, directory, format, + uid, gid, allow_probe, cycle, + meta) < 0) + goto cleanup; + ret = meta; + meta = NULL; + cleanup: + virStorageFileFreeMetadata(meta); VIR_FREE(canonPath); VIR_FREE(directory); virHashFree(cycle); -- 1.9.0

A later patch will be adding some new fields to virStorageFileMetadata; to minimize confusion, renaming the test fields now will make it more obvious which fields are being tested later. * tests/virstoragetest.c (_testFileData): Alter names. (testStorageChain, mymain): Adjust clients. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 78 +++++++++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 39 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 74ad15c..093053a 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -212,9 +212,9 @@ struct _testFileData { const char *expBackingStore; const char *expBackingStoreRaw; - const char *expDirectoryRel; - const char *expDirectoryAbs; - enum virStorageFileFormat expFormat; + const char *expBackingDirRel; + const char *expBackingDirAbs; + enum virStorageFileFormat expBackingFormat; bool expIsFile; unsigned long long expCapacity; bool expEncrypted; @@ -285,21 +285,21 @@ testStorageChain(const void *args) while (elt) { char *expect = NULL; char *actual = NULL; - const char *expDirectory; + const char *expBackingDirectory; if (i == data->nfiles) { fprintf(stderr, "probed chain was too long\n"); goto cleanup; } - expDirectory = abs ? data->files[i]->expDirectoryAbs - : data->files[i]->expDirectoryRel; + expBackingDirectory = abs ? data->files[i]->expBackingDirAbs + : data->files[i]->expBackingDirRel; if (virAsprintf(&expect, "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d", NULLSTR(data->files[i]->expBackingStore), NULLSTR(data->files[i]->expBackingStoreRaw), - NULLSTR(expDirectory), - data->files[i]->expFormat, + NULLSTR(expBackingDirectory), + data->files[i]->expBackingFormat, data->files[i]->expIsFile, data->files[i]->expCapacity, data->files[i]->expEncrypted) < 0 || @@ -388,7 +388,7 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { - .expFormat = VIR_STORAGE_FILE_NONE, + .expBackingFormat = VIR_STORAGE_FILE_NONE, }; TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW, (&raw), EXP_PASS, @@ -405,9 +405,9 @@ mymain(void) testFileData qcow2 = { .expBackingStore = canonraw, .expBackingStoreRaw = "raw", - .expDirectoryRel = ".", - .expDirectoryAbs = datadir, - .expFormat = VIR_STORAGE_FILE_RAW, + .expBackingDirRel = ".", + .expBackingDirAbs = datadir, + .expBackingFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, }; @@ -429,7 +429,7 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = absraw; - qcow2.expDirectoryRel = datadir; + qcow2.expBackingDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format provided */ TEST_CHAIN(5, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -447,9 +447,9 @@ mymain(void) testFileData wrap = { .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, - .expDirectoryRel = datadir, - .expDirectoryAbs = datadir, - .expFormat = VIR_STORAGE_FILE_QCOW2, + .expBackingDirRel = datadir, + .expBackingDirAbs = datadir, + .expBackingFormat = VIR_STORAGE_FILE_QCOW2, .expIsFile = true, .expCapacity = 1024, }; @@ -471,16 +471,16 @@ mymain(void) "-b", absqcow2, "wrap", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - wrap.expFormat = VIR_STORAGE_FILE_AUTO; - qcow2.expFormat = VIR_STORAGE_FILE_AUTO; + wrap.expBackingFormat = VIR_STORAGE_FILE_AUTO; + qcow2.expBackingFormat = VIR_STORAGE_FILE_AUTO; /* Qcow2 file with raw as absolute backing, backing format omitted */ testFileData wrap_as_raw = { .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, - .expDirectoryRel = datadir, - .expDirectoryAbs = datadir, - .expFormat = VIR_STORAGE_FILE_RAW, + .expBackingDirRel = datadir, + .expBackingDirAbs = datadir, + .expBackingFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, }; @@ -499,7 +499,7 @@ mymain(void) ret = -1; qcow2.expBackingStore = NULL; qcow2.expBackingStoreRaw = datadir "/bogus"; - qcow2.expFormat = VIR_STORAGE_FILE_NONE; + qcow2.expBackingFormat = VIR_STORAGE_FILE_NONE; qcow2.expIsFile = false; /* Qcow2 file with missing backing file but specified type */ @@ -532,9 +532,9 @@ mymain(void) ret = -1; qcow2.expBackingStore = "nbd:example.org:6000"; qcow2.expBackingStoreRaw = NULL; - qcow2.expDirectoryRel = NULL; - qcow2.expDirectoryAbs = NULL; - qcow2.expFormat = VIR_STORAGE_FILE_RAW; + qcow2.expBackingDirRel = NULL; + qcow2.expBackingDirAbs = NULL; + qcow2.expBackingFormat = VIR_STORAGE_FILE_RAW; /* Qcow2 file with backing protocol instead of file */ TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -547,9 +547,9 @@ mymain(void) testFileData qed = { .expBackingStore = canonraw, .expBackingStoreRaw = absraw, - .expDirectoryRel = datadir, - .expDirectoryAbs = datadir, - .expFormat = VIR_STORAGE_FILE_RAW, + .expBackingDirRel = datadir, + .expBackingDirAbs = datadir, + .expBackingFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, }; @@ -594,18 +594,18 @@ mymain(void) testFileData link1 = { .expBackingStore = canonraw, .expBackingStoreRaw = "../raw", - .expDirectoryRel = "sub/../sub/..", - .expDirectoryAbs = datadir "/sub/../sub/..", - .expFormat = VIR_STORAGE_FILE_RAW, + .expBackingDirRel = "sub/../sub/..", + .expBackingDirAbs = datadir "/sub/../sub/..", + .expBackingFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, }; testFileData link2 = { .expBackingStore = canonqcow2, .expBackingStoreRaw = "../sub/link1", - .expDirectoryRel = "sub/../sub", - .expDirectoryAbs = datadir "/sub/../sub", - .expFormat = VIR_STORAGE_FILE_QCOW2, + .expBackingDirRel = "sub/../sub", + .expBackingDirAbs = datadir "/sub/../sub", + .expBackingFormat = VIR_STORAGE_FILE_QCOW2, .expIsFile = true, .expCapacity = 1024, }; @@ -624,9 +624,9 @@ mymain(void) ret = -1; qcow2.expBackingStore = NULL; qcow2.expBackingStoreRaw = "qcow2"; - qcow2.expDirectoryRel = "."; - qcow2.expDirectoryAbs = datadir; - qcow2.expFormat= VIR_STORAGE_FILE_NONE; + qcow2.expBackingDirRel = "."; + qcow2.expBackingDirAbs = datadir; + qcow2.expBackingFormat= VIR_STORAGE_FILE_NONE; qcow2.expIsFile = true; /* Behavior of an infinite loop chain */ @@ -649,8 +649,8 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = "wrap"; - qcow2.expDirectoryRel = datadir; - wrap.expFormat = VIR_STORAGE_FILE_QCOW2; + qcow2.expBackingDirRel = datadir; + wrap.expBackingFormat = VIR_STORAGE_FILE_QCOW2; /* Behavior of an infinite loop chain */ TEST_CHAIN(17, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, -- 1.9.0

The current use of virStorageFileMetadata is awkward; to learn some of the information about a child node, you have to read fields in the parent node. This does not lend itself well to modifying backing chains (whether inserting a new node in the chain, or consolidating existing nodes); better would be to learn about a child node directly in that node. This patch sets up some new fields which contain redundant information, although not necessarily in the final desired state for the new fields (see the next patch for actual tests of what is there now). Then later patches will do any refactoring necessary to get the fields to their desired states, and update clients to get the information from the new fields, so we can finally delete the fields that are tracking information about the wrong node. More concretely, compare these three example backing chains: good <- one missing <- two gluster://server/vol/img <- three Pre-patch, querying the chains gives: { .backingStore = "/path/to/good", .backingStoreRaw = "good", .backingStoreIsFile = true, .backingStoreFormat = VIR_STORAGE_FILE_RAW, .backingMeta = { .backingStore = NULL, .backingStoreRaw = NULL, .backingStoreIsFile = false, .backingMeta = NULL, } } { .backingStore = NULL, .backingStoreRaw = "missing", .backingStoreIsFile = false, .backingStoreFormat = VIR_STORAGE_FILE_NONE, .backingMeta = NULL, } { .backingStore = "gluster://server/vol/img", .backingStoreRaw = NULL, .backingStoreIsFile = false, .backingStoreFormat = VIR_STORAGE_FILE_RAW, .backingMeta = NULL, } Deciding whether to ignore a missing backing file (as in virsh vol-dumpxml) or report an error (as in security manager sVirt labeling) requires reading multiple fields. Plus, the format is hard-coded to treat all network protocols as end-of-the-chain, as if they were raw. By the end of this patch series, the goal is to instead represent these three situations as: { .path = "one", .canonPath = "/path/to/one", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, .backingStoreRaw = "good", .backingMeta = { .path = "good", .canonPath = "/path/to/good", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, .backingStoreRaw = NULL, .backingMeta = NULL, } } { .path = "two", .canonPath = "/path/to/two", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, .backingStoreRaw = "missing", .backingMeta = NULL, } { .path = "three", .canonPath = "/path/to/three", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, .backingStoreRaw = "gluster://server/vol/img", .backingMeta = { .path = "gluster://server/vol/img", .canonPath = "gluster://server/vol/img", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, .backingStoreRaw = NULL, .backingMeta = NULL, } } or, for the second file, maybe also allowing: { .path = "two", .canonPath = "/path/to/two", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, .backingStoreRaw = "missing", .backingMeta = { .path = "missing", .canonPath = NULL, .type = VIR_STORAGE_TYPE_NONE, .format = VIR_STORAGE_FILE_NONE, .backingStoreRaw = NULL, .backingMeta = NULL, } } * src/util/virstoragefile.h (_virStorageFileMetadata): Add path, canonPath, relDir, type, and format fields. Reorder existing fields, and add lots of comments. * src/util/virstoragefile.c (virStorageFileFreeMetadata): Clean new fields. (virStorageFileGetMetadataInternal) (virStorageFileGetMetadataFromFDInternal): Start populating new fields. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 26 +++++++++++++++++++++++++- src/util/virstoragefile.h | 36 +++++++++++++++++++++++++++++++----- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e516acf..0008cd7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -789,6 +789,13 @@ virStorageFileGetMetadataInternal(const char *path, VIR_DEBUG("path=%s, canonPath=%s, dir=%s, buf=%p, len=%zu, format=%d", path, canonPath, directory, buf, len, 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 (format == VIR_STORAGE_FILE_AUTO) format = virStorageFileProbeFormatFromBuf(path, buf, len); @@ -798,6 +805,7 @@ virStorageFileGetMetadataInternal(const char *path, format); goto cleanup; } + meta->format = format; /* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files @@ -1020,8 +1028,14 @@ virStorageFileGetMetadataFromFDInternal(const char *path, return -1; } - /* No header to probe for directories, but also no backing file */ 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. */ + if (VIR_STRDUP(meta->path, path) < 0) + goto cleanup; + meta->type = VIR_STORAGE_TYPE_DIR; + meta->format = VIR_STORAGE_FILE_DIR; ret = 0; goto cleanup; } @@ -1039,6 +1053,12 @@ virStorageFileGetMetadataFromFDInternal(const char *path, ret = virStorageFileGetMetadataInternal(path, canonPath, directory, buf, len, format, meta); + if (ret == 0) { + if (S_ISREG(sb.st_mode)) + meta->type = VIR_STORAGE_TYPE_FILE; + else if (S_ISBLK(sb.st_mode)) + meta->type = VIR_STORAGE_TYPE_BLOCK; + } cleanup: VIR_FREE(buf); return ret; @@ -1266,6 +1286,10 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) if (!meta) return; + VIR_FREE(meta->path); + VIR_FREE(meta->canonPath); + VIR_FREE(meta->relDir); + virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); VIR_FREE(meta->backingStoreRaw); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 83ec2bd..984d1e3 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -112,17 +112,43 @@ struct _virStorageTimestamps { typedef struct _virStorageFileMetadata virStorageFileMetadata; typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { - char *backingStore; /* Canonical name (absolute file, or protocol) */ - char *backingStoreRaw; /* If file, original name, possibly relative */ - char *directory; /* The directory containing basename of backingStoreRaw */ - int backingStoreFormat; /* enum virStorageFileFormat */ - bool backingStoreIsFile; + /* Name of the current file as spelled by the user (top level) or + * metadata of the overlay (if this is a backing store). */ + char *path; + /* Canonical name of the current file, used to detect loops in the + * backing store chain. */ + char *canonPath; + /* 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; + + /* 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 */ + char *directory; /* The directory containing basename of backingStoreRaw. Should be same as backingMeta->relDir */ + int backingStoreFormat; /* enum virStorageFileFormat. Should be same as backingMeta->format */ + bool backingStoreIsFile; /* Should be same as backingMeta->type < VIR_STORAGE_TYPE_NETWORK */ }; -- 1.9.0

The testsuite is absolutely essential to feeling comfortable about swapping the backing chain structure over to a new format. This patch tests the path settings, and demonstrates that the correct short name is being passed to the child. * tests/virstoragetest.c (testStorageChain): Test path. (mymain): Update expected data. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 69 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 13 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 093053a..9c2f815 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -218,6 +218,8 @@ struct _testFileData bool expIsFile; unsigned long long expCapacity; bool expEncrypted; + const char *pathRel; + const char *pathAbs; }; enum { @@ -286,6 +288,7 @@ testStorageChain(const void *args) char *expect = NULL; char *actual = NULL; const char *expBackingDirectory; + const char *expPath; if (i == data->nfiles) { fprintf(stderr, "probed chain was too long\n"); @@ -294,22 +297,28 @@ testStorageChain(const void *args) expBackingDirectory = abs ? data->files[i]->expBackingDirAbs : data->files[i]->expBackingDirRel; + expPath = abs ? data->files[i]->pathAbs + : data->files[i]->pathRel; if (virAsprintf(&expect, - "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d", + "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d\n" + "path:%s\n", NULLSTR(data->files[i]->expBackingStore), NULLSTR(data->files[i]->expBackingStoreRaw), NULLSTR(expBackingDirectory), data->files[i]->expBackingFormat, data->files[i]->expIsFile, data->files[i]->expCapacity, - data->files[i]->expEncrypted) < 0 || + data->files[i]->expEncrypted, + NULLSTR(expPath)) < 0 || virAsprintf(&actual, - "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d", + "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d\n" + "path:%s\n", NULLSTR(elt->backingStore), NULLSTR(elt->backingStoreRaw), NULLSTR(elt->directory), elt->backingStoreFormat, elt->backingStoreIsFile, - elt->capacity, !!elt->encryption) < 0) { + elt->capacity, !!elt->encryption, + NULLSTR(elt->path)) < 0) { VIR_FREE(expect); VIR_FREE(actual); goto cleanup; @@ -389,6 +398,8 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { .expBackingFormat = VIR_STORAGE_FILE_NONE, + .pathRel = "raw", + .pathAbs = canonraw, }; TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW, (&raw), EXP_PASS, @@ -402,6 +413,7 @@ mymain(void) (&raw), ALLOW_PROBE | EXP_PASS); /* Qcow2 file with relative raw backing, format provided */ + raw.pathAbs = "raw"; testFileData qcow2 = { .expBackingStore = canonraw, .expBackingStoreRaw = "raw", @@ -410,6 +422,13 @@ mymain(void) .expBackingFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .pathRel = "qcow2", + .pathAbs = canonqcow2, + }; + testFileData qcow2_as_raw = { + .expBackingFormat = VIR_STORAGE_FILE_NONE, + .pathRel = "qcow2", + .pathAbs = canonqcow2, }; TEST_CHAIN(3, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &raw), EXP_PASS, @@ -417,9 +436,9 @@ mymain(void) (&qcow2, &raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(4, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - (&raw), EXP_PASS, + (&qcow2_as_raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, - (&raw), EXP_PASS, + (&qcow2_as_raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 file to use absolute backing name */ @@ -430,6 +449,8 @@ mymain(void) ret = -1; qcow2.expBackingStoreRaw = absraw; qcow2.expBackingDirRel = datadir; + raw.pathRel = absraw; + raw.pathAbs = absraw; /* Qcow2 file with raw as absolute backing, backing format provided */ TEST_CHAIN(5, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -438,9 +459,9 @@ mymain(void) (&qcow2, &raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); TEST_CHAIN(6, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - (&raw), EXP_PASS, + (&qcow2_as_raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, - (&raw), EXP_PASS, + (&qcow2_as_raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); /* Wrapped file access */ @@ -452,7 +473,10 @@ mymain(void) .expBackingFormat = VIR_STORAGE_FILE_QCOW2, .expIsFile = true, .expCapacity = 1024, + .pathRel = "wrap", + .pathAbs = abswrap, }; + qcow2.pathRel = absqcow2; TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2, &raw), EXP_PASS, (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS, @@ -473,6 +497,7 @@ mymain(void) ret = -1; wrap.expBackingFormat = VIR_STORAGE_FILE_AUTO; qcow2.expBackingFormat = VIR_STORAGE_FILE_AUTO; + qcow2_as_raw.pathRel = absqcow2; /* Qcow2 file with raw as absolute backing, backing format omitted */ testFileData wrap_as_raw = { @@ -483,11 +508,13 @@ mymain(void) .expBackingFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .pathRel = "wrap", + .pathAbs = abswrap, }; TEST_CHAIN(8, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap_as_raw, &raw), EXP_PASS, + (&wrap_as_raw, &qcow2_as_raw), EXP_PASS, (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS, - (&wrap_as_raw, &raw), EXP_PASS, + (&wrap_as_raw, &qcow2_as_raw), EXP_PASS, (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS); /* Rewrite qcow2 to a missing backing file, with backing type */ @@ -501,6 +528,7 @@ mymain(void) qcow2.expBackingStoreRaw = datadir "/bogus"; qcow2.expBackingFormat = VIR_STORAGE_FILE_NONE; qcow2.expIsFile = false; + qcow2.pathRel = "qcow2"; /* Qcow2 file with missing backing file but specified type */ TEST_CHAIN(9, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -552,16 +580,24 @@ mymain(void) .expBackingFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .pathRel = "qed", + .pathAbs = absqed, + }; + testFileData qed_as_raw = { + .expBackingFormat = VIR_STORAGE_FILE_NONE, + .pathRel = "qed", + .pathAbs = absqed, }; TEST_CHAIN(12, "qed", absqed, VIR_STORAGE_FILE_AUTO, - (&raw), EXP_PASS, + (&qed_as_raw), EXP_PASS, (&qed, &raw), ALLOW_PROBE | EXP_PASS, - (&raw), EXP_PASS, + (&qed_as_raw), EXP_PASS, (&qed, &raw), ALLOW_PROBE | EXP_PASS); /* directory */ testFileData dir = { - .expIsFile = false, + .pathRel = "dir", + .pathAbs = absdir, }; TEST_CHAIN(13, "dir", absdir, VIR_STORAGE_FILE_AUTO, (&dir), EXP_PASS, @@ -599,6 +635,8 @@ mymain(void) .expBackingFormat = VIR_STORAGE_FILE_RAW, .expIsFile = true, .expCapacity = 1024, + .pathRel = "../sub/link1", + .pathAbs = "../sub/link1", }; testFileData link2 = { .expBackingStore = canonqcow2, @@ -608,7 +646,11 @@ mymain(void) .expBackingFormat = VIR_STORAGE_FILE_QCOW2, .expIsFile = true, .expCapacity = 1024, + .pathRel = "sub/link2", + .pathAbs = abslink2, }; + raw.pathRel = "../raw"; + raw.pathAbs = "../raw"; TEST_CHAIN(15, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, (&link2, &link1, &raw), EXP_PASS, (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS, @@ -650,6 +692,7 @@ mymain(void) ret = -1; qcow2.expBackingStoreRaw = "wrap"; qcow2.expBackingDirRel = datadir; + qcow2.pathRel = absqcow2; wrap.expBackingFormat = VIR_STORAGE_FILE_QCOW2; /* Behavior of an infinite loop chain */ -- 1.9.0