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

[Markus Armbruster]

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.

> +
> +##
> +# @DisplaySDL:
> +#
> +# SDL2 display options.
> +#
> +# @grab-mod:  Modifier keys that should be pressed together with the
> +#             "G" key to release the mouse grab.
> +#
> +# Since: 7.1
> +##
> +{ 'struct'  : 'DisplaySDL',
> +  'data'    : { '*grab-mod'   : 'HotKeyMod' } }
> +
>  ##
>  # @DisplayType:
>  #
> @@ -1374,7 +1397,8 @@
>        'curses': { 'type': 'DisplayCurses', 'if': 'CONFIG_CURSES' },
>        'egl-headless': { 'type': 'DisplayEGLHeadless',
>                          'if': { 'all': ['CONFIG_OPENGL', 'CONFIG_GBM'] } },
> -      'dbus': { 'type': 'DisplayDBus', 'if': 'CONFIG_DBUS_DISPLAY' }
> +      'dbus': { 'type': 'DisplayDBus', 'if': 'CONFIG_DBUS_DISPLAY' },
> +      'sdl': { 'type': 'DisplaySDL', 'if': 'CONFIG_SDL' }
>    }
>  }
>  
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b4030acd74..812f66a31a 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -42,8 +42,6 @@ extern int graphic_depth;
>  extern int display_opengl;
>  extern const char *keyboard_layout;
>  extern int win2k_install_hack;
> -extern int alt_grab;
> -extern int ctrl_grab;
>  extern int graphic_rotate;
>  extern int old_param;
>  extern uint8_t *boot_splash_filedata;
> diff --git a/softmmu/globals.c b/softmmu/globals.c
> index 916bc12e2b..527edbefdd 100644
> --- a/softmmu/globals.c
> +++ b/softmmu/globals.c
> @@ -50,8 +50,6 @@ QEMUOptionRom option_rom[MAX_OPTION_ROMS];
>  int nb_option_roms;
>  int old_param;
>  const char *qemu_name;
> -int alt_grab;
> -int ctrl_grab;
>  unsigned int nb_prom_envs;
>  const char *prom_envs[MAX_PROM_ENVS];
>  uint8_t *boot_splash_filedata;
> 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.

> -    } else if (strstart(p, "vnc", &opts)) {
> +    if (strstart(p, "vnc", &opts)) {
>          /*
>           * vnc isn't a (local) DisplayType but a protocol for remote
>           * display access.
> diff --git a/ui/sdl2.c b/ui/sdl2.c
> index d3741f9b75..8cb77416af 100644
> --- a/ui/sdl2.c
> +++ b/ui/sdl2.c
> @@ -40,6 +40,8 @@ static struct sdl2_console *sdl2_console;
>  
>  static SDL_Surface *guest_sprite_surface;
>  static int gui_grab; /* if true, all keyboard/mouse events are grabbed */
> +static bool alt_grab;
> +static bool ctrl_grab;

I like that these become internal to sdl2.c.

>  
>  static int gui_saved_grab;
>  static int gui_fullscreen;
> @@ -853,6 +855,14 @@ static void sdl2_display_init(DisplayState *ds, 
> DisplayOptions *o)
>  
>      gui_fullscreen = o->has_full_screen && o->full_screen;
>  
> +    if (o->u.sdl.has_grab_mod) {
> +        if (o->u.sdl.grab_mod == HOT_KEY_MOD_LSHIFT_LCTRL_LALT) {
> +            alt_grab = true;
> +        } else if (o->u.sdl.grab_mod == HOT_KEY_MOD_RCTRL) {
> +            ctrl_grab = true;
> +        }
> +    }
> +
>      for (i = 0;; i++) {
>          QemuConsole *con = qemu_console_lookup_by_index(i);
>          if (!con) {

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