util: storage: Invert the way recursive metadata retrieval works

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

@@ -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,

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:

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;

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

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 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;
 }

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,

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;