[libvirt] [PATCH] apparmor: QEMU monitor socket moved

[Guido Günther]

Hi Martin,
On Fri, Apr 01, 2016 at 01:11:21PM +0200, Martin Kletzander wrote:
> On Thu, Mar 31, 2016 at 05:00:09PM +0200, Guido Günther wrote:
> >The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b.
> >---
> >src/security/virt-aa-helper.c | 2 ++
> >1 file changed, 2 insertions(+)
> >
> >diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> >index a2d7226..0ded671 100644
> >--- a/src/security/virt-aa-helper.c
> >+++ b/src/security/virt-aa-helper.c
> >@@ -1366,6 +1366,8 @@ main(int argc, char **argv)
> >                                  LOCALSTATEDIR, ctl->def->name);
> >                virBufferAsprintf(&buf, "  \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n",
> >                                  LOCALSTATEDIR, ctl->def->name);
> >+                virBufferAsprintf(&buf, "  \"%s/lib/libvirt/qemu/domain-*-%.*s/monitor.sock\" rw,\n",
> 
> Shouldn't this be domain-%d-... with the %d being ctl->def->id?  Or is
> it not known at this point?  Then I think it should allow only numbers
> between the dashes.  If that's possible.

It is if we change virt-aa-helper slightly to not only parse
VIR_DOMAIN_DEF_PARSE_INACTIVE:

>From 13a93c59af04785317d3e33b4f5c308cf4c9c3de Mon Sep 17 00:00:00 2001
Message-Id: <13a93c59af04785317d3e33b4f5c308cf4c9c3de.1459516663.git.agx at sigxcpu.org>
From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx at sigxcpu.org>
Date: Thu, 31 Mar 2016 15:44:59 +0200
Subject: [PATCH] apparmor: QEMU monitor socket moved
To: libvir-list at redhat.com

The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b.

This unbreaks launching QEMU/KVM VMs with apparmor enabled. It also adds
the directory for the qemu guest-agent socket which is not known when
parsing the domain XML.
---
 src/security/virt-aa-helper.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index a2d7226..50d2a08 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -762,8 +762,8 @@ get_definition(vahControl * ctl, const char *xmlStr)
     }
 
     ctl->def = virDomainDefParseString(xmlStr,
-                                       ctl->caps, ctl->xmlopt,
-                                       VIR_DOMAIN_DEF_PARSE_INACTIVE);
+                                       ctl->caps, ctl->xmlopt, 0);
+
     if (ctl->def == NULL) {
         vah_error(ctl, 0, _("could not parse XML"));
         goto exit;
@@ -1366,6 +1366,10 @@ main(int argc, char **argv)
                                   LOCALSTATEDIR, ctl->def->name);
                 virBufferAsprintf(&buf, "  \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n",
                                   LOCALSTATEDIR, ctl->def->name);
+                virBufferAsprintf(&buf, "  \"%s/lib/libvirt/qemu/domain-%d-%.*s/*\" rw,\n",
+                                  LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name);
+                virBufferAsprintf(&buf, "  \"%s/lib/libvirt/qemu/channel/target/domain-%d-%.*s/*\" rw,\n",
+                                  LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name);
                 virBufferAsprintf(&buf, "  \"%s/run/libvirt/**/%s.pid\" rwk,\n",
                                   LOCALSTATEDIR, ctl->def->name);
                 virBufferAsprintf(&buf, "  \"/run/libvirt/**/%s.pid\" rwk,\n",
-- 
2.8.0.rc3

> Another question, though: shouldn't there be also vnc.sock in case that
> is enabled?  Basically we create this (and the
> qemu/channel/target/domain-...) directory just for that particular
> domain, so it should have access to the whole directory.  Also the
> channel/target one, I believe.  Or did I miss something?

I added the channel/target one as well since the socket path does not
seem to be available when we parse the XML. Since the directories are
domain specific I went for allowing access to the whole dir.

While the above works as expected on the _first_ start of a domain it
fails on the second start since (at least in 1.3.3~rc1) we don't change
the domain id in the directory names on subsequent starts:

First start:
    # virsh list
    Id    Name                           State
    ----------------------------------------------------
    1     jessie                         running
    # find  /var/lib/libvirt/ -name 'domain-*-jessie'
    /var/lib/libvirt/qemu/channel/target/domain-1-jessie
    /var/lib/libvirt/qemu/domain-1-jessie

Second start:
    # virsh list
    Id    Name                           State
    ----------------------------------------------------
    2     jessie                         running
    # find  /var/lib/libvirt/ -name 'domain-*-jessie'
    /var/lib/libvirt/qemu/channel/target/domain-1-jessie
    /var/lib/libvirt/qemu/domain-1-jessie

Shouldn't that be:
    /var/lib/libvirt/qemu/channel/target/domain-2-jessie
    /var/lib/libvirt/qemu/domain-2-jessie

Is this on purpose or rather a bug in the code that generates the socket
paths?

Cheers,
 -- Guido