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

[Daniel P. Berrange]

On Tue, Oct 11, 2016 at 04:18:46PM +0000, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 11, 2016 at 6:28 PM Marc-André Lureau <
> address@hidden> wrote:
> 
> > 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)
> > --
> >
> 
> 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 <address@hidden>
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/ :|