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

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

[Gal Hammer]

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 | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

V5: - remove_fd_in_watch in fd_chr_update_read_handler as well.
    - fix pty backend.

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.

diff --git a/qemu-char.c b/qemu-char.c
index 418dc69..019c178 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,7 +870,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
 
-    remove_fd_in_watch(chr);
+    assert(!chr->fd_in_tag);
     if (s->fd_in) {
         chr->fd_in_tag = io_add_watch_poll(s->fd_in, fd_chr_read_poll,
                                            fd_chr_read, chr);
@@ -1136,13 +1136,14 @@ static void pty_chr_state(CharDriverState *chr, int connected)
         if (!s->connected) {
             s->connected = 1;
             qemu_chr_be_generic_open(chr);
+        }
+        if (!chr->fd_in_tag) {
             chr->fd_in_tag = io_add_watch_poll(s->fd, pty_chr_read_poll,
                                                pty_chr_read, chr);
         }
     }
 }
 
-
 static void pty_chr_close(struct CharDriverState *chr)
 {
     PtyCharDriver *s = chr->opaque;
@@ -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);
     if (s->chan) {
         chr->fd_in_tag = io_add_watch_poll(s->chan, udp_chr_read_poll,
                                            udp_chr_read, chr);
@@ -2510,6 +2511,17 @@ static void tcp_chr_connect(void *opaque)
     qemu_chr_be_generic_open(chr);
 }
 
+static void tcp_chr_update_read_handler(CharDriverState *chr)
+{
+    TCPCharDriver *s = chr->opaque;
+
+    assert(!chr->fd_in_tag);
+    if (s->chan) {
+        chr->fd_in_tag = io_add_watch_poll(s->chan, tcp_chr_read_poll,
+                                           tcp_chr_read, chr);
+    }
+}
+
 #define IACSET(x,a,b,c) x[0] = a; x[1] = b; x[2] = c;
 static void tcp_chr_telnet_init(int fd)
 {
@@ -2665,6 +2677,7 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
     chr->get_msgfd = tcp_get_msgfd;
     chr->chr_add_client = tcp_chr_add_client;
     chr->chr_add_watch = tcp_chr_add_watch;
+    chr->chr_update_read_handler = tcp_chr_update_read_handler;
     /* be isn't opened until we get a connection */
     chr->explicit_be_open = true;
 
-- 
1.8.1.4

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

[Amit Shah]

On (Wed) 18 Dec 2013 [16:15:19], 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 | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> V5: - remove_fd_in_watch in fd_chr_update_read_handler as well.
>     - fix pty backend.

Reviewed-by: Amit Shah <amit.shah@redhat.com>

Gerd, could you take a look as well?

		Amit

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

[Gal Hammer]

Hi,

Anyone?

Thanks,

     Gal.

On 07/01/2014 09:25, Amit Shah wrote:
> On (Wed) 18 Dec 2013 [16:15:19], 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 | 21 +++++++++++++++++----
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> V5: - remove_fd_in_watch in fd_chr_update_read_handler as well.
>>      - fix pty backend.
>
> Reviewed-by: Amit Shah <amit.shah@redhat.com>
>
> Gerd, could you take a look as well?
>
> 		Amit
>