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 | 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;
}
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;
+
+ /* 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;
}
--
1.7.11.7
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list