On Tue, Jun 04, 2013 at 11:58:12AM +0200, Hans de Goede wrote:
> Hi,
>
> On 06/03/2013 10:25 PM, Michael Roth wrote:
>
> <snip>
>
> >To fix these, let's just drop the BH.
> >
> >Since the initial reasoning for using it still applies to an extent,
> >work around that by deferring the delivery of CHR_EVENT_OPENED until
> >after the chardevs have been fully initialized, toward the end of
> >qmp_chardev_add() (or some cases, qemu_chr_new_from_opts()). This
> >defers delivery long enough that we can be assured a CharDriverState
> >is fully initialized before CHR_EVENT_OPENED is sent.
> >
> >Also, rather than requiring each chardev to do an explicit open, do it
> >automatically, and allow the small few who don't desire such behavior to
> >suppress the OPENED-on-init behavior by setting a
> >'suppress_be_open_on_init' flag.
> >
> >We additionally add missing OPENED events for stdio backends on w32,
> >which were previously not being issued, causing us to not recieve the
> >banner and initial prompts for qmp/hmp.
>
> 2 remarks:
>
> 1) We already have something very similar to suppress_be_open_on_init
> on the frontend side, it is called explicit_fe_open, I suggest calling
> the backend variant explicit_be_open for consistency
I think the naming is a bit confusing at first glance, but the behavior
does seem consistent with what we're trying to do for backends, so I'll
go ahead and make the change for v4.
>
> 2) It would be good to do a full audit to check if you've caught all
> backends which need suppress_be_open_on_init:
> a) check all callers of register_char_driver*
> b) check if their open method calls qemu_chr_be_generic_open
> c) if it does not they need suppress_be_open_on_init
>
Agreed; that was the intention here, but it looks like I missed some.
> You've missed at least the spicevmc & spiceport backends,
> which both need suppress_be_open_on_init
Looks like we need one for qemu_chr_open_msmouse() as well. Will fix
these in v4.
Thanks!
>
> Regards,
>
> Hans
>
>
>
>
> >
> >Reported-by: Stefan Priebe <s.pri...@profihost.ag>
> >Cc: qemu-sta...@nongnu.org
> >Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> >---
> >v2->v3:
> > * removed artifact in from v1 in backends/baum.c, test build with
> > BRLAPI=y (Anthony)
> > * rebased on latest origin/master
> >
> >v1->v2:
> > * default to sending OPENED on backend init, add flag to suppress
> > it (Anthony)
> > * fix missing OPENED for stdio backends on w32
> > * fix missing OPENED when qemu_chr_new_from_opts() doesn't use
> > qmp_chardev_add()
> > * clean up/update commit message
> >
> > backends/baum.c | 2 --
> > include/sysemu/char.h | 2 +-
> > qemu-char.c | 38 +++++++++++++++++---------------------
> > ui/console.c | 1 -
> > ui/gtk.c | 1 -
> > 5 files changed, 18 insertions(+), 26 deletions(-)
> >
> >diff --git a/backends/baum.c b/backends/baum.c
> >index 4cba79f..62aa784 100644
> >--- a/backends/baum.c
> >+++ b/backends/baum.c
> >@@ -611,8 +611,6 @@ CharDriverState *chr_baum_init(void)
> >
> > qemu_set_fd_handler(baum->brlapi_fd, baum_chr_read, NULL, baum);
> >
> >- qemu_chr_be_generic_open(chr);
> >-
> > return chr;
> >
> > fail:
> >diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> >index 5e42c90..b0ae749 100644
> >--- a/include/sysemu/char.h
> >+++ b/include/sysemu/char.h
> >@@ -70,13 +70,13 @@ struct CharDriverState {
> > void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
> > void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
> > void *opaque;
> >- int idle_tag;
> > char *label;
> > char *filename;
> > int be_open;
> > int fe_open;
> > int explicit_fe_open;
> > int avail_connections;
> >+ bool suppress_be_open_on_init;
> > QemuOpts *opts;
> > QTAILQ_ENTRY(CharDriverState) next;
> > };
> >diff --git a/qemu-char.c b/qemu-char.c
> >index d04b429..8043f86 100644
> >--- a/qemu-char.c
> >+++ b/qemu-char.c
> >@@ -110,19 +110,9 @@ void qemu_chr_be_event(CharDriverState *s, int event)
> > s->chr_event(s->handler_opaque, event);
> > }
> >
> >-static gboolean qemu_chr_be_generic_open_bh(gpointer opaque)
> >-{
> >- CharDriverState *s = opaque;
> >- qemu_chr_be_event(s, CHR_EVENT_OPENED);
> >- s->idle_tag = 0;
> >- return FALSE;
> >-}
> >-
> > void qemu_chr_be_generic_open(CharDriverState *s)
> > {
> >- if (s->idle_tag == 0) {
> >- s->idle_tag = g_idle_add(qemu_chr_be_generic_open_bh, s);
> >- }
> >+ qemu_chr_be_event(s, CHR_EVENT_OPENED);
> > }
> >
> > int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
> >@@ -247,6 +237,7 @@ static CharDriverState *qemu_chr_open_null(void)
> >
> > chr = g_malloc0(sizeof(CharDriverState));
> > chr->chr_write = null_chr_write;
> >+ chr->suppress_be_open_on_init = true;
> > return chr;
> > }
> >
> >@@ -504,9 +495,6 @@ static CharDriverState
> >*qemu_chr_open_mux(CharDriverState *drv)
> > /* Frontend guest-open / -close notification is not support with muxes
> > */
> > chr->chr_set_fe_open = NULL;
> >
> >- /* Muxes are always open on creation */
> >- qemu_chr_be_generic_open(chr);
> >-
> > return chr;
> > }
> >
> >@@ -883,8 +871,6 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int
> >fd_out)
> > chr->chr_update_read_handler = fd_chr_update_read_handler;
> > chr->chr_close = fd_chr_close;
> >
> >- qemu_chr_be_generic_open(chr);
> >-
> > return chr;
> > }
> >
> >@@ -1243,6 +1229,7 @@ static CharDriverState *qemu_chr_open_pty(const char
> >*id,
> > chr->chr_update_read_handler = pty_chr_update_read_handler;
> > chr->chr_close = pty_chr_close;
> > chr->chr_add_watch = pty_chr_add_watch;
> >+ chr->suppress_be_open_on_init = true;
> >
> > s->fd = io_channel_from_fd(master_fd);
> > s->timer_tag = 0;
> >@@ -1595,8 +1582,6 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
> > chr->chr_close = pp_close;
> > chr->opaque = drv;
> >
> >- qemu_chr_be_generic_open(chr);
> >-
> > return chr;
> > }
> > #endif /* __linux__ */
> >@@ -1650,6 +1635,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
> > chr->opaque = (void *)(intptr_t)fd;
> > chr->chr_write = null_chr_write;
> > chr->chr_ioctl = pp_ioctl;
> >+ chr->suppress_be_open_on_init = true;
> > return chr;
> > }
> > #endif
> >@@ -1880,7 +1866,6 @@ static CharDriverState *qemu_chr_open_win_path(const
> >char *filename)
> > g_free(chr);
> > return NULL;
> > }
> >- qemu_chr_be_generic_open(chr);
> > return chr;
> > }
> >
> >@@ -1980,7 +1965,6 @@ static CharDriverState
> >*qemu_chr_open_pipe(ChardevHostdev *opts)
> > g_free(chr);
> > return NULL;
> > }
> >- qemu_chr_be_generic_open(chr);
> > return chr;
> > }
> >
> >@@ -1994,7 +1978,6 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE
> >fd_out)
> > s->hcom = fd_out;
> > chr->opaque = s;
> > chr->chr_write = win_chr_write;
> >- qemu_chr_be_generic_open(chr);
> > return chr;
> > }
> >
> >@@ -2329,6 +2312,8 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
> > chr->chr_write = udp_chr_write;
> > chr->chr_update_read_handler = udp_chr_update_read_handler;
> > chr->chr_close = udp_chr_close;
> >+ /* be isn't opened until we get a connection */
> >+ chr->suppress_be_open_on_init = true;
> > return chr;
> > }
> >
> >@@ -2731,6 +2716,8 @@ static CharDriverState *qemu_chr_open_socket_fd(int
> >fd, bool do_nodelay,
> > chr->get_msgfd = tcp_get_msgfd;
> > chr->chr_add_client = tcp_chr_add_client;
> > chr->chr_add_watch = tcp_chr_add_watch;
> >+ /* be isn't opened until we get a connection */
> >+ chr->suppress_be_open_on_init = true;
> >
> > if (is_listen) {
> > s->listen_fd = fd;
> >@@ -3325,6 +3312,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts
> >*opts,
> > if (!chr->filename)
> > chr->filename = g_strdup(qemu_opt_get(opts, "backend"));
> > chr->init = init;
> >+ /* if we didn't create the chardev via qmp_chardev_add, we
> >+ * need to send the OPENED event here
> >+ */
> >+ if (!chr->suppress_be_open_on_init) {
> >+ qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> >+ }
> > QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> >
> > if (qemu_opt_get_bool(opts, "mux", 0)) {
> >@@ -3804,6 +3797,9 @@ ChardevReturn *qmp_chardev_add(const char *id,
> >ChardevBackend *backend,
> > if (!chr->filename) {
> > chr->filename =
> > g_strdup(ChardevBackendKind_lookup[backend->kind]);
> > }
> >+ if (!chr->suppress_be_open_on_init) {
> >+ qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> >+ }
> > QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> > return ret;
> > } else {
> >diff --git a/ui/console.c b/ui/console.c
> >index b30853f..54b192d 100644
> >--- a/ui/console.c
> >+++ b/ui/console.c
> >@@ -1746,7 +1746,6 @@ static void text_console_do_init(CharDriverState *chr,
> >DisplayState *ds)
> > s->t_attrib = s->t_attrib_default;
> > }
> >
> >- qemu_chr_be_generic_open(chr);
> > if (chr->init)
> > chr->init(chr);
> > }
> >diff --git a/ui/gtk.c b/ui/gtk.c
> >index 52c3f95..71b2497 100644
> >--- a/ui/gtk.c
> >+++ b/ui/gtk.c
> >@@ -1214,7 +1214,6 @@ static GSList *gd_vc_init(GtkDisplayState *s,
> >VirtualConsole *vc, int index, GSL
> >
> > gtk_menu_shell_append(GTK_MENU_SHELL(view_menu), vc->menu_item);
> >
> >- qemu_chr_be_generic_open(vc->chr);
> > if (vc->chr->init) {
> > vc->chr->init(vc->chr);
> > }
> >
>
