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

[mdroth]

On Mon, Jun 03, 2013 at 12:17:00PM -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
> >
> > 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_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, 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
> > 'supress_be_open_on_init' 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>
> > ---
> > 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 +-
> >  include/sysemu/char.h |    2 +-
> >  qemu-char.c           |   38 ++++++++++++++++++--------------------
> >  ui/console.c          |    1 -
> >  ui/gtk.c              |    1 -
> >  5 files changed, 20 insertions(+), 24 deletions(-)
> >
> > diff --git a/backends/baum.c b/backends/baum.c
> > index 4cba79f..8384ef2 100644
> > --- a/backends/baum.c
> > +++ b/backends/baum.c
> > @@ -611,7 +611,7 @@ CharDriverState *chr_baum_init(void)
> >  
> >      qemu_set_fd_handler(baum->brlapi_fd, baum_chr_read, NULL, baum);
> >  
> > -    qemu_chr_be_generic_open(chr);
> > +    chr->be_open_on_init = true;
> 
> A carry over from the last patch...  baum is installed on ccnode4 if you
> want to test the patch there to make sure you have everything.

Shoot, thanks, I'll give it a whirl on ccnode4

> 
> I like this very a lot more.  Will give my Reviewed-by on v3.
> 
> Regards,
> 
> Anthony Liguori
> 
> >      return chr;
> >  
> > diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> > index 5e42c90..b0ae749 100644
> > --- a/include/sysemu/char.h
> > +++ b/include/sysemu/char.h
> > @@ -70,13 +70,13 @@ 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 avail_connections;
> > +    bool suppress_be_open_on_init;
> >      QemuOpts *opts;
> >      QTAILQ_ENTRY(CharDriverState) next;
> >  };
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 4f8382e..3071ca1 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->suppress_be_open_on_init = true;
> >      return chr;
> >  }
> >  
> > @@ -504,8 +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 +872,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 +1230,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->suppress_be_open_on_init = true;
> >  
> >      s->fd = io_channel_from_fd(master_fd);
> >      s->timer_tag = 0;
> > @@ -1594,8 +1582,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
> >      chr->chr_ioctl = pp_ioctl;
> >      chr->chr_close = pp_close;
> >      chr->opaque = drv;
> > -
> > -    qemu_chr_be_generic_open(chr);
> > +    chr->suppress_be_open_on_init = true;
> >  
> >      return chr;
> >  }
> > @@ -1650,6 +1637,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->suppress_be_open_on_init = true;
> >      return chr;
> >  }
> >  #endif
> > @@ -1880,7 +1868,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 +1967,6 @@ static CharDriverState 
> > *qemu_chr_open_pipe(ChardevHostdev *opts)
> >          g_free(chr);
> >          return NULL;
> >      }
> > -    qemu_chr_be_generic_open(chr);
> >      return chr;
> >  }
> >  
> > @@ -1994,7 +1980,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 +2314,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->suppress_be_open_on_init = true;
> >      return chr;
> >  }
> >  
> > @@ -2731,6 +2718,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->suppress_be_open_on_init = true;
> >  
> >      if (is_listen) {
> >          s->listen_fd = fd;
> > @@ -3327,6 +3316,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->suppress_be_open_on_init) {
> > +        qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> > +    }
> >      QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> >  
> >      if (qemu_opt_get_bool(opts, "mux", 0)) {
> > @@ -3803,6 +3798,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->suppress_be_open_on_init) {
> > +            qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> > +        }
> >          QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> >          return ret;
> >      } else {
> > 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 52c3f95..71b2497 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -1214,7 +1214,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);
> >      }
> > -- 
> > 1.7.9.5
>