Re: [Qemu-devel] [PATCH 1/2][RESEND] vl: initialize all displaystates

[andrzej zaborowski]

On 4 March 2011 01:48, Dmitry Eremin-Solenikov <address@hidden> wrote:
> Init not only first displaystate, but all. Otherwise machines with
> multiple display devices (e.g. tosa, as it exists now) will just
> segfault on ds switch.
>
> Signed-off-by: Dmitry Eremin-Solenikov <address@hidden>
> ---
>  vl.c |  104 
> +++++++++++++++++++++++++++++++++---------------------------------
>  1 files changed, 52 insertions(+), 52 deletions(-)
>
>  Basically this patch is equal to:
>  @@ -3009,9 +3009,7 @@ int main(int argc, char **argv, char **envp)
>
>      net_check_clients();
>
>  -    /* just use the first displaystate for the moment */
>  -    ds = get_displaystate();
>  -
>  +    for (ds = get_displaystate(); ds; ds = ds->next) {
>      if (using_spice)
>          display_remote++;
>      if (display_type == DT_DEFAULT && !display_remote) {
>  @@ -3077,7 +3075,9 @@ int main(int argc, char **argv, char **envp)
>          nographic_timer = qemu_new_timer(rt_clock, nographic_update, NULL);
>          qemu_mod_timer(nographic_timer, qemu_get_clock(rt_clock));
>      }
>  -    text_consoles_set_display(ds);
>  +    }
>  +
>  +    text_consoles_set_display(get_displaystate());
>
>      if (gdbstub_dev && gdbserver_start(gdbstub_dev) < 0) {
>          fprintf(stderr, "qemu: could not open gdbserver on device '%s'\n",
>
> diff --git a/vl.c b/vl.c
> index 14255c4..b8cd455 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3009,75 +3009,75 @@ int main(int argc, char **argv, char **envp)
>
>     net_check_clients();
>
> -    /* just use the first displaystate for the moment */
> -    ds = get_displaystate();
> -
> -    if (using_spice)
> -        display_remote++;
> -    if (display_type == DT_DEFAULT && !display_remote) {
> +    for (ds = get_displaystate(); ds; ds = ds->next) {
> +        if (using_spice)
> +            display_remote++;
> +        if (display_type == DT_DEFAULT && !display_remote) {
>  #if defined(CONFIG_SDL) || defined(CONFIG_COCOA)
> -        display_type = DT_SDL;
> +            display_type = DT_SDL;
>  #else
> -        vnc_display = "localhost:0,to=99";
> -        show_vnc_port = 1;
> +            vnc_display = "localhost:0,to=99";
> +            show_vnc_port = 1;
>  #endif
> -    }
> -
> +        }
> +
>
> -    /* init local displays */
> -    switch (display_type) {
> -    case DT_NOGRAPHIC:
> -        break;
> +        /* init local displays */
> +        switch (display_type) {
> +        case DT_NOGRAPHIC:
> +            break;
>  #if defined(CONFIG_CURSES)
> -    case DT_CURSES:
> -        curses_display_init(ds, full_screen);
> -        break;
> +        case DT_CURSES:
> +            curses_display_init(ds, full_screen);
> +            break;
>  #endif
>  #if defined(CONFIG_SDL)
> -    case DT_SDL:
> -        sdl_display_init(ds, full_screen, no_frame);
> -        break;
> +        case DT_SDL:
> +            sdl_display_init(ds, full_screen, no_frame);
> +            break;
>  #elif defined(CONFIG_COCOA)
> -    case DT_SDL:
> -        cocoa_display_init(ds, full_screen);
> -        break;
> +        case DT_SDL:
> +            cocoa_display_init(ds, full_screen);
> +            break;

I'm not sure this will work as intended, I think we shouldn't call
curses/sdl/cocoa_display_init() for every display state, we should
just call register_displaychangelistener() etc. for each display
state.  My assumption is that we want each ds to appear as a graphical
console in the same window, not open N SDL windows / VNC servers (for
curses that would break completely I think).

Either way I'd like more people to comment on this.

Cheers