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

[Guido Günther]

Hi,
On Fri, Apr 01, 2016 at 03:51:08PM +0200, Martin Kletzander wrote:
> On Fri, Apr 01, 2016 at 03:30:37PM +0200, Guido Günther wrote:
> >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
> >
> 
> That looks nicer, exactly what I had in mind.
> 
> >>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?
> >
> 
> Definitely a bug in the code.  I'll fix this (right after fixing another
> one for this release).  Thanks for spotting that out.  I'd say ACK for
> the second version if you're OK with only my ACK and a promise that I'll
> look into that aforementioned bug.

Pushed now to unbreak apparmor before 1.3.3. If Jamie or Cedric (or
anybody else caring about the apparmor suport) have any comments I'm
happy to rework this then.

Cheers and thanks,
 -- Guido

> 
> >Cheers,
> >-- Guido