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

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

> 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