Re: [Qemu-devel] [PATCH V4] char: restore read callback on a reattached (hotplug) chardev

[Gal Hammer]

On 16/12/2013 22:32, Amit Shah wrote:
> On (Sun) 15 Dec 2013 [12:26:37], Gal Hammer wrote:
> > Fix a bug that was introduced in commit 386a5a1e. A removal of a device
> > set the chr handlers to NULL. However when the device is plugged back,
> > its read callback is not restored so data can't be transferred from the
> > host to the guest (e.g. via the virtio-serial port).
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1027181
> >
> > Signed-off-by: Gal Hammer <address@hidden>
> >
> > ---
> >   qemu-char.c | 17 +++++++++++++++--
> >   1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > V4: - Same as V3, but this time done right.
> >
> > V3: - fix a typo in comment.
> >      - move the revision history after the "signed-off-by" tag.
> >
> > V2: - do not call chr_update_read_handler on device removal.
> >      - add asserts to verify chr_update_read_handler is not called
> >        with an assigned fd_in_tag to prevent fd leaks.
> >      - update fd and udp backends' chr_update_read_handler function
> >        so it won't remove fd_in to prevent a double release.
>
> Looks like you missed the pty backend.  Can't blame you -- this
> chardev stuff is really messy.  pty is doing is own handling + polling
> + HUP detection, which really is equally applicable to tcp and udp as
> well.

As far as I could tell the pty backend doesn't suffer from this issue. 
That's why I didn't change anything there.

> More below.
>
> > diff --git a/qemu-char.c b/qemu-char.c
> > index e00f84c..69649f0 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -213,7 +213,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
> >       s->chr_read = fd_read;
> >       s->chr_event = fd_event;
> >       s->handler_opaque = opaque;
> > -    if (s->chr_update_read_handler)
> > +    if (fe_open && s->chr_update_read_handler)
> >           s->chr_update_read_handler(s);
> >
> >       if (!s->explicit_fe_open) {
> > @@ -870,6 +870,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
> >   {
> >       FDCharDriver *s = chr->opaque;
> >
> > +    assert(!chr->fd_in_tag);
> >       remove_fd_in_watch(chr);
> >       if (s->fd_in) {
> >           chr->fd_in_tag = io_add_watch_poll(s->fd_in, fd_chr_read_poll,
> > @@ -2228,7 +2229,7 @@ static void udp_chr_update_read_handler(CharDriverState 
> > *chr)
> >   {
> >       NetCharDriver *s = chr->opaque;
> >
> > -    remove_fd_in_watch(chr);
> > +    assert(!chr->fd_in_tag);
>
> You're removing remove_fd_in_watch() here but not in
> fd_chr_update_read_handler() above.  Any particular reason?

I missed it while "commit -p" the changes. Fixed.

> Also, this assert would be a risky thing in pty, since it has its own
> HUP detection method.  A common way to deal with all this would be
> nice, but chardevs are hacks upon hacks already, and this fixes a
> regression, so I'm fine with applying the patch in this form now.
>
> Please update for pty.
>
> Gerd, can you please look at this and pick it up?  I'm going to be
> away till Jan.
>
> Thanks,
>
>                 Amit

    Gal.