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

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(a)sigxcpu.org&gt; >From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx(a)sigxcpu.org&gt; >Date: Thu, 31 Mar 2016 15:44:59 +0200 >Subject: [PATCH] apparmor: QEMU monitor socket moved >To: libvir-list(a)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.

>Cheers, >-- Guido