[libvirt] [PATCH 00/10] Setup cgroups for firmware devices and related refactors
Peter Krempa (10): qemu: cgroup: Remove abandoned function qemuAddToCgroup qemu: migration: Refactor code now that we assume support for fd migration util: cgroup: Instrument virCgroupDenyDevice to handle -1 device number as * util: cgroup: Drop virCgroup(Allow|Deny)DeviceMajor util: cgroup: Allow ignoring EACCES in virCgroup(Allow|Deny)DevicePath qemu: cgroup: Avoid reporting errors from inaccessible NFS volumes qemu: cgroup: Switch to qemu(Setup|Teardown)ImageCgroup qemu: cgroup: Split up qemuSetImageCgroupInternal qemu: cgroup: Extract guts of qemuSetupImageCgroupInternal qemu: cgroup: Setup cgroups for bios/firmware images src/libvirt_private.syms | 2 - src/lxc/lxc_cgroup.c | 16 ++--- src/lxc/lxc_driver.c | 9 ++- src/qemu/qemu_cgroup.c | 151 +++++++++++++++++++++++++++------------------- src/qemu/qemu_cgroup.h | 8 +-- src/qemu/qemu_driver.c | 8 +-- src/qemu/qemu_migration.c | 114 ++++++++++++---------------------- src/qemu/qemu_migration.h | 2 +- src/util/vircgroup.c | 142 ++++++++++++++----------------------------- src/util/vircgroup.h | 14 ++--- 10 files changed, 199 insertions(+), 267 deletions(-) -- 2.6.2
This function doesn't do anything useful since 2049ef99425db33f1e66fa8. --- src/qemu/qemu_cgroup.c | 11 ----------- src/qemu/qemu_cgroup.h | 1 - 2 files changed, 12 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 4b5cf36..7579f42 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1100,14 +1100,3 @@ qemuRemoveCgroup(virDomainObjPtr vm) return virCgroupRemove(priv->cgroup); } - -int -qemuAddToCgroup(virDomainObjPtr vm) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - - if (priv->cgroup == NULL) - return 0; /* Not supported, so claim success */ - - return 0; -} diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 021bfe6..a31fa34 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -55,6 +55,5 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); int qemuSetupCgroupForEmulator(virDomainObjPtr vm); int qemuRemoveCgroup(virDomainObjPtr vm); -int qemuAddToCgroup(virDomainObjPtr vm); #endif /* __QEMU_CGROUP_H__ */ -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:43PM +0100, Peter Krempa wrote:
This function doesn't do anything useful since 2049ef99425db33f1e66fa8. --- src/qemu/qemu_cgroup.c | 11 ----------- src/qemu/qemu_cgroup.h | 1 - 2 files changed, 12 deletions(-)
ACK Jan
After removing capability check for fd migration the code that was left behind didn't make quite sense. The old exec migration would be used in case when pipe() failed. Remove the old code and make failure of pipe() a hard error. This additionally removes usage of virCgroupAllowDevicePath outside of qemu_cgroup.c. --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_migration.c | 114 ++++++++++++++++------------------------------ src/qemu/qemu_migration.h | 2 +- 3 files changed, 41 insertions(+), 79 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2bbc724..fa1d9b7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3187,7 +3187,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, goto cleanup; /* Perform the migration */ - if (qemuMigrationToFile(driver, vm, fd, offset, path, + if (qemuMigrationToFile(driver, vm, fd, path, qemuCompressProgramName(compressed), bypassSecurityDriver, asyncJob) < 0) @@ -3690,7 +3690,7 @@ doCoreDump(virQEMUDriverPtr driver, if (!qemuMigrationIsAllowed(driver, vm, false, 0)) goto cleanup; - ret = qemuMigrationToFile(driver, vm, fd, 0, path, + ret = qemuMigrationToFile(driver, vm, fd, path, qemuCompressProgramName(compress), false, QEMU_ASYNC_JOB_DUMP); } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f2c7b61..f3b84c8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5940,7 +5940,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver, /* Helper function called while vm is active. */ int qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, - int fd, off_t offset, const char *path, + int fd, const char *path, const char *compressor, bool bypassSecurityDriver, qemuDomainAsyncJob asyncJob) @@ -5972,54 +5972,28 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, return -1; } - if ((!compressor || pipe(pipeFD) == 0)) { - /* All right! We can use fd migration, which means that qemu - * doesn't have to open() the file, so while we still have to - * grant SELinux access, we can do it on fd and avoid cleanup - * later, as well as skip futzing with cgroup. */ - if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def, - compressor ? pipeFD[1] : fd) < 0) - goto cleanup; - bypassSecurityDriver = true; - } else { - /* Phooey - we have to fall back on exec migration, where qemu - * has to popen() the file by name, and block devices have to be - * given cgroup ACL permission. We might also stumble on - * a race present in some qemu versions where it does a wait() - * that botches pclose. */ - if (virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_DEVICES)) { - int rv = virCgroupAllowDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RW); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rv == 0); - if (rv == 1) { - /* path was not a device, no further need for cgroup */ - } else if (rv < 0) { - goto cleanup; - } - } - if ((!bypassSecurityDriver) && - virSecurityManagerSetSavedStateLabel(driver->securityManager, - vm->def, path) < 0) - goto cleanup; - restoreLabel = true; + if (compressor && pipe(pipeFD) < 0) { + virReportSystemError(errno, "%s", + _("Failed to create pipe for migration")); + return -1; } + /* All right! We can use fd migration, which means that qemu + * doesn't have to open() the file, so while we still have to + * grant SELinux access, we can do it on fd and avoid cleanup + * later, as well as skip futzing with cgroup. */ + if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def, + compressor ? pipeFD[1] : fd) < 0) + goto cleanup; + bypassSecurityDriver = true; + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; if (!compressor) { - const char *args[] = { "cat", NULL }; - - if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) { - rc = qemuMonitorMigrateToFd(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - fd); - } else { - rc = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, offset); - } + rc = qemuMonitorMigrateToFd(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + fd); } else { const char *prog = compressor; const char *args[] = { @@ -6027,33 +6001,28 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, "-c", NULL }; - if (pipeFD[0] != -1) { - cmd = virCommandNewArgs(args); - virCommandSetInputFD(cmd, pipeFD[0]); - virCommandSetOutputFD(cmd, &fd); - virCommandSetErrorBuffer(cmd, &errbuf); - virCommandDoAsyncIO(cmd); - if (virSetCloseExec(pipeFD[1]) < 0) { - virReportSystemError(errno, "%s", - _("Unable to set cloexec flag")); - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup; - } - if (virCommandRunAsync(cmd, NULL) < 0) { - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup; - } - rc = qemuMonitorMigrateToFd(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - pipeFD[1]); - if (VIR_CLOSE(pipeFD[0]) < 0 || - VIR_CLOSE(pipeFD[1]) < 0) - VIR_WARN("failed to close intermediate pipe"); - } else { - rc = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, offset); + + cmd = virCommandNewArgs(args); + virCommandSetInputFD(cmd, pipeFD[0]); + virCommandSetOutputFD(cmd, &fd); + virCommandSetErrorBuffer(cmd, &errbuf); + virCommandDoAsyncIO(cmd); + if (virSetCloseExec(pipeFD[1]) < 0) { + virReportSystemError(errno, "%s", + _("Unable to set cloexec flag")); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; } + if (virCommandRunAsync(cmd, NULL) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + rc = qemuMonitorMigrateToFd(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + pipeFD[1]); + if (VIR_CLOSE(pipeFD[0]) < 0 || + VIR_CLOSE(pipeFD[1]) < 0) + VIR_WARN("failed to close intermediate pipe"); } if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -6105,13 +6074,6 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, vm->def, path) < 0) VIR_WARN("failed to restore save state label on %s", path); - if (virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_DEVICES)) { - int rv = virCgroupDenyDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RWM); - virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", rv == 0); - } - if (orig_err) { virSetError(orig_err); virFreeError(orig_err); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 2445e13..fd94d6e 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -177,7 +177,7 @@ bool qemuMigrationIsAllowed(virQEMUDriverPtr driver, virDomainObjPtr vm, bool remote, unsigned int flags); int qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, - int fd, off_t offset, const char *path, + int fd, const char *path, const char *compressor, bool bypassSecurityDriver, qemuDomainAsyncJob asyncJob) -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:44PM +0100, Peter Krempa wrote:
After removing capability check for fd migration the code that was left behind didn't make quite sense. The old exec migration would be used in case when pipe() failed. Remove the old code and make failure of pipe() a hard error.
This additionally removes usage of virCgroupAllowDevicePath outside of qemu_cgroup.c. --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_migration.c | 114 ++++++++++++++++------------------------------ src/qemu/qemu_migration.h | 2 +- 3 files changed, 41 insertions(+), 79 deletions(-)
@@ -5972,54 +5972,28 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, return -1; }
- if ((!compressor || pipe(pipeFD) == 0)) { - /* All right! We can use fd migration, which means that qemu - * doesn't have to open() the file, so while we still have to - * grant SELinux access, we can do it on fd and avoid cleanup - * later, as well as skip futzing with cgroup. */ - if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def, - compressor ? pipeFD[1] : fd) < 0) - goto cleanup; - bypassSecurityDriver = true; - } else { - /* Phooey - we have to fall back on exec migration, where qemu - * has to popen() the file by name, and block devices have to be - * given cgroup ACL permission. We might also stumble on - * a race present in some qemu versions where it does a wait() - * that botches pclose. */ - if (virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_DEVICES)) { - int rv = virCgroupAllowDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RW); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rv == 0); - if (rv == 1) { - /* path was not a device, no further need for cgroup */ - } else if (rv < 0) { - goto cleanup; - } - } - if ((!bypassSecurityDriver) && - virSecurityManagerSetSavedStateLabel(driver->securityManager, - vm->def, path) < 0) - goto cleanup; - restoreLabel = true;
This is the only place that set restoreLabel to true.
+ if (compressor && pipe(pipeFD) < 0) { + virReportSystemError(errno, "%s", + _("Failed to create pipe for migration")); + return -1; }
+ /* All right! We can use fd migration, which means that qemu + * doesn't have to open() the file, so while we still have to + * grant SELinux access, we can do it on fd and avoid cleanup + * later, as well as skip futzing with cgroup. */ + if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def, + compressor ? pipeFD[1] : fd) < 0) + goto cleanup;
Can you really relabel a pipe after it's been created?
+ bypassSecurityDriver = true; + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup;
if (!compressor) { - const char *args[] = { "cat", NULL }; - - if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
While it seems it's (in theory) possible to get a different monType through qemuProcessReconnect, I think we can safely ignore it.
- rc = qemuMonitorMigrateToFd(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - fd); - } else { - rc = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, offset); - } + rc = qemuMonitorMigrateToFd(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + fd); } else { const char *prog = compressor; const char *args[] = { @@ -6027,33 +6001,28 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, "-c", NULL }; - if (pipeFD[0] != -1) { - cmd = virCommandNewArgs(args); - virCommandSetInputFD(cmd, pipeFD[0]); - virCommandSetOutputFD(cmd, &fd); - virCommandSetErrorBuffer(cmd, &errbuf); - virCommandDoAsyncIO(cmd); - if (virSetCloseExec(pipeFD[1]) < 0) { - virReportSystemError(errno, "%s", - _("Unable to set cloexec flag")); - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup; - } - if (virCommandRunAsync(cmd, NULL) < 0) { - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup; - } - rc = qemuMonitorMigrateToFd(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - pipeFD[1]); - if (VIR_CLOSE(pipeFD[0]) < 0 || - VIR_CLOSE(pipeFD[1]) < 0) - VIR_WARN("failed to close intermediate pipe"); - } else { - rc = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, offset);
This was the last usage of qemuMonitorMigrateToFile.
+ + cmd = virCommandNewArgs(args); + virCommandSetInputFD(cmd, pipeFD[0]); + virCommandSetOutputFD(cmd, &fd); + virCommandSetErrorBuffer(cmd, &errbuf); + virCommandDoAsyncIO(cmd); + if (virSetCloseExec(pipeFD[1]) < 0) { + virReportSystemError(errno, "%s", + _("Unable to set cloexec flag")); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; } + if (virCommandRunAsync(cmd, NULL) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + rc = qemuMonitorMigrateToFd(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + pipeFD[1]); + if (VIR_CLOSE(pipeFD[0]) < 0 || + VIR_CLOSE(pipeFD[1]) < 0) + VIR_WARN("failed to close intermediate pipe"); } if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -6105,13 +6074,6 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, vm->def, path) < 0) VIR_WARN("failed to restore save state label on %s", path);
The hunk of code not displayed here will disappear after you delete the unused bool. ACK with restoreLabel removed. Your call if you remove qemuMonitorMigrateToFile here or in a separate patch. Jan
Similarly to commit 47e5b5ae virCgroupDenyDevice will handle -1 as *. --- src/util/vircgroup.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 6ce208e..ab0cd47 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3056,8 +3056,8 @@ virCgroupAllowDevicePath(virCgroupPtr group, const char *path, int perms) * * @group: The cgroup to deny a device for * @type: The device type (i.e., 'c' or 'b') - * @major: The major number of the device - * @minor: The minor number of the device + * @major: The major number of the device, a negative value means '*' + * @minor: The minor number of the device, a negative value means '*' * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to deny * * Returns: 0 on success @@ -3068,8 +3068,18 @@ virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, { int ret = -1; char *devstr = NULL; + char *majorstr = NULL; + char *minorstr = NULL; + + if ((major < 0 && VIR_STRDUP(majorstr, "*") < 0) || + (major >= 0 && virAsprintf(&majorstr, "%i", major) < 0)) + goto cleanup; - if (virAsprintf(&devstr, "%c %i:%i %s", type, major, minor, + if ((minor < 0 && VIR_STRDUP(minorstr, "*") < 0) || + (minor >= 0 && virAsprintf(&minorstr, "%i", minor) < 0)) + goto cleanup; + + if (virAsprintf(&devstr, "%c %s:%s %s", type, majorstr, minorstr, virCgroupGetDevicePermsString(perms)) < 0) goto cleanup; @@ -3083,6 +3093,8 @@ virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, cleanup: VIR_FREE(devstr); + VIR_FREE(majorstr); + VIR_FREE(minorstr); return ret; } -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:45PM +0100, Peter Krempa wrote:
Similarly to commit 47e5b5ae virCgroupDenyDevice will handle -1 as *. --- src/util/vircgroup.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
ACK Jan
Since commit 47e5b5ae virCgroupAllowDevice allows to pass -1 as either the minor or major device number and it automatically uses '*' in place of that. Reuse the new approach through the code and drop the duplicated functions. --- src/libvirt_private.syms | 2 -- src/lxc/lxc_cgroup.c | 4 +-- src/qemu/qemu_cgroup.c | 8 ++--- src/util/vircgroup.c | 94 ------------------------------------------------ src/util/vircgroup.h | 8 ----- 5 files changed, 6 insertions(+), 110 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4cfaed5..dc692ca 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1189,7 +1189,6 @@ virCgroupAddTask; virCgroupAddTaskController; virCgroupAllowAllDevices; virCgroupAllowDevice; -virCgroupAllowDeviceMajor; virCgroupAllowDevicePath; virCgroupAvailable; virCgroupBindMount; @@ -1198,7 +1197,6 @@ virCgroupControllerTypeFromString; virCgroupControllerTypeToString; virCgroupDenyAllDevices; virCgroupDenyDevice; -virCgroupDenyDeviceMajor; virCgroupDenyDevicePath; virCgroupDetectMountsFromFile; virCgroupFree; diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 3148946..60805af 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -466,8 +466,8 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, } } - if (virCgroupAllowDeviceMajor(cgroup, 'c', LXC_DEV_MAJ_PTY, - VIR_CGROUP_DEVICE_RWM) < 0) + if (virCgroupAllowDevice(cgroup, 'c', LXC_DEV_MAJ_PTY, -1, + VIR_CGROUP_DEVICE_RWM) < 0) goto cleanup; VIR_DEBUG("Device whitelist complete"); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 7579f42..5a4cd55 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -558,8 +558,8 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, goto cleanup; } - rv = virCgroupAllowDeviceMajor(priv->cgroup, 'c', DEVICE_PTY_MAJOR, - VIR_CGROUP_DEVICE_RW); + rv = virCgroupAllowDevice(priv->cgroup, 'c', DEVICE_PTY_MAJOR, -1, + VIR_CGROUP_DEVICE_RW); virDomainAuditCgroupMajor(vm, priv->cgroup, "allow", DEVICE_PTY_MAJOR, "pty", "rw", rv == 0); if (rv < 0) @@ -576,8 +576,8 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, ((vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && cfg->vncAllowHostAudio) || (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL))))) { - rv = virCgroupAllowDeviceMajor(priv->cgroup, 'c', DEVICE_SND_MAJOR, - VIR_CGROUP_DEVICE_RW); + rv = virCgroupAllowDevice(priv->cgroup, 'c', DEVICE_SND_MAJOR, -1, + VIR_CGROUP_DEVICE_RW); virDomainAuditCgroupMajor(vm, priv->cgroup, "allow", DEVICE_SND_MAJOR, "sound", "rw", rv == 0); if (rv < 0) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index ab0cd47..a35bac7 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2981,41 +2981,6 @@ virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor, /** - * virCgroupAllowDeviceMajor: - * - * @group: The cgroup to allow an entire device major type for - * @type: The device type (i.e., 'c' or 'b') - * @major: The major number of the device type - * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to allow - * - * Returns: 0 on success - */ -int -virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major, - int perms) -{ - int ret = -1; - char *devstr = NULL; - - if (virAsprintf(&devstr, "%c %i:* %s", type, major, - virCgroupGetDevicePermsString(perms)) < 0) - goto cleanup; - - if (virCgroupSetValueStr(group, - VIR_CGROUP_CONTROLLER_DEVICES, - "devices.allow", - devstr) < 0) - goto cleanup; - - ret = 0; - - cleanup: - VIR_FREE(devstr); - return ret; -} - - -/** * virCgroupAllowDevicePath: * * @group: The cgroup to allow the device for @@ -3099,41 +3064,6 @@ virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, } -/** - * virCgroupDenyDeviceMajor: - * - * @group: The cgroup to deny an entire device major type for - * @type: The device type (i.e., 'c' or 'b') - * @major: The major number of the device type - * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to deny - * - * Returns: 0 on success - */ -int -virCgroupDenyDeviceMajor(virCgroupPtr group, char type, int major, - int perms) -{ - int ret = -1; - char *devstr = NULL; - - if (virAsprintf(&devstr, "%c %i:* %s", type, major, - virCgroupGetDevicePermsString(perms)) < 0) - goto cleanup; - - if (virCgroupSetValueStr(group, - VIR_CGROUP_CONTROLLER_DEVICES, - "devices.deny", - devstr) < 0) - goto cleanup; - - ret = 0; - - cleanup: - VIR_FREE(devstr); - return ret; -} - - int virCgroupDenyDevicePath(virCgroupPtr group, const char *path, int perms) { @@ -4705,18 +4635,6 @@ virCgroupAllowDevice(virCgroupPtr group ATTRIBUTE_UNUSED, int -virCgroupAllowDeviceMajor(virCgroupPtr group ATTRIBUTE_UNUSED, - char type ATTRIBUTE_UNUSED, - int major ATTRIBUTE_UNUSED, - int perms ATTRIBUTE_UNUSED) -{ - virReportSystemError(ENOSYS, "%s", - _("Control groups not supported on this platform")); - return -1; -} - - -int virCgroupAllowDevicePath(virCgroupPtr group ATTRIBUTE_UNUSED, const char *path ATTRIBUTE_UNUSED, int perms ATTRIBUTE_UNUSED) @@ -4741,18 +4659,6 @@ virCgroupDenyDevice(virCgroupPtr group ATTRIBUTE_UNUSED, int -virCgroupDenyDeviceMajor(virCgroupPtr group ATTRIBUTE_UNUSED, - char type ATTRIBUTE_UNUSED, - int major ATTRIBUTE_UNUSED, - int perms ATTRIBUTE_UNUSED) -{ - virReportSystemError(ENOSYS, "%s", - _("Control groups not supported on this platform")); - return -1; -} - - -int virCgroupDenyDevicePath(virCgroupPtr group ATTRIBUTE_UNUSED, const char *path ATTRIBUTE_UNUSED, int perms ATTRIBUTE_UNUSED) diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index aeb641c..0f687a5 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -220,10 +220,6 @@ int virCgroupAllowDevice(virCgroupPtr group, int major, int minor, int perms); -int virCgroupAllowDeviceMajor(virCgroupPtr group, - char type, - int major, - int perms); int virCgroupAllowDevicePath(virCgroupPtr group, const char *path, int perms); @@ -233,10 +229,6 @@ int virCgroupDenyDevice(virCgroupPtr group, int major, int minor, int perms); -int virCgroupDenyDeviceMajor(virCgroupPtr group, - char type, - int major, - int perms); int virCgroupDenyDevicePath(virCgroupPtr group, const char *path, int perms); -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:46PM +0100, Peter Krempa wrote:
Since commit 47e5b5ae virCgroupAllowDevice allows to pass -1 as either the minor or major device number and it automatically uses '*' in place of that. Reuse the new approach through the code and drop the duplicated functions. --- src/libvirt_private.syms | 2 -- src/lxc/lxc_cgroup.c | 4 +-- src/qemu/qemu_cgroup.c | 8 ++--- src/util/vircgroup.c | 94 ------------------------------------------------ src/util/vircgroup.h | 8 ----- 5 files changed, 6 insertions(+), 110 deletions(-)
ACK Jan
When adding disk images to ACL we may call those functions on NFS shares. In that case we might get an EACCES, which isn't really relevant since NFS would not hold a block device. This patch adds a flag that allows to stop reporting an error on EACCES to avoid spaming logs. Currently there's no functional change. --- src/lxc/lxc_cgroup.c | 12 ++++++------ src/lxc/lxc_driver.c | 9 ++++++--- src/qemu/qemu_cgroup.c | 20 ++++++++++---------- src/util/vircgroup.c | 38 +++++++++++++++++++++++++++++++++----- src/util/vircgroup.h | 6 ++++-- 5 files changed, 59 insertions(+), 26 deletions(-) diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 60805af..4afe7e2 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -331,7 +331,7 @@ virLXCSetupHostUSBDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, VIR_DEBUG("Process path '%s' for USB device", path); if (virCgroupAllowDevicePath(cgroup, path, - VIR_CGROUP_DEVICE_RWM) < 0) + VIR_CGROUP_DEVICE_RWM, false) < 0) return -1; return 0; @@ -347,7 +347,7 @@ virLXCTeardownHostUSBDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, VIR_DEBUG("Process path '%s' for USB device", path); if (virCgroupDenyDevicePath(cgroup, path, - VIR_CGROUP_DEVICE_RWM) < 0) + VIR_CGROUP_DEVICE_RWM, false) < 0) return -1; return 0; @@ -401,7 +401,7 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, (def->disks[i]->src->readonly ? VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW) | - VIR_CGROUP_DEVICE_MKNOD) < 0) + VIR_CGROUP_DEVICE_MKNOD, false) < 0) goto cleanup; } @@ -414,7 +414,7 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, def->fss[i]->src, def->fss[i]->readonly ? VIR_CGROUP_DEVICE_READ : - VIR_CGROUP_DEVICE_RW) < 0) + VIR_CGROUP_DEVICE_RW, false) < 0) goto cleanup; } @@ -448,14 +448,14 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, if (virCgroupAllowDevicePath(cgroup, hostdev->source.caps.u.storage.block, VIR_CGROUP_DEVICE_RW | - VIR_CGROUP_DEVICE_MKNOD) < 0) + VIR_CGROUP_DEVICE_MKNOD, false) < 0) goto cleanup; break; case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_MISC: if (virCgroupAllowDevicePath(cgroup, hostdev->source.caps.u.misc.chardev, VIR_CGROUP_DEVICE_RW | - VIR_CGROUP_DEVICE_MKNOD) < 0) + VIR_CGROUP_DEVICE_MKNOD, false) < 0) goto cleanup; break; default: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 41639ac..3c6c839 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4646,7 +4646,8 @@ lxcDomainDetachDeviceDiskLive(virDomainObjPtr vm, } virDomainAuditDisk(vm, def->src, NULL, "detach", true); - if (virCgroupDenyDevicePath(priv->cgroup, src, VIR_CGROUP_DEVICE_RWM) != 0) + if (virCgroupDenyDevicePath(priv->cgroup, src, + VIR_CGROUP_DEVICE_RWM, false) != 0) VIR_WARN("cannot deny device %s for domain %s", src, vm->def->name); @@ -4822,7 +4823,8 @@ lxcDomainDetachDeviceHostdevStorageLive(virDomainObjPtr vm, } virDomainAuditHostdev(vm, def, "detach", true); - if (virCgroupDenyDevicePath(priv->cgroup, def->source.caps.u.storage.block, VIR_CGROUP_DEVICE_RWM) != 0) + if (virCgroupDenyDevicePath(priv->cgroup, def->source.caps.u.storage.block, + VIR_CGROUP_DEVICE_RWM, false) != 0) VIR_WARN("cannot deny device %s for domain %s", def->source.caps.u.storage.block, vm->def->name); @@ -4871,7 +4873,8 @@ lxcDomainDetachDeviceHostdevMiscLive(virDomainObjPtr vm, } virDomainAuditHostdev(vm, def, "detach", true); - if (virCgroupDenyDevicePath(priv->cgroup, def->source.caps.u.misc.chardev, VIR_CGROUP_DEVICE_RWM) != 0) + if (virCgroupDenyDevicePath(priv->cgroup, def->source.caps.u.misc.chardev, + VIR_CGROUP_DEVICE_RWM, false) != 0) VIR_WARN("cannot deny device %s for domain %s", def->source.caps.u.misc.chardev, vm->def->name); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 5a4cd55..2c0ac30 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -77,7 +77,7 @@ qemuSetImageCgroupInternal(virDomainObjPtr vm, VIR_DEBUG("Deny path %s", src->path); - ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms); + ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, false); } else { if (!src->readonly && !forceReadonly) perms |= VIR_CGROUP_DEVICE_WRITE; @@ -85,7 +85,7 @@ qemuSetImageCgroupInternal(virDomainObjPtr vm, VIR_DEBUG("Allow path %s, perms: %s", src->path, virCgroupGetDevicePermsString(perms)); - ret = virCgroupAllowDevicePath(priv->cgroup, src->path, perms); + ret = virCgroupAllowDevicePath(priv->cgroup, src->path, perms, false); } virDomainAuditCgroupPath(vm, priv->cgroup, @@ -162,7 +162,7 @@ qemuSetupChrSourceCgroup(virDomainObjPtr vm, VIR_DEBUG("Process path '%s' for device", source->data.file.path); ret = virCgroupAllowDevicePath(priv->cgroup, source->data.file.path, - VIR_CGROUP_DEVICE_RW); + VIR_CGROUP_DEVICE_RW, false); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", source->data.file.path, "rw", ret == 0); @@ -209,7 +209,7 @@ qemuSetupInputCgroup(virDomainObjPtr vm, case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: VIR_DEBUG("Process path '%s' for input device", dev->source.evdev); ret = virCgroupAllowDevicePath(priv->cgroup, dev->source.evdev, - VIR_CGROUP_DEVICE_RW); + VIR_CGROUP_DEVICE_RW, false); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", dev->source.evdev, "rw", ret == 0); break; } @@ -229,7 +229,7 @@ qemuSetupHostUSBDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, VIR_DEBUG("Process path '%s' for USB device", path); ret = virCgroupAllowDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RW); + VIR_CGROUP_DEVICE_RW, false); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", ret == 0); return ret; @@ -249,7 +249,7 @@ qemuSetupHostSCSIDeviceCgroup(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, ret = virCgroupAllowDevicePath(priv->cgroup, path, virSCSIDeviceGetReadonly(dev) ? VIR_CGROUP_DEVICE_READ : - VIR_CGROUP_DEVICE_RW); + VIR_CGROUP_DEVICE_RW, false); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, virSCSIDeviceGetReadonly(dev) ? "r" : "rw", ret == 0); @@ -298,7 +298,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, VIR_DEBUG("Cgroup allow %s for PCI device assignment", path); rv = virCgroupAllowDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RW); + VIR_CGROUP_DEVICE_RW, false); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rv == 0); if (rv < 0) @@ -407,7 +407,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, VIR_DEBUG("Cgroup deny %s for PCI device assignment", path); rv = virCgroupDenyDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RWM); + VIR_CGROUP_DEVICE_RWM, false); virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", rv == 0); if (rv < 0) @@ -591,7 +591,7 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, } rv = virCgroupAllowDevicePath(priv->cgroup, deviceACL[i], - VIR_CGROUP_DEVICE_RW); + VIR_CGROUP_DEVICE_RW, false); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", deviceACL[i], "rw", rv == 0); if (rv < 0 && !virLastErrorIsSystemErrno(ENOENT)) @@ -622,7 +622,7 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, VIR_DEBUG("Setting Cgroup ACL for RNG device"); rv = virCgroupAllowDevicePath(priv->cgroup, vm->def->rngs[i]->source.file, - VIR_CGROUP_DEVICE_RW); + VIR_CGROUP_DEVICE_RW, false); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", vm->def->rngs[i]->source.file, "rw", rv == 0); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index a35bac7..d032c60 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2986,19 +2986,26 @@ virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor, * @group: The cgroup to allow the device for * @path: the device to allow * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to allow + * @ignoreEacces: Ignore lack of permission (mostly for NFS mounts) * * Queries the type of device and its major/minor number, and * adds that to the cgroup ACL * - * Returns: 0 on success, 1 if path exists but is not a device, or - * -1 on error + * Returns: 0 on success, 1 if path exists but is not a device or is not + * accesible, or * -1 on error */ int -virCgroupAllowDevicePath(virCgroupPtr group, const char *path, int perms) +virCgroupAllowDevicePath(virCgroupPtr group, + const char *path, + int perms, + bool ignoreEacces) { struct stat sb; if (stat(path, &sb) < 0) { + if (errno == EACCES && ignoreEacces) + return 1; + virReportSystemError(errno, _("Path '%s' is not accessible"), path); @@ -3064,12 +3071,32 @@ virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, } +/** + * virCgroupDenyDevicePath: + * + * @group: The cgroup to deny the device for + * @path: the device to deny + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to allow + * @ignoreEacces: Ignore lack of permission (mostly for NFS mounts) + * + * Queries the type of device and its major/minor number, and + * removes it from the cgroup ACL + * + * Returns: 0 on success, 1 if path exists but is not a device or is not + * accessible, or -1 on error. + */ int -virCgroupDenyDevicePath(virCgroupPtr group, const char *path, int perms) +virCgroupDenyDevicePath(virCgroupPtr group, + const char *path, + int perms, + bool ignoreEacces) { struct stat sb; if (stat(path, &sb) < 0) { + if (errno == EACCES && ignoreEacces) + return 1; + virReportSystemError(errno, _("Path '%s' is not accessible"), path); @@ -4637,7 +4664,8 @@ virCgroupAllowDevice(virCgroupPtr group ATTRIBUTE_UNUSED, int virCgroupAllowDevicePath(virCgroupPtr group ATTRIBUTE_UNUSED, const char *path ATTRIBUTE_UNUSED, - int perms ATTRIBUTE_UNUSED) + int perms ATTRIBUTE_UNUSED, + bool ignoreEaccess ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Control groups not supported on this platform")); diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 0f687a5..7914cbb 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -222,7 +222,8 @@ int virCgroupAllowDevice(virCgroupPtr group, int perms); int virCgroupAllowDevicePath(virCgroupPtr group, const char *path, - int perms); + int perms, + bool ignoreEacces); int virCgroupDenyDevice(virCgroupPtr group, char type, @@ -231,7 +232,8 @@ int virCgroupDenyDevice(virCgroupPtr group, int perms); int virCgroupDenyDevicePath(virCgroupPtr group, const char *path, - int perms); + int perms, + bool ignoreEacces); int virCgroupGetPercpuStats(virCgroupPtr group, -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:47PM +0100, Peter Krempa wrote:
When adding disk images to ACL we may call those functions on NFS shares. In that case we might get an EACCES, which isn't really relevant since NFS would not hold a block device. This patch adds a flag that allows to stop reporting an error on EACCES to avoid spaming logs.
spamming
Currently there's no functional change. --- src/lxc/lxc_cgroup.c | 12 ++++++------ src/lxc/lxc_driver.c | 9 ++++++--- src/qemu/qemu_cgroup.c | 20 ++++++++++---------- src/util/vircgroup.c | 38 +++++++++++++++++++++++++++++++++----- src/util/vircgroup.h | 6 ++++-- 5 files changed, 59 insertions(+), 26 deletions(-)
ACK Jan
Rather than reporting it and then reseting the error, don't report it in the first place. --- src/qemu/qemu_cgroup.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2c0ac30..b37af6d 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -77,7 +77,7 @@ qemuSetImageCgroupInternal(virDomainObjPtr vm, VIR_DEBUG("Deny path %s", src->path); - ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, false); + ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); } else { if (!src->readonly && !forceReadonly) perms |= VIR_CGROUP_DEVICE_WRITE; @@ -85,7 +85,7 @@ qemuSetImageCgroupInternal(virDomainObjPtr vm, VIR_DEBUG("Allow path %s, perms: %s", src->path, virCgroupGetDevicePermsString(perms)); - ret = virCgroupAllowDevicePath(priv->cgroup, src->path, perms, false); + ret = virCgroupAllowDevicePath(priv->cgroup, src->path, perms, true); } virDomainAuditCgroupPath(vm, priv->cgroup, @@ -94,14 +94,6 @@ qemuSetImageCgroupInternal(virDomainObjPtr vm, virCgroupGetDevicePermsString(perms), ret == 0); - /* Get this for root squash NFS */ - if (ret < 0 && - virLastErrorIsSystemErrno(EACCES)) { - VIR_DEBUG("Ignoring EACCES for %s", src->path); - virResetLastError(); - ret = 0; - } - return ret; } -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:48PM +0100, Peter Krempa wrote:
Rather than reporting it and then reseting the error, don't report it in the first place. --- src/qemu/qemu_cgroup.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
ACK Jan
For other objects we use the two functions rather than one with a bool. Convert qemuSetImageCgroup to the same approach. --- src/qemu/qemu_cgroup.c | 17 ++++++++++++----- src/qemu/qemu_cgroup.h | 7 ++++--- src/qemu/qemu_driver.c | 4 ++-- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index b37af6d..978dfa2 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -99,11 +99,18 @@ qemuSetImageCgroupInternal(virDomainObjPtr vm, int -qemuSetImageCgroup(virDomainObjPtr vm, - virStorageSourcePtr src, - bool deny) +qemuSetupImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src) { - return qemuSetImageCgroupInternal(vm, src, deny, false); + return qemuSetImageCgroupInternal(vm, src, false, false); +} + + +int +qemuTeardownImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src) +{ + return qemuSetImageCgroupInternal(vm, src, true, false); } @@ -133,7 +140,7 @@ qemuTeardownDiskCgroup(virDomainObjPtr vm, virStorageSourcePtr next; for (next = disk->src; next; next = next->backingStore) { - if (qemuSetImageCgroup(vm, next, true) < 0) + if (qemuSetImageCgroupInternal(vm, next, true, false) < 0) return -1; } diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index a31fa34..a8b8e1b 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -29,9 +29,10 @@ # include "domain_conf.h" # include "qemu_conf.h" -int qemuSetImageCgroup(virDomainObjPtr vm, - virStorageSourcePtr src, - bool deny); +int qemuSetupImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src); +int qemuTeardownImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src); int qemuSetupDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk); int qemuTeardownDiskCgroup(virDomainObjPtr vm, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fa1d9b7..be42d77 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13374,7 +13374,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, vm->def, elem) < 0) VIR_WARN("Unable to restore security label on %s", elem->path); - if (qemuSetImageCgroup(vm, elem, true) < 0) + if (qemuTeardownImageCgroup(vm, elem) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", elem->path); if (virDomainLockImageDetach(driver->lockManager, vm, elem) < 0) @@ -13384,7 +13384,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, vm, elem) < 0) goto cleanup; - if (qemuSetImageCgroup(vm, elem, false) < 0) + if (qemuSetupImageCgroup(vm, elem) < 0) goto cleanup; if (virSecurityManagerSetImageLabel(driver->securityManager, -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:49PM +0100, Peter Krempa wrote:
For other objects we use the two functions rather than one with a bool. Convert qemuSetImageCgroup to the same approach. --- src/qemu/qemu_cgroup.c | 17 ++++++++++++----- src/qemu/qemu_cgroup.h | 7 ++++--- src/qemu/qemu_driver.c | 4 ++-- 3 files changed, 18 insertions(+), 10 deletions(-)
ACK Jan
Separate the Teardown and Setup code paths into separate helpers. --- src/qemu/qemu_cgroup.c | 60 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 978dfa2..6a34bab 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -53,10 +53,9 @@ static const char *const defaultDeviceACL[] = { #define DEVICE_SND_MAJOR 116 static int -qemuSetImageCgroupInternal(virDomainObjPtr vm, - virStorageSourcePtr src, - bool deny, - bool forceReadonly) +qemuSetupImageCgroupInternal(virDomainObjPtr vm, + virStorageSourcePtr src, + bool forceReadonly) { qemuDomainObjPrivatePtr priv = vm->privateData; int perms = VIR_CGROUP_DEVICE_READ; @@ -72,25 +71,15 @@ qemuSetImageCgroupInternal(virDomainObjPtr vm, return 0; } - if (deny) { - perms |= VIR_CGROUP_DEVICE_WRITE | VIR_CGROUP_DEVICE_MKNOD; + if (!src->readonly && !forceReadonly) + perms |= VIR_CGROUP_DEVICE_WRITE; - VIR_DEBUG("Deny path %s", src->path); + VIR_DEBUG("Allow path %s, perms: %s", + src->path, virCgroupGetDevicePermsString(perms)); - ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); - } else { - if (!src->readonly && !forceReadonly) - perms |= VIR_CGROUP_DEVICE_WRITE; + ret = virCgroupAllowDevicePath(priv->cgroup, src->path, perms, true); - VIR_DEBUG("Allow path %s, perms: %s", - src->path, virCgroupGetDevicePermsString(perms)); - - ret = virCgroupAllowDevicePath(priv->cgroup, src->path, perms, true); - } - - virDomainAuditCgroupPath(vm, priv->cgroup, - deny ? "deny" : "allow", - src->path, + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", src->path, virCgroupGetDevicePermsString(perms), ret == 0); @@ -102,7 +91,7 @@ int qemuSetupImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) { - return qemuSetImageCgroupInternal(vm, src, false, false); + return qemuSetupImageCgroupInternal(vm, src, false); } @@ -110,7 +99,30 @@ int qemuTeardownImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) { - return qemuSetImageCgroupInternal(vm, src, true, false); + qemuDomainObjPrivatePtr priv = vm->privateData; + int perms = VIR_CGROUP_DEVICE_READ | + VIR_CGROUP_DEVICE_WRITE | + VIR_CGROUP_DEVICE_MKNOD; + int ret; + + if (!virCgroupHasController(priv->cgroup, + VIR_CGROUP_CONTROLLER_DEVICES)) + return 0; + + if (!src->path || !virStorageSourceIsLocalStorage(src)) { + VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", + NULLSTR(src->path), virStorageTypeToString(src->type)); + return 0; + } + + VIR_DEBUG("Deny path %s", src->path); + + ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); + + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", src->path, + virCgroupGetDevicePermsString(perms), ret == 0); + + return ret; } @@ -122,7 +134,7 @@ qemuSetupDiskCgroup(virDomainObjPtr vm, bool forceReadonly = false; for (next = disk->src; next; next = next->backingStore) { - if (qemuSetImageCgroupInternal(vm, next, false, forceReadonly) < 0) + if (qemuSetupImageCgroupInternal(vm, next, forceReadonly) < 0) return -1; /* setup only the top level image for read-write */ @@ -140,7 +152,7 @@ qemuTeardownDiskCgroup(virDomainObjPtr vm, virStorageSourcePtr next; for (next = disk->src; next; next = next->backingStore) { - if (qemuSetImageCgroupInternal(vm, next, true, false) < 0) + if (qemuTeardownImageCgroup(vm, next) < 0) return -1; } -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:50PM +0100, Peter Krempa wrote:
Separate the Teardown and Setup code paths into separate helpers. --- src/qemu/qemu_cgroup.c | 60 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 24 deletions(-)
ack jan
They will later be reused for setting cgroup for other image backed devices. --- src/qemu/qemu_cgroup.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 6a34bab..9f24545 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -52,34 +52,28 @@ static const char *const defaultDeviceACL[] = { #define DEVICE_PTY_MAJOR 136 #define DEVICE_SND_MAJOR 116 + static int -qemuSetupImageCgroupInternal(virDomainObjPtr vm, - virStorageSourcePtr src, - bool forceReadonly) +qemuSetupImagePathCgroup(virDomainObjPtr vm, + const char *path, + bool readonly) { qemuDomainObjPrivatePtr priv = vm->privateData; int perms = VIR_CGROUP_DEVICE_READ; int ret; - if (!virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_DEVICES)) - return 0; - - if (!src->path || !virStorageSourceIsLocalStorage(src)) { - VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", - NULLSTR(src->path), virStorageTypeToString(src->type)); + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - } - if (!src->readonly && !forceReadonly) + if (readonly) perms |= VIR_CGROUP_DEVICE_WRITE; VIR_DEBUG("Allow path %s, perms: %s", - src->path, virCgroupGetDevicePermsString(perms)); + path, virCgroupGetDevicePermsString(perms)); - ret = virCgroupAllowDevicePath(priv->cgroup, src->path, perms, true); + ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", src->path, + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, virCgroupGetDevicePermsString(perms), ret == 0); @@ -87,6 +81,21 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm, } +static int +qemuSetupImageCgroupInternal(virDomainObjPtr vm, + virStorageSourcePtr src, + bool forceReadonly) +{ + if (!src->path || !virStorageSourceIsLocalStorage(src)) { + VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", + NULLSTR(src->path), virStorageTypeToString(src->type)); + return 0; + } + + return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly); +} + + int qemuSetupImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:51PM +0100, Peter Krempa wrote:
They will later be reused for setting cgroup for other image backed devices. --- src/qemu/qemu_cgroup.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)
ack jan
oVirt wants to use OVMF images on top of lvm for their 'logical' storage thus we should set up device ACLs for them so it will actually work. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1305922 --- src/qemu/qemu_cgroup.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 9f24545..90f8397 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -548,6 +548,23 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm) static int +qemuSetupFirmwareCgroup(virDomainObjPtr vm) +{ + bool readonly = vm->def->os.loader->readonly == VIR_TRISTATE_BOOL_YES; + + if (vm->def->os.loader->path && + qemuSetupImagePathCgroup(vm, vm->def->os.loader->path, readonly) < 0) + return -1; + + if (vm->def->os.loader->nvram && + qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0) + return -1; + + return 0; +} + + +static int qemuSetupDevicesCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm) { @@ -573,6 +590,9 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, goto cleanup; } + if (qemuSetupFirmwareCgroup(vm) < 0) + goto cleanup; + for (i = 0; i < vm->def->ndisks; i++) { if (qemuSetupDiskCgroup(vm, vm->def->disks[i]) < 0) goto cleanup; -- 2.6.2
On Tue, Feb 16, 2016 at 04:29:52PM +0100, Peter Krempa wrote:
oVirt wants to use OVMF images on top of lvm for their 'logical' storage thus we should set up device ACLs for them so it will actually work.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1305922 --- src/qemu/qemu_cgroup.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
@@ -573,6 +590,9 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, goto cleanup; }
+ if (qemuSetupFirmwareCgroup(vm) < 0) + goto cleanup; +
if (vm->def->os.loader && ..
for (i = 0; i < vm->def->ndisks; i++) { if (qemuSetupDiskCgroup(vm, vm->def->disks[i]) < 0) goto cleanup;
ACK with the crash fixed. Jan
On Wed, Feb 17, 2016 at 10:11:24 +0100, Ján Tomko wrote:
On Tue, Feb 16, 2016 at 04:29:52PM +0100, Peter Krempa wrote:
oVirt wants to use OVMF images on top of lvm for their 'logical' storage thus we should set up device ACLs for them so it will actually work.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1305922 --- src/qemu/qemu_cgroup.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
@@ -573,6 +590,9 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, goto cleanup; }
+ if (qemuSetupFirmwareCgroup(vm) < 0) + goto cleanup; +
if (vm->def->os.loader && ..
I've added this check to the function itself.
for (i = 0; i < vm->def->ndisks; i++) { if (qemuSetupDiskCgroup(vm, vm->def->disks[i]) < 0) goto cleanup;
ACK with the crash fixed.
I've pushed this series with the requested changes. I'll follow up with the cleanup of the unused monitor command from 2/10. Thanks. Peter
participants (2)
-
Ján Tomko -
Peter Krempa