Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
Hi On Tue, Oct 11, 2016 at 8:44 PM Daniel P. Berrange wrote: > > Not sure if this is immediately helpful to your scneario or > not, but I'd like to see the qemu_chr_add_handlers method > removed long term, and everything converted to use the > qemu_chr_fe_add_watch function instead. This reverses the data > flow pattern - with chr_add_handlers the chardev code pushes > data from the backend into the frontends, but with fe_add_watch > the frontend pulls data from the backend. > Very interesting, that would help, but I think that wouldn't be enough, since we would still have the "event" handlers to properly register/track. Furthermore, that upside-down change is not easy. I think I will simply go with a new fe handler tag. > > To properly fix the non-blocking writes from the frontend to > the backend[1] will likely require use of qemu_chr_fe_add_watch, > and so having that function used for everything will make the > code clearer overall IMHO. > > Regards, > Daniel > > [1] eg the long term solution to replace this hack: > > commit 90f998f5f4267a0c22e983f533d19b9de1849283 > Author: Daniel P. Berrange > Date: Tue Sep 6 14:56:05 2016 +0100 > > char: convert qemu_chr_fe_write to qemu_chr_fe_write_all > > > -- > |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ > :| > |: http://libvirt.org -o- http://virt-manager.org > :| > |: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ > :| > -- Marc-André Lureau
Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
On Tue, Oct 11, 2016 at 04:18:46PM +, Marc-André Lureau wrote: > Hi > > On Tue, Oct 11, 2016 at 6:28 PM Marc-André Lureau < > marcandre.lur...@gmail.com> wrote: > > > Hi > > > > On Tue, Oct 11, 2016 at 4:21 PM Claudio Imbrenda < > > imbre...@linux.vnet.ibm.com> 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) > > -- > > > > I am quite undecided how to fix this. qemu_chr_add_handlers users have no > associated tag: this works ok as long as there is a single user per > chardev. But it becomes problematic when there are multiple users, when the > backing chardev is a mux: the mux will just grow more fe handlers, And you > can't ever remove your fe callback which may lead to a crash. I am > surprised this problem didn't raise before. > > I can imagine 2 solutions, either to associate each fe with it's opaque > pointer to lookup the corresponding mux registered callbacks (less > intrusive, yet not very clean), or to create a tag when calling > qemu_chr_add_handlers() which can be used later with a new function like > qemu_chr_remove_handlers(). This would be very intrusive, since all chr fe > will have to hold a tag in their struct and pass it accordingly. > > Other thoughts? Not sure if this is immediately helpful to your scneario or not, but I'd like to see the qemu_chr_add_handlers method removed long term, and everything converted to use the qemu_chr_fe_add_watch function instead. This reverses the data flow pattern - with chr_add_handlers the chardev code pushes data from the backend into the frontends, but with fe_add_watch the frontend pulls data from the backend. To properly fix the non-blocking writes from the frontend to the backend[1] will likely require use of qemu_chr_fe_add_watch, and so having that function used for everything will make the code clearer overall IMHO. Regards, Daniel [1] eg the long term solution to replace this hack: commit 90f998f5f4267a0c22e983f533d19b9de1849283 Author: Daniel P. Berrange Date: Tue Sep 6 14:56:05 2016 +0100 char: convert qemu_chr_fe_write to qemu_chr_fe_write_all -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :|
Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
Hi On Tue, Oct 11, 2016 at 6:28 PM Marc-André Lureau < marcandre.lur...@gmail.com> wrote: > Hi > > On Tue, Oct 11, 2016 at 4:21 PM Claudio Imbrenda < > imbre...@linux.vnet.ibm.com> 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) > -- > I am quite undecided how to fix this. qemu_chr_add_handlers users have no associated tag: this works ok as long as there is a single user per chardev. But it becomes problematic when there are multiple users, when the backing chardev is a mux: the mux will just grow more fe handlers, And you can't ever remove your fe callback which may lead to a crash. I am surprised this problem didn't raise before. I can imagine 2 solutions, either to associate each fe with it's opaque pointer to lookup the corresponding mux registered callbacks (less intrusive, yet not very clean), or to create a tag when calling qemu_chr_add_handlers() which can be used later with a new function like qemu_chr_remove_handlers(). This would be very intrusive, since all chr fe will have to hold a tag in their struct and pass it accordingly. Other thoughts? -- Marc-André Lureau
Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
Hi
On Tue, Oct 11, 2016 at 4:21 PM Claudio Imbrenda <
imbre...@linux.vnet.ibm.com> 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
> > ---
> > 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)
Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
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:
>
> 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
> ---
> 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;
>
