From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55754) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ui7ub-00048G-0a for qemu-devel@nongnu.org; Thu, 30 May 2013 14:49:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ui7uV-0003XQ-Ph for qemu-devel@nongnu.org; Thu, 30 May 2013 14:49:20 -0400 Sender: fluxion Date: Thu, 30 May 2013 13:48:42 -0500 From: mdroth Message-ID: <20130530184842.GJ4599@vm> References: <1369866423-18801-1-git-send-email-mdroth@linux.vnet.ibm.com> <87hahkcspv.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87hahkcspv.fsf@codemonkey.ws> Subject: Re: [Qemu-devel] [PATCH] qemu-char: don't issue CHR_EVENT_OPEN in a BH List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: s.priebe@profihost.ag, lcapitulino@redhat.com, qemu-devel@nongnu.org, qemu-stable@nongnu.org On Thu, May 30, 2013 at 11:55:56AM -0500, Anthony Liguori wrote: > Michael Roth writes: > > > When CHR_EVENT_OPEN was initially added, it was CHR_EVENT_RESET, and > > it was issued as a bottom-half: > > > > 86e94dea5b740dad65446c857f6959eae43e0ba6 > > > > AFAICT the only reason this was ever done in a BH was because it was > > initially used to to issue a CHR_EVENT_RESET when we initialized the > > monitor, > > It was specifically added so that we could redisplay the monitor > banner. Because the event was emitted in open(), if we tried to > redisplay the monitor banner in the callback, the device would be in a > partially initialized state and badness would ensue. The BH was there > to ensure the CharDriverState was fully initialized before firing the > callback. > > > and we would in some cases modify the chr_write handler for > > a new chardev backend *after* the site where we issued the reset > > (see: 86e94d:qemu_chr_open_stdio()) > > > > So we executed the reset in a BH to ensure the chardev was fully > > initialized before we executed the CHR_EVENT_RESET handler (which > > generally involved printing out a greeting/prompt for the monitor). > > > > 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 by setting a > > 'be_open_on_init' flag that gets checked toward the end of > > qmp_chardev_add(). This defers delivery long enough that we can > > be assured a CharDriverState is fully initialized before > > CHR_EVENT_OPENED is sent. > > > > Reported-by: Stefan Priebe > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Michael Roth > > --- > > backends/baum.c | 2 +- > > include/sysemu/char.h | 2 +- > > qemu-char.c | 29 ++++++++++------------------- > > ui/console.c | 2 +- > > ui/gtk.c | 2 +- > > 5 files changed, 14 insertions(+), 23 deletions(-) > > > > diff --git a/backends/baum.c b/backends/baum.c > > index 4cba79f..8384ef2 100644 > > > > > @@ -3803,6 +3791,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->be_open_on_init) { > > + qemu_chr_be_event(chr, CHR_EVENT_OPENED); > > + } > > Why does this need to be called conditionally? Could we drop > be_open_on_init and just call this unconditionally here? We have a couple instances where we don't immediately set the backend to an open state on init. The main one is socket backends, where where we don't send the OPENED event until a client connects. > > Regards, > > Anthony Liguori >