[Qemu-devel] [PATCH v2 0/2] Fixing non-blocking operation of chardevs

[Qemu-devel] [PATCH v2 0/2] Fixing non-blocking operation of chardevs

[Daniel P. Berrange]

This fixes socket chardevs to always be in non-blocking
mode, as they were before the QIOChannel conversion. The
second patch was already posted before, but dropped when
Peter discovered a problem on OS-X causing ahci-test to
hang:

  https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg05807.html

I traced this down to broken EAGAIN handling affecting
OS-X, hence the first patch in this series.

Changed in v2:

 - Also fix qemu_chr_fe_write_log method

Daniel P. Berrange (2):
  char: fix broken EAGAIN retry on OS-X due to errno clobbering
  char: ensure all clients are in non-blocking mode

 qemu-char.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v2 1/2] char: fix broken EAGAIN retry on OS-X due to errno clobbering

[Daniel P. Berrange]

Some of the chardev I/O paths really want to write the
complete data buffer even though the channel is in
non-blocking mode. To achieve this they look for EAGAIN
and g_usleep() for 100ms. Unfortunately the code is set
to check errno == EAGAIN a second time, after the g_usleep()
call has completed. On OS-X at least, g_usleep clobbers
errno to ETIMEDOUT, causing the retry to be skipped.

This failure to retry means the full data isn't written
to the chardev backend, which causes various failures
including making the tests/ahci-test qtest hang.

Rather than playing games trying to reset errno just
simplify the code to use a goto to retry instead of a
a loop.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-char.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 270819a..93fd733 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -225,12 +225,12 @@ static void qemu_chr_fe_write_log(CharDriverState *s,
     }
 
     while (done < len) {
-        do {
-            ret = write(s->logfd, buf + done, len - done);
-            if (ret == -1 && errno == EAGAIN) {
-                g_usleep(100);
-            }
-        } while (ret == -1 && errno == EAGAIN);
+    retry:
+        ret = write(s->logfd, buf + done, len - done);
+        if (ret == -1 && errno == EAGAIN) {
+            g_usleep(100);
+            goto retry;
+        }
 
         if (ret <= 0) {
             return;
@@ -246,12 +246,12 @@ static int qemu_chr_fe_write_buffer(CharDriverState *s, const uint8_t *buf, int
 
     qemu_mutex_lock(&s->chr_write_lock);
     while (*offset < len) {
-        do {
-            res = s->chr_write(s, buf + *offset, len - *offset);
-            if (res == -1 && errno == EAGAIN) {
-                g_usleep(100);
-            }
-        } while (res == -1 && errno == EAGAIN);
+    retry:
+        res = s->chr_write(s, buf + *offset, len - *offset);
+        if (res < 0 && errno == EAGAIN) {
+            g_usleep(100);
+            goto retry;
+        }
 
         if (res <= 0) {
             break;
@@ -333,12 +333,12 @@ int qemu_chr_fe_read_all(CharDriverState *s, uint8_t *buf, int len)
     }
 
     while (offset < len) {
-        do {
-            res = s->chr_sync_read(s, buf + offset, len - offset);
-            if (res == -1 && errno == EAGAIN) {
-                g_usleep(100);
-            }
-        } while (res == -1 && errno == EAGAIN);
+    retry:
+        res = s->chr_sync_read(s, buf + offset, len - offset);
+        if (res == -1 && errno == EAGAIN) {
+            g_usleep(100);
+            goto retry;
+        }
 
         if (res == 0) {
             break;
-- 
2.5.5

[Qemu-devel] [PATCH v2 2/2] char: ensure all clients are in non-blocking mode

[Daniel P. Berrange]

Only some callers of tcp_chr_new_client are putting the
socket client into non-blocking mode. Move the call to
qio_channel_set_blocking() into the tcp_chr_new_client
method to guarantee that all code paths set non-blocking
mode

Reported-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
Reported-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-char.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 93fd733..b597ee1 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3081,6 +3081,8 @@ static int tcp_chr_new_client(CharDriverState *chr, QIOChannelSocket *sioc)
     s->sioc = sioc;
     object_ref(OBJECT(sioc));
 
+    qio_channel_set_blocking(s->ioc, false, NULL);
+
     if (s->do_nodelay) {
         qio_channel_set_delay(s->ioc, false);
     }
@@ -3112,7 +3114,6 @@ static int tcp_chr_add_client(CharDriverState *chr, int fd)
     if (!sioc) {
         return -1;
     }
-    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
     ret = tcp_chr_new_client(chr, sioc);
     object_unref(OBJECT(sioc));
     return ret;
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 0/2] Fixing non-blocking operation of chardevs

[Paolo Bonzini]

On 31/03/2016 17:29, Daniel P. Berrange wrote:
> This fixes socket chardevs to always be in non-blocking
> mode, as they were before the QIOChannel conversion. The
> second patch was already posted before, but dropped when
> Peter discovered a problem on OS-X causing ahci-test to
> hang:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg05807.html
> 
> I traced this down to broken EAGAIN handling affecting
> OS-X, hence the first patch in this series.
> 
> Changed in v2:
> 
>  - Also fix qemu_chr_fe_write_log method
> 
> Daniel P. Berrange (2):
>   char: fix broken EAGAIN retry on OS-X due to errno clobbering
>   char: ensure all clients are in non-blocking mode
> 
>  qemu-char.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 

Thanks, queued for the next pull request (next week).

Paolo