Re: [libvirt] [PATCHv2 2/5] conf: fix detection of infinite backing loop

While trying to refactor the backing file chain, I noticed that if you have a self-referential qcow2 file via a relative name: qemu-img create -f qcow2 loop 10M qemu-img rebase -u -f qcow2 -F qcow2 -b loop loop then libvirt was creating a chain 2 deep before realizing it had hit a loop; furthermore, virStorageFileChainCheckBroken was not identifying the chain as broken. With this patch, the loop is detected when the chain is only 1 deep; still enough for storage volume XML to display the file, but now with a proper error report about where the loop was found. * src/util/virstoragefile.c (virStorageFileGetMetadataRecurse): Add parameter, require canonical path up front. Mark chain broken on OOM or loop detection. (virStorageFileGetMetadata): Pass in canonical name. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v2: hoist canonical computation out of virStorageFileGetMetadataRecurse rest of patch series still works as is src/util/virstoragefile.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3cdc89c..413ea5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1065,26 +1065,27 @@ virStorageFileGetMetadataFromFD(const char *path, /* Recursive workhorse for virStorageFileGetMetadata. */ static virStorageFileMetadataPtr -virStorageFileGetMetadataRecurse(const char *path, const char *directory, +virStorageFileGetMetadataRecurse(const char *path, const char *canonPath,

+ const char *directory, int format, uid_t uid, gid_t gid, bool allow_probe, virHashTablePtr cycle) { int fd; - VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", - path, format, (int)uid, (int)gid, allow_probe); + VIR_DEBUG("path=%s canonPath=%s format=%d uid=%d gid=%d probe=%d", + path, canonPath, format, (int)uid, (int)gid, allow_probe); virStorageFileMetadataPtr ret = NULL; - if (virHashLookup(cycle, path)) { + if (virHashLookup(cycle, canonPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store for %s is self-referential"), path); return NULL; } - if (virHashAddEntry(cycle, path, (void *)1) < 0) + if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) return NULL; - if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { + if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), path); return NULL; } @@ -1100,14 +1101,19 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory, else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO; format = ret->backingStoreFormat; - ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStore, + ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStoreRaw, + ret->backingStore, ret->directory, format, uid, gid, allow_probe, cycle); + if (!ret->backingMeta) { + /* If we failed to get backing data, mark the chain broken */ + ret->backingStoreFormat = VIR_STORAGE_FILE_NONE; + VIR_FREE(ret->backingStore); + } } - return ret; } @@ -1142,15 +1148,23 @@ virStorageFileGetMetadata(const char *path, int format, path, format, (int)uid, (int)gid, allow_probe); virHashTablePtr cycle = virHashCreate(5, NULL); - virStorageFileMetadataPtr ret; + virStorageFileMetadataPtr ret = NULL; + char *canonPath; if (!cycle) return NULL; + if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + goto cleanup; + } + if (format <= VIR_STORAGE_FILE_NONE) format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(path, NULL, format, uid, gid, - allow_probe, cycle); + ret = virStorageFileGetMetadataRecurse(path, canonPath, NULL, format,

+ uid, gid, allow_probe, cycle); + cleanup: + VIR_FREE(canonPath); virHashFree(cycle); return ret; }