[PATCH] ui: Remove deprecated parameters of -display sdl and switch to QAPI parser

Thomas Huth posted 1 patch 1 week, 2 days ago
docs/about/deprecated.rst       |  16 -----
docs/about/removed-features.rst |  17 +++++
qapi/ui.json                    |  16 ++++-
include/sysemu/sysemu.h         |   2 -
softmmu/globals.c               |   2 -
softmmu/vl.c                    | 109 +-------------------------------
ui/sdl2.c                       |  13 ++++
qemu-options.hx                 |  32 +---------
8 files changed, 48 insertions(+), 159 deletions(-)
[PATCH] ui: Remove deprecated parameters of -display sdl and switch to QAPI parser
Posted by Thomas Huth 1 week, 2 days ago
The "-display sdl" option still used a hand-crafted parser for its
parameters since some of them used underscores which is forbidden
in QAPI. Now that they've been deprecated and the deprecation period
is over, we can remove the problematic parameters and switch to use
the QAPI parser instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 docs/about/deprecated.rst       |  16 -----
 docs/about/removed-features.rst |  17 +++++
 qapi/ui.json                    |  16 ++++-
 include/sysemu/sysemu.h         |   2 -
 softmmu/globals.c               |   2 -
 softmmu/vl.c                    | 109 +-------------------------------
 ui/sdl2.c                       |  13 ++++
 qemu-options.hx                 |  32 +---------
 8 files changed, 48 insertions(+), 159 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 896e5a97ab..ce7a5b97fc 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -109,22 +109,6 @@ other options have been processed.  This will either have no effect (if
 if they were not given.  The property is therefore useless and should not be
 specified.
 
-``-display sdl,window_close=...`` (since 6.1)
-'''''''''''''''''''''''''''''''''''''''''''''
-
-Use ``-display sdl,window-close=...`` instead (i.e. with a minus instead of
-an underscore between "window" and "close").
-
-``-alt-grab`` and ``-display sdl,alt_grab=on`` (since 6.2)
-''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
-
-Use ``-display sdl,grab-mod=lshift-lctrl-lalt`` instead.
-
-``-ctrl-grab`` and ``-display sdl,ctrl_grab=on`` (since 6.2)
-''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
-
-Use ``-display sdl,grab-mod=rctrl`` instead.
-
 ``-sdl`` (since 6.2)
 ''''''''''''''''''''
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 4a0b270296..51bfc844f6 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -355,6 +355,23 @@ The ``-writeconfig`` option was not able to serialize the entire contents
 of the QEMU command line.  It is thus considered a failed experiment
 and removed without a replacement.
 
+``-display sdl,window_close=...`` (removed in 7.1)
+''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Use ``-display sdl,window-close=...`` instead (i.e. with a minus instead of
+an underscore between "window" and "close").
+
+``-alt-grab`` and ``-display sdl,alt_grab=on`` (removed in 7.1)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Use ``-display sdl,grab-mod=lshift-lctrl-lalt`` instead.
+
+``-ctrl-grab`` and ``-display sdl,ctrl_grab=on`` (removed in 7.1)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Use ``-display sdl,grab-mod=rctrl`` instead.
+
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
diff --git a/qapi/ui.json b/qapi/ui.json
index 059302a5ef..7d5097808a 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1309,6 +1309,19 @@
       '*swap-opt-cmd': 'bool'
   } }
 
+##
+# @DisplaySDL:
+#
+# SDL2 display options.
+#
+# @grab-mod:  Modifier keys that should be pressed together
+#             with "G" to release the mouse grab.
+#
+# Since: 7.1
+##
+{ 'struct'  : 'DisplaySDL',
+  'data'    : { '*grab-mod'   : 'str' } }
+
 ##
 # @DisplayType:
 #
@@ -1391,7 +1404,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 360a408edf..7cca797450 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 int boot_menu;
diff --git a/softmmu/globals.c b/softmmu/globals.c
index 98b64e0492..12611c2a7a 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];
 int boot_menu;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 488cc4d09e..90a0a4d393 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1045,100 +1045,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, ",alt_grab=", &nextopt)) {
-                opts = nextopt;
-                if (strstart(opts, "on", &nextopt)) {
-                    alt_grab = 1;
-                } else if (strstart(opts, "off", &nextopt)) {
-                    alt_grab = 0;
-                } else {
-                    goto invalid_sdl_args;
-                }
-                warn_report("alt_grab is deprecated, use grab-mod instead.");
-            } else if (strstart(opts, ",ctrl_grab=", &nextopt)) {
-                opts = nextopt;
-                if (strstart(opts, "on", &nextopt)) {
-                    ctrl_grab = 1;
-                } else if (strstart(opts, "off", &nextopt)) {
-                    ctrl_grab = 0;
-                } else {
-                    goto invalid_sdl_args;
-                }
-                warn_report("ctrl_grab is deprecated, use grab-mod instead.");
-            } else if (strstart(opts, ",window_close=", &nextopt) ||
-                       strstart(opts, ",window-close=", &nextopt)) {
-                if (strstart(opts, ",window_close=", NULL)) {
-                    warn_report("window_close with an underscore is deprecated,"
-                                " please use window-close instead.");
-                }
-                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
-    } else if (strstart(p, "vnc", &opts)) {
+    if (strstart(p, "vnc", &opts)) {
         /*
          * vnc isn't a (local) DisplayType but a protocol for remote
          * display access.
@@ -1943,10 +1850,6 @@ static void qemu_create_early_backends(void)
     const bool use_gtk = false;
 #endif
 
-    if ((alt_grab || ctrl_grab) && !use_sdl) {
-        error_report("-alt-grab and -ctrl-grab are only valid "
-                     "for SDL, ignoring option");
-    }
     if (dpy.has_window_close && !use_gtk && !use_sdl) {
         error_report("window-close is only valid for GTK and SDL, "
                      "ignoring option");
@@ -3302,16 +3205,6 @@ void qemu_init(int argc, char **argv, char **envp)
                 dpy.has_full_screen = true;
                 dpy.full_screen = true;
                 break;
-            case QEMU_OPTION_alt_grab:
-                alt_grab = 1;
-                warn_report("-alt-grab is deprecated, please use "
-                            "-display sdl,grab-mod=lshift-lctrl-lalt instead.");
-                break;
-            case QEMU_OPTION_ctrl_grab:
-                ctrl_grab = 1;
-                warn_report("-ctrl-grab is deprecated, please use "
-                            "-display sdl,grab-mod=rctrl instead.");
-                break;
             case QEMU_OPTION_sdl:
                 warn_report("-sdl is deprecated, use -display sdl instead.");
 #ifdef CONFIG_SDL
diff --git a/ui/sdl2.c b/ui/sdl2.c
index d3741f9b75..18c63e1fc9 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;
 
 static int gui_saved_grab;
 static int gui_fullscreen;
@@ -853,6 +855,17 @@ 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 (g_str_equal(o->u.sdl.grab_mod, "lshift-lctrl-lalt")) {
+            alt_grab = true;
+        } else if (g_str_equal(o->u.sdl.grab_mod, "rctrl")) {
+            ctrl_grab = true;
+        } else {
+            error_report("Unsupported grab-mod: %s", o->u.sdl.grab_mod);
+            exit(1);
+        }
+    }
+
     for (i = 0;; i++) {
         QemuConsole *con = qemu_console_lookup_by_index(i);
         if (!con) {
diff --git a/qemu-options.hx b/qemu-options.hx
index 796229c433..0fa873d088 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1903,8 +1903,8 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
     "-display spice-app[,gl=on|off]\n"
 #endif
 #if defined(CONFIG_SDL)
-    "-display sdl[,alt_grab=on|off][,ctrl_grab=on|off][,gl=on|core|es|off]\n"
-    "            [,grab-mod=<mod>][,show-cursor=on|off][,window-close=on|off]\n"
+    "-display sdl[,gl=on|core|es|off][,grab-mod=<mod>][,show-cursor=on|off]\n"
+    "            [,window-close=on|off]\n"
 #endif
 #if defined(CONFIG_GTK)
     "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
@@ -1977,12 +1977,6 @@ SRST
         the mouse grabbing in conjunction with the "g" key. ``<mods>`` can be
         either ``lshift-lctrl-lalt`` or ``rctrl``.
 
-        ``alt_grab=on|off`` : Use Control+Alt+Shift-g to toggle mouse grabbing.
-        This parameter is deprecated - use ``grab-mod`` instead.
-
-        ``ctrl_grab=on|off`` : Use Right-Control-g to toggle mouse grabbing.
-        This parameter is deprecated - use ``grab-mod`` instead.
-
         ``gl=on|off|core|es`` : Use OpenGL for displaying
 
         ``show-cursor=on|off`` :  Force showing the mouse cursor
@@ -2068,28 +2062,6 @@ SRST
     is displayed in graphical mode.
 ERST
 
-DEF("alt-grab", 0, QEMU_OPTION_alt_grab,
-    "-alt-grab       use Ctrl-Alt-Shift to grab mouse (instead of Ctrl-Alt)\n",
-    QEMU_ARCH_ALL)
-SRST
-``-alt-grab``
-    Use Ctrl-Alt-Shift to grab mouse (instead of Ctrl-Alt). Note that
-    this also affects the special keys (for fullscreen, monitor-mode
-    switching, etc). This option is deprecated - please use
-    ``-display sdl,grab-mod=lshift-lctrl-lalt`` instead.
-ERST
-
-DEF("ctrl-grab", 0, QEMU_OPTION_ctrl_grab,
-    "-ctrl-grab      use Right-Ctrl to grab mouse (instead of Ctrl-Alt)\n",
-    QEMU_ARCH_ALL)
-SRST
-``-ctrl-grab``
-    Use Right-Ctrl to grab mouse (instead of Ctrl-Alt). Note that this
-    also affects the special keys (for fullscreen, monitor-mode
-    switching, etc). This option is deprecated - please use
-    ``-display sdl,grab-mod=rctrl`` instead.
-ERST
-
 DEF("sdl", 0, QEMU_OPTION_sdl,
     "-sdl            shorthand for -display sdl\n", QEMU_ARCH_ALL)
 SRST
-- 
2.27.0
Re: [PATCH] ui: Remove deprecated parameters of -display sdl and switch to QAPI parser
Posted by Daniel P. Berrangé 1 week, 2 days ago
On Wed, May 11, 2022 at 02:17:25PM +0200, Thomas Huth wrote:
> The "-display sdl" option still used a hand-crafted parser for its
> parameters since some of them used underscores which is forbidden
> in QAPI. Now that they've been deprecated and the deprecation period
> is over, we can remove the problematic parameters and switch to use
> the QAPI parser instead.

I'd say the removal of deprecated bits ought to be separate
from the addition of new QAPI bits.


> diff --git a/qapi/ui.json b/qapi/ui.json
> index 059302a5ef..7d5097808a 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1309,6 +1309,19 @@
>        '*swap-opt-cmd': 'bool'
>    } }
>  
> +##
> +# @DisplaySDL:
> +#
> +# SDL2 display options.
> +#
> +# @grab-mod:  Modifier keys that should be pressed together
> +#             with "G" to release the mouse grab.
> +#
> +# Since: 7.1
> +##
> +{ 'struct'  : 'DisplaySDL',
> +  'data'    : { '*grab-mod'   : 'str' } }

So any arbitrary string here, but...

> diff --git a/ui/sdl2.c b/ui/sdl2.c
> index d3741f9b75..18c63e1fc9 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;
>  
>  static int gui_saved_grab;
>  static int gui_fullscreen;
> @@ -853,6 +855,17 @@ 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 (g_str_equal(o->u.sdl.grab_mod, "lshift-lctrl-lalt")) {
> +            alt_grab = true;
> +        } else if (g_str_equal(o->u.sdl.grab_mod, "rctrl")) {
> +            ctrl_grab = true;
> +        } else {
> +            error_report("Unsupported grab-mod: %s", o->u.sdl.grab_mod);
> +            exit(1);
> +        }
> +    }

We're treating this more like an enum here. It does leave the door
open for adding a generic parsing of arbitrary grab mods later
I guess


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] ui: Remove deprecated parameters of -display sdl and switch to QAPI parser
Posted by Thomas Huth 1 week, 2 days ago
On 11/05/2022 14.40, Daniel P. Berrangé wrote:
> On Wed, May 11, 2022 at 02:17:25PM +0200, Thomas Huth wrote:
>> The "-display sdl" option still used a hand-crafted parser for its
>> parameters since some of them used underscores which is forbidden
>> in QAPI. Now that they've been deprecated and the deprecation period
>> is over, we can remove the problematic parameters and switch to use
>> the QAPI parser instead.
> 
> I'd say the removal of deprecated bits ought to be separate
> from the addition of new QAPI bits.

Ok, agreed, that likely makes sense.

> 
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index 059302a5ef..7d5097808a 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -1309,6 +1309,19 @@
>>         '*swap-opt-cmd': 'bool'
>>     } }
>>   
>> +##
>> +# @DisplaySDL:
>> +#
>> +# SDL2 display options.
>> +#
>> +# @grab-mod:  Modifier keys that should be pressed together
>> +#             with "G" to release the mouse grab.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct'  : 'DisplaySDL',
>> +  'data'    : { '*grab-mod'   : 'str' } }
> 
> So any arbitrary string here, but...
> 
>> diff --git a/ui/sdl2.c b/ui/sdl2.c
>> index d3741f9b75..18c63e1fc9 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;
>>   
>>   static int gui_saved_grab;
>>   static int gui_fullscreen;
>> @@ -853,6 +855,17 @@ 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 (g_str_equal(o->u.sdl.grab_mod, "lshift-lctrl-lalt")) {
>> +            alt_grab = true;
>> +        } else if (g_str_equal(o->u.sdl.grab_mod, "rctrl")) {
>> +            ctrl_grab = true;
>> +        } else {
>> +            error_report("Unsupported grab-mod: %s", o->u.sdl.grab_mod);
>> +            exit(1);
>> +        }
>> +    }
> 
> We're treating this more like an enum here. It does leave the door
> open for adding a generic parsing of arbitrary grab mods later
> I guess

Yes, that's the basic idea - so one day, the user might specify an arbitrary 
string here consisting of their favorite modifiers.

  Thomas