On Fri, 07 Jun 2013 16:01:34 -0500 Anthony Liguori <aligu...@us.ibm.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes: > > > On Fri, 7 Jun 2013 09:56:00 -0500 > > mdroth <mdr...@linux.vnet.ibm.com> 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 <mdr...@linux.vnet.ibm.com> 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 <s.pri...@profihost.ag> > >> > > Cc: qemu-sta...@nongnu.org > >> > > Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com> > >> > > >> > 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
