[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qemu-char: don't issue CHR_EVENT_OPEN in a BH
From: |
mdroth |
Subject: |
Re: [Qemu-devel] [PATCH] qemu-char: don't issue CHR_EVENT_OPEN in a BH |
Date: |
Thu, 30 May 2013 13:48:42 -0500 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Thu, May 30, 2013 at 11:55:56AM -0500, Anthony Liguori wrote:
> Michael Roth <address@hidden> 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 <address@hidden>
> > Cc: address@hidden
> > Signed-off-by: Michael Roth <address@hidden>
> > ---
> > 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
>
> <snip>
>
> > @@ -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
>