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