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

[Daniel P . Berrangé]

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
-- 
|: 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 :|