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 <address@hidden> 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 <address@hidden>
> > ---
> >  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 :|