Re: [libvirt] [PATCH 05/10] qemu_security: Use more transactions

On 02/02/2017 04:03 PM, Martin Kletzander wrote: > On Fri, Jan 20, 2017 at 10:42:45AM +0100, Michal Privoznik wrote: >> The idea is to move all the seclabel setting to security driver. >> Having the relabel code spread all over the place looks very >> messy. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/qemu/qemu_security.c | 112 >> +++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 80 insertions(+), 32 deletions(-) >> >> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c >> index 13d99cdbd..9d1a87971 100644 >> --- a/src/qemu/qemu_security.c >> +++ b/src/qemu/qemu_security.c >> @@ -90,14 +90,26 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, >> virDomainObjPtr vm, >> virDomainDiskDefPtr disk) >> { >> - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { >> - /* Already handled by namespace code. */ >> - return 0; >> - } >> + int ret = -1; >> >> - return virSecurityManagerSetDiskLabel(driver->securityManager, >> + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && >> + virSecurityManagerTransactionStart(driver->securityManager) < 0) >> + goto cleanup; >> + >> + if (virSecurityManagerSetDiskLabel(driver->securityManager, >> vm->def, >> - disk); >> + disk) < 0) > > indentation > >> + goto cleanup; >> + >> + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && >> + virSecurityManagerTransactionCommit(driver->securityManager, >> + vm->pid) < 0) >> + goto cleanup; >> + >> + ret = 0; >> + cleanup: >> + virSecurityManagerTransactionAbort(driver->securityManager); >> + return ret; >> } >> > > So much code duplication. I have a patch ready that kills that and replaces it with a macro. > Wasn't there an idea to have a callback in > the security manager that would be called before/after the labelling? The problem is that security driver sees just virDomainDef while all the namespace stuff (e.g. pid, enabled namespace bitmap) lives in a domain object. Therefore security driver can't make such decision on its own. Thus transactions were born. Having a chown callback that would enter the namespace and change ownership proved to be very suboptimal: entering a namespace requires a fork(). Therefore, instead of forking like crazy, a list of all the desired chown()-s is constructed, one fork() is called and then all the operations from the list are performed. > Since this is only for a disk and hostdev, let's leave it like this, I > guess, but I'm expecting this much code duplication to bite us back in a > while. None of my ideas for how to make this better are finalized, but > I have some, if you want to discuss. Sure. I'm up for making this better.

Michal