[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] char: fix broken EAGAIN retry on OS-X due t
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] char: fix broken EAGAIN retry on OS-X due to errno clobbering |
Date: |
Thu, 31 Mar 2016 15:37:48 +0100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 :|