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

[klim]

On 01/18/2018 06:49 PM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Jan 18, 2018 at 3:33 PM, Klim Kireev <address@hidden> 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 <address@hidden>
> > ---
> > Changelog:
> > v2: Remove timer as a redundant feature
> >
> >   chardev/char-socket.c | 29 ++++++++++++++++++++++++++++-
> >   1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 77cdf487eb..d3fe903ab6 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,19 @@ 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);
> > +    SocketChardev *s = SOCKET_CHARDEV(chr);
> > +    tcp_chr_read(channel, cond, opaque);
> > +    if (s->connected != 0) {
> tcp_chr_read() shouldn't be called unless frontend is ready to read.
> qemu_chr_be_can_write() is regularly updated with tcp_chr_read_poll()
> but this may create some race here (if it read all it could read
> previously for example)
>
> If frontend can't read, s->connected won't be updated, so you'll busy
> loop in the source callback, not good.
>
> I think it needs further rework of how s->connected is updated.
>
> Why call tcp_chr_read() if you received HUP event ? could it call
> tcp_chr_free_connection()?

The reason is that:

if client sends data and closes the socket between two ppoll(), POLLHUP 
handler is called
and data in channel is lost, so read is used to pass it to guest.

if there is no data in channel, tcp_chr_recv() returns 0
and tcp_chr_read() calls tcp_chr_disconnect() which calls 
tcp_chr_free_connection().

If there is some data in channel it calls qemu_chr_be_write() and then 
in tcp_chr_disconnect()
tcp_free_connection() will be called.

In any case connection will be closed, so where is busy loop?
> > +        tcp_chr_disconnect(chr);
> > +    }
> > +    return TRUE;
> please use G_SOURCE_CONTINUE/REMOVE (I know it's not being used
> widely, but we have define now, and it is much clearer)
>
> > +}
> > +
> >   static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
> >   {
> >       SocketChardev *s = SOCKET_CHARDEV(chr);
> > @@ -528,6 +547,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 +569,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