[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]