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

[Qemu-devel] [PATCH v4] 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

v4: Move to GSource API

 chardev/char-socket.c | 22 ++++++++++++++++++++++
 dtc                   |  2 +-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 77cdf487eb..062d131079 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;
+    GSource *hup_source;
     QCryptoTLSCreds *tls_creds;
     int connected;
     int max_size;
@@ -352,6 +353,12 @@ static void tcp_chr_free_connection(Chardev *chr)
         s->read_msgfds_num = 0;
     }
 
+    if (s->hup_source != NULL) {
+        g_source_destroy(s->hup_source);
+        g_source_unref(s->hup_source);
+        s->hup_source = NULL;
+    }
+
     tcp_set_msgfds(chr, NULL, 0);
     remove_fd_in_watch(chr);
     object_unref(OBJECT(s->sioc));
@@ -455,6 +462,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 +544,12 @@ static void tcp_chr_connect(void *opaque)
                                            tcp_chr_read,
                                            chr, chr->gcontext);
     }
+
+    s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
+    g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
+                          chr, NULL);
+    g_source_attach(s->hup_source, chr->gcontext);
+    
     qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }
 
diff --git a/dtc b/dtc
index e54388015a..558cd81bdd 160000
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
+Subproject commit 558cd81bdd432769b59bff01240c44f82cfb1a9d
-- 
2.13.6

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180125123955.4919-1-klim.kireev@virtuozzo.com
Subject: [Qemu-devel] [PATCH v4] chardev/char-socket: add POLLHUP handler

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   f78b6f9b11..0f79bfe38a  master     -> master
 t [tag update]            patchew/1516732013-18272-1-git-send-email-walling@linux.vnet.ibm.com -> patchew/1516732013-18272-1-git-send-email-walling@linux.vnet.ibm.com
 t [tag update]            patchew/20180117174047.6382-1-david@redhat.com -> patchew/20180117174047.6382-1-david@redhat.com
 * [new tag]               patchew/20180125123955.4919-1-klim.kireev@virtuozzo.com -> patchew/20180125123955.4919-1-klim.kireev@virtuozzo.com
Switched to a new branch 'test'
47fcb0f720 chardev/char-socket: add POLLHUP handler

=== OUTPUT BEGIN ===
Checking PATCH 1/1: chardev/char-socket: add POLLHUP handler...
ERROR: trailing whitespace
#98: FILE: chardev/char-socket.c:552:
+    $

total: 1 errors, 0 warnings, 48 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Daniel P. Berrangé]

On Thu, Jan 25, 2018 at 03:39:55PM +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
> 
> v4: Move to GSource API
> 
>  chardev/char-socket.c | 22 ++++++++++++++++++++++
>  dtc                   |  2 +-
>  2 files changed, 23 insertions(+), 1 deletion(-)


> diff --git a/dtc b/dtc
> index e54388015a..558cd81bdd 160000
> --- a/dtc
> +++ b/dtc
> @@ -1 +1 @@
> -Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
> +Subproject commit 558cd81bdd432769b59bff01240c44f82cfb1a9d

Presumably accidental. You can fix this by doing

  cd dtc
  git checkout e54388015af1fb4bf04d0bca99caba1074d9cc42
  cd ..
  git add dtc
  git commit --amend

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Marc-André Lureau]

Hi

On Thu, Jan 25, 2018 at 1:39 PM, 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
>
> v4: Move to GSource API
>
>  chardev/char-socket.c | 22 ++++++++++++++++++++++
>  dtc                   |  2 +-
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 77cdf487eb..062d131079 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;
> +    GSource *hup_source;
>      QCryptoTLSCreds *tls_creds;
>      int connected;
>      int max_size;
> @@ -352,6 +353,12 @@ static void tcp_chr_free_connection(Chardev *chr)
>          s->read_msgfds_num = 0;
>      }
>
> +    if (s->hup_source != NULL) {
> +        g_source_destroy(s->hup_source);
> +        g_source_unref(s->hup_source);
> +        s->hup_source = NULL;
> +    }
> +
>      tcp_set_msgfds(chr, NULL, 0);
>      remove_fd_in_watch(chr);
>      object_unref(OBJECT(s->sioc));
> @@ -455,6 +462,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);

you should set s->hup_source to NULL

> +    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 +544,12 @@ static void tcp_chr_connect(void *opaque)
>                                             tcp_chr_read,
>                                             chr, chr->gcontext);
>      }
> +
> +    s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
> +    g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
> +                          chr, NULL);
> +    g_source_attach(s->hup_source, chr->gcontext);
> +
>      qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>  }
>
> diff --git a/dtc b/dtc
> index e54388015a..558cd81bdd 160000
> --- a/dtc
> +++ b/dtc
> @@ -1 +1 @@
> -Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
> +Subproject commit 558cd81bdd432769b59bff01240c44f82cfb1a9d
> --

unrelated

looks good otherwise



-- 
Marc-André Lureau

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

[Paolo Bonzini]

On 25/01/2018 13:45, Daniel P. Berrangé wrote:
>> index e54388015a..558cd81bdd 160000
>> --- a/dtc
>> +++ b/dtc
>> @@ -1 +1 @@
>> -Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
>> +Subproject commit 558cd81bdd432769b59bff01240c44f82cfb1a9d
> Presumably accidental. You can fix this by doing
> 
>   cd dtc
>   git checkout e54388015af1fb4bf04d0bca99caba1074d9cc42
>   cd ..
>   git add dtc
>   git commit --amend

Also, I think:

    git checkout HEAD^ -- dtc
    git commit --amend
    git submodule update

Paolo