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

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

[Marc-André Lureau]

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.

Marc-André Lureau (9):
  Revert "char: use a fixed idx for child muxed chr"
  char: return a tag when adding the fe handlers
  char: add qemu_chr_remove_handlers()
  char: keep track of qemu_chr_add_handlers()
  char: warn on unused qemu_chr_add_handlers() result
  qdev: remove call to qemu_chr_add_handlers()
  char: handle qemu_chr_add_handlers() error
  ringbuf: fix chr_write return value
  tests: start chardev unit tests

 backends/rng-egd.c|  13 ++-
 gdbstub.c |  14 ++-
 hw/arm/pxa2xx.c   |  17 ++-
 hw/arm/strongarm.c|  22 +++-
 hw/char/bcm2835_aux.c |  15 ++-
 hw/char/cadence_uart.c|  18 ++-
 hw/char/debugcon.c|  33 +++---
 hw/char/digic-uart.c  |  14 ++-
 hw/char/escc.c|  20 +++-
 hw/char/etraxfs_ser.c |  18 ++-
 hw/char/exynos4210_uart.c |  21 +++-
 hw/char/grlib_apbuart.c   |  17 ++-
 hw/char/imx_serial.c  |  15 ++-
 hw/char/ipoctal232.c  |  23 +++-
 hw/char/lm32_juart.c  |  15 ++-
 hw/char/lm32_uart.c   |  15 ++-
 hw/char/mcf_uart.c|   7 +-
 hw/char/milkymist-uart.c  |  15 ++-
 hw/char/pl011.c   |  16 ++-
 hw/char/sclpconsole-lm.c  |  18 ++-
 hw/char/sclpconsole.c |  12 +-
 hw/char/serial.c  |  14 ++-
 hw/char/sh_serial.c   |   9 +-
 hw/char/spapr_vty.c   |  16 ++-
 hw/char/stm32f2xx_usart.c |  15 ++-
 hw/char/virtio-console.c  |  17 ++-
 hw/char/xen_console.c |  17 ++-
 hw/char/xilinx_uartlite.c |  18 ++-
 hw/core/qdev-properties-system.c  |   1 -
 hw/ipmi/ipmi_bmc_extern.c |  15 ++-
 hw/misc/ivshmem.c |  15 ++-
 hw/usb/ccid-card-passthru.c   |  18 ++-
 hw/usb/dev-serial.c   |  20 +++-
 hw/usb/redirect.c |  19 ++-
 monitor.c |  21 +++-
 net/colo-compare.c|  37 --
 net/filter-mirror.c   |  20 +++-
 net/slirp.c   |  12 +-
 net/vhost-user.c  |  20 ++--
 qemu-char.c   | 173 +--
 qtest.c   |   7 +-
 stubs/monitor-init.c  |   2 +-
 tests/test-char.c | 238 ++
 tests/vhost-user-test.c   |   8 +-
 vl.c  |   2 +-
 tests/Makefile.include|   4 +
 include/hw/char/bcm2835_aux.h |   1 +
 include/hw/char/cadence_uart.h|   1 +
 include/hw/char/digic-uart.h  |   1 +
 include/hw/char/imx_serial.h  |   1 +
 include/hw/char/serial.h  |   1 +
 include/hw/char/stm32f2xx_usart.h |   1 +
 include/monitor/monitor.h |   2 +-
 include/sysemu/char.h |  40 ---
 54 files changed, 954 insertions(+), 190 deletions(-)
 create mode 100644 tests/test-char.c

-- 
2.10.0