From 38c4a9cc40476bd2e598a1876b1254e88a09c760 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 9 Oct 2012 16:08:14 -0600 Subject: [PATCH] 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 | 100 +++++--------------------------- src/conf/domain_conf.h | 2 - src/qemu/qemu_cgroup.c | 12 ++-- src/security/security_dac.c | 7 --- src/security/security_selinux.c | 11 ---- src/security/virt-aa-helper.c | 20 ++++--- 6 files changed, 32 insertions(+), 120 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 71d1666c54..0933c08496 100644 --- a/src/conf/domain_conf.c +++ b/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 { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("no disk format for %s and probing is disabled"), - disk->src); - goto cleanup; - } - } - - paths = virHashCreate(5, NULL); - - do { - const char *path = nextpath ? nextpath : disk->src; - int fd; - - if (iter(disk, path, depth, opaque) < 0) - goto cleanup; + if (iter(disk, disk->src, 0, opaque) < 0) + goto cleanup; - if (virHashLookup(paths, path)) { + tmp = disk->backingChain; + while (tmp && tmp->backingStoreIsFile) { + if (!ignoreOpenFailure && !tmp->backingMeta) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - disk->src); + _("unable to visit backing chain file %s"), + tmp->backingStore); 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) + if (iter(disk, tmp->backingStore, ++depth, opaque) < 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); + tmp = tmp->backingMeta; + } ret = 0; cleanup: - virHashFree(paths); - VIR_FREE(nextpath); - virStorageFileFreeMetadata(meta); - return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6e6b259d10..4151bc2c52 100644 --- a/src/conf/domain_conf.h +++ b/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); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 6b94686a79..428befdad3 100644 --- a/src/qemu/qemu_cgroup.c +++ b/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; } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9dbf95d5ba..a67f5d63a0 100644 --- a/src/security/security_dac.c +++ b/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); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b5108abb29..eee8d71790 100644 --- a/src/security/security_selinux.c +++ b/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); } diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 51daa4b0ba..263fc92af3 100644 --- a/src/security/virt-aa-helper.c +++ b/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; } -- GitLab