On Thu, Jan 17, 2019 at 11:05 AM Erik Skultety <eskultet@xxxxxxxxxx> wrote: > > On Tue, Jan 15, 2019 at 06:34:41PM +0200, Christian Ehrhardt wrote: > > This adds the virt-aa-helper support for gl enabled graphics devices to > > generate rules for the needed rendernode paths. > > > > Example in domain xml: > > <graphics type='spice'> > > <gl enable='yes' rendernode='/dev/dri/bar'/> > > </graphics> > > > > results in: > > "/dev/dri" r, > > "/dev/dri/bar" rw, > > > > Special cases are: > > - multiple devices with rendernodes -> all are added > > - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode > > - rendernode without opengl (in egl-headless for example) -> still add > > the node > > > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085 > > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx> > > --- > > src/security/virt-aa-helper.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > > index 64a425671d..327a8a0202 100644 > > --- a/src/security/virt-aa-helper.c > > +++ b/src/security/virt-aa-helper.c > > @@ -938,7 +938,7 @@ get_files(vahControl * ctl) > > size_t i; > > char *uuid; > > char uuidstr[VIR_UUID_STRING_BUFLEN]; > > - bool needsVfio = false, needsvhost = false; > > + bool needsVfio = false, needsvhost = false, needDefaultRenderNode = false; > > > > /* verify uuid is same as what we were given on the command line */ > > virUUIDFormat(ctl->def->uuid, uuidstr); > > @@ -1062,6 +1062,10 @@ get_files(vahControl * ctl) > > for (i = 0; i < ctl->def->ngraphics; i++) { > > virDomainGraphicsDefPtr graphics = ctl->def->graphics[i]; > > size_t n; > > + const char *rendernode = virDomainGraphicsGetRenderNode(graphics); > > + > > + if (rendernode) > > + vah_add_file(&buf, rendernode, "rw"); > > > > for (n = 0; n < graphics->nListens; n++) { > > virDomainGraphicsListenDef listenObj = graphics->listens[n]; > > @@ -1071,6 +1075,20 @@ get_files(vahControl * ctl) > > vah_add_file(&buf, listenObj.socket, "rw")) > > goto cleanup; > > } > > + > > + if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES) { > > + if (!rendernode) > > + needDefaultRenderNode = true; > > + } > > ^This feels a bit detached from the hunk above. Agreed, better grouped in an upcoming V2 > Also, what about EGL_HEADLESS, > you will want to add (potentially a default one) rendernode for egl_headless > too. Yes I'd want EGL Headless as well, using the suggested virDomainGraphicsNeedsAutoRenderNode to cover both. As it does the same checks, but better - thanks for the hint! > You could also optimize this with > virDomainGraphicsNeedsAutoRenderNode and cover both graphics types. > > > + } > > + > > + if (virDomainGraphicsDefHasOpenGL(ctl->def)) > > + vah_add_file(&buf, "/dev/dri", "r"); > > Why is ^this needed? I started on this before the virDomainGraphics helpers and before you changed it to always pass a defined value. Back then qemu then would have enumerated /dev/dri, but you are right we - no more - need the base dir since we pass an explicit value. In case I later on find cases this still triggers I can come back with a small patch. > > + > > + if (needDefaultRenderNode) { > > + const char *rendernode = virHostGetDRMRenderNode(); > > + if (rendernode) > > + vah_add_file(&buf, virHostGetDRMRenderNode(), "rw"); > > IUC the virDomainGraphicsNeedsAutoRenderNode part would come ^here. Yeah, since we already iterate on "graphics" elements above and there is no drawback in adding the default virHostGetDRMRenderNode path twice (would only happen in awkward guest XMLs anyway) this can be simplified even further. it essentially becomes: "if it has explicit node add it, otherwise if it needs a rendernode at all add the default" > > I also agree with Michal's suggestion. > > Erik -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
