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

[Qemu-devel] [PATCH v2] 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 transfter from the
host to the guest via the virtio-serial port.

https://bugzilla.redhat.com/show_bug.cgi?id=1027181

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.

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 qemu-char.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

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);
     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) {
+        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 v2] char: restore read callback on a reattached (hotplug) chardev

[Paolo Bonzini]

Il 11/12/2013 10:47, Gal Hammer ha scritto:
> 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 transfter from the
> host to the guest via the virtio-serial port.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1027181
> 
> 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.
> 
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  qemu-char.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> 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);
>      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) {
> +        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;
>  
> 

Cc: qemu-stable@nongnu.org

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]

On 12/11/2013 02:47 AM, 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 transfter from the

s/transfter/transferred/

> host to the guest via the virtio-serial port.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1027181

The S-o-B tag below, and the commit message up to here is fine...

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

...whereas this version information belongs better...

> 
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---

...here, after the --- separator.  It's useful information to reviewers,
but in this location, 'git am' will strip it, as in the long run,
someone reading 'git log' history doesn't care about how many revisions
a patch took or what changed from the earlier versions of the patch,
since none of the earlier versions are stored in git.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]