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

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

[Gal Hammer]

Added a patch which fix an assertion error.

Gal Hammer (2):
  char: restore read callback on a reattached (hotplug) chardev
  qtest: fix a "!chr->fd_in_tag" assertion error in qtest.

 monitor.c   |  1 +
 qemu-char.c | 21 +++++++++++++++++----
 qtest.c     |  2 ++
 3 files changed, 20 insertions(+), 4 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/2] 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>

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

diff --git a/qemu-char.c b/qemu-char.c
index 4d50838..75e287c 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;
@@ -2227,7 +2228,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);
@@ -2509,6 +2510,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)
 {
@@ -2664,6 +2676,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.5.3

[Qemu-devel] [PATCH 2/2] qtest: fix a "!chr->fd_in_tag" assertion error in qtest.

[Gal Hammer]

Replacement of the default chardev handlers now requires a call to
release the current handlers.

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 monitor.c | 1 +
 qtest.c   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/monitor.c b/monitor.c
index de90fba..db52e7f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5024,6 +5024,7 @@ void monitor_init(CharDriverState *chr, int flags)
     if (monitor_ctrl_mode(mon)) {
         mon->mc = g_malloc0(sizeof(MonitorControl));
         /* Control mode requires special handlers */
+        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
         qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
                               monitor_control_event, mon);
         qemu_chr_fe_set_echo(chr, true);
diff --git a/qtest.c b/qtest.c
index ae941d6..a5682ee 100644
--- a/qtest.c
+++ b/qtest.c
@@ -519,6 +519,8 @@ void qtest_init(const char *qtest_chrdev, const char *qtest_log, Error **errp)
         return;
     }
 
+    /* Replace the default tcp's handlers with qtest's handlers. */
+    qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
     qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, qtest_event, chr);
     qemu_chr_fe_set_echo(chr, true);
 
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 2/2] qtest: fix a "!chr->fd_in_tag" assertion error in qtest.

[Paolo Bonzini]

Il 24/02/2014 11:16, Gal Hammer ha scritto:
> Replacement of the default chardev handlers now requires a call to
> release the current handlers.
>
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  monitor.c | 1 +
>  qtest.c   | 2 ++
>  2 files changed, 3 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index de90fba..db52e7f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -5024,6 +5024,7 @@ void monitor_init(CharDriverState *chr, int flags)
>      if (monitor_ctrl_mode(mon)) {
>          mon->mc = g_malloc0(sizeof(MonitorControl));
>          /* Control mode requires special handlers */
> +        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>          qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
>                                monitor_control_event, mon);
>          qemu_chr_fe_set_echo(chr, true);
> diff --git a/qtest.c b/qtest.c
> index ae941d6..a5682ee 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -519,6 +519,8 @@ void qtest_init(const char *qtest_chrdev, const char *qtest_log, Error **errp)
>          return;
>      }
>
> +    /* Replace the default tcp's handlers with qtest's handlers. */
> +    qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>      qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, qtest_event, chr);
>      qemu_chr_fe_set_echo(chr, true);
>
>

Why can't qemu_chr_add_handlers call remove_fd_in_watch when handlers 
are replaced, before setting the new ones?

Paolo

Re: [Qemu-devel] [PATCH 2/2] qtest: fix a "!chr->fd_in_tag" assertion error in qtest.

[Gal Hammer]

On 24/02/2014 12:32, Paolo Bonzini wrote:
> Il 24/02/2014 11:16, Gal Hammer ha scritto:
>> Replacement of the default chardev handlers now requires a call to
>> release the current handlers.
>>
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> ---
>>  monitor.c | 1 +
>>  qtest.c   | 2 ++
>>  2 files changed, 3 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index de90fba..db52e7f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -5024,6 +5024,7 @@ void monitor_init(CharDriverState *chr, int flags)
>>      if (monitor_ctrl_mode(mon)) {
>>          mon->mc = g_malloc0(sizeof(MonitorControl));
>>          /* Control mode requires special handlers */
>> +        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>>          qemu_chr_add_handlers(chr, monitor_can_read,
>> monitor_control_read,
>>                                monitor_control_event, mon);
>>          qemu_chr_fe_set_echo(chr, true);
>> diff --git a/qtest.c b/qtest.c
>> index ae941d6..a5682ee 100644
>> --- a/qtest.c
>> +++ b/qtest.c
>> @@ -519,6 +519,8 @@ void qtest_init(const char *qtest_chrdev, const
>> char *qtest_log, Error **errp)
>>          return;
>>      }
>>
>> +    /* Replace the default tcp's handlers with qtest's handlers. */
>> +    qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>>      qemu_chr_add_handlers(chr, qtest_can_read, qtest_read,
>> qtest_event, chr);
>>      qemu_chr_fe_set_echo(chr, true);
>>
>>
>
> Why can't qemu_chr_add_handlers call remove_fd_in_watch when handlers
> are replaced, before setting the new ones?
>
> Paolo

It used to be like that (see patch 1/2 for the change). However, I 
removed it after I saw that the function remove_fd_in_watch() is always 
called when setting all the handlers to NULL (qemu-char.c +3872) so a 
second call in every chr_update_read_handler callback seems redundant.

As far as I can tell only the qtest change the read handlers and not 
using the default ones.

     Gal.

Re: [Qemu-devel] [PATCH 2/2] qtest: fix a "!chr->fd_in_tag" assertion error in qtest.

[Gal Hammer]

Hi,

After reading the code again. It make sense to leave the call to remove_fd_in_watch() because I changed the code so the chr_update_read_handler function is called only when the front end is opened.

Submitting a new version of the patch.

    Gal.


----- Original Message -----
From: "Gal Hammer" <ghammer@redhat.com>
To: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
Cc: "amit shah" <amit.shah@redhat.com>, "peter maydell" <peter.maydell@linaro.org>, anthony@codemonkey.ws
Sent: Monday, February 24, 2014 1:44:31 PM
Subject: Re: [Qemu-devel] [PATCH 2/2] qtest: fix a "!chr->fd_in_tag" assertion error in qtest.

On 24/02/2014 12:32, Paolo Bonzini wrote:
> Il 24/02/2014 11:16, Gal Hammer ha scritto:
>> Replacement of the default chardev handlers now requires a call to
>> release the current handlers.
>>
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> ---
>>  monitor.c | 1 +
>>  qtest.c   | 2 ++
>>  2 files changed, 3 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index de90fba..db52e7f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -5024,6 +5024,7 @@ void monitor_init(CharDriverState *chr, int flags)
>>      if (monitor_ctrl_mode(mon)) {
>>          mon->mc = g_malloc0(sizeof(MonitorControl));
>>          /* Control mode requires special handlers */
>> +        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>>          qemu_chr_add_handlers(chr, monitor_can_read,
>> monitor_control_read,
>>                                monitor_control_event, mon);
>>          qemu_chr_fe_set_echo(chr, true);
>> diff --git a/qtest.c b/qtest.c
>> index ae941d6..a5682ee 100644
>> --- a/qtest.c
>> +++ b/qtest.c
>> @@ -519,6 +519,8 @@ void qtest_init(const char *qtest_chrdev, const
>> char *qtest_log, Error **errp)
>>          return;
>>      }
>>
>> +    /* Replace the default tcp's handlers with qtest's handlers. */
>> +    qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>>      qemu_chr_add_handlers(chr, qtest_can_read, qtest_read,
>> qtest_event, chr);
>>      qemu_chr_fe_set_echo(chr, true);
>>
>>
>
> Why can't qemu_chr_add_handlers call remove_fd_in_watch when handlers
> are replaced, before setting the new ones?
>
> Paolo

It used to be like that (see patch 1/2 for the change). However, I 
removed it after I saw that the function remove_fd_in_watch() is always 
called when setting all the handlers to NULL (qemu-char.c +3872) so a 
second call in every chr_update_read_handler callback seems redundant.

As far as I can tell only the qtest change the read handlers and not 
using the default ones.

     Gal.