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

[Darren Kenny]

On Fri, Nov 17, 2017 at 11:30:41AM +0100, Gerd Hoffmann wrote:
> Using the new registry instead of #ifdefs in vl.c.
>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
> include/ui/console.h |  1 +
> ui/console.c         | 19 +++++++++++++++++++
> vl.c                 | 15 +++++----------
> 3 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 6c89599355..7c9e40cc9a 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -442,6 +442,7 @@ struct QemuDisplay {
> };
>
> void qemu_display_register(QemuDisplay *ui);
> +bool qemu_display_find_default(DisplayOptions *opts);
> void qemu_display_early_init(DisplayOptions *opts);
> void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>
> diff --git a/ui/console.c b/ui/console.c
> index aa8afbfe26..4c9ac8b21a 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -2176,6 +2176,25 @@ void qemu_display_register(QemuDisplay *ui)
>     dpys[ui->type] = ui;
> }
>
> +bool qemu_display_find_default(DisplayOptions *opts)
> +{
> +    static DisplayType prio[] = {
> +        DISPLAY_TYPE_GTK,
> +        DISPLAY_TYPE_SDL,
> +        DISPLAY_TYPE_COCOA
> +    };
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(prio); i++) {
> +        if (dpys[prio[i]] == NULL) {
> +            continue;
> +        }
> +        opts->type = prio[i];
> +        return true;
> +    }
> +    return false;
> +}
> +
> void qemu_display_early_init(DisplayOptions *opts)
> {
>     assert(opts->type < DISPLAY_TYPE__MAX);
> diff --git a/vl.c b/vl.c
> index df0c10398d..f861562bb2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4406,17 +4406,12 @@ int main(int argc, char **argv, char **envp)
>     }
> #endif
>     if (dpy.type == DISPLAY_TYPE_DEFAULT && !display_remote) {
> -#if defined(CONFIG_GTK)
> -        dpy.type = DISPLAY_TYPE_GTK;
> -#elif defined(CONFIG_SDL)
> -        dpy.type = DISPLAY_TYPE_SDL;
> -#elif defined(CONFIG_COCOA)
> -        dpy.type = DISPLAY_TYPE_COCOA;
> -#elif defined(CONFIG_VNC)
> -        vnc_parse("localhost:0,to=99,id=default", &error_abort);
> -#else
> -        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()? 

- What if there is another display type added? Would it be added to
 qemu_display_find_default() or in this if statement?

- 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).

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

Thanks,

Darren.