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

Eric Blake eblake at redhat.com
Tue Apr 8 04:07:53 UTC 2014


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 at 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




More information about the libvir-list mailing list