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

[Peter Xu <peterx@redhat.com>]

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