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@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>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
