[Qemu-devel] [PATCH v3] chardev/char-socket: add POLLHUP handler

[Qemu-devel] [PATCH v3] chardev/char-socket: add POLLHUP handler

[Klim Kireev]

The following behavior was observed for QEMU configured by libvirt
to use guest agent as usual for the guests without virtio-serial
driver (Windows or the guest remaining in BIOS stage).

In QEMU on first connect to listen character device socket
the listen socket is removed from poll just after the accept().
virtio_serial_guest_ready() returns 0 and the descriptor
of the connected Unix socket is removed from poll and it will
not be present in poll() until the guest will initialize the driver
and change the state of the serial to "guest connected".

In libvirt connect() to guest agent is performed on restart and
is run under VM state lock. Connect() is blocking and can
wait forever.
In this case libvirt can not perform ANY operation on that VM.

The bug can be easily reproduced this way:

Terminal 1:
qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev socket,id=serial1,path=/tmp/console.sock,server,nowait
(virtio-serial and isa-serial also fit)

Terminal 2:
minicom -D unix\#/tmp/console.sock
(type something and press enter)
C-a x (to exit)

Do 3 times:
minicom -D unix\#/tmp/console.sock
C-a x

It needs 4 connections, because the first one is accepted by QEMU, then two are queued by
the kernel, and the 4th blocks.

The problem is that QEMU doesn't add a read watcher after succesful read
until the guest device wants to acquire recieved data, so
I propose to install a separate pullhup watcher regardless of
whether the device waits for data or not.

Signed-off-by: Klim Kireev <klim.kireev@virtuozzo.com>
---
Changelog:
v2: Remove timer as a redundant feature

v3: Remove read call and return G_SOURCE_REMOVE

 chardev/char-socket.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 77cdf487eb..83fa7b70f0 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -42,6 +42,7 @@ typedef struct {
     QIOChannel *ioc; /* Client I/O channel */
     QIOChannelSocket *sioc; /* Client master channel */
     QIONetListener *listener;
+    guint hup_tag;
     QCryptoTLSCreds *tls_creds;
     int connected;
     int max_size;
@@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr)
         s->read_msgfds_num = 0;
     }
 
+    if (s->hup_tag != 0) {
+        g_source_remove(s->hup_tag);
+        s->hup_tag = 0;
+    }
+
     tcp_set_msgfds(chr, NULL, 0);
     remove_fd_in_watch(chr);
     object_unref(OBJECT(s->sioc));
@@ -455,6 +461,15 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     return TRUE;
 }
 
+static gboolean tcp_chr_hup(QIOChannel *channel,
+                               GIOCondition cond,
+                               void *opaque)
+{
+    Chardev *chr = CHARDEV(opaque);
+    tcp_chr_disconnect(chr);
+    return G_SOURCE_REMOVE;
+}
+
 static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -528,6 +543,10 @@ static void tcp_chr_connect(void *opaque)
                                            tcp_chr_read,
                                            chr, chr->gcontext);
     }
+    if (s->hup_tag == 0) {
+        s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
+                                           tcp_chr_hup, chr, NULL);
+    }
     qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }
 
@@ -546,7 +565,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
                                            tcp_chr_read, chr,
                                            chr->gcontext);
     }
-}
+    if (s->hup_tag == 0) {
+        s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
+                                           tcp_chr_hup, chr, NULL);
+    }
+ }
 
 typedef struct {
     Chardev *chr;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v3] chardev/char-socket: add POLLHUP handler

[Marc-Andre Lureau]

Hi

On Fri, Jan 19, 2018 at 11:47 AM, Klim Kireev <klim.kireev@virtuozzo.com> wrote:
> The following behavior was observed for QEMU configured by libvirt
> to use guest agent as usual for the guests without virtio-serial
> driver (Windows or the guest remaining in BIOS stage).
>
> In QEMU on first connect to listen character device socket
> the listen socket is removed from poll just after the accept().
> virtio_serial_guest_ready() returns 0 and the descriptor
> of the connected Unix socket is removed from poll and it will
> not be present in poll() until the guest will initialize the driver
> and change the state of the serial to "guest connected".
>
> In libvirt connect() to guest agent is performed on restart and
> is run under VM state lock. Connect() is blocking and can
> wait forever.
> In this case libvirt can not perform ANY operation on that VM.
>
> The bug can be easily reproduced this way:
>
> Terminal 1:
> qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev socket,id=serial1,path=/tmp/console.sock,server,nowait
> (virtio-serial and isa-serial also fit)
>
> Terminal 2:
> minicom -D unix\#/tmp/console.sock
> (type something and press enter)
> C-a x (to exit)
>
> Do 3 times:
> minicom -D unix\#/tmp/console.sock
> C-a x
>
> It needs 4 connections, because the first one is accepted by QEMU, then two are queued by
> the kernel, and the 4th blocks.
>
> The problem is that QEMU doesn't add a read watcher after succesful read
> until the guest device wants to acquire recieved data, so
> I propose to install a separate pullhup watcher regardless of
> whether the device waits for data or not.
>
> Signed-off-by: Klim Kireev <klim.kireev@virtuozzo.com>
> ---
> Changelog:
> v2: Remove timer as a redundant feature
>
> v3: Remove read call and return G_SOURCE_REMOVE
>
>  chardev/char-socket.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 77cdf487eb..83fa7b70f0 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -42,6 +42,7 @@ typedef struct {
>      QIOChannel *ioc; /* Client I/O channel */
>      QIOChannelSocket *sioc; /* Client master channel */
>      QIONetListener *listener;
> +    guint hup_tag;
>      QCryptoTLSCreds *tls_creds;
>      int connected;
>      int max_size;
> @@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr)
>          s->read_msgfds_num = 0;
>      }
>
> +    if (s->hup_tag != 0) {
> +        g_source_remove(s->hup_tag);
> +        s->hup_tag = 0;
> +    }
> +
>      tcp_set_msgfds(chr, NULL, 0);
>      remove_fd_in_watch(chr);
>      object_unref(OBJECT(s->sioc));
> @@ -455,6 +461,15 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>      return TRUE;
>  }
>
> +static gboolean tcp_chr_hup(QIOChannel *channel,
> +                               GIOCondition cond,
> +                               void *opaque)
> +{
> +    Chardev *chr = CHARDEV(opaque);
> +    tcp_chr_disconnect(chr);
> +    return G_SOURCE_REMOVE;

The source is removed, it should clear s->hup_tag to 0

> +}
> +
>  static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> @@ -528,6 +543,10 @@ static void tcp_chr_connect(void *opaque)
>                                             tcp_chr_read,
>                                             chr, chr->gcontext);
>      }
> +    if (s->hup_tag == 0) {
> +        s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
> +                                           tcp_chr_hup, chr, NULL);

Why not use chr->gcontext ?


> +    }
>      qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>  }
>
> @@ -546,7 +565,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
>                                             tcp_chr_read, chr,
>                                             chr->gcontext);
>      }
> -}
> +    if (s->hup_tag == 0) {
> +        s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
> +                                           tcp_chr_hup, chr, NULL);
> +    }

Why not set the handler on tcp_chr_connect() ?

thanks

> + }
>
>  typedef struct {
>      Chardev *chr;
> --
> 2.14.3
>

Re: [Qemu-devel] [PATCH v3] chardev/char-socket: add POLLHUP handler

[Peter Xu]

On Fri, Jan 19, 2018 at 01:47:15PM +0300, Klim Kireev wrote:
> The following behavior was observed for QEMU configured by libvirt
> to use guest agent as usual for the guests without virtio-serial
> driver (Windows or the guest remaining in BIOS stage).
> 
> In QEMU on first connect to listen character device socket
> the listen socket is removed from poll just after the accept().
> virtio_serial_guest_ready() returns 0 and the descriptor
> of the connected Unix socket is removed from poll and it will
> not be present in poll() until the guest will initialize the driver
> and change the state of the serial to "guest connected".
> 
> In libvirt connect() to guest agent is performed on restart and
> is run under VM state lock. Connect() is blocking and can
> wait forever.
> In this case libvirt can not perform ANY operation on that VM.
> 
> The bug can be easily reproduced this way:
> 
> Terminal 1:
> qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev socket,id=serial1,path=/tmp/console.sock,server,nowait
> (virtio-serial and isa-serial also fit)
> 
> Terminal 2:
> minicom -D unix\#/tmp/console.sock
> (type something and press enter)
> C-a x (to exit)
> 
> Do 3 times:
> minicom -D unix\#/tmp/console.sock
> C-a x
> 
> It needs 4 connections, because the first one is accepted by QEMU, then two are queued by
> the kernel, and the 4th blocks.
> 
> The problem is that QEMU doesn't add a read watcher after succesful read
> until the guest device wants to acquire recieved data, so
> I propose to install a separate pullhup watcher regardless of
> whether the device waits for data or not.
> 
> Signed-off-by: Klim Kireev <klim.kireev@virtuozzo.com>
> ---
> Changelog:
> v2: Remove timer as a redundant feature
> 
> v3: Remove read call and return G_SOURCE_REMOVE
> 
>  chardev/char-socket.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 77cdf487eb..83fa7b70f0 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -42,6 +42,7 @@ typedef struct {
>      QIOChannel *ioc; /* Client I/O channel */
>      QIOChannelSocket *sioc; /* Client master channel */
>      QIONetListener *listener;
> +    guint hup_tag;
>      QCryptoTLSCreds *tls_creds;
>      int connected;
>      int max_size;
> @@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr)
>          s->read_msgfds_num = 0;
>      }
>  
> +    if (s->hup_tag != 0) {
> +        g_source_remove(s->hup_tag);

Note that recently we are preparing the chardev to support non-default
gcontext.  So IMHO we'd better start to use GSource always, instead of
the old tags. g_source_remove() is tailored for default gcontext only.

Please refer to 938eb9e9c8 ("chardev: let g_idle_add() be with chardev
gcontext", 2018-01-12) as an example.

So even if the current patch works for now, it may break in a very
unpredictable way if we start to run chardevs in non-default
gcontexts.  Thanks,

> +        s->hup_tag = 0;
> +    }
> +
>      tcp_set_msgfds(chr, NULL, 0);
>      remove_fd_in_watch(chr);
>      object_unref(OBJECT(s->sioc));
> @@ -455,6 +461,15 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>      return TRUE;
>  }
>  
> +static gboolean tcp_chr_hup(QIOChannel *channel,
> +                               GIOCondition cond,
> +                               void *opaque)
> +{
> +    Chardev *chr = CHARDEV(opaque);
> +    tcp_chr_disconnect(chr);
> +    return G_SOURCE_REMOVE;
> +}
> +
>  static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> @@ -528,6 +543,10 @@ static void tcp_chr_connect(void *opaque)
>                                             tcp_chr_read,
>                                             chr, chr->gcontext);
>      }
> +    if (s->hup_tag == 0) {
> +        s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
> +                                           tcp_chr_hup, chr, NULL);

-- 
Peter Xu