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(a)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