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.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
