[libvirt] [PATCH 05/10] qemu_security: Use more transactions
Martin Kletzander
mkletzan at redhat.com
Tue Feb 7 11:05:43 UTC 2017
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 at redhat.com>
>>> ---
>>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170207/6e7c947c/attachment-0001.sig>
More information about the libvir-list
mailing list