[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]