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

[Anthony Liguori]

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.

Regards,

Anthony Liguori

>
>> 
>> Thanks!
>> 
>> > 
>> > 
>> > > ---
>> > > v3->v4:
>> > >  * renamed 'suppress_be_open_on_init' to 'explicit_be_open' to match
>> > >    existing 'explicit_fe_open' flag (Hans)
>> > >  * added missing 'explicit_be_open' flags for spice vmc/port and
>> > >    msmouse backends
>> > > 
>> > > v2->v3:
>> > >  * removed artifact in from v1 in backends/baum.c, test build with
>> > >    BRLAPI=y (Anthony)
>> > >  * rebased on latest origin/master
>> > > 
>> > > v1->v2:
>> > >  * default to sending OPENED on backend init, add flag to suppress
>> > >    it (Anthony)
>> > >  * fix missing OPENED for stdio backends on w32
>> > >  * fix missing OPENED when qemu_chr_new_from_opts() doesn't use
>> > >    qmp_chardev_add()
>> > >  * clean up/update commit message
>> > > 
>> > >  backends/baum.c       |    2 --
>> > >  backends/msmouse.c    |    1 +
>> > >  include/sysemu/char.h |    2 +-
>> > >  qemu-char.c           |   38 +++++++++++++++++---------------------
>> > >  spice-qemu-char.c     |    1 +
>> > >  ui/console.c          |    1 -
>> > >  ui/gtk.c              |    1 -
>> > >  7 files changed, 20 insertions(+), 26 deletions(-)
>> > > 
>> > > diff --git a/backends/baum.c b/backends/baum.c
>> > > index 4cba79f..62aa784 100644
>> > > --- a/backends/baum.c
>> > > +++ b/backends/baum.c
>> > > @@ -611,8 +611,6 @@ CharDriverState *chr_baum_init(void)
>> > >  
>> > >      qemu_set_fd_handler(baum->brlapi_fd, baum_chr_read, NULL, baum);
>> > >  
>> > > -    qemu_chr_be_generic_open(chr);
>> > > -
>> > >      return chr;
>> > >  
>> > >  fail:
>> > > diff --git a/backends/msmouse.c b/backends/msmouse.c
>> > > index 0ac05a0..c0dbfcd 100644
>> > > --- a/backends/msmouse.c
>> > > +++ b/backends/msmouse.c
>> > > @@ -70,6 +70,7 @@ CharDriverState *qemu_chr_open_msmouse(void)
>> > >      chr = g_malloc0(sizeof(CharDriverState));
>> > >      chr->chr_write = msmouse_chr_write;
>> > >      chr->chr_close = msmouse_chr_close;
>> > > +    chr->explicit_be_open = true;
>> > >  
>> > >      qemu_add_mouse_event_handler(msmouse_event, chr, 0, "QEMU Microsoft 
>> > > Mouse");
>> > >  
>> > > diff --git a/include/sysemu/char.h b/include/sysemu/char.h
>> > > index 5e42c90..066c216 100644
>> > > --- a/include/sysemu/char.h
>> > > +++ b/include/sysemu/char.h
>> > > @@ -70,12 +70,12 @@ struct CharDriverState {
>> > >      void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
>> > >      void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
>> > >      void *opaque;
>> > > -    int idle_tag;
>> > >      char *label;
>> > >      char *filename;
>> > >      int be_open;
>> > >      int fe_open;
>> > >      int explicit_fe_open;
>> > > +    int explicit_be_open;
>> > >      int avail_connections;
>> > >      QemuOpts *opts;
>> > >      QTAILQ_ENTRY(CharDriverState) next;
>> > > diff --git a/qemu-char.c b/qemu-char.c
>> > > index d04b429..0b6ae95 100644
>> > > --- a/qemu-char.c
>> > > +++ b/qemu-char.c
>> > > @@ -110,19 +110,9 @@ void qemu_chr_be_event(CharDriverState *s, int 
>> > > event)
>> > >      s->chr_event(s->handler_opaque, event);
>> > >  }
>> > >  
>> > > -static gboolean qemu_chr_be_generic_open_bh(gpointer opaque)
>> > > -{
>> > > -    CharDriverState *s = opaque;
>> > > -    qemu_chr_be_event(s, CHR_EVENT_OPENED);
>> > > -    s->idle_tag = 0;
>> > > -    return FALSE;
>> > > -}
>> > > -
>> > >  void qemu_chr_be_generic_open(CharDriverState *s)
>> > >  {
>> > > -    if (s->idle_tag == 0) {
>> > > -        s->idle_tag = g_idle_add(qemu_chr_be_generic_open_bh, s);
>> > > -    }
>> > > +    qemu_chr_be_event(s, CHR_EVENT_OPENED);
>> > >  }
>> > >  
>> > >  int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
>> > > @@ -247,6 +237,7 @@ static CharDriverState *qemu_chr_open_null(void)
>> > >  
>> > >      chr = g_malloc0(sizeof(CharDriverState));
>> > >      chr->chr_write = null_chr_write;
>> > > +    chr->explicit_be_open = true;
>> > >      return chr;
>> > >  }
>> > >  
>> > > @@ -504,9 +495,6 @@ static CharDriverState 
>> > > *qemu_chr_open_mux(CharDriverState *drv)
>> > >      /* Frontend guest-open / -close notification is not support with 
>> > > muxes */
>> > >      chr->chr_set_fe_open = NULL;
>> > >  
>> > > -    /* Muxes are always open on creation */
>> > > -    qemu_chr_be_generic_open(chr);
>> > > -
>> > >      return chr;
>> > >  }
>> > >  
>> > > @@ -883,8 +871,6 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, 
>> > > int fd_out)
>> > >      chr->chr_update_read_handler = fd_chr_update_read_handler;
>> > >      chr->chr_close = fd_chr_close;
>> > >  
>> > > -    qemu_chr_be_generic_open(chr);
>> > > -
>> > >      return chr;
>> > >  }
>> > >  
>> > > @@ -1243,6 +1229,7 @@ static CharDriverState *qemu_chr_open_pty(const 
>> > > char *id,
>> > >      chr->chr_update_read_handler = pty_chr_update_read_handler;
>> > >      chr->chr_close = pty_chr_close;
>> > >      chr->chr_add_watch = pty_chr_add_watch;
>> > > +    chr->explicit_be_open = true;
>> > >  
>> > >      s->fd = io_channel_from_fd(master_fd);
>> > >      s->timer_tag = 0;
>> > > @@ -1595,8 +1582,6 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
>> > >      chr->chr_close = pp_close;
>> > >      chr->opaque = drv;
>> > >  
>> > > -    qemu_chr_be_generic_open(chr);
>> > > -
>> > >      return chr;
>> > >  }
>> > >  #endif /* __linux__ */
>> > > @@ -1650,6 +1635,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
>> > >      chr->opaque = (void *)(intptr_t)fd;
>> > >      chr->chr_write = null_chr_write;
>> > >      chr->chr_ioctl = pp_ioctl;
>> > > +    chr->explicit_be_open = true;
>> > >      return chr;
>> > >  }
>> > >  #endif
>> > > @@ -1880,7 +1866,6 @@ static CharDriverState 
>> > > *qemu_chr_open_win_path(const char *filename)
>> > >          g_free(chr);
>> > >          return NULL;
>> > >      }
>> > > -    qemu_chr_be_generic_open(chr);
>> > >      return chr;
>> > >  }
>> > >  
>> > > @@ -1980,7 +1965,6 @@ static CharDriverState 
>> > > *qemu_chr_open_pipe(ChardevHostdev *opts)
>> > >          g_free(chr);
>> > >          return NULL;
>> > >      }
>> > > -    qemu_chr_be_generic_open(chr);
>> > >      return chr;
>> > >  }
>> > >  
>> > > @@ -1994,7 +1978,6 @@ static CharDriverState 
>> > > *qemu_chr_open_win_file(HANDLE fd_out)
>> > >      s->hcom = fd_out;
>> > >      chr->opaque = s;
>> > >      chr->chr_write = win_chr_write;
>> > > -    qemu_chr_be_generic_open(chr);
>> > >      return chr;
>> > >  }
>> > >  
>> > > @@ -2329,6 +2312,8 @@ static CharDriverState *qemu_chr_open_udp_fd(int 
>> > > fd)
>> > >      chr->chr_write = udp_chr_write;
>> > >      chr->chr_update_read_handler = udp_chr_update_read_handler;
>> > >      chr->chr_close = udp_chr_close;
>> > > +    /* be isn't opened until we get a connection */
>> > > +    chr->explicit_be_open = true;
>> > >      return chr;
>> > >  }
>> > >  
>> > > @@ -2731,6 +2716,8 @@ static CharDriverState 
>> > > *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
>> > >      chr->get_msgfd = tcp_get_msgfd;
>> > >      chr->chr_add_client = tcp_chr_add_client;
>> > >      chr->chr_add_watch = tcp_chr_add_watch;
>> > > +    /* be isn't opened until we get a connection */
>> > > +    chr->explicit_be_open = true;
>> > >  
>> > >      if (is_listen) {
>> > >          s->listen_fd = fd;
>> > > @@ -3325,6 +3312,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts 
>> > > *opts,
>> > >      if (!chr->filename)
>> > >          chr->filename = g_strdup(qemu_opt_get(opts, "backend"));
>> > >      chr->init = init;
>> > > +    /* if we didn't create the chardev via qmp_chardev_add, we
>> > > +     * need to send the OPENED event here
>> > > +     */
>> > > +    if (!chr->explicit_be_open) {
>> > > +        qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>> > > +    }
>> > >      QTAILQ_INSERT_TAIL(&chardevs, chr, next);
>> > >  
>> > >      if (qemu_opt_get_bool(opts, "mux", 0)) {
>> > > @@ -3804,6 +3797,9 @@ ChardevReturn *qmp_chardev_add(const char *id, 
>> > > ChardevBackend *backend,
>> > >          if (!chr->filename) {
>> > >              chr->filename = 
>> > > g_strdup(ChardevBackendKind_lookup[backend->kind]);
>> > >          }
>> > > +        if (!chr->explicit_be_open) {
>> > > +            qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>> > > +        }
>> > >          QTAILQ_INSERT_TAIL(&chardevs, chr, next);
>> > >          return ret;
>> > >      } else {
>> > > diff --git a/spice-qemu-char.c b/spice-qemu-char.c
>> > > index f10970c..6d147a7 100644
>> > > --- a/spice-qemu-char.c
>> > > +++ b/spice-qemu-char.c
>> > > @@ -255,6 +255,7 @@ static CharDriverState *chr_open(const char *subtype)
>> > >      chr->chr_add_watch = spice_chr_add_watch;
>> > >      chr->chr_close = spice_chr_close;
>> > >      chr->chr_set_fe_open = spice_chr_set_fe_open;
>> > > +    chr->explicit_be_open = true;
>> > >  
>> > >      QLIST_INSERT_HEAD(&spice_chars, s, next);
>> > >  
>> > > diff --git a/ui/console.c b/ui/console.c
>> > > index b30853f..54b192d 100644
>> > > --- a/ui/console.c
>> > > +++ b/ui/console.c
>> > > @@ -1746,7 +1746,6 @@ static void text_console_do_init(CharDriverState 
>> > > *chr, DisplayState *ds)
>> > >          s->t_attrib = s->t_attrib_default;
>> > >      }
>> > >  
>> > > -    qemu_chr_be_generic_open(chr);
>> > >      if (chr->init)
>> > >          chr->init(chr);
>> > >  }
>> > > diff --git a/ui/gtk.c b/ui/gtk.c
>> > > index 3bc2842..ef1bb71 100644
>> > > --- a/ui/gtk.c
>> > > +++ b/ui/gtk.c
>> > > @@ -1216,7 +1216,6 @@ static GSList *gd_vc_init(GtkDisplayState *s, 
>> > > VirtualConsole *vc, int index, GSL
>> > >  
>> > >      gtk_menu_shell_append(GTK_MENU_SHELL(view_menu), vc->menu_item);
>> > >  
>> > > -    qemu_chr_be_generic_open(vc->chr);
>> > >      if (vc->chr->init) {
>> > >          vc->chr->init(vc->chr);
>> > >      }
>> > 
>>