[libvirt] [PATCH 0/4] Few more graphics-related fixes
So, this series fixes a few issues that are very tightly coupled, so buckle up if you want to read further, if not, here you go: TL;DR: Issue: there's a test passing even though the code doesn't really work Result: fix both the code and the test LONG STORY: The main issue is that we allow starting a domain with multiple OpenGL-enabled graphics devices: <graphics type='spice'> <listen type='none'/> <gl enable='yes' rendernode='foo'/> </graphics> <graphics type='egl-headless'/> But hey, we have both a validation code handling this and a test case and everything seems to work. Well, not really, since the validation code for graphics was never run because it runs as a callback from our device iterator which only iterates over devices which provide boot info data which graphics do not provide. Any attempt to pass some arbitrary boot info data for graphics will cause troubles in other callbacks that are invoked from within the iterator. The way to enable it is to introduce some flags to alter the behaviour of the iterator. So why didn't we this scenario? We did, but the test case was lacking some capabilities and since it's a negative test case, it happily accepted any kind of error from the library, therefore, negative versions of DO_TEST_CAPS_LATEST have been introduced to mitigate failures like these in the future. Erik
Since the code was never run, this stupid mistake could have only been spotted by an accident. Signed-off-by: Erik Skultety <eskultet@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; } -- 2.19.2
On 12/7/18 9:47 AM, Erik Skultety wrote:
Since the code was never run, this stupid mistake could have only been spotted by an accident.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Gah - that wasn't obvious... and it's scary the compiler was silent. Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Tue, Dec 11, 2018 at 06:48:26PM -0500, John Ferlan wrote:
On 12/7/18 9:47 AM, Erik Skultety wrote:
Since the code was never run, this stupid mistake could have only been spotted by an accident.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Gah - that wasn't obvious... and it's scary the compiler was silent.
Indeed it is. I'm just wondering when would I actually want to use this legitimately since it's perfectly fine to assign into a "copy" pointer from the standard's point of view. Thanks, Erik
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 <eskultet@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) { -- 2.19.2
On 12/7/18 9:47 AM, Erik Skultety wrote:
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.
Somewhat confusing commit message or I'm just being dense, your choice. Although I think that @info data for graphics devices could be moved into patch4 since that's where it makes more sense (eventually at least).
Signed-off-by: Erik Skultety <eskultet@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; +
Logic seems right vis-a-vis replacing a bool with a flag properly. I don't get the name and comment, but I'm not sure I could name it better. The called function with the @all flag only processes when @all == false as I'm reading things. Anyway, if someone has a better idea, then they should speak up before you push! Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Tue, Dec 11, 2018 at 06:50:19PM -0500, John Ferlan wrote:
On 12/7/18 9:47 AM, Erik Skultety wrote:
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.
Somewhat confusing commit message or I'm just being dense, your choice.
I see, especially the mention of graphics devices in a patch where there's nothing related, how about this: Validation of domain devices is accomplished via a generic device iterator which takes a callback, iterates over all kinds of supported device types and invokes the callback on every single device. However, there might be cases when we need to alter the behaviour of the iteration (most notably skip or include a group of devices). Therefore, this patch introduces iterator flags.
Although I think that @info data for graphics devices could be moved into patch4 since that's where it makes more sense (eventually at least).
Signed-off-by: Erik Skultety <eskultet@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; +
Logic seems right vis-a-vis replacing a bool with a flag properly. I don't get the name and comment, but I'm not sure I could name it better.
The iterator function itself doesn't have a perfect name, I was just trying to use that name to derive a new one to reflect the relation, but I can use virDomainDeviceIteratorFlags instead. Going one step further we might even want to rename the iterator and drop the "Info" part since that won't be 100% once I enable it for graphics.
The called function with the @all flag only processes when @all == false
So, because console[0] corresponds to the first serial console, we often special-case these (in general). The @all flag here just says whether the processing callback should skip console[0] or not, if @all == false, then virDomainSkipBackcompatConsole will return true, thus skipping invocation of the callback on this device.
as I'm reading things. Anyway, if someone has a better idea, then they should speak up before you push!
Let me know whether the suggested update to the commit message along with the changes to naming are okay and I'll proceed with merging the patch. Thanks, Erik
On 12/12/18 3:29 AM, Erik Skultety wrote:
On Tue, Dec 11, 2018 at 06:50:19PM -0500, John Ferlan wrote:
On 12/7/18 9:47 AM, Erik Skultety wrote:
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.
Somewhat confusing commit message or I'm just being dense, your choice.
I see, especially the mention of graphics devices in a patch where there's nothing related, how about this:
Validation of domain devices is accomplished via a generic device iterator which takes a callback, iterates over all kinds of supported device types and invokes the callback on every single device. However, there might be cases when we need to alter the behaviour of the iteration (most notably skip or include a group of devices). Therefore, this patch introduces iterator flags.
That's fine.
Although I think that @info data for graphics devices could be moved into patch4 since that's where it makes more sense (eventually at least).
Signed-off-by: Erik Skultety <eskultet@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; +
Logic seems right vis-a-vis replacing a bool with a flag properly. I don't get the name and comment, but I'm not sure I could name it better.
The iterator function itself doesn't have a perfect name, I was just trying to use that name to derive a new one to reflect the relation, but I can use virDomainDeviceIteratorFlags instead. Going one step further we might even want to rename the iterator and drop the "Info" part since that won't be 100% once I enable it for graphics.
<facepalm> I meant just the flag name itself - the typedef name is just that and used only once, so no big deal.
The called function with the @all flag only processes when @all == false
So, because console[0] corresponds to the first serial console, we often special-case these (in general). The @all flag here just says whether the processing callback should skip console[0] or not, if @all == false, then virDomainSkipBackcompatConsole will return true, thus skipping invocation of the callback on this device.
Yeah I recall the [0] console entry being quite special with the details being found in various places in the code. In any case, if @all == false, then true is only returned if other conditions are met too such as using/checking the [0]th entry of the consoles list. When @all == true, then false is returned and that's the "more normal" case as prior to this patch @all was true for all except when virDomainDeviceInfoIterate calls virDomainDeviceInfoIterateInternal
as I'm reading things. Anyway, if someone has a better idea, then they should speak up before you push!
Let me know whether the suggested update to the commit message along with the changes to naming are okay and I'll proceed with merging the patch.
Sure things are fine John
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 <eskultet@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 -- 2.19.2
On 12/7/18 9:47 AM, Erik Skultety wrote:
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
precautionary
negative versions of DO_TEST_CAPS_LATEST macros, so that we eliminate this vector from now on.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tests/qemuxml2argvtest.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
Pulling the needle out of the haystack of d8266ebe1 which of the 3 new tests in qemuxml2argvtest was bad? None used the TEST_CAPS_LATEST nomenclature and you didn't change the test in this patch, so I'm left wondering. Of course, patch4 has the answer. Nothing major here, but understand coming in cold on this and reading the commit message is, well, confusing. I think it's simple enough to indicate you are introducing the two new macros to allow for test and parse failure checking. The "details" about the previous commit could be moved into patch 4 I suppose to show what you're fixing since you're not changing a test here.
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) \
There should not be a \ at the end of the previous line. Perhaps a holdover from the copy-pasta with the 'virHashLookup...' line.
+ # define DO_TEST_CAPS_LATEST(name) \ DO_TEST_CAPS_ARCH_LATEST(name, "x86_64")
+# define DO_TEST_FAILURE_CAPS_LATEST(name) \
To follow DO_TEST_CAPS_LATEST how about DO_TEST_CAPS_LATEST_FAILURE
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_FAILURE, 0)
But this one never gets used - so do we really need it? IDC, but it could be removed...
+ +# define DO_TEST_PARSE_ERROR_CAPS_LATEST(name) \
and likewise DO_TEST_CAPS_LATEST_PARSE_FAILURE with the changes, Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ 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
On Tue, Dec 11, 2018 at 06:51:56PM -0500, John Ferlan wrote:
On 12/7/18 9:47 AM, Erik Skultety wrote:
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
precautionary
negative versions of DO_TEST_CAPS_LATEST macros, so that we eliminate this vector from now on.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tests/qemuxml2argvtest.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
Pulling the needle out of the haystack of d8266ebe1 which of the 3 new tests in qemuxml2argvtest was bad? None used the TEST_CAPS_LATEST nomenclature and you didn't change the test in this patch, so I'm left wondering. Of course, patch4 has the answer.
Nothing major here, but understand coming in cold on this and reading the commit message is, well, confusing.
I think it's simple enough to indicate you are introducing the two new macros to allow for test and parse failure checking. The "details" about the previous commit could be moved into patch 4 I suppose to show what you're fixing since you're not changing a test here.
Noted, will do.
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) \
There should not be a \ at the end of the previous line. Perhaps a holdover from the copy-pasta with the 'virHashLookup...' line.
Yep, nice catch :).
+ # define DO_TEST_CAPS_LATEST(name) \ DO_TEST_CAPS_ARCH_LATEST(name, "x86_64")
+# define DO_TEST_FAILURE_CAPS_LATEST(name) \
To follow DO_TEST_CAPS_LATEST how about
DO_TEST_CAPS_LATEST_FAILURE
Better.
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_FAILURE, 0)
But this one never gets used - so do we really need it? IDC, but it could be removed...
FAILURE was the first one I created (only then I figured the error I'm getting is coming from the parser :/) so I thought why not add both since I'm already creating new macros, so I'd like to keep it in regardless, might get handy soon.
+ +# define DO_TEST_PARSE_ERROR_CAPS_LATEST(name) \
and likewise
DO_TEST_CAPS_LATEST_PARSE_FAILURE
with the changes,
Thanks, Erik
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 <eskultet@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); -- 2.19.2
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@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)
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@redhat.com> 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);
On Tue, Dec 11, 2018 at 06:54:31PM -0500, John Ferlan wrote:
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.
By iterator I mean virDomainDeviceInfoIterateInternal.
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@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)
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)
Looking at the function signature, it might be worth doing a bit of a refactor on the iterator, since @info is treated as unused in a few cases, I'll try looking into that as a follow-up.
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?
It most certainly will.
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
It is not. Note the flag VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE which we pass during driver initialization, thus the validation is only run on new definitions and before starting a guest.
back to my comment at the top dang confusing postparse and validation processing, so I just want to make sure...
It was a bit confusing to me too, the test case never worked as expected even though it failed. Guests having this issue would not disappear, because it's within the validation code. However, validation is still part of the parser code, so as far as the test suite is concerned, it's a PARSE_ERROR, so I had to change it accordingly, otherwise the test started to FAIL (because we expected a failure after the parsing phase).
If my assumption is right about not being able to start like this, then
We shouldn't start a guest like that, but we did anyway, not to mention QEMU didn't really care, but it's wrong anyway.
with a couple of small comment fixups in then you have my
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks, Erik
On Tue, Dec 11, 2018 at 06:54:31PM -0500, John Ferlan wrote:
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@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)
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@redhat.com>
So, I made the adjustments as suggested, added a few comments, used a slightly different name for the iterator flags, dropped the 'typedef' on the iterator flag enum, ran travis on the series and pushed, thanks. Erik
participants (2)
-
Erik Skultety -
John Ferlan