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

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

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 | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 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 | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 270819a..6e623c3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -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 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 6e623c3..3558429 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 1/2] char: fix broken EAGAIN retry on OS-X due to errno clobbering

[Peter Maydell]

On 31 March 2016 at 15:29, Daniel P. Berrange <berrange@redhat.com> wrote:
> 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 | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 270819a..6e623c3 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -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;

qemu_chr_fe_write_log() also seems to have this broken retry pattern.

thanks
-- PMM

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

[Daniel P. Berrange]

On Thu, Mar 31, 2016 at 03:35:44PM +0100, Peter Maydell wrote:
> On 31 March 2016 at 15:29, Daniel P. Berrange <berrange@redhat.com> wrote:
> > 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 | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 270819a..6e623c3 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -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;
> 
> qemu_chr_fe_write_log() also seems to have this broken retry pattern.

Oh yes, so it does. Will resend a v2.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|