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

[Peter Maydell]

On 13 October 2016 at 12: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.

As discussed on IRC, I have applied patch 1/9 to master to fix
the regression for the moment.

thanks
-- PMM

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

[Paolo Bonzini]

On 13/10/2016 13:50, Marc-André Lureau wrote:
> 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?

The front-end is in hw/char.  The back-end as seen by the front-end is a
(chr, tag) pair---so that struct should be CharBackend.  The actual
back-end is the CharDriverState, but the front-end doesn't know.

I guess you can keep the qemu_chr_fe_* naming convention, which is
confusing anyway...

>> 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?

You can always start the development like that, I won't notice.  If you
squash everything together and re-separate the patches from scratch at
the end, I won't notice either. :)

But note that for example you don't need to do all the new unrealize
stuff, so perhaps you can start by undoing that in patch 4.

>> 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?

I'm thinking of the separation between BlockBackend and
BlockDriverState.  BlockBackend started as a very thin veneer over
BlockDriverState, but we've moved functionality out of BDS slowly
whenever it made sense.

Paolo

>>
>> [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.
> 

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.

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

[Paolo Bonzini]

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;
}

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.

The conversion is a bit tedious, but I think it's much easier compared
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.

[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.

Thanks,

Paolo