On 10/13/2012 06:00 PM, Eric Blake wrote:
> 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.
Obviously creating the rule that you can't mess with the backing chain
outside the libvirt API, but I think you've already said that :-)
>
> * 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 | 27 +++++++----
> 6 files changed, 40 insertions(+), 119 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f4c05a3..2c0b296 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14633,110 +14633,42 @@ done:
> }
>
>
> +/* Call iter(disk, name, depth, opaque) for each element of disk and
> + its backing chain in the pre-populated disk->chain.
> + 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->chain;
> + 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;
> }
Whew! Can't see the code for all the deletions! :-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 06c1ccd..87faa7e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2139,9 +2139,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 166f9b9..10acb26 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 f126aa5..5f11810 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 10135ed..00c34c2 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1022,13 +1022,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;
>
> @@ -1038,16 +1035,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 51daa4b..729c0d1 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -919,19 +919,30 @@ get_files(vahControl * ctl)
> }
>
> for (i = 0; i < ctl->def->ndisks; i++) {
> + int ret;
> + int format;
> + virDomainDiskDefPtr disk = ctl->def->disks[i];
> +
> + if (disk->format > 0)
> + format = disk->format;
> + else if (ctl->allowDiskFormatProbing)
> + format = VIR_STORAGE_FILE_AUTO;
> + else
> + format = VIR_STORAGE_FILE_RAW;
It seems like I've seen this same bit of code a few times now...
> +
> + /* 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, 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);
> + ret = virDomainDiskDefForeachPath(disk, true, add_file_path, &buf);
> if (ret != 0)
> goto clean;
> }
ACK.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list