Re: [libvirt] [PATCH 4/4] conf: domain: gfx: Iterate over graphics devices when doing validation

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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt;