mdroth <mdr...@linux.vnet.ibm.com> writes: > On Thu, May 30, 2013 at 02:35:37PM -0500, Anthony Liguori wrote: >> mdroth <mdr...@linux.vnet.ibm.com> writes: >> >> > On Thu, May 30, 2013 at 11:55:56AM -0500, Anthony Liguori wrote: >> >> Michael Roth <mdr...@linux.vnet.ibm.com> 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 <s.pri...@profihost.ag> >> >> > Cc: qemu-sta...@nongnu.org >> >> > Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com> >> >> > --- >> >> > 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. 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 >> >> >>