[libvirt] [PATCH v4 00/15] Implement alternative metadata locking
v4 of: https://www.redhat.com/archives/libvir-list/2018-October/msg00861.html diff to v3: - Introduced a config knob to enable/disable metadata locking (except not really). We want to have a knob that enables/disables remembering of original owner. This knob in turn enables metadata locking. The reason is that metadata locking on its own doesn't make any sense. Anyway, the qemu.conf change is not done (it'll be done in upcoming patch set that implements original owner remembering), so if you want to see these patches in action you'll need to apply the following patch: diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 32da9a7351..0080b0d021 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -347,6 +347,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST))) goto error; + cfg->rememberOwner = true; + if (privileged && qemuDomainNamespaceAvailable(QEMU_DOMAIN_NS_MOUNT) && virBitmapSetBit(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT) < 0) - I've fixed small issues raised in review of v3. Note that patches 01 and 02 are ACKed already but I'm sending them for completeness (probably doesn't make much sense to merge them while this is still under review, does it?). Michal Prívozník (15): virprocess: Introduce virProcessRunInFork virprocess: Make virProcessRunInMountNamespace use virProcessRunInFork qemu_tpm: Pass virDomainObjPtr instead of virDomainDefPtr qemu_domain: Track if domain remembers original owner virSecurityManagerTransactionCommit: Do metadata locking iff enabled in config security_manager: Rework metadata locking Revert "security_manager: Load lock plugin on init" Revert "qemu_conf: Introduce metadata_lock_manager" Revert "lock_manager: Allow disabling configFile for virLockManagerPluginNew" Revert "lock_driver: Introduce VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK" Revert "lock_driver: Introduce VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA" Revert "_virLockManagerLockDaemonPrivate: Move @hasRWDisks into dom union" Revert "lock_driver: Introduce new VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON" Revert "lock_driver_lockd: Introduce VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA flag" Revert "virlockspace: Allow caller to specify start and length offset in virLockSpaceAcquireResource" cfg.mk | 4 +- src/libvirt_private.syms | 1 + src/locking/lock_daemon_dispatch.c | 11 +- src/locking/lock_driver.h | 12 - src/locking/lock_driver_lockd.c | 421 ++++++++++------------------- src/locking/lock_driver_lockd.h | 1 - src/locking/lock_driver_sanlock.c | 44 +-- src/locking/lock_manager.c | 10 +- src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_conf.c | 1 - src/qemu/qemu_conf.h | 2 +- src/qemu/qemu_domain.c | 7 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 3 - src/qemu/qemu_extdevice.c | 16 +- src/qemu/qemu_extdevice.h | 4 +- src/qemu/qemu_process.c | 9 +- src/qemu/qemu_security.c | 87 ++++-- src/qemu/qemu_security.h | 4 +- src/qemu/qemu_tpm.c | 24 +- src/qemu/qemu_tpm.h | 4 +- src/security/security_dac.c | 54 ++-- src/security/security_driver.h | 3 +- src/security/security_manager.c | 259 +++++++++--------- src/security/security_manager.h | 22 +- src/security/security_selinux.c | 53 ++-- src/security/security_stack.c | 5 +- src/util/virlockspace.c | 15 +- src/util/virlockspace.h | 4 - src/util/virprocess.c | 82 ++++-- src/util/virprocess.h | 16 ++ tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- tests/securityselinuxtest.c | 2 +- tests/testutilsqemu.c | 2 +- tests/virlockspacetest.c | 29 +- 37 files changed, 573 insertions(+), 650 deletions(-) -- 2.18.1
This new helper can be used to spawn a child process and run passed callback from it. This will come handy esp. if the callback is not thread safe. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virprocess.c | 86 ++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 16 ++++++++ 3 files changed, 103 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2343a757c1..7906d90f24 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2626,6 +2626,7 @@ virProcessKill; virProcessKillPainfully; virProcessKillPainfullyDelay; virProcessNamespaceAvailable; +virProcessRunInFork; virProcessRunInMountNamespace; virProcessSchedPolicyTypeFromString; virProcessSchedPolicyTypeToString; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 3883c708fc..17e7cfa4ee 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1165,6 +1165,92 @@ virProcessRunInMountNamespace(pid_t pid, } +static int +virProcessRunInForkHelper(int errfd, + pid_t ppid, + virProcessForkCallback cb, + void *opaque) +{ + if (cb(ppid, opaque) < 0) { + virErrorPtr err = virGetLastError(); + if (err) { + size_t len = strlen(err->message) + 1; + ignore_value(safewrite(errfd, err->message, len)); + } + + return -1; + } + + return 0; +} + + +/** + * virProcessRunInFork: + * @cb: callback to run + * @opaque: opaque data to @cb + * + * Do the fork and run @cb in the child. This can be used when + * @cb does something thread unsafe, for instance. All signals + * will be reset to have their platform default handlers and + * unmasked. @cb must only use async signal safe functions. In + * particular no mutexes should be used in @cb, unless steps were + * taken before forking to guarantee a predictable state. @cb + * must not exec any external binaries, instead + * virCommand/virExec should be used for that purpose. + * + * On return, the returned value is either -1 with error message + * reported if something went bad in the parent, if child has + * died from a signal or if the child returned EXIT_CANCELED. + * Otherwise the returned value is the exit status of the child. + */ +int +virProcessRunInFork(virProcessForkCallback cb, + void *opaque) +{ + int ret = -1; + pid_t child = -1; + pid_t parent = getpid(); + int errfd[2] = { -1, -1 }; + + if (pipe2(errfd, O_CLOEXEC) < 0) { + virReportSystemError(errno, "%s", + _("Cannot create pipe for child")); + return -1; + } + + if ((child = virFork()) < 0) + goto cleanup; + + if (child == 0) { + VIR_FORCE_CLOSE(errfd[0]); + ret = virProcessRunInForkHelper(errfd[1], parent, cb, opaque); + VIR_FORCE_CLOSE(errfd[1]); + _exit(ret < 0 ? EXIT_CANCELED : ret); + } else { + int status; + VIR_AUTOFREE(char *) buf = NULL; + + VIR_FORCE_CLOSE(errfd[1]); + ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf)); + ret = virProcessWait(child, &status, false); + if (ret == 0) { + ret = status == EXIT_CANCELED ? -1 : status; + if (ret) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("child reported (status=%d): %s"), + status, NULLSTR(buf)); + } + } + } + + cleanup: + VIR_FORCE_CLOSE(errfd[0]); + VIR_FORCE_CLOSE(errfd[1]); + return ret; +} + + #if defined(HAVE_SYS_MOUNT_H) && defined(HAVE_UNSHARE) int virProcessSetupPrivateMountNS(void) diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 5faa0892fe..b1166902f0 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -93,6 +93,22 @@ int virProcessRunInMountNamespace(pid_t pid, virProcessNamespaceCallback cb, void *opaque); +/** + * virProcessForkCallback: + * @ppid: parent's pid + * @opaque: opaque data + * + * Callback to run in fork()-ed process. + * + * Returns: 0 on success, + * -1 on error (treated as EXIT_CANCELED) + */ +typedef int (*virProcessForkCallback)(pid_t ppid, + void *opaque); + +int virProcessRunInFork(virProcessForkCallback cb, + void *opaque); + int virProcessSetupPrivateMountNS(void); int virProcessSetScheduler(pid_t pid, -- 2.18.1
Both virProcessRunInMountNamespace() and virProcessRunInFork() look very similar. De-duplicate the code and make virProcessRunInMountNamespace() call virProcessRunInFork(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/util/virprocess.c | 64 +++++++++---------------------------------- 1 file changed, 13 insertions(+), 51 deletions(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 17e7cfa4ee..87f32464db 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1073,16 +1073,22 @@ int virProcessGetStartTime(pid_t pid, #endif -static int virProcessNamespaceHelper(int errfd, - pid_t pid, - virProcessNamespaceCallback cb, +typedef struct _virProcessNamespaceHelperData virProcessNamespaceHelperData; +struct _virProcessNamespaceHelperData { + pid_t pid; + virProcessNamespaceCallback cb; + void *opaque; +}; + +static int virProcessNamespaceHelper(pid_t pid ATTRIBUTE_UNUSED, void *opaque) { + virProcessNamespaceHelperData *data = opaque; int fd = -1; int ret = -1; VIR_AUTOFREE(char *) path = NULL; - if (virAsprintf(&path, "/proc/%lld/ns/mnt", (long long) pid) < 0) + if (virAsprintf(&path, "/proc/%lld/ns/mnt", (long long) data->pid) < 0) goto cleanup; if ((fd = open(path, O_RDONLY)) < 0) { @@ -1097,16 +1103,9 @@ static int virProcessNamespaceHelper(int errfd, goto cleanup; } - ret = cb(pid, opaque); + ret = data->cb(data->pid, data->opaque); cleanup: - if (ret < 0) { - virErrorPtr err = virGetLastError(); - if (err) { - size_t len = strlen(err->message) + 1; - ignore_value(safewrite(errfd, err->message, len)); - } - } VIR_FORCE_CLOSE(fd); return ret; } @@ -1122,46 +1121,9 @@ virProcessRunInMountNamespace(pid_t pid, virProcessNamespaceCallback cb, void *opaque) { - int ret = -1; - pid_t child = -1; - int errfd[2] = { -1, -1 }; + virProcessNamespaceHelperData data = {.pid = pid, .cb = cb, .opaque = opaque}; - if (pipe2(errfd, O_CLOEXEC) < 0) { - virReportSystemError(errno, "%s", - _("Cannot create pipe for child")); - return -1; - } - - if ((child = virFork()) < 0) - goto cleanup; - - if (child == 0) { - VIR_FORCE_CLOSE(errfd[0]); - ret = virProcessNamespaceHelper(errfd[1], pid, - cb, opaque); - VIR_FORCE_CLOSE(errfd[1]); - _exit(ret < 0 ? EXIT_CANCELED : ret); - } else { - int status; - VIR_AUTOFREE(char *) buf = NULL; - - VIR_FORCE_CLOSE(errfd[1]); - ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf)); - ret = virProcessWait(child, &status, false); - if (!ret) { - ret = status == EXIT_CANCELED ? -1 : status; - if (ret) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("child reported: %s"), - NULLSTR(buf)); - } - } - } - - cleanup: - VIR_FORCE_CLOSE(errfd[0]); - VIR_FORCE_CLOSE(errfd[1]); - return ret; + return virProcessRunInFork(virProcessNamespaceHelper, &data); } -- 2.18.1
The TPM code currently accepts pointer to a domain definition. This is okay for now, but in near future the security driver APIs it calls will require domain object. Therefore, change the TPM code to accept the domain object pointer. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_extdevice.c | 16 ++++++++-------- src/qemu/qemu_extdevice.h | 4 ++-- src/qemu/qemu_process.c | 6 +++--- src/qemu/qemu_security.c | 14 +++++++------- src/qemu/qemu_security.h | 4 ++-- src/qemu/qemu_tpm.c | 24 ++++++++++++------------ src/qemu/qemu_tpm.h | 4 ++-- 7 files changed, 36 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index d982922470..27cf118c14 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -129,16 +129,16 @@ qemuExtDevicesCleanupHost(virQEMUDriverPtr driver, int qemuExtDevicesStart(virQEMUDriverPtr driver, - virDomainDefPtr def, + virDomainObjPtr vm, qemuDomainLogContextPtr logCtxt) { int ret = 0; - if (qemuExtDevicesInitPaths(driver, def) < 0) + if (qemuExtDevicesInitPaths(driver, vm->def) < 0) return -1; - if (def->tpm) - ret = qemuExtTPMStart(driver, def, logCtxt); + if (vm->def->tpm) + ret = qemuExtTPMStart(driver, vm, logCtxt); return ret; } @@ -146,13 +146,13 @@ qemuExtDevicesStart(virQEMUDriverPtr driver, void qemuExtDevicesStop(virQEMUDriverPtr driver, - virDomainDefPtr def) + virDomainObjPtr vm) { - if (qemuExtDevicesInitPaths(driver, def) < 0) + if (qemuExtDevicesInitPaths(driver, vm->def) < 0) return; - if (def->tpm) - qemuExtTPMStop(driver, def); + if (vm->def->tpm) + qemuExtTPMStop(driver, vm); } diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h index c557778ddb..c26cdd50b2 100644 --- a/src/qemu/qemu_extdevice.h +++ b/src/qemu/qemu_extdevice.h @@ -41,13 +41,13 @@ void qemuExtDevicesCleanupHost(virQEMUDriverPtr driver, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuExtDevicesStart(virQEMUDriverPtr driver, - virDomainDefPtr def, + virDomainObjPtr vm, qemuDomainLogContextPtr logCtxt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; void qemuExtDevicesStop(virQEMUDriverPtr driver, - virDomainDefPtr def) + virDomainObjPtr vm) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); bool qemuExtDevicesHasDevice(virDomainDefPtr def); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1850923914..2c9e605047 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6392,7 +6392,7 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessGenID(vm, flags) < 0) goto cleanup; - if (qemuExtDevicesStart(driver, vm->def, logCtxt) < 0) + if (qemuExtDevicesStart(driver, vm, logCtxt) < 0) goto cleanup; VIR_DEBUG("Building emulator command line"); @@ -6648,7 +6648,7 @@ qemuProcessLaunch(virConnectPtr conn, cleanup: if (ret < 0) - qemuExtDevicesStop(driver, vm->def); + qemuExtDevicesStop(driver, vm); qemuDomainSecretDestroy(vm); virCommandFree(cmd); virObjectUnref(logCtxt); @@ -7079,7 +7079,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, qemuDomainCleanupRun(driver, vm); - qemuExtDevicesStop(driver, vm->def); + qemuExtDevicesStop(driver, vm); /* Stop autodestroy in case guest is restarted */ qemuProcessAutoDestroyRemove(driver, vm); diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 34921b4046..bf45abf93a 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -453,7 +453,7 @@ qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, * qemuSecurityStartTPMEmulator: * * @driver: the QEMU driver - * @def: the domain definition + * @vm: the domain object * @cmd: the command to run * @uid: the uid to run the emulator * @gid: the gid to run the emulator @@ -469,7 +469,7 @@ qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, */ int qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, - virDomainDefPtr def, + virDomainObjPtr vm, virCommandPtr cmd, uid_t uid, gid_t gid, @@ -484,7 +484,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, transactionStarted = true; if (virSecurityManagerSetTPMLabels(driver->securityManager, - def) < 0) { + vm->def) < 0) { virSecurityManagerTransactionAbort(driver->securityManager); return -1; } @@ -494,7 +494,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, transactionStarted = false; if (virSecurityManagerSetChildProcessLabel(driver->securityManager, - def, cmd) < 0) + vm->def, cmd) < 0) goto cleanup; if (virSecurityManagerPreFork(driver->securityManager) < 0) @@ -519,7 +519,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, virSecurityManagerTransactionStart(driver->securityManager) >= 0) transactionStarted = true; - virSecurityManagerRestoreTPMLabels(driver->securityManager, def); + virSecurityManagerRestoreTPMLabels(driver->securityManager, vm->def); if (transactionStarted && virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) @@ -532,14 +532,14 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, void qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, - virDomainDefPtr def) + virDomainObjPtr vm) { bool transactionStarted = false; if (virSecurityManagerTransactionStart(driver->securityManager) >= 0) transactionStarted = true; - virSecurityManagerRestoreTPMLabels(driver->securityManager, def); + virSecurityManagerRestoreTPMLabels(driver->securityManager, vm->def); if (transactionStarted && virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 13fc05152c..45d26a0dbf 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -84,7 +84,7 @@ int qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, virDomainChrDefPtr chr); int qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, - virDomainDefPtr def, + virDomainObjPtr vm, virCommandPtr cmd, uid_t uid, gid_t gid, @@ -92,7 +92,7 @@ int qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, int *cmdret); void qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, - virDomainDefPtr def); + virDomainObjPtr vm); int qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index c64114feac..af85f7b25f 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -756,7 +756,7 @@ qemuExtTPMCleanupHost(virDomainDefPtr def) */ static int qemuExtTPMStartEmulator(virQEMUDriverPtr driver, - virDomainDefPtr def, + virDomainObjPtr vm, qemuDomainLogContextPtr logCtxt) { int ret = -1; @@ -764,8 +764,8 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, int exitstatus = 0; char *errbuf = NULL; virQEMUDriverConfigPtr cfg; - virDomainTPMDefPtr tpm = def->tpm; - char *shortName = virDomainDefGetShortName(def); + virDomainTPMDefPtr tpm = vm->def->tpm; + char *shortName = virDomainDefGetShortName(vm->def); int cmdret = 0, timeout, rc; pid_t pid; @@ -777,7 +777,7 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, /* stop any left-over TPM emulator for this VM */ qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName); - if (!(cmd = qemuTPMEmulatorBuildCommand(tpm, def->name, def->uuid, + if (!(cmd = qemuTPMEmulatorBuildCommand(tpm, vm->def->name, vm->def->uuid, driver->privileged, cfg->swtpm_user, cfg->swtpm_group, @@ -789,7 +789,7 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, virCommandSetErrorBuffer(cmd, &errbuf); - if (qemuSecurityStartTPMEmulator(driver, def, cmd, + if (qemuSecurityStartTPMEmulator(driver, vm, cmd, cfg->swtpm_user, cfg->swtpm_group, &exitstatus, &cmdret) < 0) goto cleanup; @@ -837,15 +837,15 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, int qemuExtTPMStart(virQEMUDriverPtr driver, - virDomainDefPtr def, + virDomainObjPtr vm, qemuDomainLogContextPtr logCtxt) { int ret = 0; - virDomainTPMDefPtr tpm = def->tpm; + virDomainTPMDefPtr tpm = vm->def->tpm; switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_EMULATOR: - ret = qemuExtTPMStartEmulator(driver, def, logCtxt); + ret = qemuExtTPMStartEmulator(driver, vm, logCtxt); break; case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: case VIR_DOMAIN_TPM_TYPE_LAST: @@ -858,19 +858,19 @@ qemuExtTPMStart(virQEMUDriverPtr driver, void qemuExtTPMStop(virQEMUDriverPtr driver, - virDomainDefPtr def) + virDomainObjPtr vm) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *shortName = NULL; - switch (def->tpm->type) { + switch (vm->def->tpm->type) { case VIR_DOMAIN_TPM_TYPE_EMULATOR: - shortName = virDomainDefGetShortName(def); + shortName = virDomainDefGetShortName(vm->def); if (!shortName) goto cleanup; qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName); - qemuSecurityCleanupTPMEmulator(driver, def); + qemuSecurityCleanupTPMEmulator(driver, vm); break; case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: case VIR_DOMAIN_TPM_TYPE_LAST: diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h index 6eb1294da0..c7eeaafeb9 100644 --- a/src/qemu/qemu_tpm.h +++ b/src/qemu/qemu_tpm.h @@ -38,13 +38,13 @@ void qemuExtTPMCleanupHost(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); int qemuExtTPMStart(virQEMUDriverPtr driver, - virDomainDefPtr def, + virDomainObjPtr vm, qemuDomainLogContextPtr logCtxt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; void qemuExtTPMStop(virQEMUDriverPtr driver, - virDomainDefPtr def) + virDomainObjPtr vm) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuExtTPMSetupCgroup(virQEMUDriverPtr driver, -- 2.18.1
On 11/14/18 7:44 AM, Michal Privoznik wrote:
The TPM code currently accepts pointer to a domain definition. This is okay for now, but in near future the security driver APIs it calls will require domain object. Therefore, change the TPM code to accept the domain object pointer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_extdevice.c | 16 ++++++++-------- src/qemu/qemu_extdevice.h | 4 ++-- src/qemu/qemu_process.c | 6 +++--- src/qemu/qemu_security.c | 14 +++++++------- src/qemu/qemu_security.h | 4 ++-- src/qemu/qemu_tpm.c | 24 ++++++++++++------------ src/qemu/qemu_tpm.h | 4 ++-- 7 files changed, 36 insertions(+), 36 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John At one time I really hoped we could make the obj's transparent to callers and force usage of virDomainObjGetDef, but I know that's a sheer impossibility!
For metadata locking we might need an extra fork() which given latest attempts to do fewer fork()-s is suboptimal. Therefore, there will be a qemu.conf knob to enable or this feature. But since the feature is actually not metadata locking itself rather than remembering of the original owner of the file this is named as 'rememberOwner'. But patches for that feature are not even posted yet so there is actually no qemu.conf entry in this patch nor a way to enable this feature. Even though this is effectively a dead code for now it is still desired. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_conf.h | 1 + src/qemu/qemu_domain.c | 7 +++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 3 +++ 4 files changed, 14 insertions(+) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index f876f9117c..0b5b5a314f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -92,6 +92,7 @@ struct _virQEMUDriverConfig { bool dynamicOwnership; virBitmapPtr namespaces; + bool rememberOwner; int cgroupControllers; char **cgroupDeviceACL; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 37926850b2..558c97ad36 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1963,6 +1963,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv) virBitmapFree(priv->namespaces); priv->namespaces = NULL; + priv->rememberOwner = false; + priv->reconnectBlockjobs = VIR_TRISTATE_BOOL_ABSENT; priv->allowReboot = VIR_TRISTATE_BOOL_ABSENT; @@ -2480,6 +2482,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, if (priv->chardevStdioLogd) virBufferAddLit(buf, "<chardevStdioLogd/>\n"); + if (priv->rememberOwner) + virBufferAddLit(buf, "<rememberOwner/>\n"); + qemuDomainObjPrivateXMLFormatAllowReboot(buf, priv->allowReboot); qemuDomainObjPrivateXMLFormatPR(buf, priv); @@ -2891,6 +2896,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->namespaces = NULL; } + priv->rememberOwner = virXPathBoolean("count(./rememberOwner) > 0", ctxt); + if ((n = virXPathNodeSet("./vcpus/vcpu", ctxt, &nodes)) < 0) goto error; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 87de433b22..53b5ea1678 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -369,6 +369,9 @@ struct _qemuDomainObjPrivate { /* qemuProcessStartCPUs stores the reason for starting vCPUs here for the * RESUME event handler to use it */ virDomainRunningReason runningReason; + + /* true if libvirt remembers the original owner for files */ + bool rememberOwner; }; # define QEMU_DOMAIN_PRIVATE(vm) \ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2c9e605047..44bf55bfb2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5916,6 +5916,9 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, priv->chardevStdioLogd = true; } + /* Track if this domain remembers original owner */ + priv->rememberOwner = cfg->rememberOwner; + qemuProcessPrepareAllowReboot(vm); /* clear the 'blockdev' capability for VMs which have disks that need -- 2.18.1
On 11/14/18 7:44 AM, Michal Privoznik wrote:
For metadata locking we might need an extra fork() which given latest attempts to do fewer fork()-s is suboptimal. Therefore, there will be a qemu.conf knob to enable or this feature. But
or disable or just use "{en|dis}able" if you don't want to reformat all the lines.
since the feature is actually not metadata locking itself rather than remembering of the original owner of the file this is named as 'rememberOwner'. But patches for that feature are not even posted yet so there is actually no qemu.conf entry in this patch nor a way to enable this feature.
Even though this is effectively a dead code for now it is still desired.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_conf.h | 1 + src/qemu/qemu_domain.c | 7 +++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 3 +++ 4 files changed, 14 insertions(+)
I suppose we could just create the @priv entry and leave @cfg for later, but I'm fine with this anyway. Thanks - I know it's a pain, but I'm sure it'd be even more painful to get a review on a larger patch series which would actually set/use the knob as you expect. Reviewed-by: John Ferlan <jferlan@redhat.com> John
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 73 ++++++++++++++++++++++++--------- src/security/security_dac.c | 56 ++++++++++++++++--------- src/security/security_driver.h | 3 +- src/security/security_manager.c | 9 +++- src/security/security_manager.h | 3 +- src/security/security_selinux.c | 54 ++++++++++++++++-------- src/security/security_stack.c | 5 ++- 7 files changed, 140 insertions(+), 63 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index bf45abf93a..372bc53396 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -53,7 +53,8 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -86,7 +87,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd); if (transactionStarted && - virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + virSecurityManagerTransactionCommit(driver->securityManager, + -1, priv->rememberOwner) < 0) VIR_WARN("Unable to run security manager transaction"); virSecurityManagerTransactionAbort(driver->securityManager); @@ -98,6 +100,7 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -112,7 +115,8 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, disk) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -127,6 +131,7 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -141,7 +146,8 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, disk) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -156,6 +162,7 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -170,7 +177,8 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, src) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -185,6 +193,7 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -199,7 +208,8 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, src) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -214,6 +224,7 @@ qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -229,7 +240,8 @@ qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, NULL) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -244,6 +256,7 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -259,7 +272,8 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, NULL) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -274,6 +288,7 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -288,7 +303,8 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, mem) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -303,6 +319,7 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -317,7 +334,8 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, mem) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -347,7 +365,8 @@ qemuSecuritySetInputLabel(virDomainObjPtr vm, input) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -377,7 +396,8 @@ qemuSecurityRestoreInputLabel(virDomainObjPtr vm, input) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -408,7 +428,8 @@ qemuSecuritySetChardevLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -439,7 +460,8 @@ qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -476,6 +498,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, int *exitstatus, int *cmdret) { + qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; bool transactionStarted = false; @@ -489,7 +512,8 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, return -1; } - if (virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + -1, priv->rememberOwner) < 0) goto cleanup; transactionStarted = false; @@ -522,7 +546,8 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, virSecurityManagerRestoreTPMLabels(driver->securityManager, vm->def); if (transactionStarted && - virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + virSecurityManagerTransactionCommit(driver->securityManager, + -1, priv->rememberOwner) < 0) VIR_WARN("Unable to run security manager transaction"); virSecurityManagerTransactionAbort(driver->securityManager); @@ -534,6 +559,7 @@ void qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, virDomainObjPtr vm) { + qemuDomainObjPrivatePtr priv = vm->privateData; bool transactionStarted = false; if (virSecurityManagerTransactionStart(driver->securityManager) >= 0) @@ -542,7 +568,8 @@ qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, virSecurityManagerRestoreTPMLabels(driver->securityManager, vm->def); if (transactionStarted && - virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + virSecurityManagerTransactionCommit(driver->securityManager, + -1, priv->rememberOwner) < 0) VIR_WARN("Unable to run security manager transaction"); virSecurityManagerTransactionAbort(driver->securityManager); @@ -555,6 +582,7 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, const char *path, bool allowSubtree) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -570,7 +598,8 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, allowSubtree) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -585,6 +614,7 @@ qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *savefile) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -599,7 +629,8 @@ qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, savefile) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -614,6 +645,7 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *savefile) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -628,7 +660,8 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, savefile) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index da4a6c72fe..0e100f7895 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -86,6 +86,7 @@ struct _virSecurityDACChownList { virSecurityManagerPtr manager; virSecurityDACChownItemPtr *items; size_t nItems; + bool lock; }; @@ -210,22 +211,24 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, int rv = 0; int ret = -1; - if (VIR_ALLOC_N(paths, list->nItems) < 0) - return -1; + if (list->lock) { + if (VIR_ALLOC_N(paths, list->nItems) < 0) + return -1; - for (i = 0; i < list->nItems; i++) { - const char *p = list->items[i]->path; + for (i = 0; i < list->nItems; i++) { + const char *p = list->items[i]->path; - if (!p || - virFileIsDir(p)) - continue; + if (!p || + virFileIsDir(p)) + continue; - VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); + VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); + } + + if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) + goto cleanup; } - if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) - goto cleanup; - for (i = 0; i < list->nItems; i++) { virSecurityDACChownItemPtr item = list->items[i]; @@ -246,7 +249,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, break; } - if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) + if (list->lock && + virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) goto cleanup; if (rv < 0) @@ -529,11 +533,15 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr) * virSecurityDACTransactionCommit: * @mgr: security manager * @pid: domain's PID + * @lock: lock and unlock paths that are relabeled * * If @pid is not -1 then enter the @pid namespace (usually @pid refers * to a domain) and perform all the chown()-s on the list. If @pid is -1 * then the transaction is performed in the namespace of the caller. * + * If @lock is true then all the paths that transaction would + * touch are locked before and unlocked after it is done so. + * * Note that the transaction is also freed, therefore new one has to be * started after successful return from this function. Also it is * considered as error if there's no transaction set and this function @@ -544,9 +552,11 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr) */ static int virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - pid_t pid) + pid_t pid, + bool lock) { virSecurityDACChownListPtr list; + int rc; int ret = -1; list = virThreadLocalGet(&chownList); @@ -562,12 +572,20 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto cleanup; } - if ((pid == -1 && - virSecurityDACTransactionRun(pid, list) < 0) || - (pid != -1 && - virProcessRunInMountNamespace(pid, - virSecurityDACTransactionRun, - list) < 0)) + list->lock = lock; + + if (pid == -1) { + if (lock) + rc = virProcessRunInFork(virSecurityDACTransactionRun, list); + else + rc = virSecurityDACTransactionRun(pid, list); + } else { + rc = virProcessRunInMountNamespace(pid, + virSecurityDACTransactionRun, + list); + } + + if (rc < 0) goto cleanup; ret = 0; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index cbf0ecff6e..cd221f1c78 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -53,7 +53,8 @@ typedef int (*virSecurityDriverPreFork) (virSecurityManagerPtr mgr); typedef int (*virSecurityDriverTransactionStart) (virSecurityManagerPtr mgr); typedef int (*virSecurityDriverTransactionCommit) (virSecurityManagerPtr mgr, - pid_t pid); + pid_t pid, + bool lock); typedef void (*virSecurityDriverTransactionAbort) (virSecurityManagerPtr mgr); typedef int (*virSecurityDomainRestoreDiskLabel) (virSecurityManagerPtr mgr, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index c6c80e6165..712b785ae9 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -299,12 +299,16 @@ virSecurityManagerTransactionStart(virSecurityManagerPtr mgr) * virSecurityManagerTransactionCommit: * @mgr: security manager * @pid: domain's PID + * @lock: lock and unlock paths that are relabeled * * If @pid is not -1 then enter the @pid namespace (usually @pid refers * to a domain) and perform all the operations on the transaction list. * If @pid is -1 then the transaction is performed in the namespace of * the caller. * + * If @lock is true then all the paths that transaction would + * touch are locked before and unlocked after it is done so. + * * Note that the transaction is also freed, therefore new one has to be * started after successful return from this function. Also it is * considered as error if there's no transaction set and this function @@ -315,13 +319,14 @@ virSecurityManagerTransactionStart(virSecurityManagerPtr mgr) */ int virSecurityManagerTransactionCommit(virSecurityManagerPtr mgr, - pid_t pid) + pid_t pid, + bool lock) { int ret = 0; virObjectLock(mgr); if (mgr->drv->transactionCommit) - ret = mgr->drv->transactionCommit(mgr, pid); + ret = mgr->drv->transactionCommit(mgr, pid, lock); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 10ebe5cc29..04bb54f61e 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -79,7 +79,8 @@ void virSecurityManagerPostFork(virSecurityManagerPtr mgr); int virSecurityManagerTransactionStart(virSecurityManagerPtr mgr); int virSecurityManagerTransactionCommit(virSecurityManagerPtr mgr, - pid_t pid); + pid_t pid, + bool lock); void virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 780d650c69..5e72a3589a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -93,6 +93,7 @@ struct _virSecuritySELinuxContextList { virSecurityManagerPtr manager; virSecuritySELinuxContextItemPtr *items; size_t nItems; + bool lock; }; #define SECURITY_SELINUX_VOID_DOI "0" @@ -221,21 +222,23 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, int rv; int ret = -1; - if (VIR_ALLOC_N(paths, list->nItems) < 0) - return -1; + if (list->lock) { + if (VIR_ALLOC_N(paths, list->nItems) < 0) + return -1; - for (i = 0; i < list->nItems; i++) { - const char *p = list->items[i]->path; + for (i = 0; i < list->nItems; i++) { + const char *p = list->items[i]->path; - if (virFileIsDir(p)) - continue; + if (virFileIsDir(p)) + continue; - VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); + VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); + } + + if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) + goto cleanup; } - if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) - goto cleanup; - rv = 0; for (i = 0; i < list->nItems; i++) { virSecuritySELinuxContextItemPtr item = list->items[i]; @@ -250,7 +253,8 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, } } - if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) + if (list->lock && + virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) goto cleanup; if (rv < 0) @@ -1072,12 +1076,16 @@ virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr) * virSecuritySELinuxTransactionCommit: * @mgr: security manager * @pid: domain's PID + * @lock: lock and unlock paths that are relabeled * * If @pis is not -1 then enter the @pid namespace (usually @pid refers * to a domain) and perform all the sefilecon()-s on the list. If @pid * is -1 then the transaction is performed in the namespace of the * caller. * + * If @lock is true then all the paths that transaction would + * touch are locked before and unlocked after it is done so. + * * Note that the transaction is also freed, therefore new one has to be * started after successful return from this function. Also it is * considered as error if there's no transaction set and this function @@ -1088,9 +1096,11 @@ virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr) */ static int virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - pid_t pid) + pid_t pid, + bool lock) { virSecuritySELinuxContextListPtr list; + int rc; int ret = -1; list = virThreadLocalGet(&contextList); @@ -1106,12 +1116,20 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto cleanup; } - if ((pid == -1 && - virSecuritySELinuxTransactionRun(pid, list) < 0) || - (pid != -1 && - virProcessRunInMountNamespace(pid, - virSecuritySELinuxTransactionRun, - list) < 0)) + list->lock = lock; + + if (pid == -1) { + if (lock) + rc = virProcessRunInFork(virSecuritySELinuxTransactionRun, list); + else + rc = virSecuritySELinuxTransactionRun(pid, list); + } else { + rc = virProcessRunInMountNamespace(pid, + virSecuritySELinuxTransactionRun, + list); + } + + if (rc < 0) goto cleanup; ret = 0; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index e37a681293..3e60d5d2b7 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -156,14 +156,15 @@ virSecurityStackTransactionStart(virSecurityManagerPtr mgr) static int virSecurityStackTransactionCommit(virSecurityManagerPtr mgr, - pid_t pid) + pid_t pid, + bool lock) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; int rc = 0; for (; item; item = item->next) { - if (virSecurityManagerTransactionCommit(item->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(item->securityManager, pid, lock) < 0) rc = -1; } -- 2.18.1
I would say here that we're going to "convert" the rememberOwner configuration knob into a security driver "lock" knob that will effectively enable or disable the usage of metadata locking for the object. When metadata locking is enabled that means the security commit processing will be run in a fork similar to how namespaces use fork()'s for processing. This is done to ensure libvirt can properly and synchronously modify the metadata to store the original owner data. Since fork()'s (e.g. virFork) have been seen as a performance bottleneck being able to disable them allows the admin to choose whether the performance 'hit' is worth the extra 'security' of being able to remember the original owner of a lock. [you could move some of the above back a commit too, but I think it may get lost there... wordsmithing anything I didn't quite explain right is fine too. the important part being that we're converting bool names and that it's a performance tradeoff] On 11/14/18 7:44 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 73 ++++++++++++++++++++++++--------- src/security/security_dac.c | 56 ++++++++++++++++--------- src/security/security_driver.h | 3 +- src/security/security_manager.c | 9 +++- src/security/security_manager.h | 3 +- src/security/security_selinux.c | 54 ++++++++++++++++-------- src/security/security_stack.c | 5 ++- 7 files changed, 140 insertions(+), 63 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Trying to use virlockd to lock metadata turns out to be too big gun. Since we will always spawn a separate process for relabeling we are safe to use thread unsafe POSIX locks and take out virtlockd completely out of the picture. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 12 +- src/security/security_manager.c | 225 +++++++++++++++++--------------- src/security/security_manager.h | 17 ++- src/security/security_selinux.c | 11 +- 4 files changed, 141 insertions(+), 124 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0e100f7895..6b64d2c07a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -205,6 +205,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, void *opaque) { virSecurityDACChownListPtr list = opaque; + virSecurityManagerMetadataLockStatePtr state; const char **paths = NULL; size_t npaths = 0; size_t i; @@ -218,14 +219,10 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (i = 0; i < list->nItems; i++) { const char *p = list->items[i]->path; - if (!p || - virFileIsDir(p)) - continue; - VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); } - if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) + if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths))) goto cleanup; } @@ -249,9 +246,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, break; } - if (list->lock && - virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) - goto cleanup; + if (list->lock) + virSecurityManagerMetadataUnlock(list->manager, &state); if (rv < 0) goto cleanup; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 712b785ae9..f527e6b5b3 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -21,6 +21,10 @@ */ #include <config.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> + #include "security_driver.h" #include "security_stack.h" #include "security_dac.h" @@ -30,14 +34,11 @@ #include "virlog.h" #include "locking/lock_manager.h" #include "virfile.h" -#include "virtime.h" #define VIR_FROM_THIS VIR_FROM_SECURITY VIR_LOG_INIT("security.security_manager"); -virMutex lockManagerMutex = VIR_MUTEX_INITIALIZER; - struct _virSecurityManager { virObjectLockable parent; @@ -47,10 +48,6 @@ struct _virSecurityManager { void *privateData; virLockManagerPluginPtr lockPlugin; - /* This is a FD that represents a connection to virtlockd so - * that connection is kept open in between MetdataLock() and - * MetadataUnlock() calls. */ - int clientfd; }; static virClassPtr virSecurityManagerClass; @@ -66,7 +63,6 @@ void virSecurityManagerDispose(void *obj) mgr->drv->close(mgr); virObjectUnref(mgr->lockPlugin); - VIR_FORCE_CLOSE(mgr->clientfd); VIR_FREE(mgr->privateData); } @@ -119,7 +115,6 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv, mgr->flags = flags; mgr->virtDriver = virtDriver; VIR_STEAL_PTR(mgr->privateData, privateData); - mgr->clientfd = -1; if (drv->open(mgr) < 0) goto error; @@ -1281,129 +1276,153 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr, } -static virLockManagerPtr -virSecurityManagerNewLockManager(virSecurityManagerPtr mgr, - const char * const *paths, - size_t npaths) +struct _virSecurityManagerMetadataLockState { + size_t nfds; + int *fds; +}; + + +static int +cmpstringp(const void *p1, const void *p2) { - virLockManagerPtr lock; - virLockManagerParam params[] = { - { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID, - .key = "uuid", - }, - { .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING, - .key = "name", - .value = { .cstr = "libvirtd-sec" }, - }, - { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT, - .key = "pid", - .value = { .iv = getpid() }, - }, - }; - const unsigned int flags = 0; - size_t i; + const char *s1 = *(char * const *) p1; + const char *s2 = *(char * const *) p2; - if (virGetHostUUID(params[0].value.uuid) < 0) - return NULL; + if (!s1 && !s2) + return 0; - if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(mgr->lockPlugin), - VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON, - ARRAY_CARDINALITY(params), - params, - flags))) - return NULL; + if (!s1 || !s2) + return s2 ? -1 : 1; - for (i = 0; i < npaths; i++) { - if (virLockManagerAddResource(lock, - VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA, - paths[i], 0, NULL, 0) < 0) - goto error; - } - - return lock; - error: - virLockManagerFree(lock); - return NULL; + /* from man 3 qsort */ + return strcmp(s1, s2); } +#define METADATA_OFFSET 1 +#define METADATA_LEN 1 -/* How many seconds should we try to acquire the lock before - * giving up. */ -#define LOCK_ACQUIRE_TIMEOUT 60 - -int -virSecurityManagerMetadataLock(virSecurityManagerPtr mgr, - const char * const *paths, +/** + * virSecurityManagerMetadataLock: + * @mgr: security manager object + * @paths: paths to lock + * @npaths: number of items in @paths array + * + * Lock passed @paths for metadata change. The returned state + * should be passed to virSecurityManagerMetadataUnlock. + * + * NOTE: this function is not thread safe (because of usage of + * POSIX locks). + * + * Returns: state on success, + * NULL on failure. + */ +virSecurityManagerMetadataLockStatePtr +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + const char **paths, size_t npaths) { - virLockManagerPtr lock; - virTimeBackOffVar timebackoff; - int fd = -1; - int rv = -1; - int ret = -1; + size_t i = 0; + size_t nfds = 0; + int *fds = NULL; + virSecurityManagerMetadataLockStatePtr ret = NULL; - virMutexLock(&lockManagerMutex); + if (VIR_ALLOC_N(fds, npaths) < 0) + return NULL; - if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths))) - goto cleanup; + /* Sort paths to lock in order to avoid deadlocks. */ + qsort(paths, npaths, sizeof(*paths), cmpstringp); - if (virTimeBackOffStart(&timebackoff, 1, LOCK_ACQUIRE_TIMEOUT * 1000) < 0) - goto cleanup; - while (virTimeBackOffWait(&timebackoff)) { - rv = virLockManagerAcquire(lock, NULL, - VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK, - VIR_DOMAIN_LOCK_FAILURE_DEFAULT, &fd); + for (i = 0; i < npaths; i++) { + const char *p = paths[i]; + struct stat sb; + int retries = 10 * 1000; + int fd; + + if (!p || stat(p, &sb) < 0) + continue; + + if (S_ISDIR(sb.st_mode)) { + /* Directories can't be locked */ + continue; + } + + if ((fd = open(p, O_RDWR)) < 0) { + if (S_ISSOCK(sb.st_mode)) { + /* Sockets can be opened only if there exists the + * other side that listens. */ + continue; + } + + virReportSystemError(errno, + _("unable to open %s"), + p); + goto cleanup; + } + + do { + if (virFileLock(fd, false, + METADATA_OFFSET, METADATA_LEN, false) < 0) { + if (retries && (errno == EACCES || errno == EAGAIN)) { + /* File is locked. Try again. */ + retries--; + usleep(1000); + continue; + } else { + virReportSystemError(errno, + _("unable to lock %s for metadata change"), + p); + VIR_FORCE_CLOSE(fd); + goto cleanup; + } + } - if (rv >= 0) break; + } while (1); - if (virGetLastErrorCode() == VIR_ERR_RESOURCE_BUSY) - continue; - - goto cleanup; + VIR_APPEND_ELEMENT_COPY_INPLACE(fds, nfds, fd); } - if (rv < 0) + if (VIR_ALLOC(ret) < 0) goto cleanup; - mgr->clientfd = fd; - fd = -1; + VIR_STEAL_PTR(ret->fds, fds); + ret->nfds = nfds; + nfds = 0; - ret = 0; cleanup: - virLockManagerFree(lock); - VIR_FORCE_CLOSE(fd); - if (ret < 0) - virMutexUnlock(&lockManagerMutex); + for (i = nfds; i > 0; i--) + VIR_FORCE_CLOSE(fds[i - 1]); + VIR_FREE(fds); return ret; } -int -virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr, - const char * const *paths, - size_t npaths) +void +virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virSecurityManagerMetadataLockStatePtr *state) { - virLockManagerPtr lock; - int fd; - int ret = -1; + size_t i; - /* lockManagerMutex acquired from previous - * virSecurityManagerMetadataLock() call. */ + if (!state) + return; - fd = mgr->clientfd; - mgr->clientfd = -1; + for (i = 0; i < (*state)->nfds; i++) { + char ebuf[1024]; + int fd = (*state)->fds[i]; - if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths))) - goto cleanup; + /* Technically, unlock is not needed because it will + * happen on VIR_CLOSE() anyway. But let's play it nice. */ + if (virFileUnlock(fd, METADATA_OFFSET, METADATA_LEN) < 0) { + VIR_WARN("Unable to unlock fd %d: %s", + fd, virStrerror(errno, ebuf, sizeof(ebuf))); + } - if (virLockManagerRelease(lock, NULL, 0) < 0) - goto cleanup; + if (VIR_CLOSE(fd) < 0) { + VIR_WARN("Unable to close fd %d: %s", + fd, virStrerror(errno, ebuf, sizeof(ebuf))); + } + } - ret = 0; - cleanup: - virLockManagerFree(lock); - VIR_FORCE_CLOSE(fd); - virMutexUnlock(&lockManagerMutex); - return ret; + VIR_FREE((*state)->fds); + VIR_FREE(*state); } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 04bb54f61e..cacb17174f 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -200,11 +200,16 @@ int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr, int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr, virDomainDefPtr vm); -int virSecurityManagerMetadataLock(virSecurityManagerPtr mgr, - const char * const *paths, - size_t npaths); -int virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr, - const char * const *paths, - size_t npaths); +typedef struct _virSecurityManagerMetadataLockState virSecurityManagerMetadataLockState; +typedef virSecurityManagerMetadataLockState *virSecurityManagerMetadataLockStatePtr; + +virSecurityManagerMetadataLockStatePtr +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr, + const char **paths, + size_t npaths); + +void +virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr, + virSecurityManagerMetadataLockStatePtr *state); #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 5e72a3589a..95e9a1b0c7 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -215,6 +215,7 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, void *opaque) { virSecuritySELinuxContextListPtr list = opaque; + virSecurityManagerMetadataLockStatePtr state; bool privileged = virSecurityManagerGetPrivileged(list->manager); const char **paths = NULL; size_t npaths = 0; @@ -229,13 +230,10 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (i = 0; i < list->nItems; i++) { const char *p = list->items[i]->path; - if (virFileIsDir(p)) - continue; - VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); } - if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) + if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths))) goto cleanup; } @@ -253,9 +251,8 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, } } - if (list->lock && - virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) - goto cleanup; + if (list->lock) + virSecurityManagerMetadataUnlock(list->manager, &state); if (rv < 0) goto cleanup; -- 2.18.1
On 11/14/18 7:44 AM, Michal Privoznik wrote:
Trying to use virlockd to lock metadata turns out to be too big gun. Since we will always spawn a separate process for relabeling we are safe to use thread unsafe POSIX locks and take out virtlockd completely out of the picture.
NB: This patch has a 1 second delay loop in the event the lock is already taken by another thread. Since it's not expected that threads holding the lock will need more than a second or two to process the various file security operations, it was felt this was a good starting point. The time taken to execute has two factors - the number of files to be locked and the time the operation required by the security driver to perform it's operation. Usage of virTimeBackOffStart may or may not help. Similarly a configurable delay time or delay maximum time is also possible, but was deemed unnecessary at this time. [fair enough statement?? - pick and choose if you want to add all, any part, or none of it... I think it's a fair representation for someone coming along and looking at this commit to consider if they wanted to make a change to help perceived performance if the delay loop becomes that type of problem. It's not that it wasn't considered, it just wasn't easily quantifiable].
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 12 +- src/security/security_manager.c | 225 +++++++++++++++++--------------- src/security/security_manager.h | 17 ++- src/security/security_selinux.c | 11 +- 4 files changed, 141 insertions(+), 124 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 11/15/18 7:38 PM, John Ferlan wrote:
On 11/14/18 7:44 AM, Michal Privoznik wrote:
Trying to use virlockd to lock metadata turns out to be too big gun. Since we will always spawn a separate process for relabeling we are safe to use thread unsafe POSIX locks and take out virtlockd completely out of the picture.
NB: This patch has a 1 second delay loop in the event the lock is already taken by another thread. Since it's not expected that threads holding the lock will need more than a second or two to process the various file security operations, it was felt this was a good starting point. The time taken to execute has two factors - the number of files to be locked and the time the operation required by the security driver to perform it's operation. Usage of virTimeBackOffStart may or may not help. Similarly a configurable delay time or delay maximum time is also possible, but was deemed unnecessary at this time.
Darn, I forgot to undo the virTimeBackOffStart. You know what? To avoid sending v5 let me merge this version and investigate if using virTimeBackOff is any better.
[fair enough statement?? - pick and choose if you want to add all, any part, or none of it... I think it's a fair representation for someone coming along and looking at this commit to consider if they wanted to make a change to help perceived performance if the delay loop becomes that type of problem. It's not that it wasn't considered, it just wasn't easily quantifiable].
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 12 +- src/security/security_manager.c | 225 +++++++++++++++++--------------- src/security/security_manager.h | 17 ++- src/security/security_selinux.c | 11 +- 4 files changed, 141 insertions(+), 124 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks, Michal
This reverts commit 3e26b476b5f322353bf0dcd8e3f037ca672b8c62. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 4 +--- src/lxc/lxc_controller.c | 3 +-- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 3 --- src/security/security_manager.c | 25 +------------------------ src/security/security_manager.h | 2 -- tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- tests/securityselinuxtest.c | 2 +- tests/testutilsqemu.c | 2 +- 10 files changed, 8 insertions(+), 39 deletions(-) diff --git a/cfg.mk b/cfg.mk index d0183c02ff..c83b152fda 100644 --- a/cfg.mk +++ b/cfg.mk @@ -787,10 +787,8 @@ sc_prohibit_cross_inclusion: case $$dir in \ util/) safe="util";; \ access/ | conf/) safe="($$dir|conf|util)";; \ - cpu/| network/| node_device/| rpc/| storage/) \ + cpu/| network/| node_device/| rpc/| security/| storage/) \ safe="($$dir|util|conf|storage)";; \ - security/) \ - safe="($$dir|util|conf|storage|locking)";; \ xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";; \ *) safe="($$dir|$(mid_dirs)|util)";; \ esac; \ diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 62dfd09473..e853d02d65 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -2624,8 +2624,7 @@ int main(int argc, char *argv[]) ctrl->handshakeFd = handshakeFd; if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver, - LXC_DRIVER_NAME, - NULL, 0))) + LXC_DRIVER_NAME, 0))) goto cleanup; if (ctrl->def->seclabels) { diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f732305649..990871d9b3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1531,7 +1531,7 @@ lxcSecurityInit(virLXCDriverConfigPtr cfg) flags |= VIR_SECURITY_MANAGER_REQUIRE_CONFINED; virSecurityManagerPtr mgr = virSecurityManagerNew(cfg->securityDriverName, - LXC_DRIVER_NAME, NULL, flags); + LXC_DRIVER_NAME, flags); if (!mgr) goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 09e04b8544..e387c831d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -350,7 +350,6 @@ qemuSecurityInit(virQEMUDriverPtr driver) while (names && *names) { if (!(mgr = qemuSecurityNew(*names, QEMU_DRIVER_NAME, - cfg->metadataLockManagerName, flags))) goto error; if (!stack) { @@ -366,7 +365,6 @@ qemuSecurityInit(virQEMUDriverPtr driver) } else { if (!(mgr = qemuSecurityNew(NULL, QEMU_DRIVER_NAME, - cfg->metadataLockManagerName, flags))) goto error; if (!(stack = qemuSecurityNewStack(mgr))) @@ -383,7 +381,6 @@ qemuSecurityInit(virQEMUDriverPtr driver) cfg->user, cfg->group, flags, - cfg->metadataLockManagerName, qemuSecurityChownCallback))) goto error; if (!stack) { diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f527e6b5b3..a049382c7b 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -32,7 +32,6 @@ #include "viralloc.h" #include "virobject.h" #include "virlog.h" -#include "locking/lock_manager.h" #include "virfile.h" #define VIR_FROM_THIS VIR_FROM_SECURITY @@ -46,8 +45,6 @@ struct _virSecurityManager { unsigned int flags; const char *virtDriver; void *privateData; - - virLockManagerPluginPtr lockPlugin; }; static virClassPtr virSecurityManagerClass; @@ -58,12 +55,8 @@ void virSecurityManagerDispose(void *obj) { virSecurityManagerPtr mgr = obj; - if (mgr->drv && - mgr->drv->close) + if (mgr->drv->close) mgr->drv->close(mgr); - - virObjectUnref(mgr->lockPlugin); - VIR_FREE(mgr->privateData); } @@ -83,7 +76,6 @@ VIR_ONCE_GLOBAL_INIT(virSecurityManager); static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr drv, const char *virtDriver, - const char *lockManagerPluginName, unsigned int flags) { virSecurityManagerPtr mgr = NULL; @@ -103,14 +95,6 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv, if (!(mgr = virObjectLockableNew(virSecurityManagerClass))) goto error; - if (!lockManagerPluginName) - lockManagerPluginName = "nop"; - - if (!(mgr->lockPlugin = virLockManagerPluginNew(lockManagerPluginName, - NULL, NULL, 0))) { - goto error; - } - mgr->drv = drv; mgr->flags = flags; mgr->virtDriver = virtDriver; @@ -133,7 +117,6 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary) virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverStack, virSecurityManagerGetDriver(primary), - NULL, primary->flags); if (!mgr) @@ -142,8 +125,6 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary) if (virSecurityStackAddNested(mgr, primary) < 0) goto error; - mgr->lockPlugin = virObjectRef(mgr->lockPlugin); - return mgr; error: virObjectUnref(mgr); @@ -166,7 +147,6 @@ virSecurityManagerNewDAC(const char *virtDriver, uid_t user, gid_t group, unsigned int flags, - const char *lockManagerPluginName, virSecurityManagerDACChownCallback chownCallback) { virSecurityManagerPtr mgr; @@ -177,7 +157,6 @@ virSecurityManagerNewDAC(const char *virtDriver, mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, virtDriver, - lockManagerPluginName, flags & VIR_SECURITY_MANAGER_NEW_MASK); if (!mgr) @@ -199,7 +178,6 @@ virSecurityManagerNewDAC(const char *virtDriver, virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, - const char *lockManagerPluginName, unsigned int flags) { virSecurityDriverPtr drv = virSecurityDriverLookup(name, virtDriver); @@ -228,7 +206,6 @@ virSecurityManagerNew(const char *name, return virSecurityManagerNewDriver(drv, virtDriver, - lockManagerPluginName, flags); } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index cacb17174f..7e82304689 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -45,7 +45,6 @@ typedef enum { virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, - const char *lockManagerPluginName, unsigned int flags); virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary); @@ -71,7 +70,6 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, uid_t user, gid_t group, unsigned int flags, - const char *lockManagerPluginName, virSecurityManagerDACChownCallback chownCallback); int virSecurityManagerPreFork(virSecurityManagerPtr mgr); diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index 7cddf96e82..a0296c787e 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -14,7 +14,7 @@ mymain(void) if (virThreadInitialize() < 0) return EXIT_FAILURE; - mgr = virSecurityManagerNew(NULL, "QEMU", NULL, VIR_SECURITY_MANAGER_DEFAULT_CONFINED); + mgr = virSecurityManagerNew(NULL, "QEMU", VIR_SECURITY_MANAGER_DEFAULT_CONFINED); if (mgr == NULL) { fprintf(stderr, "Failed to start security driver"); return EXIT_FAILURE; diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index aa9fae7d32..39f4eb7b6a 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -346,7 +346,7 @@ mymain(void) if (!rc) return EXIT_AM_SKIP; - if (!(mgr = virSecurityManagerNew("selinux", "QEMU", NULL, + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", VIR_SECURITY_MANAGER_DEFAULT_CONFINED | VIR_SECURITY_MANAGER_PRIVILEGED))) { VIR_TEST_VERBOSE("Unable to initialize security driver: %s\n", diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index f1ea51b1ac..a2864cf57c 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -272,7 +272,7 @@ mymain(void) int ret = 0; virSecurityManagerPtr mgr; - if (!(mgr = virSecurityManagerNew("selinux", "QEMU", NULL, + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", VIR_SECURITY_MANAGER_DEFAULT_CONFINED | VIR_SECURITY_MANAGER_PRIVILEGED))) { fprintf(stderr, "Unable to initialize security driver: %s\n", diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 332885eb77..0d3e9fc7e6 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -716,7 +716,7 @@ int qemuTestDriverInit(virQEMUDriver *driver) if (qemuTestCapsCacheInsert(driver->qemuCapsCache, NULL) < 0) goto error; - if (!(mgr = virSecurityManagerNew("none", "qemu", NULL, + if (!(mgr = virSecurityManagerNew("none", "qemu", VIR_SECURITY_MANAGER_PRIVILEGED))) goto error; if (!(driver->securityManager = virSecurityManagerNewStack(mgr))) -- 2.18.1
This reverts commit 8b8aefb3d6ae2139ea3d4ef6d7dd2c06f57f6075. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_conf.c | 1 - src/qemu/qemu_conf.h | 1 - 2 files changed, 2 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 17b7e11e02..32da9a7351 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -426,7 +426,6 @@ static void virQEMUDriverConfigDispose(void *obj) virStringListFree(cfg->securityDriverNames); VIR_FREE(cfg->lockManagerName); - VIR_FREE(cfg->metadataLockManagerName); virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares); diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 0b5b5a314f..8986350fad 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -187,7 +187,6 @@ struct _virQEMUDriverConfig { bool autoStartBypassCache; char *lockManagerName; - char *metadataLockManagerName; int keepAliveInterval; unsigned int keepAliveCount; -- 2.18.1
This reverts commit 35b5b244da825fb41e35e4dc62e740d716214ec9. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 4 ---- src/locking/lock_driver_lockd.c | 4 +--- src/locking/lock_driver_sanlock.c | 4 +--- src/locking/lock_manager.c | 10 +++------- 4 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index ae30abda7d..7c8f744be3 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -124,7 +124,6 @@ struct _virLockManagerParam { /** * virLockDriverInit: * @version: the libvirt requested plugin ABI version - * @configFile: path to config file * @flags: the libvirt requested plugin optional extras * * Allow the plugin to validate the libvirt requested @@ -132,9 +131,6 @@ struct _virLockManagerParam { * to block its use in versions of libvirtd which are * too old to support key features. * - * The @configFile variable points to config file that the driver - * should load. If NULL, no config file should be loaded. - * * NB: A plugin may be loaded multiple times, for different * libvirt drivers (eg QEMU, LXC, UML) * diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 0c672b05b1..85cdcf97be 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -371,10 +371,8 @@ static int virLockManagerLockDaemonInit(unsigned int version, driver->requireLeaseForDisks = true; driver->autoDiskLease = true; - if (configFile && - virLockManagerLockDaemonLoadConfig(configFile) < 0) { + if (virLockManagerLockDaemonLoadConfig(configFile) < 0) goto error; - } if (driver->autoDiskLease) { if (driver->fileLockSpaceDir && diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 3ad0dc9bed..b10d8197c5 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -446,10 +446,8 @@ static int virLockManagerSanlockInit(unsigned int version, goto error; } - if (configFile && - virLockManagerSanlockLoadConfig(driver, configFile) < 0) { + if (virLockManagerSanlockLoadConfig(driver, configFile) < 0) goto error; - } if (driver->autoDiskLease && !driver->hostID) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index 9067f5a01a..d421b6acfc 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -104,8 +104,6 @@ static void virLockManagerLogParams(size_t nparams, /** * virLockManagerPluginNew: * @name: the name of the plugin - * @driverName: the hypervisor driver that loads the plugin - * @configDir: path to dir where config files are stored * @flag: optional plugin flags * * Attempt to load the plugin $(libdir)/libvirt/lock-driver/@name.so @@ -131,13 +129,11 @@ virLockManagerPluginPtr virLockManagerPluginNew(const char *name, char *configFile = NULL; VIR_DEBUG("name=%s driverName=%s configDir=%s flags=0x%x", - name, NULLSTR(driverName), NULLSTR(configDir), flags); + name, driverName, configDir, flags); - if (driverName && configDir && - virAsprintf(&configFile, "%s/%s-%s.conf", - configDir, driverName, name) < 0) { + if (virAsprintf(&configFile, "%s/%s-%s.conf", + configDir, driverName, name) < 0) return NULL; - } if (STREQ(name, "nop")) { driver = &virLockDriverNop; -- 2.18.1
This reverts commit 385eb8399bdb1610447c2857abfe99cee4a9fb9e. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 4 -- src/locking/lock_driver_lockd.c | 82 ++++++++++----------------------- 2 files changed, 24 insertions(+), 62 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 7c8f744be3..9be0abcfba 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -67,10 +67,6 @@ typedef enum { VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY = (1 << 0), /* Prevent further lock/unlock calls from this process */ VIR_LOCK_MANAGER_ACQUIRE_RESTRICT = (1 << 1), - /* Used when acquiring more resources in which one of them - * can't be acquired, perform a rollback and release all - * resources acquired so far. */ - VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK = (1 << 2), } virLockManagerAcquireFlags; typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 85cdcf97be..d6551e125c 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -735,34 +735,6 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, } -static int virLockManagerLockDaemonReleaseImpl(virNetClientPtr client, - virNetClientProgramPtr program, - int counter, - virLockManagerLockDaemonResourcePtr res) -{ - virLockSpaceProtocolReleaseResourceArgs args; - - memset(&args, 0, sizeof(args)); - - args.path = res->lockspace; - args.name = res->name; - args.flags = res->flags; - - args.flags &= - ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA); - - return virNetClientProgramCall(program, - client, - counter, - VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE, - 0, NULL, NULL, NULL, - (xdrproc_t)xdr_virLockSpaceProtocolReleaseResourceArgs, &args, - (xdrproc_t)xdr_void, NULL); -} - - static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, const char *state ATTRIBUTE_UNUSED, unsigned int flags, @@ -773,13 +745,10 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, virNetClientProgramPtr program = NULL; int counter = 0; int rv = -1; - ssize_t i; - ssize_t lastGood = -1; virLockManagerLockDaemonPrivatePtr priv = lock->privateData; virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY | - VIR_LOCK_MANAGER_ACQUIRE_RESTRICT | - VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK, -1); + VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1); if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN && priv->nresources == 0 && @@ -798,6 +767,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, goto cleanup; if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) { + size_t i; for (i = 0; i < priv->nresources; i++) { virLockSpaceProtocolAcquireResourceArgs args; @@ -815,7 +785,6 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, (xdrproc_t)xdr_virLockSpaceProtocolAcquireResourceArgs, &args, (xdrproc_t)xdr_void, NULL) < 0) goto cleanup; - lastGood = i; } } @@ -826,28 +795,8 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, rv = 0; cleanup: - if (rv < 0) { - int saved_errno = errno; - virErrorPtr origerr; - - virErrorPreserveLast(&origerr); - if (fd) - VIR_FORCE_CLOSE(*fd); - - if (flags & VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK) { - for (i = lastGood; i >= 0; i--) { - virLockManagerLockDaemonResourcePtr res = &priv->resources[i]; - - if (virLockManagerLockDaemonReleaseImpl(client, program, - counter++, res) < 0) - VIR_WARN("Unable to release resource lockspace=%s name=%s", - res->lockspace, res->name); - } - } - - virErrorRestore(&origerr); - errno = saved_errno; - } + if (rv != 0 && fd) + VIR_FORCE_CLOSE(*fd); virNetClientClose(client); virObjectUnref(client); virObjectUnref(program); @@ -875,10 +824,27 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, goto cleanup; for (i = 0; i < priv->nresources; i++) { - virLockManagerLockDaemonResourcePtr res = &priv->resources[i]; + virLockSpaceProtocolReleaseResourceArgs args; - if (virLockManagerLockDaemonReleaseImpl(client, program, - counter++, res) < 0) + memset(&args, 0, sizeof(args)); + + if (priv->resources[i].lockspace) + args.path = priv->resources[i].lockspace; + args.name = priv->resources[i].name; + args.flags = priv->resources[i].flags; + + args.flags &= + ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA); + + if (virNetClientProgramCall(program, + client, + counter++, + VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE, + 0, NULL, NULL, NULL, + (xdrproc_t)xdr_virLockSpaceProtocolReleaseResourceArgs, &args, + (xdrproc_t)xdr_void, NULL) < 0) goto cleanup; } -- 2.18.1
This reverts commit 997283b54b0e1f599aed3085ceba027eb8110acb. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 -- src/locking/lock_driver_lockd.c | 47 +++++++++---------------------- src/locking/lock_driver_sanlock.c | 3 +- 3 files changed, 14 insertions(+), 38 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 9be0abcfba..a9d2041c30 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -51,8 +51,6 @@ typedef enum { VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK = 0, /* A lease against an arbitrary resource */ VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE = 1, - /* The resource to be locked is a metadata */ - VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA = 2, } virLockManagerResourceType; typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index d6551e125c..268676c407 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -563,7 +563,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, virLockManagerLockDaemonPrivatePtr priv = lock->privateData; char *newName = NULL; char *newLockspace = NULL; - int newFlags = 0; + bool autoCreate = false; int ret = -1; virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | @@ -575,7 +575,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, switch (priv->type) { case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: - switch ((virLockManagerResourceType) type) { + switch (type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (params || nparams) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -602,7 +602,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, VIR_DEBUG("Got an LVM UUID %s for %s", newName, name); if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0) goto cleanup; - newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE; + autoCreate = true; break; } virResetLastError(); @@ -619,7 +619,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, VIR_DEBUG("Got an SCSI ID %s for %s", newName, name); if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0) goto cleanup; - newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE; + autoCreate = true; break; } virResetLastError(); @@ -631,7 +631,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, goto cleanup; if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) goto cleanup; - newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE; + autoCreate = true; VIR_DEBUG("Using indirect lease %s for %s", newName, name); } else { if (VIR_STRDUP(newLockspace, "") < 0) @@ -676,8 +676,6 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, goto cleanup; } break; - - case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d for domain lock object"), @@ -687,29 +685,6 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, break; case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: - switch ((virLockManagerResourceType) type) { - case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA: - if (params || nparams) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unexpected parameters for metadata resource")); - goto cleanup; - } - if (VIR_STRDUP(newLockspace, "") < 0 || - VIR_STRDUP(newName, name) < 0) - goto cleanup; - newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA; - break; - - case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: - case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown lock manager object type %d for daemon lock object"), - type); - goto cleanup; - } - break; - default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), @@ -717,15 +692,19 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, goto cleanup; } - if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED) - newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED; - if (VIR_EXPAND_N(priv->resources, priv->nresources, 1) < 0) goto cleanup; VIR_STEAL_PTR(priv->resources[priv->nresources-1].lockspace, newLockspace); VIR_STEAL_PTR(priv->resources[priv->nresources-1].name, newName); - priv->resources[priv->nresources-1].flags = newFlags; + + if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED) + priv->resources[priv->nresources-1].flags |= + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED; + + if (autoCreate) + priv->resources[priv->nresources-1].flags |= + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE; ret = 0; cleanup: diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index b10d8197c5..86efc83b5a 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -811,7 +811,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0; - switch ((virLockManagerResourceType) type) { + switch (type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (driver->autoDiskLease) { if (virLockManagerSanlockAddDisk(driver, lock, name, nparams, params, @@ -835,7 +835,6 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, return -1; break; - case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d for domain lock object"), -- 2.18.1
This reverts commit aaf34cb9013d6d746f4edf9807408cb9dfbcf01d. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_lockd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 268676c407..22a5a97913 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -63,8 +63,6 @@ struct _virLockManagerLockDaemonPrivate { char *name; int id; pid_t pid; - - bool hasRWDisks; } dom; struct { @@ -76,6 +74,8 @@ struct _virLockManagerLockDaemonPrivate { size_t nresources; virLockManagerLockDaemonResourcePtr resources; + + bool hasRWDisks; }; @@ -585,7 +585,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, if (!driver->autoDiskLease) { if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | VIR_LOCK_MANAGER_RESOURCE_READONLY))) - priv->t.dom.hasRWDisks = true; + priv->hasRWDisks = true; return 0; } @@ -731,7 +731,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN && priv->nresources == 0 && - priv->t.dom.hasRWDisks && + priv->hasRWDisks && driver->requireLeaseForDisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Read/write, exclusive access, disks were present, but no leases specified")); -- 2.18.1
This reverts commit 22baf6e08c65d9174b24f66370724ce961ce9576. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 - src/locking/lock_driver_lockd.c | 297 +++++++++++------------------- src/locking/lock_driver_sanlock.c | 37 ++-- 3 files changed, 116 insertions(+), 220 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index a9d2041c30..8b7cccc521 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -42,8 +42,6 @@ typedef enum { typedef enum { /* The managed object is a virtual guest domain */ VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN = 0, - /* The managed object is a daemon (e.g. libvirtd) */ - VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON = 1, } virLockManagerObjectType; typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 22a5a97913..ca825e6026 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -56,21 +56,10 @@ struct _virLockManagerLockDaemonResource { }; struct _virLockManagerLockDaemonPrivate { - virLockManagerObjectType type; - union { - struct { - unsigned char uuid[VIR_UUID_BUFLEN]; - char *name; - int id; - pid_t pid; - } dom; - - struct { - unsigned char uuid[VIR_UUID_BUFLEN]; - char *name; - pid_t pid; - } daemon; - } t; + unsigned char uuid[VIR_UUID_BUFLEN]; + char *name; + int id; + pid_t pid; size_t nresources; virLockManagerLockDaemonResourcePtr resources; @@ -167,30 +156,10 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock, memset(&args, 0, sizeof(args)); args.flags = 0; - - switch (priv->type) { - case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: - memcpy(args.owner.uuid, priv->t.dom.uuid, VIR_UUID_BUFLEN); - args.owner.name = priv->t.dom.name; - args.owner.id = priv->t.dom.id; - args.owner.pid = priv->t.dom.pid; - break; - - case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: - memcpy(args.owner.uuid, priv->t.daemon.uuid, VIR_UUID_BUFLEN); - args.owner.name = priv->t.daemon.name; - args.owner.pid = priv->t.daemon.pid; - /* This one should not be needed. However, virtlockd - * checks for ID because not every domain has a PID. */ - args.owner.id = priv->t.daemon.pid; - break; - - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown lock manager object type %d"), - priv->type); - return -1; - } + memcpy(args.owner.uuid, priv->uuid, VIR_UUID_BUFLEN); + args.owner.name = priv->name; + args.owner.id = priv->id; + args.owner.pid = priv->pid; if (virNetClientProgramCall(program, client, @@ -422,18 +391,7 @@ virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv) } VIR_FREE(priv->resources); - switch (priv->type) { - case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: - VIR_FREE(priv->t.dom.name); - break; - - case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: - VIR_FREE(priv->t.daemon.name); - break; - - default: - break; - } + VIR_FREE(priv->name); VIR_FREE(priv); } @@ -462,82 +420,46 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, if (VIR_ALLOC(priv) < 0) return -1; - priv->type = type; - - switch ((virLockManagerObjectType) type) { + switch (type) { case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: for (i = 0; i < nparams; i++) { if (STREQ(params[i].key, "uuid")) { - memcpy(priv->t.dom.uuid, params[i].value.uuid, VIR_UUID_BUFLEN); + memcpy(priv->uuid, params[i].value.uuid, VIR_UUID_BUFLEN); } else if (STREQ(params[i].key, "name")) { - if (VIR_STRDUP(priv->t.dom.name, params[i].value.str) < 0) + if (VIR_STRDUP(priv->name, params[i].value.str) < 0) goto cleanup; } else if (STREQ(params[i].key, "id")) { - priv->t.dom.id = params[i].value.iv; + priv->id = params[i].value.iv; } else if (STREQ(params[i].key, "pid")) { - priv->t.dom.pid = params[i].value.iv; + priv->pid = params[i].value.iv; } else if (STREQ(params[i].key, "uri")) { /* ignored */ } else { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected parameter %s for domain object"), + _("Unexpected parameter %s for object"), params[i].key); goto cleanup; } } - if (priv->t.dom.id == 0) { + if (priv->id == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing ID parameter for domain object")); goto cleanup; } - if (priv->t.dom.pid == 0) + if (priv->pid == 0) VIR_DEBUG("Missing PID parameter for domain object"); - if (!priv->t.dom.name) { + if (!priv->name) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing name parameter for domain object")); goto cleanup; } - if (!virUUIDIsValid(priv->t.dom.uuid)) { + if (!virUUIDIsValid(priv->uuid)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing UUID parameter for domain object")); goto cleanup; } break; - case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: - for (i = 0; i < nparams; i++) { - if (STREQ(params[i].key, "uuid")) { - memcpy(priv->t.daemon.uuid, params[i].value.uuid, VIR_UUID_BUFLEN); - } else if (STREQ(params[i].key, "name")) { - if (VIR_STRDUP(priv->t.daemon.name, params[i].value.str) < 0) - goto cleanup; - } else if (STREQ(params[i].key, "pid")) { - priv->t.daemon.pid = params[i].value.iv; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected parameter %s for daemon object"), - params[i].key); - goto cleanup; - } - } - - if (!virUUIDIsValid(priv->t.daemon.uuid)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing UUID parameter for daemon object")); - goto cleanup; - } - if (!priv->t.daemon.name) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing name parameter for daemon object")); - goto cleanup; - } - if (priv->t.daemon.pid == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing PID parameter for daemon object")); - goto cleanup; - } - break; - default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), @@ -572,119 +494,107 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0; - switch (priv->type) { - case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + switch (type) { + case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: + if (params || nparams) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected parameters for disk resource")); + goto cleanup; + } + if (!driver->autoDiskLease) { + if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_READONLY))) + priv->hasRWDisks = true; + return 0; + } - switch (type) { - case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: - if (params || nparams) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unexpected parameters for disk resource")); + /* XXX we should somehow pass in TYPE=BLOCK info + * from the domain_lock code, instead of assuming /dev + */ + if (STRPREFIX(name, "/dev") && + driver->lvmLockSpaceDir) { + VIR_DEBUG("Trying to find an LVM UUID for %s", name); + if (virStorageFileGetLVMKey(name, &newName) < 0) goto cleanup; - } - if (!driver->autoDiskLease) { - if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | - VIR_LOCK_MANAGER_RESOURCE_READONLY))) - priv->hasRWDisks = true; - return 0; - } - /* XXX we should somehow pass in TYPE=BLOCK info - * from the domain_lock code, instead of assuming /dev - */ - if (STRPREFIX(name, "/dev") && - driver->lvmLockSpaceDir) { - VIR_DEBUG("Trying to find an LVM UUID for %s", name); - if (virStorageFileGetLVMKey(name, &newName) < 0) + if (newName) { + VIR_DEBUG("Got an LVM UUID %s for %s", newName, name); + if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0) goto cleanup; - - if (newName) { - VIR_DEBUG("Got an LVM UUID %s for %s", newName, name); - if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0) - goto cleanup; - autoCreate = true; - break; - } - virResetLastError(); - /* Fallback to generic non-block code */ + autoCreate = true; + break; } + virResetLastError(); + /* Fallback to generic non-block code */ + } - if (STRPREFIX(name, "/dev") && - driver->scsiLockSpaceDir) { - VIR_DEBUG("Trying to find an SCSI ID for %s", name); - if (virStorageFileGetSCSIKey(name, &newName) < 0) - goto cleanup; - - if (newName) { - VIR_DEBUG("Got an SCSI ID %s for %s", newName, name); - if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0) - goto cleanup; - autoCreate = true; - break; - } - virResetLastError(); - /* Fallback to generic non-block code */ - } + if (STRPREFIX(name, "/dev") && + driver->scsiLockSpaceDir) { + VIR_DEBUG("Trying to find an SCSI ID for %s", name); + if (virStorageFileGetSCSIKey(name, &newName) < 0) + goto cleanup; - if (driver->fileLockSpaceDir) { - if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0) - goto cleanup; - if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) + if (newName) { + VIR_DEBUG("Got an SCSI ID %s for %s", newName, name); + if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0) goto cleanup; autoCreate = true; - VIR_DEBUG("Using indirect lease %s for %s", newName, name); - } else { - if (VIR_STRDUP(newLockspace, "") < 0) - goto cleanup; - if (VIR_STRDUP(newName, name) < 0) - goto cleanup; - VIR_DEBUG("Using direct lease for %s", name); + break; } + virResetLastError(); + /* Fallback to generic non-block code */ + } - break; - case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: { - size_t i; - char *path = NULL; - char *lockspace = NULL; - for (i = 0; i < nparams; i++) { - if (STREQ(params[i].key, "offset")) { - if (params[i].value.ul != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Offset must be zero for this lock manager")); - goto cleanup; - } - } else if (STREQ(params[i].key, "lockspace")) { - lockspace = params[i].value.str; - } else if (STREQ(params[i].key, "path")) { - path = params[i].value.str; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected parameter %s for lease resource"), - params[i].key); - goto cleanup; - } - } - if (!path || !lockspace) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing path or lockspace for lease resource")); + if (driver->fileLockSpaceDir) { + if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0) goto cleanup; - } - if (virAsprintf(&newLockspace, "%s/%s", - path, lockspace) < 0) + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) + goto cleanup; + autoCreate = true; + VIR_DEBUG("Using indirect lease %s for %s", newName, name); + } else { + if (VIR_STRDUP(newLockspace, "") < 0) goto cleanup; if (VIR_STRDUP(newName, name) < 0) goto cleanup; - - } break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown lock manager object type %d for domain lock object"), - type); - goto cleanup; + VIR_DEBUG("Using direct lease for %s", name); } + break; + case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: { + size_t i; + char *path = NULL; + char *lockspace = NULL; + for (i = 0; i < nparams; i++) { + if (STREQ(params[i].key, "offset")) { + if (params[i].value.ul != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Offset must be zero for this lock manager")); + goto cleanup; + } + } else if (STREQ(params[i].key, "lockspace")) { + lockspace = params[i].value.str; + } else if (STREQ(params[i].key, "path")) { + path = params[i].value.str; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected parameter %s for lease resource"), + params[i].key); + goto cleanup; + } + } + if (!path || !lockspace) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing path or lockspace for lease resource")); + goto cleanup; + } + if (virAsprintf(&newLockspace, "%s/%s", + path, lockspace) < 0) + goto cleanup; + if (VIR_STRDUP(newName, name) < 0) + goto cleanup; - case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + } break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), @@ -729,8 +639,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY | VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1); - if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN && - priv->nresources == 0 && + if (priv->nresources == 0 && priv->hasRWDisks && driver->requireLeaseForDisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 86efc83b5a..ff0c9be8f7 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -509,32 +509,21 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, priv->flags = flags; - switch ((virLockManagerObjectType) type) { - case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: - for (i = 0; i < nparams; i++) { - param = ¶ms[i]; + for (i = 0; i < nparams; i++) { + param = ¶ms[i]; - if (STREQ(param->key, "uuid")) { - memcpy(priv->vm_uuid, param->value.uuid, 16); - } else if (STREQ(param->key, "name")) { - if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) - goto error; - } else if (STREQ(param->key, "pid")) { - priv->vm_pid = param->value.iv; - } else if (STREQ(param->key, "id")) { - priv->vm_id = param->value.ui; - } else if (STREQ(param->key, "uri")) { - priv->vm_uri = param->value.cstr; - } + if (STREQ(param->key, "uuid")) { + memcpy(priv->vm_uuid, param->value.uuid, 16); + } else if (STREQ(param->key, "name")) { + if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) + goto error; + } else if (STREQ(param->key, "pid")) { + priv->vm_pid = param->value.iv; + } else if (STREQ(param->key, "id")) { + priv->vm_id = param->value.ui; + } else if (STREQ(param->key, "uri")) { + priv->vm_uri = param->value.cstr; } - break; - - case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown lock manager object type %d"), - type); - goto error; } /* Sanlock needs process registration, but the only way how to probe -- 2.18.1
This reverts commit 21c34b86be5233634eb38f77be64e2263bfc4e48. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon_dispatch.c | 10 ++-------- src/locking/lock_driver_lockd.c | 3 +-- src/locking/lock_driver_lockd.h | 1 - 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index a683ad3d6b..10248ec0b5 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -37,9 +37,6 @@ VIR_LOG_INIT("locking.lock_daemon_dispatch"); #include "lock_daemon_dispatch_stubs.h" -#define DEFAULT_OFFSET 0 -#define METADATA_OFFSET 1 - static int virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client, @@ -53,14 +50,13 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU virNetServerClientGetPrivateData(client); virLockSpacePtr lockspace; unsigned int newFlags; - off_t start = DEFAULT_OFFSET; + off_t start = 0; off_t len = 1; virMutexLock(&priv->lock); virCheckFlagsGoto(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA, cleanup); + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE, cleanup); if (priv->restricted) { virReportError(VIR_ERR_OPERATION_DENIED, "%s", @@ -86,8 +82,6 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU newFlags |= VIR_LOCK_SPACE_ACQUIRE_SHARED; if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE) newFlags |= VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE; - if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA) - start = METADATA_OFFSET; if (virLockSpaceAcquireResource(lockspace, args->name, diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index ca825e6026..16fce551c3 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -723,8 +723,7 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, args.flags &= ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA); + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE); if (virNetClientProgramCall(program, client, diff --git a/src/locking/lock_driver_lockd.h b/src/locking/lock_driver_lockd.h index bebd804365..6931fe7425 100644 --- a/src/locking/lock_driver_lockd.h +++ b/src/locking/lock_driver_lockd.h @@ -25,7 +25,6 @@ enum virLockSpaceProtocolAcquireResourceFlags { VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED = (1 << 0), VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = (1 << 1), - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA = (1 << 2), }; #endif /* __VIR_LOCK_DRIVER_LOCKD_H__ */ -- 2.18.1
This reverts commit afd5a27575e8b6a494d2728552fe0e89c71e32b4. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon_dispatch.c | 3 --- src/util/virlockspace.c | 15 +++++---------- src/util/virlockspace.h | 4 ---- tests/virlockspacetest.c | 29 +++++------------------------ 4 files changed, 10 insertions(+), 41 deletions(-) diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 10248ec0b5..1b479db55d 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -50,8 +50,6 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU virNetServerClientGetPrivateData(client); virLockSpacePtr lockspace; unsigned int newFlags; - off_t start = 0; - off_t len = 1; virMutexLock(&priv->lock); @@ -86,7 +84,6 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU if (virLockSpaceAcquireResource(lockspace, args->name, priv->ownerPid, - start, len, newFlags) < 0) goto cleanup; diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 79fa48d365..0736b4b85b 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -115,10 +115,8 @@ static void virLockSpaceResourceFree(virLockSpaceResourcePtr res) static virLockSpaceResourcePtr virLockSpaceResourceNew(virLockSpacePtr lockspace, const char *resname, - pid_t owner, - off_t start, - off_t len, - unsigned int flags) + unsigned int flags, + pid_t owner) { virLockSpaceResourcePtr res; bool shared = !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED); @@ -159,7 +157,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, start, len, false) < 0) { + if (virFileLock(res->fd, shared, 0, 1, false) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -206,7 +204,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, start, len, false) < 0) { + if (virFileLock(res->fd, shared, 0, 1, false) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -614,8 +612,6 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace, int virLockSpaceAcquireResource(virLockSpacePtr lockspace, const char *resname, pid_t owner, - off_t start, - off_t len, unsigned int flags) { int ret = -1; @@ -645,8 +641,7 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace, goto cleanup; } - if (!(res = virLockSpaceResourceNew(lockspace, resname, - owner, start, len, flags))) + if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner))) goto cleanup; if (virHashAddEntry(lockspace->resources, resname, res) < 0) { diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h index 24f2c89be6..041cf20396 100644 --- a/src/util/virlockspace.h +++ b/src/util/virlockspace.h @@ -22,8 +22,6 @@ #ifndef __VIR_LOCK_SPACE_H__ # define __VIR_LOCK_SPACE_H__ -# include <sys/types.h> - # include "internal.h" # include "virjson.h" @@ -52,8 +50,6 @@ typedef enum { int virLockSpaceAcquireResource(virLockSpacePtr lockspace, const char *resname, pid_t owner, - off_t start, - off_t len, unsigned int flags); int virLockSpaceReleaseResource(virLockSpacePtr lockspace, diff --git a/tests/virlockspacetest.c b/tests/virlockspacetest.c index 3c621e7eb0..93353be285 100644 --- a/tests/virlockspacetest.c +++ b/tests/virlockspacetest.c @@ -98,8 +98,6 @@ static int testLockSpaceResourceLockExcl(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; - const off_t start = 0; - const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -112,13 +110,13 @@ static int testLockSpaceResourceLockExcl(const void *args ATTRIBUTE_UNUSED) if (virLockSpaceCreateResource(lockspace, "foo") < 0) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), start, len, 0) < 0) + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) < 0) goto cleanup; if (!virFileExists(LOCKSPACE_DIR "/foo")) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), start, len, 0) == 0) + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0) goto cleanup; if (virLockSpaceDeleteResource(lockspace, "foo") == 0) @@ -146,8 +144,6 @@ static int testLockSpaceResourceLockExclAuto(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; - const off_t start = 0; - const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -161,7 +157,6 @@ static int testLockSpaceResourceLockExclAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), - start, len, VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) goto cleanup; @@ -187,8 +182,6 @@ static int testLockSpaceResourceLockShr(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; - const off_t start = 0; - const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -202,16 +195,13 @@ static int testLockSpaceResourceLockShr(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), - start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, "foo", - geteuid(), start, len, 0) == 0) + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), - start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0) goto cleanup; @@ -246,8 +236,6 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; - const off_t start = 0; - const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -261,7 +249,6 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), - start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED | VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) goto cleanup; @@ -270,7 +257,6 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), - start, len, VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) == 0) goto cleanup; @@ -278,7 +264,6 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), - start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED | VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) goto cleanup; @@ -311,8 +296,6 @@ static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; - const off_t start = 0; - const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -325,15 +308,13 @@ static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED) if (virLockSpaceCreateResource(lockspace, LOCKSPACE_DIR "/foo") < 0) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", - geteuid(), start, len, 0) < 0) + if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(), 0) < 0) goto cleanup; if (!virFileExists(LOCKSPACE_DIR "/foo")) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", - geteuid(), start, len, 0) == 0) + if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(), 0) == 0) goto cleanup; if (virLockSpaceDeleteResource(lockspace, LOCKSPACE_DIR "/foo") == 0) -- 2.18.1
On 11/14/18 7:44 AM, Michal Privoznik wrote:
v4 of:
https://www.redhat.com/archives/libvir-list/2018-October/msg00861.html
diff to v3: - Introduced a config knob to enable/disable metadata locking (except not really). We want to have a knob that enables/disables remembering of original owner. This knob in turn enables metadata locking. The reason is that metadata locking on its own doesn't make any sense. Anyway, the qemu.conf change is not done (it'll be done in upcoming patch set that implements original owner remembering), so if you want to see these patches in action you'll need to apply the following patch:
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 32da9a7351..0080b0d021 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -347,6 +347,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST))) goto error;
+ cfg->rememberOwner = true; + if (privileged && qemuDomainNamespaceAvailable(QEMU_DOMAIN_NS_MOUNT) && virBitmapSetBit(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT) < 0)
- I've fixed small issues raised in review of v3. Note that patches 01 and 02 are ACKed already but I'm sending them for completeness (probably doesn't make much sense to merge them while this is still under review, does it?).
Michal Prívozník (15): virprocess: Introduce virProcessRunInFork virprocess: Make virProcessRunInMountNamespace use virProcessRunInFork qemu_tpm: Pass virDomainObjPtr instead of virDomainDefPtr qemu_domain: Track if domain remembers original owner virSecurityManagerTransactionCommit: Do metadata locking iff enabled in config security_manager: Rework metadata locking Revert "security_manager: Load lock plugin on init" Revert "qemu_conf: Introduce metadata_lock_manager" Revert "lock_manager: Allow disabling configFile for virLockManagerPluginNew" Revert "lock_driver: Introduce VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK" Revert "lock_driver: Introduce VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA" Revert "_virLockManagerLockDaemonPrivate: Move @hasRWDisks into dom union" Revert "lock_driver: Introduce new VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON" Revert "lock_driver_lockd: Introduce VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA flag" Revert "virlockspace: Allow caller to specify start and length offset in virLockSpaceAcquireResource"
cfg.mk | 4 +- src/libvirt_private.syms | 1 + src/locking/lock_daemon_dispatch.c | 11 +- src/locking/lock_driver.h | 12 - src/locking/lock_driver_lockd.c | 421 ++++++++++------------------- src/locking/lock_driver_lockd.h | 1 - src/locking/lock_driver_sanlock.c | 44 +-- src/locking/lock_manager.c | 10 +- src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_conf.c | 1 - src/qemu/qemu_conf.h | 2 +- src/qemu/qemu_domain.c | 7 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 3 - src/qemu/qemu_extdevice.c | 16 +- src/qemu/qemu_extdevice.h | 4 +- src/qemu/qemu_process.c | 9 +- src/qemu/qemu_security.c | 87 ++++-- src/qemu/qemu_security.h | 4 +- src/qemu/qemu_tpm.c | 24 +- src/qemu/qemu_tpm.h | 4 +- src/security/security_dac.c | 54 ++-- src/security/security_driver.h | 3 +- src/security/security_manager.c | 259 +++++++++--------- src/security/security_manager.h | 22 +- src/security/security_selinux.c | 53 ++-- src/security/security_stack.c | 5 +- src/util/virlockspace.c | 15 +- src/util/virlockspace.h | 4 - src/util/virprocess.c | 82 ++++-- src/util/virprocess.h | 16 ++ tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- tests/securityselinuxtest.c | 2 +- tests/testutilsqemu.c | 2 +- tests/virlockspacetest.c | 29 +- 37 files changed, 573 insertions(+), 650 deletions(-)
Consider the "Revert" patches all : Reviewed-by: John Ferlan <jferlan@redhat.com> John I ran the series through my Coverity checker and it didn't find anything new
On 11/15/18 7:40 PM, John Ferlan wrote:
On 11/14/18 7:44 AM, Michal Privoznik wrote:
v4 of:
https://www.redhat.com/archives/libvir-list/2018-October/msg00861.html
diff to v3: - Introduced a config knob to enable/disable metadata locking (except not really). We want to have a knob that enables/disables remembering of original owner. This knob in turn enables metadata locking. The reason is that metadata locking on its own doesn't make any sense. Anyway, the qemu.conf change is not done (it'll be done in upcoming patch set that implements original owner remembering), so if you want to see these patches in action you'll need to apply the following patch:
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 32da9a7351..0080b0d021 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -347,6 +347,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST))) goto error;
+ cfg->rememberOwner = true; + if (privileged && qemuDomainNamespaceAvailable(QEMU_DOMAIN_NS_MOUNT) && virBitmapSetBit(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT) < 0)
- I've fixed small issues raised in review of v3. Note that patches 01 and 02 are ACKed already but I'm sending them for completeness (probably doesn't make much sense to merge them while this is still under review, does it?).
Michal Prívozník (15): virprocess: Introduce virProcessRunInFork virprocess: Make virProcessRunInMountNamespace use virProcessRunInFork qemu_tpm: Pass virDomainObjPtr instead of virDomainDefPtr qemu_domain: Track if domain remembers original owner virSecurityManagerTransactionCommit: Do metadata locking iff enabled in config security_manager: Rework metadata locking Revert "security_manager: Load lock plugin on init" Revert "qemu_conf: Introduce metadata_lock_manager" Revert "lock_manager: Allow disabling configFile for virLockManagerPluginNew" Revert "lock_driver: Introduce VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK" Revert "lock_driver: Introduce VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA" Revert "_virLockManagerLockDaemonPrivate: Move @hasRWDisks into dom union" Revert "lock_driver: Introduce new VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON" Revert "lock_driver_lockd: Introduce VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA flag" Revert "virlockspace: Allow caller to specify start and length offset in virLockSpaceAcquireResource"
cfg.mk | 4 +- src/libvirt_private.syms | 1 + src/locking/lock_daemon_dispatch.c | 11 +- src/locking/lock_driver.h | 12 - src/locking/lock_driver_lockd.c | 421 ++++++++++------------------- src/locking/lock_driver_lockd.h | 1 - src/locking/lock_driver_sanlock.c | 44 +-- src/locking/lock_manager.c | 10 +- src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_conf.c | 1 - src/qemu/qemu_conf.h | 2 +- src/qemu/qemu_domain.c | 7 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 3 - src/qemu/qemu_extdevice.c | 16 +- src/qemu/qemu_extdevice.h | 4 +- src/qemu/qemu_process.c | 9 +- src/qemu/qemu_security.c | 87 ++++-- src/qemu/qemu_security.h | 4 +- src/qemu/qemu_tpm.c | 24 +- src/qemu/qemu_tpm.h | 4 +- src/security/security_dac.c | 54 ++-- src/security/security_driver.h | 3 +- src/security/security_manager.c | 259 +++++++++--------- src/security/security_manager.h | 22 +- src/security/security_selinux.c | 53 ++-- src/security/security_stack.c | 5 +- src/util/virlockspace.c | 15 +- src/util/virlockspace.h | 4 - src/util/virprocess.c | 82 ++++-- src/util/virprocess.h | 16 ++ tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- tests/securityselinuxtest.c | 2 +- tests/testutilsqemu.c | 2 +- tests/virlockspacetest.c | 29 +- 37 files changed, 573 insertions(+), 650 deletions(-)
Consider the "Revert" patches all :
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
I ran the series through my Coverity checker and it didn't find anything new
Thank you for the review. I've pushed these. Michal
participants (2)
-
John Ferlan -
Michal Privoznik