From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43976) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ujo0T-0001jS-Py for qemu-devel@nongnu.org; Tue, 04 Jun 2013 05:58:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ujo0R-0006yd-Qs for qemu-devel@nongnu.org; Tue, 04 Jun 2013 05:58:21 -0400 Message-ID: <51ADBA34.7060709@redhat.com> Date: Tue, 04 Jun 2013 11:58:12 +0200 From: Hans de Goede MIME-Version: 1.0 References: <1370291118-24947-1-git-send-email-mdroth@linux.vnet.ibm.com> In-Reply-To: <1370291118-24947-1-git-send-email-mdroth@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] qemu-char: don't issue CHR_EVENT_OPEN in a BH List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: lcapitulino@redhat.com, aliguori@us.ibm.com, qemu-stable@nongnu.org, qemu-devel@nongnu.org, s.priebe@profihost.ag Hi, On 06/03/2013 10:25 PM, Michael Roth wrote: > 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 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 You've missed at least the spicevmc & spiceport backends, which both need suppress_be_open_on_init Regards, Hans > > Reported-by: Stefan Priebe > Cc: qemu-stable@nongnu.org > Signed-off-by: Michael Roth > --- > 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); > } >