| Message ID | 20230830093843.3531473-44-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make pixman an optional dependency | expand |
On Wed, Aug 30, 2023 at 01:38:17PM +0400, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > In commit 6f974c843c ("gtk: overwrite the console.c char driver"), I > shared the VC console parse handler with GTK. And later on in commit > d8aec9d9 ("display: add -display spice-app launching a Spice client"), > I also used it to handle spice-app VC. > > This is not necessary, the VC console options (width/height/cols/rows) > are specific, and unused by tty-level GTK/Spice VC. > > This is not a breaking change, as those options are still being parsed > by QAPI ChardevVC. Adjust the documentation about it. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qapi/char.json | 4 ++++ > include/chardev/char.h | 3 --- > ui/console.c | 4 ++-- > ui/gtk.c | 1 - > ui/spice-app.c | 7 ++++++- > 5 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/qapi/char.json b/qapi/char.json > index 52aaff25eb..7e23fe3180 100644 > --- a/qapi/char.json > +++ b/qapi/char.json > @@ -390,6 +390,10 @@ > # > # @rows: console height, in chars > # > +# Note: the options are only effective when using the built-in QEMU graphical VC > +# (with the SDL display). When the VC is handled by the display backend (with > +# GTK/VTE, Spice or D-Bus), they are simply ignored. I don't find this explains the situation very well, I had to look at the code to understand what's ultimately going on, as I didn't really know what it meant by "built-in QEMU graphical VC". From the end user's POV, they're just using '-chardev vc...'. IIUC, the command line -chardev vc,..... will end up instantiating TYPE_CHARDEV_VC. We actually then have 4 completely different implementations of TYPE_CHARDEV_VC, and depending on which display backend is enabled, a different TYPE_CHARDEV_VC will get registered. So what your comment is saying is that the width/height/rows/cols properties will only get honoured if the TYPE_CHARDEV_VC that is registered, is the one that maps to the SDL display backend. Wow, is this situation confusing and gross in the code :-( So for the comment I think we can just cut out a few words to make it simpler # Note: the options are only effective when the SDL graphical # display backend is active. They are ignored with the GTK, # Spice, VNC and D-Bus display backends. As a future exercise for anyone motiviated, I would say that TYPE_CHARDEV_VC ought to be abstract and we then have subclasses for each of the impls we have that are registered unconditionally with type_init(), then pick the subclass to instantiate based on the display backend. That way we can ultimately make the QAPI schema reflect that we have multiple ChardevVC impls and only expose the cols/width/etc for the SDL impl. Anyway, if the comment is simplied/clarified then Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
Hi On Fri, Sep 1, 2023 at 9:14 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Aug 30, 2023 at 01:38:17PM +0400, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > In commit 6f974c843c ("gtk: overwrite the console.c char driver"), I > > shared the VC console parse handler with GTK. And later on in commit > > d8aec9d9 ("display: add -display spice-app launching a Spice client"), > > I also used it to handle spice-app VC. > > > > This is not necessary, the VC console options (width/height/cols/rows) > > are specific, and unused by tty-level GTK/Spice VC. > > > > This is not a breaking change, as those options are still being parsed > > by QAPI ChardevVC. Adjust the documentation about it. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > qapi/char.json | 4 ++++ > > include/chardev/char.h | 3 --- > > ui/console.c | 4 ++-- > > ui/gtk.c | 1 - > > ui/spice-app.c | 7 ++++++- > > 5 files changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/qapi/char.json b/qapi/char.json > > index 52aaff25eb..7e23fe3180 100644 > > --- a/qapi/char.json > > +++ b/qapi/char.json > > @@ -390,6 +390,10 @@ > > # > > # @rows: console height, in chars > > # > > +# Note: the options are only effective when using the built-in QEMU graphical VC > > +# (with the SDL display). When the VC is handled by the display backend (with > > +# GTK/VTE, Spice or D-Bus), they are simply ignored. > > I don't find this explains the situation very well, I had to look at > the code to understand what's ultimately going on, as I didn't really > know what it meant by "built-in QEMU graphical VC". From the end user's > POV, they're just using '-chardev vc...'. > > IIUC, the command line -chardev vc,..... will end up instantiating > TYPE_CHARDEV_VC. > > We actually then have 4 completely different implementations > of TYPE_CHARDEV_VC, and depending on which display backend > is enabled, a different TYPE_CHARDEV_VC will get registered. > > So what your comment is saying is that the width/height/rows/cols > properties will only get honoured if the TYPE_CHARDEV_VC that is > registered, is the one that maps to the SDL display backend. > > Wow, is this situation confusing and gross in the code :-( > > So for the comment I think we can just cut out a few words to > make it simpler > > # Note: the options are only effective when the SDL graphical > # display backend is active. They are ignored with the GTK, > # Spice, VNC and D-Bus display backends. Actually, VNC too. I adjusted the doc. thanks > > As a future exercise for anyone motiviated, I would say that > TYPE_CHARDEV_VC ought to be abstract and we then have subclasses > for each of the impls we have that are registered unconditionally > with type_init(), then pick the subclass to instantiate based > on the display backend. That way we can ultimately make the > QAPI schema reflect that we have multiple ChardevVC impls and > only expose the cols/width/etc for the SDL impl. > > > Anyway, if the comment is simplied/clarified then > > 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 :| > > -- Marc-André Lureau
diff --git a/qapi/char.json b/qapi/char.json index 52aaff25eb..7e23fe3180 100644 --- a/qapi/char.json +++ b/qapi/char.json @@ -390,6 +390,10 @@ # # @rows: console height, in chars # +# Note: the options are only effective when using the built-in QEMU graphical VC +# (with the SDL display). When the VC is handled by the display backend (with +# GTK/VTE, Spice or D-Bus), they are simply ignored. +# # Since: 1.5 ## { 'struct': 'ChardevVC', diff --git a/include/chardev/char.h b/include/chardev/char.h index 44cd82e405..01df55f9e8 100644 --- a/include/chardev/char.h +++ b/include/chardev/char.h @@ -320,7 +320,4 @@ GSource *qemu_chr_timeout_add_ms(Chardev *chr, guint ms, void suspend_mux_open(void); void resume_mux_open(void); -/* console.c */ -void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error **errp); - #endif diff --git a/ui/console.c b/ui/console.c index 7be2d4eef3..9fccecafd7 100644 --- a/ui/console.c +++ b/ui/console.c @@ -2708,7 +2708,7 @@ void qemu_display_help(void) } } -void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error **errp) +static void vc_chr_parse(QemuOpts *opts, ChardevBackend *backend, Error **errp) { int val; ChardevVC *vc; @@ -2746,7 +2746,7 @@ static void char_vc_class_init(ObjectClass *oc, void *data) { ChardevClass *cc = CHARDEV_CLASS(oc); - cc->parse = qemu_chr_parse_vc; + cc->parse = vc_chr_parse; cc->open = vc_chr_open; cc->chr_write = vc_chr_write; cc->chr_accept_input = vc_chr_accept_input; diff --git a/ui/gtk.c b/ui/gtk.c index 8ba41c8f13..ef98bb0648 100644 --- a/ui/gtk.c +++ b/ui/gtk.c @@ -1860,7 +1860,6 @@ static void char_gd_vc_class_init(ObjectClass *oc, void *data) { ChardevClass *cc = CHARDEV_CLASS(oc); - cc->parse = qemu_chr_parse_vc; cc->open = gd_vc_open; cc->chr_write = gd_vc_chr_write; cc->chr_accept_input = gd_vc_chr_accept_input; diff --git a/ui/spice-app.c b/ui/spice-app.c index ad7f0551ad..405fb7f9f5 100644 --- a/ui/spice-app.c +++ b/ui/spice-app.c @@ -96,6 +96,11 @@ static void vc_chr_set_echo(Chardev *chr, bool echo) /* TODO: set echo for frontends QMP and qtest */ } +static void vc_chr_parse(QemuOpts *opts, ChardevBackend *backend, Error **errp) +{ + /* fqdn is dealt with in vc_chr_open() */ +} + static void char_vc_class_init(ObjectClass *oc, void *data) { VCChardevClass *vc = CHARDEV_VC_CLASS(oc); @@ -103,7 +108,7 @@ static void char_vc_class_init(ObjectClass *oc, void *data) vc->parent_open = cc->open; - cc->parse = qemu_chr_parse_vc; + cc->parse = vc_chr_parse; cc->open = vc_chr_open; cc->chr_set_echo = vc_chr_set_echo; }