| Message ID | 1363169922-24335-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
Gerd Hoffmann <kraxel@redhat.com> writes: > Kill the dirty hack which hooks gtk vte initialization into the qemu > consoles subsystem. The vte terminals are not related to qemu consoles > at all. This simply doesn't belong there and it stands in the way when > cleaning up the qemu consoles subsystem. So fix it up, quickly, before > it sneaks into a release. My only concern is that if someone had a command line like: qemu -serial stdio -monitor vc It now breaks with your series. 'vc' is a graphical chardev and it should be up to what UI layer to decide how to express it. That's the idea behind the hooks. What's the issue your having with console cleanup? Regards, Anthony Liguori > > Make vtes normal chardev backends, like everybody else is. Tweak the > default chardevs accordingly, so you'll get monitor+serial on vte > when using gtk. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > > To be applied on top of the chardev qapi conversion patch series > > include/char/char.h | 6 ++++++ > include/ui/console.h | 5 ----- > qapi-schema.json | 1 + > qemu-char.c | 9 ++++++++- > ui/console.c | 14 +------------- > ui/gtk.c | 7 +------ > vl.c | 44 ++++++++++++++++++++------------------------ > 7 files changed, 37 insertions(+), 49 deletions(-) > > diff --git a/include/char/char.h b/include/char/char.h > index d6a0351..09edfcc 100644 > --- a/include/char/char.h > +++ b/include/char/char.h > @@ -267,4 +267,10 @@ CharDriverState *qemu_chr_open_msmouse(void); > /* baum.c */ > CharDriverState *chr_baum_init(void); > > +/* console.c */ > +CharDriverState *text_console_init(ChardevVC *vc); > + > +/* gtk.c */ > +CharDriverState *gd_vte_init(void); > + > #endif > diff --git a/include/ui/console.h b/include/ui/console.h > index a37cf65..7ae32aa 100644 > --- a/include/ui/console.h > +++ b/include/ui/console.h > @@ -450,11 +450,6 @@ void qemu_console_resize(DisplayState *ds, int width, int height); > void qemu_console_copy(DisplayState *ds, int src_x, int src_y, > int dst_x, int dst_y, int w, int h); > > -typedef CharDriverState *(VcHandler)(ChardevVC *vc); > - > -CharDriverState *vc_init(ChardevVC *vc); > -void register_vc_handler(VcHandler *handler); > - > /* sdl.c */ > void sdl_display_init(DisplayState *ds, int full_screen, int no_frame); > > diff --git a/qapi-schema.json b/qapi-schema.json > index fdaa9da..c26ffd2 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3296,6 +3296,7 @@ > 'spicevmc' : 'ChardevSpiceChannel', > 'spiceport' : 'ChardevSpicePort', > 'vc' : 'ChardevVC', > + 'vte' : 'ChardevDummy', > 'memory' : 'ChardevRingbuf' } } > > ## > diff --git a/qemu-char.c b/qemu-char.c > index e633797..e18728a 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -3027,6 +3027,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) > strcmp(filename, "pty") == 0 || > strcmp(filename, "msmouse") == 0 || > strcmp(filename, "braille") == 0 || > + strcmp(filename, "vte") == 0 || > strcmp(filename, "stdio") == 0) { > qemu_opt_set(opts, "backend", filename); > return opts; > @@ -3758,8 +3759,13 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, > break; > #endif > case CHARDEV_BACKEND_KIND_VC: > - chr = vc_init(backend->vc); > + chr = text_console_init(backend->vc); > break; > +#ifdef CONFIG_GTK > + case CHARDEV_BACKEND_KIND_VTE: > + chr = gd_vte_init(); > + break; > +#endif > case CHARDEV_BACKEND_KIND_MEMORY: > chr = qemu_chr_open_ringbuf(backend->memory, errp); > break; > @@ -3823,6 +3829,7 @@ static void register_types(void) > register_char_driver_qapi("console", CHARDEV_BACKEND_KIND_CONSOLE, NULL); > register_char_driver_qapi("pipe", CHARDEV_BACKEND_KIND_PIPE, > qemu_chr_parse_pipe); > + register_char_driver_qapi("vte", CHARDEV_BACKEND_KIND_VTE, NULL); > } > > type_init(register_types); > diff --git a/ui/console.c b/ui/console.c > index 27e87f8..820ec6a 100644 > --- a/ui/console.c > +++ b/ui/console.c > @@ -1537,7 +1537,7 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds) > chr->init(chr); > } > > -static CharDriverState *text_console_init(ChardevVC *vc) > +CharDriverState *text_console_init(ChardevVC *vc) > { > CharDriverState *chr; > QemuConsole *s; > @@ -1577,18 +1577,6 @@ static CharDriverState *text_console_init(ChardevVC *vc) > return chr; > } > > -static VcHandler *vc_handler = text_console_init; > - > -CharDriverState *vc_init(ChardevVC *vc) > -{ > - return vc_handler(vc); > -} > - > -void register_vc_handler(VcHandler *handler) > -{ > - vc_handler = handler; > -} > - > void text_consoles_set_display(DisplayState *ds) > { > int i; > diff --git a/ui/gtk.c b/ui/gtk.c > index 794dab1..76111b4 100644 > --- a/ui/gtk.c > +++ b/ui/gtk.c > @@ -991,7 +991,7 @@ static int gd_vc_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > static int nb_vcs; > static CharDriverState *vcs[MAX_VCS]; > > -static CharDriverState *gd_vc_handler(ChardevVC *unused) > +CharDriverState *gd_vte_init(void) > { > CharDriverState *chr; > > @@ -1003,11 +1003,6 @@ static CharDriverState *gd_vc_handler(ChardevVC *unused) > return chr; > } > > -void early_gtk_display_init(void) > -{ > - register_vc_handler(gd_vc_handler); > -} > - > static gboolean gd_vc_in(GIOChannel *chan, GIOCondition cond, void *opaque) > { > VirtualConsole *vc = opaque; > diff --git a/vl.c b/vl.c > index a621aec..2efa1eb 100644 > --- a/vl.c > +++ b/vl.c > @@ -4016,6 +4016,19 @@ int main(int argc, char **argv, char **envp) > #endif > } > > + if (display_type == DT_DEFAULT && !display_remote) { > +#if defined(CONFIG_GTK) > + display_type = DT_GTK; > +#elif defined(CONFIG_SDL) || defined(CONFIG_COCOA) > + display_type = DT_SDL; > +#elif defined(CONFIG_VNC) > + vnc_display = "localhost:0,to=99"; > + show_vnc_port = 1; > +#else > + display_type = DT_NONE; > +#endif > + } > + > if (display_type == DT_NOGRAPHIC) { > if (default_parallel) > add_device_config(DEV_PARALLEL, "null"); > @@ -4037,38 +4050,21 @@ int main(int argc, char **argv, char **envp) > monitor_parse("stdio", "readline"); > } > } else { > + const char *chardev = (display_type == DT_GTK) > + ? "vte" : "vc:80Cx24C"; > if (default_serial) > - add_device_config(DEV_SERIAL, "vc:80Cx24C"); > + add_device_config(DEV_SERIAL, chardev); > if (default_parallel) > - add_device_config(DEV_PARALLEL, "vc:80Cx24C"); > + add_device_config(DEV_PARALLEL, chardev); > if (default_monitor) > - monitor_parse("vc:80Cx24C", "readline"); > + monitor_parse(chardev, "readline"); > if (default_virtcon) > - add_device_config(DEV_VIRTCON, "vc:80Cx24C"); > + add_device_config(DEV_VIRTCON, chardev); > if (default_sclp) { > - add_device_config(DEV_SCLP, "vc:80Cx24C"); > + add_device_config(DEV_SCLP, chardev); > } > } > > - if (display_type == DT_DEFAULT && !display_remote) { > -#if defined(CONFIG_GTK) > - display_type = DT_GTK; > -#elif defined(CONFIG_SDL) || defined(CONFIG_COCOA) > - display_type = DT_SDL; > -#elif defined(CONFIG_VNC) > - vnc_display = "localhost:0,to=99"; > - show_vnc_port = 1; > -#else > - display_type = DT_NONE; > -#endif > - } > - > -#if defined(CONFIG_GTK) > - if (display_type == DT_GTK) { > - early_gtk_display_init(); > - } > -#endif > - > socket_init(); > > if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0) > -- > 1.7.9.7
On 03/13/13 13:29, Anthony Liguori wrote: > Gerd Hoffmann <kraxel@redhat.com> writes: > >> Kill the dirty hack which hooks gtk vte initialization into the >> qemu consoles subsystem. The vte terminals are not related to qemu >> consoles at all. This simply doesn't belong there and it stands in >> the way when cleaning up the qemu consoles subsystem. So fix it >> up, quickly, before it sneaks into a release. > > > My only concern is that if someone had a command line like: > > qemu -serial stdio -monitor vc > > It now breaks with your series. Yes. I'm not worried. The switch to gtk is a pretty big one anyway with a bunch of user interface changes. If we want avoid surprises for the user at any cost we must not make gtk the default ui. Also: Once I'm done with the console cleanup it will be easy to make 'vc' actually work with gtk. > 'vc' is a graphical chardev and it should be up to what UI layer to > decide how to express it No. 'vc' is a DisplaySurface filled by the qemu terminal emulation. It's generic and works with any UI. 'vte' is a chardev data stream linked up to the gtk vte widget. Obviously works with gtk only. > What's the issue your having with console cleanup? It just doesn't fit into the QemuConsoles at all, and with multiple UI's active (such as gtk+vnc at the same time) it doesn't make sense any more. cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: > On 03/13/13 13:29, Anthony Liguori wrote: >> Gerd Hoffmann <kraxel@redhat.com> writes: >> >>> Kill the dirty hack which hooks gtk vte initialization into the >>> qemu consoles subsystem. The vte terminals are not related to qemu >>> consoles at all. This simply doesn't belong there and it stands in >>> the way when cleaning up the qemu consoles subsystem. So fix it >>> up, quickly, before it sneaks into a release. >> >> >> My only concern is that if someone had a command line like: >> >> qemu -serial stdio -monitor vc >> >> It now breaks with your series. > > Yes. I'm not worried. The switch to gtk is a pretty big one anyway > with a bunch of user interface changes. If we want avoid surprises for > the user at any cost we must not make gtk the default ui. > > Also: Once I'm done with the console cleanup it will be easy to make > 'vc' actually work with gtk. Please no. 'vc' needs to die. I don't want anyone using it... >> 'vc' is a graphical chardev and it should be up to what UI layer to >> decide how to express it > > No. > > 'vc' is a DisplaySurface filled by the qemu terminal emulation. It's > generic and works with any UI. QEMU shouldn't be in the terminal emulation business. > 'vte' is a chardev data stream linked up to the gtk vte widget. > Obviously works with gtk only. > >> What's the issue your having with console cleanup? > > It just doesn't fit into the QemuConsoles at all, and with multiple UI's > active (such as gtk+vnc at the same time) it doesn't make sense any > more. Okay, that makes a bit more sense. > > cheers, > Gerd
Hi, >> Also: Once I'm done with the console cleanup it will be easy to make >> 'vc' actually work with gtk. > > Please no. 'vc' needs to die. I don't want anyone using it... Likewise easy. >>> 'vc' is a graphical chardev and it should be up to what UI layer to >>> decide how to express it >> >> No. >> >> 'vc' is a DisplaySurface filled by the qemu terminal emulation. It's >> generic and works with any UI. > > QEMU shouldn't be in the terminal emulation business. Well, it already is. Not that I want extend it or would recommend using it if there are better options. But there is no reason to break it, and it is useful to me now and then. cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: > Hi, > >>> Also: Once I'm done with the console cleanup it will be easy to make >>> 'vc' actually work with gtk. >> >> Please no. 'vc' needs to die. I don't want anyone using it... > > Likewise easy. > >>>> 'vc' is a graphical chardev and it should be up to what UI layer to >>>> decide how to express it >>> >>> No. >>> >>> 'vc' is a DisplaySurface filled by the qemu terminal emulation. It's >>> generic and works with any UI. >> >> QEMU shouldn't be in the terminal emulation business. > > Well, it already is. Not that I want extend it or would recommend using > it if there are better options. But there is no reason to break it, and > it is useful to me now and then. qemu -display sdl -vnc :1 -serial vc Continues to do what it did before. qemu -display gtk -vnc :1 -serial vc Seg faults which is bad. I can look more deeply into that. But with your proposal, the above would either (1) use the old style console in GTK or (2) not make the console available in gtk. Both are bad because they have consequences for: qemu -serial vc The above must continue to have a reasonable behavior by default and with GTK as the default, that means using VTE as the terminal emulation. If you can find a way to make 'qemu -display gtk -vnc :1 -serial vc' behave without impacting the other use-cases, I'm open to that but just renaming 'vc' to 'vte' for gtk is going to be a compat breaker. Regards, Anthony Liguori > > cheers, > Gerd
Hi, >> Well, it already is. Not that I want extend it or would recommend using >> it if there are better options. But there is no reason to break it, and >> it is useful to me now and then. > > qemu -display sdl -vnc :1 -serial vc > > Continues to do what it did before. Yes. > qemu -display gtk -vnc :1 -serial vc > > Seg faults which is bad. I can look more deeply into that. The "console: data structures overhaul" patch series posted earlier this week fixes this. > But with your proposal, the above would either (1) use the old style > console in GTK or (2) not make the console available in gtk. Yes. If you want the text consoles on vnc you must use 'vc'. We then can decide to show or not show the 'vc' consoles with gtk too, at the end this will be just two lines of code. If you don't care about text consoles on vnc you can pic 'vte'. Then they will show up on gtk only. And having separate chardevs for 'vc' and 'vte' empowers the user to pick what he wants. > Both are bad because they have consequences for: > > qemu -serial vc > > The above must continue to have a reasonable behavior by default and > with GTK as the default, that means using VTE as the terminal emulation. Why? We can wind up vc text consoles for gtk, then the above will give you a text console. As compatible as possible, as it will be the exactly same thing you got in qemu 1.4 with sdl as default ui. If you want the new features, use the new options. That always has been the policy with qemu, I fail to see why gtk should be different here. > If you can find a way to make 'qemu -display gtk -vnc :1 -serial vc' > behave without impacting the other use-cases, I'm open to that but just > renaming 'vc' to 'vte' for gtk is going to be a compat breaker. IMO current gtk code breaks compat by hijacking 'vc'. cheers, Gerd
diff --git a/include/char/char.h b/include/char/char.h index d6a0351..09edfcc 100644 --- a/include/char/char.h +++ b/include/char/char.h @@ -267,4 +267,10 @@ CharDriverState *qemu_chr_open_msmouse(void); /* baum.c */ CharDriverState *chr_baum_init(void); +/* console.c */ +CharDriverState *text_console_init(ChardevVC *vc); + +/* gtk.c */ +CharDriverState *gd_vte_init(void); + #endif diff --git a/include/ui/console.h b/include/ui/console.h index a37cf65..7ae32aa 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -450,11 +450,6 @@ void qemu_console_resize(DisplayState *ds, int width, int height); void qemu_console_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int dst_y, int w, int h); -typedef CharDriverState *(VcHandler)(ChardevVC *vc); - -CharDriverState *vc_init(ChardevVC *vc); -void register_vc_handler(VcHandler *handler); - /* sdl.c */ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame); diff --git a/qapi-schema.json b/qapi-schema.json index fdaa9da..c26ffd2 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3296,6 +3296,7 @@ 'spicevmc' : 'ChardevSpiceChannel', 'spiceport' : 'ChardevSpicePort', 'vc' : 'ChardevVC', + 'vte' : 'ChardevDummy', 'memory' : 'ChardevRingbuf' } } ## diff --git a/qemu-char.c b/qemu-char.c index e633797..e18728a 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -3027,6 +3027,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) strcmp(filename, "pty") == 0 || strcmp(filename, "msmouse") == 0 || strcmp(filename, "braille") == 0 || + strcmp(filename, "vte") == 0 || strcmp(filename, "stdio") == 0) { qemu_opt_set(opts, "backend", filename); return opts; @@ -3758,8 +3759,13 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, break; #endif case CHARDEV_BACKEND_KIND_VC: - chr = vc_init(backend->vc); + chr = text_console_init(backend->vc); break; +#ifdef CONFIG_GTK + case CHARDEV_BACKEND_KIND_VTE: + chr = gd_vte_init(); + break; +#endif case CHARDEV_BACKEND_KIND_MEMORY: chr = qemu_chr_open_ringbuf(backend->memory, errp); break; @@ -3823,6 +3829,7 @@ static void register_types(void) register_char_driver_qapi("console", CHARDEV_BACKEND_KIND_CONSOLE, NULL); register_char_driver_qapi("pipe", CHARDEV_BACKEND_KIND_PIPE, qemu_chr_parse_pipe); + register_char_driver_qapi("vte", CHARDEV_BACKEND_KIND_VTE, NULL); } type_init(register_types); diff --git a/ui/console.c b/ui/console.c index 27e87f8..820ec6a 100644 --- a/ui/console.c +++ b/ui/console.c @@ -1537,7 +1537,7 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds) chr->init(chr); } -static CharDriverState *text_console_init(ChardevVC *vc) +CharDriverState *text_console_init(ChardevVC *vc) { CharDriverState *chr; QemuConsole *s; @@ -1577,18 +1577,6 @@ static CharDriverState *text_console_init(ChardevVC *vc) return chr; } -static VcHandler *vc_handler = text_console_init; - -CharDriverState *vc_init(ChardevVC *vc) -{ - return vc_handler(vc); -} - -void register_vc_handler(VcHandler *handler) -{ - vc_handler = handler; -} - void text_consoles_set_display(DisplayState *ds) { int i; diff --git a/ui/gtk.c b/ui/gtk.c index 794dab1..76111b4 100644 --- a/ui/gtk.c +++ b/ui/gtk.c @@ -991,7 +991,7 @@ static int gd_vc_chr_write(CharDriverState *chr, const uint8_t *buf, int len) static int nb_vcs; static CharDriverState *vcs[MAX_VCS]; -static CharDriverState *gd_vc_handler(ChardevVC *unused) +CharDriverState *gd_vte_init(void) { CharDriverState *chr; @@ -1003,11 +1003,6 @@ static CharDriverState *gd_vc_handler(ChardevVC *unused) return chr; } -void early_gtk_display_init(void) -{ - register_vc_handler(gd_vc_handler); -} - static gboolean gd_vc_in(GIOChannel *chan, GIOCondition cond, void *opaque) { VirtualConsole *vc = opaque; diff --git a/vl.c b/vl.c index a621aec..2efa1eb 100644 --- a/vl.c +++ b/vl.c @@ -4016,6 +4016,19 @@ int main(int argc, char **argv, char **envp) #endif } + if (display_type == DT_DEFAULT && !display_remote) { +#if defined(CONFIG_GTK) + display_type = DT_GTK; +#elif defined(CONFIG_SDL) || defined(CONFIG_COCOA) + display_type = DT_SDL; +#elif defined(CONFIG_VNC) + vnc_display = "localhost:0,to=99"; + show_vnc_port = 1; +#else + display_type = DT_NONE; +#endif + } + if (display_type == DT_NOGRAPHIC) { if (default_parallel) add_device_config(DEV_PARALLEL, "null"); @@ -4037,38 +4050,21 @@ int main(int argc, char **argv, char **envp) monitor_parse("stdio", "readline"); } } else { + const char *chardev = (display_type == DT_GTK) + ? "vte" : "vc:80Cx24C"; if (default_serial) - add_device_config(DEV_SERIAL, "vc:80Cx24C"); + add_device_config(DEV_SERIAL, chardev); if (default_parallel) - add_device_config(DEV_PARALLEL, "vc:80Cx24C"); + add_device_config(DEV_PARALLEL, chardev); if (default_monitor) - monitor_parse("vc:80Cx24C", "readline"); + monitor_parse(chardev, "readline"); if (default_virtcon) - add_device_config(DEV_VIRTCON, "vc:80Cx24C"); + add_device_config(DEV_VIRTCON, chardev); if (default_sclp) { - add_device_config(DEV_SCLP, "vc:80Cx24C"); + add_device_config(DEV_SCLP, chardev); } } - if (display_type == DT_DEFAULT && !display_remote) { -#if defined(CONFIG_GTK) - display_type = DT_GTK; -#elif defined(CONFIG_SDL) || defined(CONFIG_COCOA) - display_type = DT_SDL; -#elif defined(CONFIG_VNC) - vnc_display = "localhost:0,to=99"; - show_vnc_port = 1; -#else - display_type = DT_NONE; -#endif - } - -#if defined(CONFIG_GTK) - if (display_type == DT_GTK) { - early_gtk_display_init(); - } -#endif - socket_init(); if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
Kill the dirty hack which hooks gtk vte initialization into the qemu consoles subsystem. The vte terminals are not related to qemu consoles at all. This simply doesn't belong there and it stands in the way when cleaning up the qemu consoles subsystem. So fix it up, quickly, before it sneaks into a release. Make vtes normal chardev backends, like everybody else is. Tweak the default chardevs accordingly, so you'll get monitor+serial on vte when using gtk. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- To be applied on top of the chardev qapi conversion patch series include/char/char.h | 6 ++++++ include/ui/console.h | 5 ----- qapi-schema.json | 1 + qemu-char.c | 9 ++++++++- ui/console.c | 14 +------------- ui/gtk.c | 7 +------ vl.c | 44 ++++++++++++++++++++------------------------ 7 files changed, 37 insertions(+), 49 deletions(-)