Re: [Qemu-devel] [PATCH] qemu-char: don't issue CHR_EVENT_OPEN in a BH

[mdroth]

On Thu, May 30, 2013 at 03:24:14PM -0500, Anthony Liguori wrote:
> mdroth <address@hidden> writes:
> 
> > On Thu, May 30, 2013 at 02:35:37PM -0500, Anthony Liguori wrote:
> >> mdroth <address@hidden> writes:
> >> 
> >> > 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.
> >> 
> >> Would it make more sense to mark those since they are less common?
> >
> > It's less code, but emitting an event on device init seems like it
> > should be more of an opt-in type of thing, IMO. I'm fine with either
> > approach though.
> 
> I'd prefer the other way around.  There's less harm in generating an
> extra event than not generating one.

Ok, I'll send a v2 shortly

> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >> 
> >> Regards,
> >> 
> >> Anthony Liguori
> >> 
> >> > The main one is socket backends, where where we
> >> > don't send the OPENED event until a client connects.
> >> >
> >> >> 
> >> >> Regards,
> >> >> 
> >> >> Anthony Liguori
> >> >> 
> >> 
>