Michael Roth <mdr...@linux.vnet.ibm.com> writes: > When CHR_EVENT_OPEN was initially added, it was CHR_EVENT_RESET, > and it was issued as a bottom-half: > > 86e94dea5b740dad65446c857f6959eae43e0ba6 > > Which we basically used to print out a greeting/prompt for the > monitor. > > AFAICT the only reason this was ever done in a BH was because in > some cases we'd modify the chr_write handler for a new chardev > backend *after* the site where we issued the reset (see: > 86e94d:qemu_chr_open_stdio()) > > At some point this event was renamed to CHR_EVENT_OPEN, and we've > maintained the use of this BH ever since. > > However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule > the BH via g_idle_add(), which is causing events to sometimes be > delivered after we've already begun processing data from backends, > leading to: > > known bugs: > > QMP: > session negotation resets with OPEN event, in some cases this > is causing new sessions to get sporadically reset > > potential bugs: > > hw/usb/redirect.c: > can_read handler checks for dev->parser != NULL, which may be > true if CLOSED BH has not been executed yet. In the past, OPENED > quiesced outstanding CLOSED events prior to us reading client > data. If it's delayed, our check may allow reads to occur even > though we haven't processed the OPENED event yet, and when we > do finally get the OPENED event, our state may get reset. > > qtest.c: > can begin session before OPENED event is processed, leading to > a spurious reset of the system and irq_levels > > gdbstub.c: > may start a gdb session prior to the machine being paused > > To fix these, let's just drop the BH. > > Since the initial reasoning for using it still applies to an extent, > work around that be 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 > 'supress_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. > > Reported-by: Stefan Priebe <s.pri...@profihost.ag> > Cc: qemu-sta...@nongnu.org > Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com> > --- > 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, 20 insertions(+), 24 deletions(-) > > diff --git a/backends/baum.c b/backends/baum.c > index 4cba79f..8384ef2 100644 > --- a/backends/baum.c > +++ b/backends/baum.c > @@ -611,7 +611,7 @@ CharDriverState *chr_baum_init(void) > > qemu_set_fd_handler(baum->brlapi_fd, baum_chr_read, NULL, baum); > > - qemu_chr_be_generic_open(chr); > + chr->be_open_on_init = true;
A carry over from the last patch... baum is installed on ccnode4 if you want to test the patch there to make sure you have everything. I like this very a lot more. Will give my Reviewed-by on v3. Regards, Anthony Liguori > return chr; > > 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 4f8382e..3071ca1 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,8 +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 +872,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 +1230,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; > @@ -1594,8 +1582,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd) > chr->chr_ioctl = pp_ioctl; > chr->chr_close = pp_close; > chr->opaque = drv; > - > - qemu_chr_be_generic_open(chr); > + chr->suppress_be_open_on_init = true; > > return chr; > } > @@ -1650,6 +1637,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 +1868,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 +1967,6 @@ static CharDriverState > *qemu_chr_open_pipe(ChardevHostdev *opts) > g_free(chr); > return NULL; > } > - qemu_chr_be_generic_open(chr); > return chr; > } > > @@ -1994,7 +1980,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 +2314,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 +2718,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; > @@ -3327,6 +3316,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)) { > @@ -3803,6 +3798,9 @@ ChardevReturn *qmp_chardev_add(const char *id, > ChardevBackend *backend, > chr->label = g_strdup(id); > chr->avail_connections = > (backend->kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX : 1; > + 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); > } > -- > 1.7.9.5