Re: [PATCH v3 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Thomas Huth]

On 23/05/2022 15.45, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
> > The "-display sdl" option still uses a hand-crafted parser for its
> > parameters since we didn't want to drag an interface we considered
> > somewhat flawed into the QAPI schema. Since the flaws are gone now,
> > it's time to QAPIfy.
> >
> > This introduces the new "DisplaySDL" QAPI struct that is used to hold
> > the parameters that are unique to the SDL display. The only specific
> > parameter is currently "grab-mod" that is used to specify the required
> > modifier keys to escape from the mouse grabbing mode.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   qapi/ui.json            | 26 ++++++++++++++-
> >   include/sysemu/sysemu.h |  2 --
> >   softmmu/globals.c       |  2 --
> >   softmmu/vl.c            | 70 +----------------------------------------
> >   ui/sdl2.c               | 10 ++++++
> >   5 files changed, 36 insertions(+), 74 deletions(-)
> >
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 11a827d10f..413371d5e8 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1295,6 +1295,29 @@
> >         '*swap-opt-cmd': 'bool'
> >     } }
> >   
> > +##
> > +# @HotKeyMod:
> > +#
> > +# Set of modifier keys that need to be held for shortcut key actions.
> > +#
> > +# Since: 7.1
> > +##
> > +{ 'enum'  : 'HotKeyMod',
> > +  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }
>
> I have a somewhat uneasy feeling about encoding what is essentially a
> subset of the sets of modifier keys as an enumeration, but it's what we
> have to do to QAPIfy existing grab-mod.

Well, that's exactly what you suggested here:

 https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg03401.html

So I really don't understand your uneasy feeling now?

...
> > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > index 57ab9d5322..484e9d9921 100644
> > --- a/softmmu/vl.c
> > +++ b/softmmu/vl.c
> > @@ -1056,75 +1056,7 @@ static void parse_display(const char *p)
> >           exit(0);
> >       }
> >   
> > -    if (strstart(p, "sdl", &opts)) {
> > -        /*
> > -         * sdl DisplayType needs hand-crafted parser instead of
> > -         * parse_display_qapi() due to some options not in
> > -         * DisplayOptions, specifically:
> > -         *   - ctrl_grab + alt_grab
> > -         *     They can't be moved into the QAPI since they use underscores,
> > -         *     thus they will get replaced by "grab-mod" in the long term
> > -         */
> > -#if defined(CONFIG_SDL)
> > -        dpy.type = DISPLAY_TYPE_SDL;
> > -        while (*opts) {
> > -            const char *nextopt;
> > -
> > -            if (strstart(opts, ",grab-mod=", &nextopt)) {
> > -                opts = nextopt;
> > -                if (strstart(opts, "lshift-lctrl-lalt", &nextopt)) {
> > -                    alt_grab = 1;
> > -                } else if (strstart(opts, "rctrl", &nextopt)) {
> > -                    ctrl_grab = 1;
> > -                } else {
> > -                    goto invalid_sdl_args;
> > -                }
> > -            } else if (strstart(opts, ",window-close=", &nextopt)) {
> > -                opts = nextopt;
> > -                dpy.has_window_close = true;
> > -                if (strstart(opts, "on", &nextopt)) {
> > -                    dpy.window_close = true;
> > -                } else if (strstart(opts, "off", &nextopt)) {
> > -                    dpy.window_close = false;
> > -                } else {
> > -                    goto invalid_sdl_args;
> > -                }
> > -            } else if (strstart(opts, ",show-cursor=", &nextopt)) {
> > -                opts = nextopt;
> > -                dpy.has_show_cursor = true;
> > -                if (strstart(opts, "on", &nextopt)) {
> > -                    dpy.show_cursor = true;
> > -                } else if (strstart(opts, "off", &nextopt)) {
> > -                    dpy.show_cursor = false;
> > -                } else {
> > -                    goto invalid_sdl_args;
> > -                }
> > -            } else if (strstart(opts, ",gl=", &nextopt)) {
> > -                opts = nextopt;
> > -                dpy.has_gl = true;
> > -                if (strstart(opts, "on", &nextopt)) {
> > -                    dpy.gl = DISPLAYGL_MODE_ON;
> > -                } else if (strstart(opts, "core", &nextopt)) {
> > -                    dpy.gl = DISPLAYGL_MODE_CORE;
> > -                } else if (strstart(opts, "es", &nextopt)) {
> > -                    dpy.gl = DISPLAYGL_MODE_ES;
> > -                } else if (strstart(opts, "off", &nextopt)) {
> > -                    dpy.gl = DISPLAYGL_MODE_OFF;
> > -                } else {
> > -                    goto invalid_sdl_args;
> > -                }
> > -            } else {
> > -            invalid_sdl_args:
> > -                error_report("invalid SDL option string");
> > -                exit(1);
> > -            }
> > -            opts = nextopt;
> > -        }
> > -#else
> > -        error_report("SDL display supported is not available in this binary");
> > -        exit(1);
> > -#endif
>
> When CONFIG_SDL is off, the error message changes from
>
>      qemu-system-x86_64: -display sdl: SDL display supported is not available 
> in this binary
>
> to
>
>      qemu-system-x86_64: -display sdl: Parameter 'type' does not accept value 
> 'sdl'
>
> I don't mind, but I'd suggest to mention it in the commit message.

I can do that if I have to respin this series for some other reasons. 
Otherwise, I don't think this really severe enough to justify a v4.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

 Thomas