storage: use cache to walk backing chain

We used to walk the backing file chain at least twice per disk,
once to set up cgroup device whitelisting, and once to set up
security labeling.  Rather than walk the chain every iteration,
which possibly includes calls to fork() in order to open root-squashed
NFS files, we can exploit the cache of the previous patch.

* src/conf/domain_conf.h (virDomainDiskDefForeachPath): Alter
signature.
* src/conf/domain_conf.c (virDomainDiskDefForeachPath): Require caller
to supply backing chain via disk, if recursion is desired.
* src/security/security_dac.c
(virSecurityDACSetSecurityImageLabel): Adjust caller.
* src/security/security_selinux.c
(virSecuritySELinuxSetSecurityImageLabel): Likewise.
* src/security/virt-aa-helper.c (get_files): Likewise.
* src/qemu/qemu_cgroup.c (qemuSetupDiskCgroup)
(qemuTeardownDiskCgroup): Likewise.
(qemuSetupCgroup): Pre-populate chain.

src/conf/domain_conf.c

@@ -14837,110 +14837,42 @@ done:
 }
 
 
+/* Call iter(disk, name, depth, opaque) for each element of disk and
+ * its backing chain in the pre-populated disk->backingChain.
+ * ignoreOpenFailure determines whether to warn about a chain that
+ * mentions a backing file without also having metadata on that
+ * file.  */
 int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
-                                bool allowProbing,
                                 bool ignoreOpenFailure,
-                                uid_t uid, gid_t gid,
                                 virDomainDiskDefPathIterator iter,
                                 void *opaque)
 {
-    virHashTablePtr paths = NULL;
-    int format;
     int ret = -1;
     size_t depth = 0;
-    char *nextpath = NULL;
-    virStorageFileMetadata *meta = NULL;
+    virStorageFileMetadata *tmp;
 
     if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
         return 0;
 
-    if (disk->format > 0) {
-        format = disk->format;
-    } else {
-        if (allowProbing) {
-            format = VIR_STORAGE_FILE_AUTO;
-        } else {
+    if (iter(disk, disk->src, 0, opaque) < 0)
+        goto cleanup;
+
+    tmp = disk->backingChain;
+    while (tmp && tmp->backingStoreIsFile) {
+        if (!ignoreOpenFailure && !tmp->backingMeta) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("no disk format for %s and probing is disabled"),
-                           disk->src);
+                           _("unable to visit backing chain file %s"),
+                           tmp->backingStore);
             goto cleanup;
         }
+        if (iter(disk, tmp->backingStore, ++depth, opaque) < 0)
+            goto cleanup;
+        tmp = tmp->backingMeta;
     }
 
-    paths = virHashCreate(5, NULL);
-
-    do {
-        const char *path = nextpath ? nextpath : disk->src;
-        int fd;
-
-        if (iter(disk, path, depth, opaque) < 0)
-            goto cleanup;
-
-        if (virHashLookup(paths, path)) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("backing store for %s is self-referential"),
-                           disk->src);
-            goto cleanup;
-        }
-
-        if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
-            if (ignoreOpenFailure) {
-                char ebuf[1024];
-                VIR_WARN("Ignoring open failure on %s: %s", path,
-                         virStrerror(-fd, ebuf, sizeof(ebuf)));
-                break;
-            } else {
-                virReportSystemError(-fd, _("unable to open disk path %s"),
-                                     path);
-                goto cleanup;
-            }
-        }
-
-        if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format))) {
-            VIR_FORCE_CLOSE(fd);
-            goto cleanup;
-        }
-
-        if (VIR_CLOSE(fd) < 0)
-            virReportSystemError(errno,
-                                 _("could not close file %s"),
-                                 path);
-
-        if (virHashAddEntry(paths, path, (void*)0x1) < 0)
-            goto cleanup;
-
-        depth++;
-        VIR_FREE(nextpath);
-        nextpath = meta->backingStore;
-        meta->backingStore = NULL;
-
-        /* Stop iterating if we reach a non-file backing store */
-        if (nextpath && !meta->backingStoreIsFile) {
-            VIR_DEBUG("Stopping iteration on non-file backing store: %s",
-                      nextpath);
-            break;
-        }
-
-        format = meta->backingStoreFormat;
-
-        if (format == VIR_STORAGE_FILE_AUTO &&
-            !allowProbing)
-            format = VIR_STORAGE_FILE_RAW; /* Stops further recursion */
-
-        /* Allow probing for image formats that are safe */
-        if (format == VIR_STORAGE_FILE_AUTO_SAFE)
-            format = VIR_STORAGE_FILE_AUTO;
-        virStorageFileFreeMetadata(meta);
-        meta = NULL;
-    } while (nextpath);
-
     ret = 0;
 
 cleanup:
-    virHashFree(paths);
-    VIR_FREE(nextpath);
-    virStorageFileFreeMetadata(meta);
-
     return ret;
 }
 

src/conf/domain_conf.h

@@ -2150,9 +2150,7 @@ typedef int (*virDomainDiskDefPathIterator)(virDomainDiskDefPtr disk,
                                             void *opaque);
 
 int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
-                                bool allowProbing,
                                 bool ignoreOpenFailure,
-                                uid_t uid, gid_t gid,
                                 virDomainDiskDefPathIterator iter,
                                 void *opaque);
 

src/qemu/qemu_cgroup.c

@@ -87,16 +87,14 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk,
 }
 
 
-int qemuSetupDiskCgroup(struct qemud_driver *driver,
+int qemuSetupDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED,
                         virDomainObjPtr vm,
                         virCgroupPtr cgroup,
                         virDomainDiskDefPtr disk)
 {
     qemuCgroupData data = { vm, cgroup };
     return virDomainDiskDefForeachPath(disk,
-                                       driver->allowDiskFormatProbing,
                                        true,
-                                       driver->user, driver->group,
                                        qemuSetupDiskPathAllow,
                                        &data);
 }
@@ -129,16 +127,14 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
 }
 
 
-int qemuTeardownDiskCgroup(struct qemud_driver *driver,
+int qemuTeardownDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED,
                            virDomainObjPtr vm,
                            virCgroupPtr cgroup,
                            virDomainDiskDefPtr disk)
 {
     qemuCgroupData data = { vm, cgroup };
     return virDomainDiskDefForeachPath(disk,
-                                       driver->allowDiskFormatProbing,
                                        true,
-                                       driver->user, driver->group,
                                        qemuTeardownDiskPathDeny,
                                        &data);
 }
@@ -232,7 +228,9 @@ int qemuSetupCgroup(struct qemud_driver *driver,
         }
 
         for (i = 0; i < vm->def->ndisks ; i++) {
-            if (qemuSetupDiskCgroup(driver, vm, cgroup, vm->def->disks[i]) < 0)
+            if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i],
+                                             false) < 0 ||
+                qemuSetupDiskCgroup(driver, vm, cgroup, vm->def->disks[i]) < 0)
                 goto cleanup;
         }
 

src/security/security_dac.c

@@ -358,8 +358,6 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
                                     virDomainDiskDefPtr disk)
 
 {
-    uid_t user;
-    gid_t group;
     void *params[2];
     virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
 
@@ -369,15 +367,10 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
     if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
         return 0;
 
-    if (virSecurityDACGetImageIds(def, priv, &user, &group))
-        return -1;
-
     params[0] = mgr;
     params[1] = def;
     return virDomainDiskDefForeachPath(disk,
-                                       virSecurityManagerGetAllowDiskFormatProbing(mgr),
                                        false,
-                                       user, group,
                                        virSecurityDACSetSecurityFileLabel,
                                        params);
 }

src/security/security_selinux.c

@@ -1062,13 +1062,10 @@ virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr,
                                         virDomainDiskDefPtr disk)
 
 {
-    bool allowDiskFormatProbing;
     virSecuritySELinuxCallbackData cbdata;
     cbdata.manager = mgr;
     cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
 
-    allowDiskFormatProbing = virSecurityManagerGetAllowDiskFormatProbing(mgr);
-
     if (cbdata.secdef == NULL)
         return -1;
 
@@ -1078,16 +1075,8 @@ virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr,
     if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
         return 0;
 
-    /* XXX On one hand, it would be nice to have the driver's uid:gid
-     * here so we could retry opens with it. On the other hand, it
-     * probably doesn't matter because in practice that's only useful
-     * for files on root-squashed NFS shares, and NFS doesn't properly
-     * support selinux anyway.
-     */
     return virDomainDiskDefForeachPath(disk,
-                                       allowDiskFormatProbing,
                                        true,
-                                       -1, -1, /* current process uid:gid */
                                        virSecuritySELinuxSetSecurityFileLabel,
                                        &cbdata);
 }

src/security/virt-aa-helper.c

@@ -919,20 +919,22 @@ get_files(vahControl * ctl)
     }
 
     for (i = 0; i < ctl->def->ndisks; i++) {
+        virDomainDiskDefPtr disk = ctl->def->disks[i];
+
+        /* 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.
+         */
+        disk->chain = virStorageFileGetMetadata(disk->src, disk->format,
+                                                -1, -1,
+                                                ctl->allowDiskFormatProbing,
+                                                NULL);
+
         /* XXX passing ignoreOpenFailure = true to get back to the behavior
          * from before using virDomainDiskDefForeachPath. actually we should
          * be passing ignoreOpenFailure = false and handle open errors more
          * careful than just ignoring them.
-         * XXX2 - 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.
          */
-        int ret = virDomainDiskDefForeachPath(ctl->def->disks[i],
-                                              ctl->allowDiskFormatProbing,
-                                              true,
-                                              -1, -1, /* current uid:gid */
-                                              add_file_path,
-                                              &buf);
-        if (ret != 0)
+        if (virDomainDiskDefForeachPath(disk, true, add_file_path, &buf) < 0)
             goto clean;
     }