[PATCH 0/3] ui: Remove deprecated sdl parameters and switch to QAPI parser

[PATCH 0/3] ui: Remove deprecated sdl parameters and switch to QAPI parser

[Thomas Huth]

The "-display sdl" option still uses 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.

While we're at it, also remove the deprecated "-sdl" and "-curses" options.

Thomas Huth (3):
  ui: Remove deprecated parameters of the "-display sdl" option
  ui: Switch "-display sdl" to use the QAPI parser
  ui: Remove deprecated options "-sdl" and "-curses"

 docs/about/deprecated.rst       |  26 -------
 docs/about/removed-features.rst |  27 +++++++
 qapi/ui.json                    |  17 ++++-
 include/sysemu/sysemu.h         |   2 -
 softmmu/globals.c               |   2 -
 softmmu/vl.c                    | 128 +-------------------------------
 ui/sdl2.c                       |  13 ++++
 qemu-options.hx                 |  56 +-------------
 8 files changed, 61 insertions(+), 210 deletions(-)

-- 
2.27.0

[PATCH 1/3] ui: Remove deprecated parameters of the "-display sdl" option

[Thomas Huth]

These parameters are in the way for further refactoring (since they
use an underscore in the name which is forbidden in QAPI), so let's
remove these now that their deprecation period is over.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 docs/about/deprecated.rst       | 16 -------------
 docs/about/removed-features.rst | 17 ++++++++++++++
 softmmu/vl.c                    | 41 +--------------------------------
 qemu-options.hx                 | 32 ++-----------------------
 4 files changed, 20 insertions(+), 86 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/softmmu/vl.c b/softmmu/vl.c
index 488cc4d09e..fdf797270c 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1068,32 +1068,7 @@ static void parse_display(const char *p)
                 } 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.");
-                }
+            } else if (strstart(opts, ",window-close=", &nextopt)) {
                 opts = nextopt;
                 dpy.has_window_close = true;
                 if (strstart(opts, "on", &nextopt)) {
@@ -1943,10 +1918,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 +3273,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/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

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

[Thomas Huth]

The "-display sdl" option still uses a hand-crafted parser for its
parameters since some of them used underscores which is forbidden
in QAPI. Now that the problematic parameters have been removed, we can
switch to use the QAPI parser instead.

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" which is modeled as a string, so that
it could be extended for other arbitrary modifiers later more easily.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qapi/ui.json            | 17 +++++++++-
 include/sysemu/sysemu.h |  2 --
 softmmu/globals.c       |  2 --
 softmmu/vl.c            | 70 +----------------------------------------
 ui/sdl2.c               | 13 ++++++++
 5 files changed, 30 insertions(+), 74 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 059302a5ef..511ade44f2 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1309,6 +1309,20 @@
       '*swap-opt-cmd': 'bool'
   } }
 
+##
+# @DisplaySDL:
+#
+# SDL2 display options.
+#
+# @grab-mod:  String with modifier keys that should be pressed together with
+#             the "G" key to release the mouse grab. Only "lshift-lctrl-lalt"
+#             and "rctrl" are currently supported.
+#
+# Since: 7.1
+##
+{ 'struct'  : 'DisplaySDL',
+  'data'    : { '*grab-mod'   : 'str' } }
+
 ##
 # @DisplayType:
 #
@@ -1391,7 +1405,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 fdf797270c..90a0a4d393 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1045,75 +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, ",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
-    } 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..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) {
-- 
2.27.0

[PATCH 3/3] ui: Remove deprecated options "-sdl" and "-curses"

[Thomas Huth]

We have "-sdl" and "-curses", but no "-gtk" and no "-cocoa" ...
these old-style options are rather confusing than helpful nowadays.
Now that the deprecation period is over, let's remove them, so we
get a cleaner interface (where "-display" is the only way to select
the user interface).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 docs/about/deprecated.rst       | 10 ----------
 docs/about/removed-features.rst | 10 ++++++++++
 softmmu/vl.c                    | 19 -------------------
 qemu-options.hx                 | 24 ++----------------------
 4 files changed, 12 insertions(+), 51 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index ce7a5b97fc..0ed0e9cb13 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -109,16 +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.
 
-``-sdl`` (since 6.2)
-''''''''''''''''''''
-
-Use ``-display sdl`` instead.
-
-``-curses`` (since 6.2)
-'''''''''''''''''''''''
-
-Use ``-display curses`` instead.
-
 ``-watchdog`` (since 6.2)
 '''''''''''''''''''''''''
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 51bfc844f6..0d5ffb353e 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -371,6 +371,16 @@ Use ``-display sdl,grab-mod=lshift-lctrl-lalt`` instead.
 
 Use ``-display sdl,grab-mod=rctrl`` instead.
 
+``-sdl`` (removed in 7.1)
+'''''''''''''''''''''''''
+
+Use ``-display sdl`` instead.
+
+``-curses`` (removed in 7.1)
+''''''''''''''''''''''''''''
+
+Use ``-display curses`` instead.
+
 
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 90a0a4d393..9887c71791 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2845,16 +2845,6 @@ void qemu_init(int argc, char **argv, char **envp)
                 nographic = true;
                 dpy.type = DISPLAY_TYPE_NONE;
                 break;
-            case QEMU_OPTION_curses:
-                warn_report("-curses is deprecated, "
-                            "use -display curses instead.");
-#ifdef CONFIG_CURSES
-                dpy.type = DISPLAY_TYPE_CURSES;
-#else
-                error_report("curses or iconv support is disabled");
-                exit(1);
-#endif
-                break;
             case QEMU_OPTION_portrait:
                 graphic_rotate = 90;
                 break;
@@ -3205,15 +3195,6 @@ void qemu_init(int argc, char **argv, char **envp)
                 dpy.has_full_screen = true;
                 dpy.full_screen = true;
                 break;
-            case QEMU_OPTION_sdl:
-                warn_report("-sdl is deprecated, use -display sdl instead.");
-#ifdef CONFIG_SDL
-                dpy.type = DISPLAY_TYPE_SDL;
-                break;
-#else
-                error_report("SDL support is disabled");
-                exit(1);
-#endif
             case QEMU_OPTION_pidfile:
                 pid_file = optarg;
                 break;
diff --git a/qemu-options.hx b/qemu-options.hx
index 0fa873d088..1abf3e500f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1946,9 +1946,8 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
     , QEMU_ARCH_ALL)
 SRST
 ``-display type``
-    Select type of display to use. This option is a replacement for the
-    old style -sdl/-curses/... options. Use ``-display help`` to list
-    the available display types. Valid values for type are
+    Select type of display to use. Use ``-display help`` to list the available
+    display types. Valid values for type are
 
     ``spice-app[,gl=on|off]``
         Start QEMU as a Spice server and launch the default Spice client
@@ -2050,25 +2049,6 @@ SRST
     Use C-a h for help on switching between the console and monitor.
 ERST
 
-DEF("curses", 0, QEMU_OPTION_curses,
-    "-curses         shorthand for -display curses\n",
-    QEMU_ARCH_ALL)
-SRST
-``-curses``
-    Normally, if QEMU is compiled with graphical window support, it
-    displays output such as guest graphics, guest console, and the QEMU
-    monitor in a window. With this option, QEMU can display the VGA
-    output when in text mode using a curses/ncurses interface. Nothing
-    is displayed in graphical mode.
-ERST
-
-DEF("sdl", 0, QEMU_OPTION_sdl,
-    "-sdl            shorthand for -display sdl\n", QEMU_ARCH_ALL)
-SRST
-``-sdl``
-    Enable SDL.
-ERST
-
 #ifdef CONFIG_SPICE
 DEF("spice", HAS_ARG, QEMU_OPTION_spice,
     "-spice [port=port][,tls-port=secured-port][,x509-dir=<dir>]\n"
-- 
2.27.0

Re: [PATCH 1/3] ui: Remove deprecated parameters of the "-display sdl" option

[Daniel P. Berrangé]

On Wed, May 11, 2022 at 07:51:45PM +0200, Thomas Huth wrote:
> These parameters are in the way for further refactoring (since they
> use an underscore in the name which is forbidden in QAPI), so let's
> remove these now that their deprecation period is over.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  docs/about/deprecated.rst       | 16 -------------
>  docs/about/removed-features.rst | 17 ++++++++++++++
>  softmmu/vl.c                    | 41 +--------------------------------
>  qemu-options.hx                 | 32 ++-----------------------
>  4 files changed, 20 insertions(+), 86 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Daniel P. Berrangé]

On Wed, May 11, 2022 at 07:51:46PM +0200, Thomas Huth wrote:
> The "-display sdl" option still uses a hand-crafted parser for its
> parameters since some of them used underscores which is forbidden
> in QAPI. Now that the problematic parameters have been removed, we can
> switch to use the QAPI parser instead.
> 
> 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" which is modeled as a string, so that
> it could be extended for other arbitrary modifiers later more easily.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  qapi/ui.json            | 17 +++++++++-
>  include/sysemu/sysemu.h |  2 --
>  softmmu/globals.c       |  2 --
>  softmmu/vl.c            | 70 +----------------------------------------
>  ui/sdl2.c               | 13 ++++++++
>  5 files changed, 30 insertions(+), 74 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 3/3] ui: Remove deprecated options "-sdl" and "-curses"

[Daniel P. Berrangé]

On Wed, May 11, 2022 at 07:51:47PM +0200, Thomas Huth wrote:
> We have "-sdl" and "-curses", but no "-gtk" and no "-cocoa" ...
> these old-style options are rather confusing than helpful nowadays.
> Now that the deprecation period is over, let's remove them, so we
> get a cleaner interface (where "-display" is the only way to select
> the user interface).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  docs/about/deprecated.rst       | 10 ----------
>  docs/about/removed-features.rst | 10 ++++++++++
>  softmmu/vl.c                    | 19 -------------------
>  qemu-options.hx                 | 24 ++----------------------
>  4 files changed, 12 insertions(+), 51 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 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 some of them used underscores which is forbidden
> in QAPI.

Kind of.  QAPI indeed requires lower-case-with-hyphens for such names,
but there is an exception mechanism for names with upper case and
underscore: pragma member-name-exceptions.  Used for old names that
predate naming rule enforcement, and for newer names we've elected to
make consistent with their old siblings.

The series that QAPIfied -display partially explained why it left
-display sdl,... and -display vnc,... unQAPIfied in commit 7a61f43859
"ui: document non-qapi parser cases.":

    diff --git a/vl.c b/vl.c
    index 5f1734d851..3b39bbd7a8 100644
    --- a/vl.c
    +++ b/vl.c
    @@ -2114,6 +2114,16 @@ static void parse_display(const char *p)
         const char *opts;

         if (strstart(p, "sdl", &opts)) {
    +        /*
    +         * sdl DisplayType needs hand-crafted parser instead of
    +         * parse_display_qapi() due to some options not in
    +         * DisplayOptions, specifically:
    +         *   - frame
    +         *     Already deprecated.
    +         *   - ctrl_grab + alt_grab
    +         *     Not clear yet what happens to them long-term.  Should
    +         *     replaced by something better or deprecated and dropped.

This sounds like it was mostly reluctance to drag undesirables into the
QAPI schema.

Commit f6b560bbc1 "softmmu/vl: Remove obsolete comment about the "frame"
parameter" dropped item "frame".

Commit 8e8e844be4 "softmmu/vl: Add a "grab-mod" parameter to the
-display sdl option" decided the future of ctrl_grab + alt_grab.  It
replaced the second item's text:

    +         *     They can't be moved into the QAPI since they use underscores,
    +         *     thus they will get replaced by "grab-mod" in the long term

I figure they could've been moved to the QAPI schema back then.
Instead, we're getting rid of them without a detour through the schema.
Good!

    +         */
             dpy.type = DISPLAY_TYPE_SDL;
             while (*opts) {
                 const char *nextopt;
    @@ -2179,6 +2189,10 @@ static void parse_display(const char *p)
                 opts = nextopt;
             }
         } else if (strstart(p, "vnc", &opts)) {
    +        /*
    +         * vnc isn't a (local) DisplayType but a protocol for remote
    +         * display access.
    +         */
             if (*opts == '=') {
                 vnc_parse(opts + 1, &error_fatal);
             } else {

This remains, and that's fine.  One step at time.

>          Now that the problematic parameters have been removed, we can
> switch to use the QAPI parser instead.

Here's my attempt at a more accurate commit message.

  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" which is modeled as a string, so that
> it could be extended for other arbitrary modifiers later more easily.

Are the values of @grab-mod parsed in any way, or do we recognize a set
of fixed strings?

The former would be problematic.  We try hard to represent complex data
as JSON instead of inventing little ad hoc languages.

If it's the latter, use an enum.  Makes introspection more useful, and
adding enumeration values is no harder than adding string literals.

> Signed-off-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

On 12/05/2022 14.16, Markus Armbruster wrote:
[...]>           if (strstart(p, "sdl", &opts)) {
>      +        /*
>      +         * sdl DisplayType needs hand-crafted parser instead of
>      +         * parse_display_qapi() due to some options not in
>      +         * DisplayOptions, specifically:
>      +         *   - frame
>      +         *     Already deprecated.
>      +         *   - ctrl_grab + alt_grab
>      +         *     Not clear yet what happens to them long-term.  Should
>      +         *     replaced by something better or deprecated and dropped.
> 
> This sounds like it was mostly reluctance to drag undesirables into the
> QAPI schema.

Yeah, ctrl_grab and alt_grab were just too ugly and inflexible to drag them 
along into the QAPI world.

>      @@ -2179,6 +2189,10 @@ static void parse_display(const char *p)
>                   opts = nextopt;
>               }
>           } else if (strstart(p, "vnc", &opts)) {
>      +        /*
>      +         * vnc isn't a (local) DisplayType but a protocol for remote
>      +         * display access.
>      +         */
>               if (*opts == '=') {
>                   vnc_parse(opts + 1, &error_fatal);
>               } else {
> 
> This remains, and that's fine.  One step at time.

Not sure how we want to proceed with that in the long run, though ... IIRC 
clearly, Paolo once said that it doesn't really belong into "-display" 
anyway and should be handled with the separate "-vnc" option instead?

>>           Now that the problematic parameters have been removed, we can
>> switch to use the QAPI parser instead.
> 
> Here's my attempt at a more accurate commit message.
> 
>    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.

Ok, I can update the description, thanks!

>> 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" which is modeled as a string, so that
>> it could be extended for other arbitrary modifiers later more easily.
> 
> Are the values of @grab-mod parsed in any way, or do we recognize a set
> of fixed strings?
> 
> The former would be problematic.  We try hard to represent complex data
> as JSON instead of inventing little ad hoc languages.
> 
> If it's the latter, use an enum.  Makes introspection more useful, and
> adding enumeration values is no harder than adding string literals.

It's currently only two strings that are used to replace the old behavior. 
But in the long run, I think it would be nice to have more flexibility here, 
so that a user could specify an arbitrary combination of modifier keys. I 
don't think that an enum will really scale here, so I'd prefer to go with 
the current approach and use the string for more flexibility.

  Thomas

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

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 12/05/2022 14.16, Markus Armbruster wrote:

[...]

>>> 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" which is modeled as a string, so that
>>> it could be extended for other arbitrary modifiers later more easily.
>> 
>> Are the values of @grab-mod parsed in any way, or do we recognize a set
>> of fixed strings?
>> 
>> The former would be problematic.  We try hard to represent complex data
>> as JSON instead of inventing little ad hoc languages.
>> 
>> If it's the latter, use an enum.  Makes introspection more useful, and
>> adding enumeration values is no harder than adding string literals.
>
> It's currently only two strings that are used to replace the old behavior. 
> But in the long run, I think it would be nice to have more flexibility here, 
> so that a user could specify an arbitrary combination of modifier keys. I 
> don't think that an enum will really scale here, so I'd prefer to go with 
> the current approach and use the string for more flexibility.

"Arbitrary combination of modifier keys" sounds like set of enum to me.
We approximate sets with lists in QAPI.

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

[Paolo Bonzini]

On 5/17/22 10:34, Thomas Huth wrote:
>> This remains, and that's fine.  One step at time.
> 
> Not sure how we want to proceed with that in the long run, though ... 
> IIRC clearly, Paolo once said that it doesn't really belong into 
> "-display" anyway and should be handled with the separate "-vnc" option 
> instead?

Not me, Gerd 
(https://lore.kernel.org/all/20210825092023.81396-2-thuth@redhat.com/T/#e8c4f826cc8ff48b9afad37703e11704137f540c8):

> Other way around, -display vnc should be dropped.  -display is about
> local displays, and there can be only one instance.  -vnc / -spice
> enable remote access, and this can be done in addition to a local
> display.
> 
> not possible:
>  -display gtk + -display sdl
> 
> possible:
>  -display gtk + -vnc
>  -display gtk + -vnc + -spice
>  -display none + -vnc + -spice

For what it's worth, Libvirt uses both -vnc and -spice, so we might very 
well proceed with Gerd's idea.  If we don't like -vnc and -spice then it 
may be possible to QOMify them and go with -object.

Thanks,

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 5/17/22 10:34, Thomas Huth wrote:
>>> This remains, and that's fine.  One step at time.
>> Not sure how we want to proceed with that in the long run, though
>> ... IIRC clearly, Paolo once said that it doesn't really belong into 
>> "-display" anyway and should be handled with the separate "-vnc"
>> option instead?
>
> Not me, Gerd
> (https://lore.kernel.org/all/20210825092023.81396-2-thuth@redhat.com/T/#e8c4f826cc8ff48b9afad37703e11704137f540c8):
>
>> Other way around, -display vnc should be dropped.  -display is about
>> local displays, and there can be only one instance.  -vnc / -spice
>> enable remote access, and this can be done in addition to a local
>> display.
>> not possible:
>>  -display gtk + -display sdl
>> possible:
>>  -display gtk + -vnc
>>  -display gtk + -vnc + -spice
>>  -display none + -vnc + -spice
>
> For what it's worth, Libvirt uses both -vnc and -spice, so we might
> very well proceed with Gerd's idea.  If we don't like -vnc and -spice
> then it may be possible to QOMify them and go with -object.

Are we ready to deprecate -display vnc?

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

[Thomas Huth]

On 17/05/2022 13.19, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 5/17/22 10:34, Thomas Huth wrote:
>>>> This remains, and that's fine.  One step at time.
>>> Not sure how we want to proceed with that in the long run, though
>>> ... IIRC clearly, Paolo once said that it doesn't really belong into
>>> "-display" anyway and should be handled with the separate "-vnc"
>>> option instead?
>>
>> Not me, Gerd
>> (https://lore.kernel.org/all/20210825092023.81396-2-thuth@redhat.com/T/#e8c4f826cc8ff48b9afad37703e11704137f540c8):
>>
>>> Other way around, -display vnc should be dropped.  -display is about
>>> local displays, and there can be only one instance.  -vnc / -spice
>>> enable remote access, and this can be done in addition to a local
>>> display.
>>> not possible:
>>>   -display gtk + -display sdl
>>> possible:
>>>   -display gtk + -vnc
>>>   -display gtk + -vnc + -spice
>>>   -display none + -vnc + -spice
>>
>> For what it's worth, Libvirt uses both -vnc and -spice, so we might
>> very well proceed with Gerd's idea.  If we don't like -vnc and -spice
>> then it may be possible to QOMify them and go with -object.
> 
> Are we ready to deprecate -display vnc?

If nobody disagrees, I'd say yes.

  Thomas