On 12/7/18 9:47 AM, Erik Skultety wrote:
> 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
dang confusing postparse and validation processing... when you say
"device iterator" you meant the hypervisor specific device iterator,
right? That is, what's called by deviceValidateCallback or the call
into qemuDomainDeviceDefValidateGraphics. I'm just trying to follow the
wording, nothing more, nothing less.
> 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
graphics tests (the s on test)
> 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.
add blank line between paragraphs
> 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 <eskultet@xxxxxxxxxx>
> ---
> 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)
So the only caller to this passes cb=virDomainDefValidateDeviceIterator
which ironically doesn't use @info (e.g. the 3rd param):
virDomainDefValidateDeviceIterator(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
virDomainDeviceInfoPtr info
ATTRIBUTE_UNUSED,
void *opaque)
I know these are generic functions, but some day if someone changes that
helper to "use" @info can I assume that some test would fail in a most
spectacular way?
Not a problem here, just looking to confirm my own feeling on this with
what you'd expect. You may want to even add a comment noting that for a
graphics device the @info isn't used (remember my patch2 comment), so
one has to be careful which callers can set the iterate flag that dumps
you here. Say nothing for the called function - if it expects something
and gets NULL, all bets are off. I can assume it wouldn't be picked up
by the compiler's ATTRIBUTE_NONNULL too unless the prototype for the @cb
was set to have that.
> + 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");
We're changing from a general execution failure to a parse failure, so
theoretically we could not have started a guest like this, right? If we
could have then I'd be concerned with the guest disappearance factor on
libvirtd restart, but I don't think that's the case here. Still I go
back to my comment at the top dang confusing postparse and validation
processing, so I just want to make sure...
If my assumption is right about not being able to start like this, then
with a couple of small comment fixups in then you have my
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
otherwise, some more thought may be necessary.
John
> 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);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
