Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr

[Marc-André Lureau]

Hi

On Tue, Oct 11, 2016 at 4:21 PM Claudio Imbrenda <
address@hidden> wrote:

> Hi,
>
> I noticed that this patch kills the Qemu monitor for me. If I start a
> text-mode guest with -nographic, then I can't switch to the monitor any
> longer with Ctrl+a c . The guest works otherwise, e.g. I can login from
> the console.
>
> Tested on s390, but I think it's a more general issue, since the patch
> is not arch-dependent.
>
>
> On 03/10/16 11:47, Marc-André Lureau wrote:
> > mux_chr_update_read_handler() is adding a new mux_cnt each time
> > mux_chr_update_read_handler() is called, it's not possible to actually
> > update the "child" chr callbacks that were set previously. This may lead
> > to crashes if the "child" chr is destroyed:
> >
>

My understanding was quite wrong, it was assuming that each mux user/fe was
a seperate chardev, that's not how it works.

The patch should probably be reverted until a better solution comes up. I
am looking at it, but no solution yet.

(obviously, it would be nice to have some minimal tests for mux, let see if
I get there)



> > valgrind x86_64-softmmu/qemu-system-x86_64 -chardev
> > stdio,mux=on,id=char0 -mon chardev=char0,mode=control,default
> >
> > when quitting:
> >
> > ==4306== Invalid read of size 8
> > ==4306==    at 0x8061D3: json_lexer_destroy (json-lexer.c:385)
> > ==4306==    by 0x7E39F8: json_message_parser_destroy
> (json-streamer.c:134)
> > ==4306==    by 0x3447F6: monitor_qmp_event (monitor.c:3908)
> > ==4306==    by 0x480153: mux_chr_send_event (qemu-char.c:630)
> > ==4306==    by 0x480694: mux_chr_event (qemu-char.c:734)
> > ==4306==    by 0x47F1E9: qemu_chr_be_event (qemu-char.c:205)
> > ==4306==    by 0x481207: fd_chr_close (qemu-char.c:1114)
> > ==4306==    by 0x481659: qemu_chr_close_stdio (qemu-char.c:1221)
> > ==4306==    by 0x486F07: qemu_chr_free (qemu-char.c:4146)
> > ==4306==    by 0x486F97: qemu_chr_delete (qemu-char.c:4154)
> > ==4306==    by 0x487E66: qemu_chr_cleanup (qemu-char.c:4678)
> > ==4306==    by 0x495A98: main (vl.c:4675)
> > ==4306==  Address 0x28439e90 is 112 bytes inside a block of size 240
> free'd
> > ==4306==    at 0x4C2CD5A: free (vg_replace_malloc.c:530)
> > ==4306==    by 0x1E4CBF2D: g_free (in /usr/lib64/libglib-2.0.so.0.4800.2)
> > ==4306==    by 0x344DE9: monitor_cleanup (monitor.c:4058)
> > ==4306==    by 0x495A93: main (vl.c:4674)
> > ==4306==  Block was alloc'd at
> > ==4306==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
> > ==4306==    by 0x1E4CBE18: g_malloc (in
> /usr/lib64/libglib-2.0.so.0.4800.2)
> > ==4306==    by 0x344BF8: monitor_init (monitor.c:4021)
> > ==4306==    by 0x49063C: mon_init_func (vl.c:2417)
> > ==4306==    by 0x7FC6DE: qemu_opts_foreach (qemu-option.c:1116)
> > ==4306==    by 0x4954E0: main (vl.c:4473)
> >
> > Instead, keep the "child" chr associated with a particular idx so its
> > handlers can be updated and removed to avoid the crash.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  qemu-char.c           | 22 +++++++++++++++-------
> >  include/sysemu/char.h |  1 +
> >  2 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index f7a07e6..4b330ea 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -165,6 +165,7 @@ CharDriverState *qemu_chr_alloc(ChardevCommon
> *backend, Error **errp)
> >      CharDriverState *chr = g_malloc0(sizeof(CharDriverState));
> >      qemu_mutex_init(&chr->chr_write_lock);
> >
> > +    chr->mux_idx = -1;
> >      if (backend->has_logfile) {
> >          int flags = O_WRONLY | O_CREAT;
> >          if (backend->has_logappend &&
> > @@ -738,17 +739,25 @@ static void
> mux_chr_update_read_handler(CharDriverState *chr,
> >                                          GMainContext *context)
> >  {
> >      MuxDriver *d = chr->opaque;
> > +    int idx;
> >
> >      if (d->mux_cnt >= MAX_MUX) {
> >          fprintf(stderr, "Cannot add I/O handlers, MUX array is full\n");
> >          return;
> >      }
> > -    d->ext_opaque[d->mux_cnt] = chr->handler_opaque;
> > -    d->chr_can_read[d->mux_cnt] = chr->chr_can_read;
> > -    d->chr_read[d->mux_cnt] = chr->chr_read;
> > -    d->chr_event[d->mux_cnt] = chr->chr_event;
> > +
> > +    if (chr->mux_idx == -1) {
> > +        chr->mux_idx = d->mux_cnt++;
> > +    }
> > +
> > +    idx = chr->mux_idx;
> > +    d->ext_opaque[idx] = chr->handler_opaque;
> > +    d->chr_can_read[idx] = chr->chr_can_read;
> > +    d->chr_read[idx] = chr->chr_read;
> > +    d->chr_event[idx] = chr->chr_event;
> > +
> >      /* Fix up the real driver with mux routines */
> > -    if (d->mux_cnt == 0) {
> > +    if (d->mux_cnt == 1) {
> >          qemu_chr_add_handlers_full(d->drv, mux_chr_can_read,
> >                                     mux_chr_read,
> >                                     mux_chr_event,
> > @@ -757,8 +766,7 @@ static void
> mux_chr_update_read_handler(CharDriverState *chr,
> >      if (d->focus != -1) {
> >          mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> >      }
> > -    d->focus = d->mux_cnt;
> > -    d->mux_cnt++;
> > +    d->focus = idx;
> >      mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
> >  }
> >
> > diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> > index 0d0465a..4593576 100644
> > --- a/include/sysemu/char.h
> > +++ b/include/sysemu/char.h
> > @@ -92,6 +92,7 @@ struct CharDriverState {
> >      int explicit_be_open;
> >      int avail_connections;
> >      int is_mux;
> > +    int mux_idx;
> >      guint fd_in_tag;
> >      QemuOpts *opts;
> >      bool replay;
> >
>
>
> --
Marc-André Lureau