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