Up until now, we formatted 'rendernode=' onto QEMU cmdline only if the
user specified it in the XML, otherwise we let QEMU do it for us. This
causes permission issues because by default the /dev/dri/renderDX
permissions are as follows:

crw-rw----. 1 root video

There's literally no reason why it shouldn't be libvirt picking the DRM
render node instead of QEMU, that way (and because we're using
namespaces by default), we can safely relabel the device within the
namespace.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/qemu/qemu_process.c                       | 22 +++++++++++++++-
 src/util/virutil.h                            |  2 +-
 .../graphics-spice-gl-no-rendernode.args      | 25 +++++++++++++++++++
 .../graphics-spice-gl-no-rendernode.xml       | 24 ++++++++++++++++++
 tests/qemuxml2argvmock.c                      |  9 +++++++
 5 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args
 create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.xml

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 12d1fca0d4..feebdc7fdc 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4784,9 +4784,25 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver,
 }
 
 
+static int
+qemuProcessGraphicsSetupRenderNode(virDomainGraphicsDefPtr graphics,
+                                   virQEMUCapsPtr qemuCaps)
+{
+    /* Don't bother picking a DRM node if QEMU doesn't support it. */
+    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE))
+        return 0;
+
+    if (!(graphics->data.spice.rendernode = virHostGetDRMRenderNode()))
+        return -1;
+
+    return 0;
+}
+
+
 static int
 qemuProcessSetupGraphics(virQEMUDriverPtr driver,
                          virDomainObjPtr vm,
+                         virQEMUCapsPtr qemuCaps,
                          unsigned int flags)
 {
     virDomainGraphicsDefPtr graphics;
@@ -4797,6 +4813,10 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
     for (i = 0; i < vm->def->ngraphics; i++) {
         graphics = vm->def->graphics[i];
 
+        if (virDomainGraphicsNeedsRenderNode(graphics) &&
+            qemuProcessGraphicsSetupRenderNode(graphics, qemuCaps) < 0)
+            goto cleanup;
+
         if (qemuProcessGraphicsSetupListen(driver, graphics, vm) < 0)
             goto cleanup;
     }
@@ -5953,7 +5973,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
         goto cleanup;
 
     VIR_DEBUG("Setting graphics devices");
-    if (qemuProcessSetupGraphics(driver, vm, flags) < 0)
+    if (qemuProcessSetupGraphics(driver, vm, priv->qemuCaps, flags) < 0)
         goto cleanup;
 
     VIR_DEBUG("Create domain masterKey");
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 89bd21b148..588d779d10 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -222,7 +222,7 @@ unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE;
 
 bool virHostHasIOMMU(void);
 
-char *virHostGetDRMRenderNode(void);
+char *virHostGetDRMRenderNode(void) ATTRIBUTE_NOINLINE;
 
 /**
  * VIR_ASSIGN_IS_OVERFLOW:
diff --git a/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args
new file mode 100644
index 0000000000..1b08811692
--- /dev/null
+++ b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args
@@ -0,0 +1,25 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=spice \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-usb \
+-spice port=0,gl=on,rendernode=/dev/dri/foo \
+-vga cirrus \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.xml b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.xml
new file mode 100644
index 0000000000..b48e7bc94e
--- /dev/null
+++ b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.xml
@@ -0,0 +1,24 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219136</memory>
+  <currentMemory unit='KiB'>219136</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='i686' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-i686</emulator>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <graphics type='spice' autoport='no'>
+      <gl enable='yes'/>
+    </graphics>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index 79152d928e..a64cd955c4 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -184,6 +184,15 @@ virNetDevRunEthernetScript(const char *ifname ATTRIBUTE_UNUSED,
     return 0;
 }
 
+char *
+virHostGetDRMRenderNode(void)
+{
+    char *dst = NULL;
+
+    ignore_value(VIR_STRDUP(dst, "/dev/dri/foo"));
+    return dst;
+}
+
 static void (*real_virCommandPassFD)(virCommandPtr cmd, int fd, unsigned int flags);
 
 static const int testCommandPassSafeFDs[] = { 1730, 1731 };
-- 
2.19.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Nov 29, 2018 at 03:20:13PM +0100, Erik Skultety wrote:
>Up until now, we formatted 'rendernode=' onto QEMU cmdline only if the
>user specified it in the XML, otherwise we let QEMU do it for us. This
>causes permission issues because by default the /dev/dri/renderDX
>permissions are as follows:
>
>crw-rw----. 1 root video
>
>There's literally no reason why it shouldn't be libvirt picking the DRM
>render node instead of QEMU, that way (and because we're using
>namespaces by default), we can safely relabel the device within the
>namespace.
>
>Signed-off-by: Erik Skultety <eskultet@redhat.com>
>---
> src/qemu/qemu_process.c                       | 22 +++++++++++++++-
> src/util/virutil.h                            |  2 +-
> .../graphics-spice-gl-no-rendernode.args      | 25 +++++++++++++++++++
> .../graphics-spice-gl-no-rendernode.xml       | 24 ++++++++++++++++++
> tests/qemuxml2argvmock.c                      |  9 +++++++
> 5 files changed, 80 insertions(+), 2 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args
> create mode 100644 tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.xml
>
>diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>index 12d1fca0d4..feebdc7fdc 100644
>--- a/src/qemu/qemu_process.c
>+++ b/src/qemu/qemu_process.c
>@@ -4784,9 +4784,25 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver,
> }
>
>
>+static int
>+qemuProcessGraphicsSetupRenderNode(virDomainGraphicsDefPtr graphics,
>+                                   virQEMUCapsPtr qemuCaps)
>+{
>+    /* Don't bother picking a DRM node if QEMU doesn't support it. */
>+    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE))
>+        return 0;
>+
>+    if (!(graphics->data.spice.rendernode = virHostGetDRMRenderNode()))
>+        return -1;
>+
>+    return 0;
>+}
>+
>+
> static int
> qemuProcessSetupGraphics(virQEMUDriverPtr driver,
>                          virDomainObjPtr vm,
>+                         virQEMUCapsPtr qemuCaps,
>                          unsigned int flags)
> {
>     virDomainGraphicsDefPtr graphics;
>@@ -4797,6 +4813,10 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
>     for (i = 0; i < vm->def->ngraphics; i++) {
>         graphics = vm->def->graphics[i];
>
>+        if (virDomainGraphicsNeedsRenderNode(graphics) &&

Just like the call below is called even for graphics with no listen,
you can call qemuProcessGraphicsSetupRenderNode unconditionally and move
the virDomainGraphicsNeedsRenderNode condition inside.

>+            qemuProcessGraphicsSetupRenderNode(graphics, qemuCaps) < 0)
>+            goto cleanup;
>+
>         if (qemuProcessGraphicsSetupListen(driver, graphics, vm) < 0)
>             goto cleanup;
>     }
>@@ -5953,7 +5973,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
>         goto cleanup;
>
>     VIR_DEBUG("Setting graphics devices");
>-    if (qemuProcessSetupGraphics(driver, vm, flags) < 0)
>+    if (qemuProcessSetupGraphics(driver, vm, priv->qemuCaps, flags) < 0)
>         goto cleanup;
>
>     VIR_DEBUG("Create domain masterKey");
>diff --git a/src/util/virutil.h b/src/util/virutil.h
>index 89bd21b148..588d779d10 100644
>--- a/src/util/virutil.h
>+++ b/src/util/virutil.h
>@@ -222,7 +222,7 @@ unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE;
>
> bool virHostHasIOMMU(void);
>
>-char *virHostGetDRMRenderNode(void);
>+char *virHostGetDRMRenderNode(void) ATTRIBUTE_NOINLINE;
>
> /**
>  * VIR_ASSIGN_IS_OVERFLOW:
>diff --git a/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args
>new file mode 100644
>index 0000000000..1b08811692
>--- /dev/null
>+++ b/tests/qemuxml2argvdata/graphics-spice-gl-no-rendernode.args

A test called '-no-rendernode'...

[...]

>+-rtc base=utc \
>+-no-shutdown \
>+-no-acpi \
>+-usb \
>+-spice port=0,gl=on,rendernode=/dev/dri/foo \

... with a rendernode on the command line.

How about '-auto-rendernode'?

>+-vga cirrus \
>+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3

The test cases are not used anywhere. Please use DO_TEST_CAPS_LATEST
when adding them.

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list