[Qemu-devel] [PATCH 0/4] ui: use qapi-based parser for most -display options.

[Qemu-devel] [PATCH 0/4] ui: use qapi-based parser for most -display options.

[Gerd Hoffmann]

Gerd Hoffmann (4):
  ui: add qapi parser for -display
  ui: switch trivial displays to qapi parser
  ui: switch gtk display to qapi parser
  ui: document non-qapi parser cases.

 vl.c | 81 ++++++++++++++++++++++++++++++++++----------------------------------
 1 file changed, 41 insertions(+), 40 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/4] ui: add qapi parser for -display

[Gerd Hoffmann]

Add parse_display_qapi() function which parses the -display command line
using a qapi visitor for DisplayOptions.  Wire up as default catch in
parse_display().

Improves the error message for unknown display types.

Also enables json as -display argument, i.e. -display "{ 'type': 'gtk' }"

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vl.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index fce1fd12d8..784c3fb795 100644
--- a/vl.c
+++ b/vl.c
@@ -126,6 +126,7 @@ int main(int argc, char **argv)
 #include "sysemu/replay.h"
 #include "qapi/qapi-events-run-state.h"
 #include "qapi/qapi-visit-block-core.h"
+#include "qapi/qapi-visit-ui.h"
 #include "qapi/qapi-commands-block-core.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qapi-commands-run-state.h"
@@ -2090,6 +2091,31 @@ static void select_vgahw(const char *p)
     }
 }
 
+static void parse_display_qapi(const char *optarg)
+{
+    Error *err = NULL;
+    DisplayOptions *opts;
+    Visitor *v;
+
+    v = qobject_input_visitor_new_str(optarg, "type", &err);
+    if (!v) {
+        error_report_err(err);
+        exit(1);
+    }
+
+    visit_type_DisplayOptions(v, NULL, &opts, &error_fatal);
+    visit_free(v);
+
+    /*
+     * We don't have any dynamically allocated stuff inside
+     * DisplayOptions, so we can simply copy the struct content and
+     * free opts without ending up with pointers pointing into
+     * nowhere.
+     */
+    dpy = *opts;
+    qapi_free_DisplayOptions(opts);
+}
+
 static void parse_display(const char *p)
 {
     const char *opts;
@@ -2201,8 +2227,7 @@ static void parse_display(const char *p)
     } else if (strstart(p, "none", &opts)) {
         dpy.type = DISPLAY_TYPE_NONE;
     } else {
-        error_report("unknown display type");
-        exit(1);
+        parse_display_qapi(p);
     }
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/4] ui: add qapi parser for -display

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On 04/19/2018 08:20 AM, Gerd Hoffmann wrote:
> Add parse_display_qapi() function which parses the -display command line
> using a qapi visitor for DisplayOptions.  Wire up as default catch in
> parse_display().
> 
> Improves the error message for unknown display types.
> 
> Also enables json as -display argument, i.e. -display "{ 'type': 'gtk' }"
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

> +
> +    /*
> +     * We don't have any dynamically allocated stuff inside
> +     * DisplayOptions, so we can simply copy the struct content and
> +     * free opts without ending up with pointers pointing into
> +     * nowhere.
> +     */
> +    dpy = *opts;
> +    qapi_free_DisplayOptions(opts);

That's risky; would it be better to use QAPI_CLONE_MEMBERS() to not have
to worry about if we add a pointer in the future?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/4] ui: add qapi parser for -display

[Gerd Hoffmann]

> > +    /*
> > +     * We don't have any dynamically allocated stuff inside
> > +     * DisplayOptions, so we can simply copy the struct content and
> > +     * free opts without ending up with pointers pointing into
> > +     * nowhere.
> > +     */
> > +    dpy = *opts;
> > +    qapi_free_DisplayOptions(opts);
> 
> That's risky; would it be better to use QAPI_CLONE_MEMBERS() to not have
> to worry about if we add a pointer in the future?

Didn't know QAPI_CLONE_MEMBERS() exists.  Yes, probably more
future-proof to just use that instead of adding that comment.
Will look into that for v2.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/4] ui: add qapi parser for -display

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Add parse_display_qapi() function which parses the -display command line
> using a qapi visitor for DisplayOptions.  Wire up as default catch in
> parse_display().
>
> Improves the error message for unknown display types.
>
> Also enables json as -display argument, i.e. -display "{ 'type': 'gtk' }"

This new option argument syntax is undocumented.  Intentional?

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  vl.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)

Re: [Qemu-devel] [PATCH 1/4] ui: add qapi parser for -display

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Gerd Hoffmann <kraxel@redhat.com> writes:
>
>> Add parse_display_qapi() function which parses the -display command line
>> using a qapi visitor for DisplayOptions.  Wire up as default catch in
>> parse_display().
>>
>> Improves the error message for unknown display types.
>>
>> Also enables json as -display argument, i.e. -display "{ 'type': 'gtk' }"
>
> This new option argument syntax is undocumented.  Intentional?

Please advise.

Re: [Qemu-devel] [PATCH 1/4] ui: add qapi parser for -display

[Gerd Hoffmann]

On Fri, Aug 24, 2018 at 08:30:23AM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Gerd Hoffmann <kraxel@redhat.com> writes:
> >
> >> Add parse_display_qapi() function which parses the -display command line
> >> using a qapi visitor for DisplayOptions.  Wire up as default catch in
> >> parse_display().
> >>
> >> Improves the error message for unknown display types.
> >>
> >> Also enables json as -display argument, i.e. -display "{ 'type': 'gtk' }"
> >
> > This new option argument syntax is undocumented.  Intentional?

No, just -ENOTIME.

> Please advise.

Well, just need to find some time to do it.

In case you want tackle it:  Works simliar to -blockdev, with a few
exceptions.  Some legacy sdl options I plan to deprecate are not
supported via json.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/4] ui: add qapi parser for -display

[Gerd Hoffmann]

On Fri, Aug 24, 2018 at 08:30:23AM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Gerd Hoffmann <kraxel@redhat.com> writes:
> >
> >> Add parse_display_qapi() function which parses the -display command line
> >> using a qapi visitor for DisplayOptions.  Wire up as default catch in
> >> parse_display().
> >>
> >> Improves the error message for unknown display types.
> >>
> >> Also enables json as -display argument, i.e. -display "{ 'type': 'gtk' }"
> >
> > This new option argument syntax is undocumented.  Intentional?
> 
> Please advise.

Hmm, checking -blockdev doc in qemu-options.hx, nothing about json.
Is this documented elsewhere?

cheers,
  Gerd

[Qemu-devel] [PATCH 2/4] ui: switch trivial displays to qapi parser

[Gerd Hoffmann]

Drop the option-less display types (egl-headless, curses, none) from
parse_display(), so they'll be handled by parse_display_qapi().

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vl.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/vl.c b/vl.c
index 784c3fb795..39303d6b55 100644
--- a/vl.c
+++ b/vl.c
@@ -2188,10 +2188,6 @@ static void parse_display(const char *p)
             error_report("VNC requires a display argument vnc=<display>");
             exit(1);
         }
-    } else if (strstart(p, "egl-headless", &opts)) {
-        dpy.type = DISPLAY_TYPE_EGL_HEADLESS;
-    } else if (strstart(p, "curses", &opts)) {
-        dpy.type = DISPLAY_TYPE_CURSES;
     } else if (strstart(p, "gtk", &opts)) {
         dpy.type = DISPLAY_TYPE_GTK;
         while (*opts) {
@@ -2224,8 +2220,6 @@ static void parse_display(const char *p)
             }
             opts = nextopt;
         }
-    } else if (strstart(p, "none", &opts)) {
-        dpy.type = DISPLAY_TYPE_NONE;
     } else {
         parse_display_qapi(p);
     }
-- 
2.9.3

[Qemu-devel] [PATCH 3/4] ui: switch gtk display to qapi parser

[Gerd Hoffmann]

Drop the gtk option parser from parse_display(), so parse_display_qapi()
will handle it instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vl.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/vl.c b/vl.c
index 39303d6b55..1123cd792e 100644
--- a/vl.c
+++ b/vl.c
@@ -2188,38 +2188,6 @@ static void parse_display(const char *p)
             error_report("VNC requires a display argument vnc=<display>");
             exit(1);
         }
-    } else if (strstart(p, "gtk", &opts)) {
-        dpy.type = DISPLAY_TYPE_GTK;
-        while (*opts) {
-            const char *nextopt;
-
-            if (strstart(opts, ",grab_on_hover=", &nextopt)) {
-                opts = nextopt;
-                dpy.u.gtk.has_grab_on_hover = true;
-                if (strstart(opts, "on", &nextopt)) {
-                    dpy.u.gtk.grab_on_hover = true;
-                } else if (strstart(opts, "off", &nextopt)) {
-                    dpy.u.gtk.grab_on_hover = false;
-                } else {
-                    goto invalid_gtk_args;
-                }
-            } else if (strstart(opts, ",gl=", &nextopt)) {
-                opts = nextopt;
-                dpy.has_gl = true;
-                if (strstart(opts, "on", &nextopt)) {
-                    dpy.gl = true;
-                } else if (strstart(opts, "off", &nextopt)) {
-                    dpy.gl = false;
-                } else {
-                    goto invalid_gtk_args;
-                }
-            } else {
-            invalid_gtk_args:
-                error_report("invalid GTK option string");
-                exit(1);
-            }
-            opts = nextopt;
-        }
     } else {
         parse_display_qapi(p);
     }
-- 
2.9.3

[Qemu-devel] [PATCH 4/4] ui: document non-qapi parser cases.

[Gerd Hoffmann]

Add comments to the cases not (yet) switched
over to parse_display_qapi().

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vl.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/vl.c b/vl.c
index 1123cd792e..d35c443b8c 100644
--- a/vl.c
+++ b/vl.c
@@ -2121,6 +2121,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.
+         */
         dpy.type = DISPLAY_TYPE_SDL;
         while (*opts) {
             const char *nextopt;
@@ -2182,6 +2192,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 {
-- 
2.9.3