Re: [PATCH 43/67] ui/vc: do not parse VC-specific options in Spice and GTK

[Marc-André Lureau]

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