[libvirt] [PATCH 1/4] qemu: domain: gfx: Fix shadowing the ptr argument to graphics validation

[Erik Skultety]

Since the code was never run, this stupid mistake could have only been
spotted by an accident.

Signed-off-by: Erik Skultety <***@redhat.com>
---
src/qemu/qemu_domain.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 509da6bfea..1eb0e31df0 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5783,9 +5783,7 @@ qemuDomainDeviceDefValidateGraphics(const virDomainGraphicsDef *graphics,
size_t i;

for (i = 0; i < def->ngraphics; i++) {
- graphics = def->graphics[i];
-
- if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS) {
+ if (def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS) {
have_egl_headless = true;
break;
}

[Erik Skultety]

One of the usages of the device iterator is to run config validation.
That's a problem for graphics devices, because they don't have any @info
data (graphics shouldn't have been considered as devices in the first
place), and simply passing NULL would crash a few callbacks invoked from
the iterator. Fix this problem by introducing iterator flags.

Signed-off-by: Erik Skultety <***@redhat.com>
---
src/conf/domain_conf.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b70dca6c61..11552bff5b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3703,10 +3703,18 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
}


+typedef enum {
+ DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
+} virDomainDeviceInfoIterateFlags;
+
+/*
+ * Iterates over domain devices which provide virDomainDeviceInfo data. The
+ * default behaviour can be altered with virDomainDeviceInfoIterateFlags.
+ */
static int
virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
virDomainDeviceInfoCallback cb,
- bool all,
+ unsigned int iteratorFlags,
void *opaque)
{
size_t i;
@@ -3772,6 +3780,8 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
return rc;
}
for (i = 0; i < def->nconsoles; i++) {
+ bool all = iteratorFlags & DEVICE_INFO_ITERATE_ALL_CONSOLES;
+
if (virDomainSkipBackcompatConsole(def, i, all))
continue;
device.data.chr = def->consoles[i];
@@ -3908,7 +3918,7 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
virDomainDeviceInfoCallback cb,
void *opaque)
{
- return virDomainDeviceInfoIterateInternal(def, cb, false, opaque);
+ return virDomainDeviceInfoIterateInternal(def, cb, 0, opaque);
}


@@ -3918,7 +3928,7 @@ virDomainDefHasDeviceAddress(virDomainDefPtr def,
{
if (virDomainDeviceInfoIterateInternal(def,
virDomainDefHasDeviceAddressIterator,
- true,
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
info) < 0)
return true;

@@ -5291,7 +5301,7 @@ virDomainDefPostParse(virDomainDefPtr def,
/* iterate the devices */
ret = virDomainDeviceInfoIterateInternal(def,
virDomainDefPostParseDeviceIterator,
- true,
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
&data);

if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0)
@@ -5927,7 +5937,8 @@ virDomainDefValidateAliases(const virDomainDef *def,

if (virDomainDeviceInfoIterateInternal((virDomainDefPtr) def,
virDomainDeviceDefValidateAliasesIterator,
- true, &data) < 0)
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
+ &data) < 0)
goto cleanup;

if (aliases) {
@@ -6337,7 +6348,8 @@ virDomainDefValidate(virDomainDefPtr def,
/* iterate the devices */
if (virDomainDeviceInfoIterateInternal(def,
virDomainDefValidateDeviceIterator,
- true, &data) < 0)
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
+ &data) < 0)
return -1;

if (virDomainDefValidateInternal(def) < 0)
@@ -29926,7 +29938,8 @@ virDomainDefFindDevice(virDomainDefPtr def,

dev->type = VIR_DOMAIN_DEVICE_NONE;
virDomainDeviceInfoIterateInternal(def, virDomainDefFindDeviceCallback,
- true, &data);
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
+ &data);

if (dev->type == VIR_DOMAIN_DEVICE_NONE) {
if (reportError) {

[Erik Skultety]

The validation code for graphics has been in place for a while, but
because it is only executed from the device iterator, that validation
code was never truly run. The unfortunate side effect of this whole mess
was that a few capabilities were missing from the test suite, which in
turn meant that a few graphics test which expected a failure happily
accepted whatever failure the parser returned which made them succeed
even though in reality we allowed to start a domain with multiple
OpenGL-enabled graphics devices.
This patch enables iteration over graphics devices. Unsurprisingly,
a few tests started failing as a result, so fix those too.

Signed-off-by: Erik Skultety <***@redhat.com>
---
src/conf/domain_conf.c | 13 ++++++++++++-
tests/qemuxml2argvtest.c | 7 ++-----
tests/qemuxml2xmltest.c | 10 +++++++---
3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 11552bff5b..a4c762a210 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3705,6 +3705,7 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,

typedef enum {
DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
+ DEVICE_INFO_ITERATE_GRAPHICS = 1 << 1 /* Iterate graphics */
} virDomainDeviceInfoIterateFlags;

/*
@@ -3870,6 +3871,15 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
return rc;
}

+ if (iteratorFlags & DEVICE_INFO_ITERATE_GRAPHICS) {
+ device.type = VIR_DOMAIN_DEVICE_GRAPHICS;
+ for (i = 0; i < def->ngraphics; i++) {
+ device.data.graphics = def->graphics[i];
+ if ((rc = cb(def, &device, NULL, opaque)) != 0)
+ return rc;
+ }
+ }
+
/* Coverity is not very happy with this - all dead_error_condition */
#if !STATIC_ANALYSIS
/* This switch statement is here to trigger compiler warning when adding
@@ -6348,7 +6358,8 @@ virDomainDefValidate(virDomainDefPtr def,
/* iterate the devices */
if (virDomainDeviceInfoIterateInternal(def,
virDomainDefValidateDeviceIterator,
- DEVICE_INFO_ITERATE_ALL_CONSOLES,
+ (DEVICE_INFO_ITERATE_ALL_CONSOLES |
+ DEVICE_INFO_ITERATE_GRAPHICS),
&data) < 0)
return -1;

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 528139654c..05451863ad 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1299,7 +1299,7 @@ mymain(void)

DO_TEST("graphics-sdl",
QEMU_CAPS_DEVICE_VGA);
- DO_TEST_FAILURE("graphics-sdl-egl-headless", NONE);
+ DO_TEST_PARSE_ERROR_CAPS_LATEST("graphics-sdl-egl-headless");
DO_TEST("graphics-sdl-fullscreen",
QEMU_CAPS_DEVICE_CIRRUS_VGA);
DO_TEST("graphics-spice",
@@ -1358,10 +1358,7 @@ mymain(void)
QEMU_CAPS_SPICE,
QEMU_CAPS_EGL_HEADLESS,
QEMU_CAPS_DEVICE_QXL);
- DO_TEST_FAILURE("graphics-spice-invalid-egl-headless",
- QEMU_CAPS_SPICE,
- QEMU_CAPS_EGL_HEADLESS,
- QEMU_CAPS_DEVICE_QXL);
+ DO_TEST_PARSE_ERROR_CAPS_LATEST("graphics-spice-invalid-egl-headless");
DO_TEST_CAPS_LATEST("graphics-spice-gl-auto-rendernode");

DO_TEST("input-usbmouse", NONE);
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index c98b9571ef..1062deee37 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -402,7 +402,8 @@ mymain(void)
cfg->vncAutoUnixSocket = false;
DO_TEST("graphics-vnc-socket", NONE);
DO_TEST("graphics-vnc-auto-socket", NONE);
- DO_TEST("graphics-vnc-egl-headless", NONE);
+ DO_TEST("graphics-vnc-egl-headless",
+ QEMU_CAPS_EGL_HEADLESS);

DO_TEST("graphics-sdl", NONE);
DO_TEST("graphics-sdl-fullscreen", NONE);
@@ -414,9 +415,12 @@ mymain(void)
cfg->spiceAutoUnixSocket = true;
DO_TEST("graphics-spice-auto-socket-cfg", NONE);
cfg->spiceAutoUnixSocket = false;
- DO_TEST("graphics-spice-egl-headless", NONE);
+ DO_TEST("graphics-spice-egl-headless",
+ QEMU_CAPS_EGL_HEADLESS);

- DO_TEST("graphics-egl-headless-rendernode", NONE);
+ DO_TEST("graphics-egl-headless-rendernode",
+ QEMU_CAPS_EGL_HEADLESS,
+ QEMU_CAPS_EGL_HEADLESS_RENDERNODE);

DO_TEST("input-usbmouse", NONE);
DO_TEST("input-usbtablet", NONE);

[Erik Skultety]

As commit d8266ebe161 demonstrated, it's so easy to forget to add a
single capability which in turn can easily fool the test suite so that
tests expecting a failure can fail with a different error than we
expected, but still making those pass. Unfortunately for commit
d8266ebe161, it allowed a domain with multiple OpenGL-enabled graphics
devices to start successfully. As a precaution measure, introduce
negative versions of DO_TEST_CAPS_LATEST macros, so that we eliminate
this vector from now on.

Signed-off-by: Erik Skultety <***@redhat.com>
---
tests/qemuxml2argvtest.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index e17709e7e1..528139654c 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -806,13 +806,22 @@ mymain(void)
# define DO_TEST_CAPS_VER(name, ver) \
DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver)

-# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
- DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, 0, 0, arch, \
+# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, flags, parseFlags) \
+ DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, flags, parseFlags, arch, \
virHashLookup(capslatest, arch), true)

+# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, 0, 0) \
+
# define DO_TEST_CAPS_LATEST(name) \
DO_TEST_CAPS_ARCH_LATEST(name, "x86_64")

+# define DO_TEST_FAILURE_CAPS_LATEST(name) \
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_FAILURE, 0)
+
+# define DO_TEST_PARSE_ERROR_CAPS_LATEST(name) \
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_PARSE_ERROR, 0)
+
/**
* The following test macros should be used only in cases when the tests require
* testing of some non-standard combination of capability flags