Re: [Qemu-devel] [PATCH 0/9] Fix mux regression (commit 949055a2)

[Marc-André Lureau]

Hi

----- Original Message -----
> 
> 
> On 13/10/2016 13:14, Marc-André Lureau wrote:
> > Hi,
> > 
> > Commit 949055a2 "char: use a fixed idx for child muxed chr" introduced
> > a regression in mux usage, since it wrongly interpreted mux as muxing
> > various chr backend. Instead, it muxes frontends.
> > 
> > The first patch reverts the broken change, the following patches add
> > tracking to frontend handler, finally the last patch adds some tests
> > that would have helped to track the crash and the regression. There is
> > also a small fix for ringbuf.
> 
> In general I like the solution, but I dislike the API.
> 
> Would it work if we had something like
> 
>     struct CharBackend {
>         CharDriverState *chr;
>         int tag;
>     }
> 

You mean front-end right?

> and we modified all qemu_chr_fe_* functions (plus
> qemu_chr_add_handlers[1]) to take a struct CharBackend.  chardev
> properties would also take a struct CharBackend.  Then removing handlers
> can still be done in release_chr, making the patches much smaller.

As long as they use chardev property, it's not always the case.

> The conversion is a bit tedious, but I think it's much easier compared

Yes, it's tedious :) Do you mind if I try to make the change on top? If it 
really reduces the patch 4/7, we could try to squash it?

> to patch 4.  I feel bad for having you redo everything and in particular
> patch 7, but this is the model that the block layer uses and it works
> very well there.

Which function btw?

> 
> [1] while at it, it's probably best to rename qemu_chr_add_handlers
>     to qemu_chr_fe_add_handlers as the first patch in the series,
>     so that qemu_chr_add_handlers(CharDriverState *chr, ..., int tag)
>     can take the role of qemu_chr_set_handlers in this series.