Re: [Qemu-devel] [PATCH] chardev: fix mess in OPENED/CLOSED events when muxed

[Marc-André Lureau]

Hi
On Mon, Nov 5, 2018 at 11:22 AM Artem Pisarenko
<address@hidden> wrote:
>
> Sorry, I forgot to check unit tests. Although, it's very strange that this 
> specific test failed while things work functionally...
>
> > I am a bit reluctant to take patches that don't actually "fix" things.
> >
> > Could you add some tests to demonstrate the problems?
>
> Ok
>
> >> @@ -257,6 +257,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
> >>  {
> >>      Chardev *s;
> >>      int fe_open;
> >> +    static __thread bool mux_reentered;
> >
> > Not very elegant. Maybe mux_chr_set_handlers() could call a refactored
> > internal chr_fe_set_handlers() with an extra arg "no_open_event" ?
>
> Agree. It would make this hack more elegant. Although, it may become 
> irrelevant as soon as I'll find soultion for failed docker test...
>
> >> @@ -272,21 +272,24 @@ static void char_mux_finalize(Object *obj)
> >>      for (i = 0; i < d->mux_cnt; i++) {
> >>          CharBackend *be = d->backends[i];
> >>          if (be) {
> >> +            if (be->chr && be->chr->be_open) {
> >> +                qemu_chr_be_event(be->chr, CHR_EVENT_CLOSED);
> >> +            }
> >
> > It looks like this could be a seperate patch, with a seperate test.
>
> Why this should be separate ? Do you mean that overall "opened/closed" 
> pairing fix should be separated to "opened" fix+test and "closed" fix+test ?
>
> Since in next version it cannot be single patch anymore, how should I proceed 
> with it ? Should I publish it as patch series with same subject marked as V2 
> or start new patch series?

A patch series would be great. You can keep the subject and make it v2.

thanks