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

[Luiz Capitulino]

On Fri, 07 Jun 2013 16:01:34 -0500
Anthony Liguori <address@hidden> wrote:

> Luiz Capitulino <address@hidden> writes:
> 
> > On Fri, 7 Jun 2013 09:56:00 -0500
> > mdroth <address@hidden> wrote:
> >
> >> On Fri, Jun 07, 2013 at 09:50:39AM -0400, Luiz Capitulino wrote:
> >> > On Tue,  4 Jun 2013 16:35:09 -0500
> >> > Michael Roth <address@hidden> wrote:
> >> > 
> >> > > When CHR_EVENT_OPENED 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_OPENED, 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 OPENED 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 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 'explicit_be_open'
> >> > > 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 <address@hidden>
> >> > > Cc: address@hidden
> >> > > Signed-off-by: Michael Roth <address@hidden>
> >> > 
> >> > I don't know if the QMP queue is the ideal queue for this patch, but
> >> > I'd happily apply it if I get at least one Reviewed-by from the CC'ed
> >> > people.
> >> 
> >> Anthony actually added his Reviewed-by for v3, but I forgot to roll it
> >> in the commit. If you do take it in through your tree can you add that?
> >
> > Sorry for the bureaucracy, but I don't like to add other people's tags
> > when the patch changes. Even when I'm sure they would be OK with the
> > new version.
> >
> > Anthony, can you please add your Reviewed-by again?
> 
> I'll apply this patch directly since it's really a chardev patch more
> than a QMP patch.

ACK