On Tue, Feb 07, 2017 at 11:02:09AM +0100, Michal Privoznik wrote:
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@xxxxxxxxxx> --- 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.
When thinking about them after a while none of them are very clean. Let's see how it looks with your macros for now.
Michal
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
