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

[Gal Hammer <ghammer@redhat.com>]

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 <ghammer@redhat.com>
>>
>> ---
>>   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.