[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