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>
---
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;
}
@@ -1176,7 +1190,8 @@ virStorageFileChainGetBroken(virStorageFileMetadataPtr chain,
tmp = chain;
while (tmp) {
- /* Break if no backing store or backing store is not file */
+ /* Break if no backing store, backing store is not file, or
+ * other problem such as infinite loop */
if (!tmp->backingStoreRaw)
break;
if (!tmp->backingStore) {
--
1.9.0