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

[Paolo Bonzini]

On 10/01/2018 14:18, 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 pres 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. After closing the connection,
> the guest has a capability to read the data within timeout.

I don't understand the timeout part.

Apart from that, maybe the bug is in io_watch_poll_prepare, which needs 
to _always_ set up a "G_IO_ERR | G_IO_HUP | G_IO_NVAL" watch.  Only 
G_IO_IN depends on iwp->fd_can_read(...) > 0.

So the change would start with something like this:

diff --git a/chardev/char-io.c b/chardev/char-io.c
index f81052481a..a5e65d4e7c 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -29,6 +29,7 @@ typedef struct IOWatchPoll {
 
     QIOChannel *ioc;
     GSource *src;
+    GIOCondition cond;
 
     IOCanReadHandler *fd_can_read;
     GSourceFunc fd_read;
@@ -41,25 +42,32 @@ static IOWatchPoll *io_watch_poll_from_source(GSource 
*source)
     return container_of(source, IOWatchPoll, parent);
 }
 
+static void io_watch_poll_destroy_source(IOWatchPoll *iwp)
+{
+    if (iwp->src) {
+        g_source_destroy(iwp->src);
+        g_source_unref(iwp->src);
+        iwp->src = NULL;
+        iwp->cond = 0;
+    }
+}
+
 static gboolean io_watch_poll_prepare(GSource *source,
                                       gint *timeout)
 {
     IOWatchPoll *iwp = io_watch_poll_from_source(source);
     bool now_active = iwp->fd_can_read(iwp->opaque) > 0;
-    bool was_active = iwp->src != NULL;
-    if (was_active == now_active) {
-        return FALSE;
+    GIOCondition cond = G_IO_ERR | G_IO_HUP | G_IO_NVAL;
+    if (now_active) {
+        cond |= G_IO_IN;
     }
 
-    if (now_active) {
-        iwp->src = qio_channel_create_watch(
-            iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
+    if (iwp->cond != cond) {
+        io_watch_poll_destroy_source(iwp);
+        iwp->cond = cond;
+        iwp->src = qio_channel_create_watch(iwp->ioc, cond);
         g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
         g_source_attach(iwp->src, iwp->context);
-    } else {
-        g_source_destroy(iwp->src);
-        g_source_unref(iwp->src);
-        iwp->src = NULL;
     }
     return FALSE;
 }
@@ -131,11 +139,7 @@ static void io_remove_watch_poll(GSource *source)
     IOWatchPoll *iwp;
 
     iwp = io_watch_poll_from_source(source);
-    if (iwp->src) {
-        g_source_destroy(iwp->src);
-        g_source_unref(iwp->src);
-        iwp->src = NULL;
-    }
+    io_watch_poll_destroy_source(iwp);
     g_source_destroy(&iwp->parent);
 }
 


Thanks,

Paolo