On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:
> On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
>> Fortunately, we have qemu wrappers so it's sufficient to put
>> lock/unlock call only there.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>> src/qemu/qemu_security.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 107 insertions(+)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index af3be42854..527563947c 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -26,6 +26,7 @@
>> #include "qemu_domain.h"
>> #include "qemu_security.h"
>> #include "virlog.h"
>> +#include "locking/domain_lock.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_QEMU
>>
>> @@ -39,6 +40,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>> {
>> int ret = -1;
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> + bool locked = false;
>> +
>> + if (virDomainLockMetadataLock(driver->lockManager, vm) < 0)
>> + goto cleanup;
>> +
>> + locked = true;
>>
>> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> virSecurityManagerTransactionStart(driver->securityManager) < 0)
>> @@ -55,9 +62,17 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>> vm->pid) < 0)
>> goto cleanup;
>>
>> + locked = false;
>> +
>> + if (virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
>> + goto cleanup;
>> +
>> ret = 0;
>> cleanup:
>> virSecurityManagerTransactionAbort(driver->securityManager);
>> + if (locked &&
>> + virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
>> + VIR_WARN("unable to release metadata lock");
>> return ret;
>> }
>
> I'm wondering if this is really the right level to plug in the metadata
> locking ? What about if we just pass a virLockManagerPtr to the security
> drivers and let them lock each resource at the time they need to modify
> its metadata. This will trivially guarantee that we always lock the exact
> set of files that we'll be changing metadata on.
>
> eg in SELinux driver the virSecuritySELinuxSetFileconHelper method
> would directly call virLockManagerAcquire & virLockManagerRelease,
> avoiding the entire virDomainLock abstraction which was really
> focused around managing the content locks around lifecycle state
> changes.
>
Yeah, I vaguely recall writing code like this (when I was trying to
solve this some time ago). Okay, let me see if that's feasible.
But boy, this is getting hairy.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
