Re: [Qemu-devel] [PATCH v2 1/2 for-2.12?] qapi: Parameter gl of DisplayType now accept an enum

[Elie Tournier]

On Fri, Apr 13, 2018 at 08:15:29AM +0200, Gerd Hoffmann wrote:
> On Thu, Apr 12, 2018 at 03:11:53PM +0100, Elie Tournier wrote:
> > Hello,
> > 
> > On Tue, Apr 10, 2018 at 03:33:35PM +0200, Gerd Hoffmann wrote:
> > > > # @off: Disable OpenGL (default).
> > 
> > Just to be sure, I have to add @ in front of all parameter, right?
> > 
> > > > 
> > > > > + # 'on'    Use OpenGL, pick context type automatically.
> > > > > + #         Would better be named 'auto' but is called 'on' for 
> > > > > backward
> > > > > + #         compatibility with bool type.
> > > > 
> > > > See below...
> > > 
> > > > DisplayOptions was added in 2.12.  This is a backwards-incompatible
> > > > change in QMP (you CANNOT change 'bool' to 'DisplayGLMode' across
> > > > releases, because the on-the-wire representation differs; pre-patch it
> > > > would be "gl":true, post-patch it is "gl":"on").  So if it affects QMP,
> > > > and we want it, this patch either HAS to go in 2.12, or we have to have
> > > > more finesse (perhaps by using an 'alternate' type in the QAPI) so that
> > > > it remains backwards-compatible.
> > > > 
> > > > /me goes and looks at introspection output...
> > > > 
> > > > You may be in luck - there is no instance of 'window-close' in the
> > > > introspection output, which means 'DisplayType' exists only for ease of
> > > > command-line parsing and is not currently used by QMP, so tweaks here
> > > > are not affecting the command line.
> > > 
> > > Yes, right now the struct is only used to store the parsed command line
> > > opts, so no effect on QMP.
> > > 
> > > Plan for the future is to also parse command line options with generic
> > > qapi/json code instead of the home-grown parser, but that switch didn't
> > > happen yet.
> > > 
> > > > That said, you can STILL name the enum value something smarter than 'on'
> > > > IF you make the change now, for 2.12, given that the QAPI type was only
> > > > introduced in 2.12 (you only have to worry about backwards compatibility
> > > > if 2.11 already parsed gl=on).
> > > 
> > > gl=on is older, so that must continue to work.  Making both "on" and
> > > "auto" work isn't a problem for our home-grown parser (aka
> > > parse_display() in vl.c).  But having quirks like this makes the switch
> > > to generic parser code more difficuilt, so I'd prefer to avoid that ...
> > 
> > Is it possible to upstream this change before 2.12 release?
> 
> No need to hurry with this.  QMP doesn't see the structs, so no
> compatibility problem here.  gl=on cmd line option was present in 2.11 +
> older already, so that must continue to work no matter whenever this
> makes 2.12 or not.

Alright, thanks.
Well, to be fair, I just want to see this feature upstream, I don't really care
about the release number.

Just a question before I resend the series.
Should I note "Since 2.12" or "Since 2.13" in qapi/ui.json?

BR,
Elie

> 
> cheers,
>   Gerd
>