Re: [Qemu-devel] [PATCH 19/24] console: add and use qemu_display_find_default

[Darren Kenny]

Hi Gerd,

Thanks for clarifying things for me.

On Fri, Nov 17, 2017 at 02:24:54PM +0100, Gerd Hoffmann wrote:
>  Hi,
>
> > > -        dpy.type = DISPLAY_TYPE_NONE;
> > > +        if (!qemu_display_find_default(&dpy)) {
> > > +            dpy.type = DISPLAY_TYPE_NONE;
> > > +#if defined(CONFIG_VNC)
> > > +            vnc_parse("localhost:0,to=99,id=default", &error_abort);
> > > #endif
> >
> > Mostly some questions on this:
> >
> > - I'm curious, why is VNC not just one of the ones searched for in
> >  qemu_display_find_default()?
>
> vnc is a bit different.  It's a remote display protocol, not a (local)
> user interface.  There is no DISPLAY_TYPE_VNC for that reason (without
> this series too).  And IMO it was a bad idea to add -display vnc=$config
> (same as -vnc $config) in the first place.
>
> There can be only one user interface instance, i.e. you can't have gtk
> and sdl at the same time.  With vnc (and spice) that works though, i.e.
> you can start qemu with some local user interface and enable vnc or
> spice (even both) remote access at the same time.

Ah, wasn't sure about being able to do both a local UI and VNC -
I've only ever used one or the other :)

OK, so the odd thing then is the check for !remote_display earlier
on in the function (missing from the quote above) which seems to end
up initializing VNC (albeit with localhost) when CONFIG_VNC is
defined, but no other local display type has been selected.

But, I suppose it's not strictly incorrect with the 'localhost' :)

> > - What if there is another display type added? Would it be added to
> >  qemu_display_find_default() or in this if statement?
>
> qemu_display_find_default() most likely,
> but depends on what exactly it is.

OK, so neither SPICE or VNC really fit into this patchset because
they are seen as additional to the display type, not the actual
display type - SPICE is even more special too given the dependency
on a guest VM driver to really function correctly.

I wonder should there be a, similar, qemu_display_find_remote_default()
which would be used when no local display is defined. And that would
use a similar mechanism to permit selecting VNC or SPICE via
registration mechanisms over rather than the #ifdef style for remote
displays that will remain ever after this patchset (certainly could
be another patchset beyond this one).

Maybe it's not worth it, given that VNC is really he only remote UI
module you can use.

> > - Is there a reason that VNC doesn't follow the same registration
> >  mechanism you're proposing here (as in you've no changes to that
> >  module in this patch set).
>
> See above ;)
>
> > - In the spirit of encapsulation, would it not make sense for the
> >  vnc_parse() to be in the VNC code itself as a fall back if no
> >  specific options are given to the -vnc option?
>
> It's not that simple.  The default vnc server is started only in case
> no (graphical) local user interface is available, so it's not something
> the vnc code can figure purely on its own.

Fair enough.

Just a thought, but maybe the specific string could be defined in
vnc.h instead of a string here at least? (e.g.
VNC_DEFAULT_LOCALHOST_OPTIONS, or similar).

Thanks,

Darren.