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

[Gerd Hoffmann]

> > 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?

2.13 please.

thanks,
  Gerd

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
> 

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

[Gerd Hoffmann]

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.

cheers,
  Gerd

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

[Eric Blake]

On 04/12/2018 09:11 AM, 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?

Yes.


>>> 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?

At this point, no, we've missed -rc3, and it's not a big enough bug to
warrant needing -rc4 on its own.  And as Gerd said, we already parsed
'gl=on' in 2.11 (the QAPI representation is new to 2.12, but only to
simplify how the existing command line parser was coded), so that's
different than if 2.12 were introducing brand-new content in a form that
should be fixed before it is baked into existing uses.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

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

[Elie Tournier]

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?

> 
> cheers,
>   Gerd
> 

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

[Elie Tournier]

On Tue, Apr 10, 2018 at 08:13:00AM -0500, Eric Blake wrote:
> On 04/10/2018 07:02 AM, Elie Tournier wrote:
> > Signed-off-by: Elie Tournier 
> > ---
> >  qapi/ui.json | 21 -
> >  vl.c | 10 +-
> >  2 files changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 5d01ad4304..c8005867e5 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1019,6 +1019,25 @@
> >  { 'struct'  : 'DisplayGTK',
> >'data': { '*grab-on-hover' : 'bool' } }
> >  
> > + ##
> > + # @DisplayGLMode:
> > + #
> > + # Display OpenGL mode.
> > + #
> > + # 'off'   Disable OpenGL (default).
> 
> Wrong format; this should be:
> 
> # @off: Disable OpenGL (default).

Fix locally
> 
> > + # 'on'Use OpenGL, pick context type automatically.
> > + # Would better be named 'auto' but is called 'on' for backward
> > + # compatibility with bool type.
> 
> See below...
> 
> > + # 'core'  Use OpenGL with Core (desktop) Context.
> > + # 'es'Use OpenGL with ES (embedded systems) Context.
> > + #
> > + # Since: 2.13
> > + #
> > + ##
> > + { 'enum': 'DisplayGLMode',
> > +   'data': [ 'off', 'on', 'core', 'es' ] }
> > +
> > +
> >  ##
> >  # @DisplayType:
> >  #
> > @@ -1048,7 +1067,7 @@
> >'base': { 'type'   : 'DisplayType',
> >  '*full-screen'   : 'bool',
> >  '*window-close'  : 'bool',
> > -'*gl': 'bool' },
> > +'*gl': 'DisplayGLMode' },
> 
> 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.
> 
> 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).

It sounds like making the change now is the best option.
So, is 'auto' a better choice than 'on' in the enum?
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 

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

[Gerd Hoffmann]

> # @off: Disable OpenGL (default).
> 
> > + # '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 ...

cheers,
  Gerd

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

[Eric Blake]

On 04/10/2018 07:02 AM, Elie Tournier wrote:
> Signed-off-by: Elie Tournier 
> ---
>  qapi/ui.json | 21 -
>  vl.c | 10 +-
>  2 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 5d01ad4304..c8005867e5 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1019,6 +1019,25 @@
>  { 'struct'  : 'DisplayGTK',
>'data': { '*grab-on-hover' : 'bool' } }
>  
> + ##
> + # @DisplayGLMode:
> + #
> + # Display OpenGL mode.
> + #
> + # 'off'   Disable OpenGL (default).

Wrong format; this should be:

# @off: Disable OpenGL (default).

> + # 'on'Use OpenGL, pick context type automatically.
> + # Would better be named 'auto' but is called 'on' for backward
> + # compatibility with bool type.

See below...

> + # 'core'  Use OpenGL with Core (desktop) Context.
> + # 'es'Use OpenGL with ES (embedded systems) Context.
> + #
> + # Since: 2.13
> + #
> + ##
> + { 'enum': 'DisplayGLMode',
> +   'data': [ 'off', 'on', 'core', 'es' ] }
> +
> +
>  ##
>  # @DisplayType:
>  #
> @@ -1048,7 +1067,7 @@
>'base': { 'type'   : 'DisplayType',
>  '*full-screen'   : 'bool',
>  '*window-close'  : 'bool',
> -'*gl': 'bool' },
> +'*gl': 'DisplayGLMode' },

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.

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).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature