From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55977) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aldjS-0001Bg-Ny for qemu-devel@nongnu.org; Thu, 31 Mar 2016 10:38:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aldjM-0001iO-Px for qemu-devel@nongnu.org; Thu, 31 Mar 2016 10:37:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55906) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aldjM-0001iH-IP for qemu-devel@nongnu.org; Thu, 31 Mar 2016 10:37:52 -0400 Date: Thu, 31 Mar 2016 15:37:48 +0100 From: "Daniel P. Berrange" Message-ID: <20160331143748.GF12462@redhat.com> References: <1459434597-31255-1-git-send-email-berrange@redhat.com> <1459434597-31255-2-git-send-email-berrange@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 1/2] char: fix broken EAGAIN retry on OS-X due to errno clobbering Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Paolo Bonzini , QEMU Developers On Thu, Mar 31, 2016 at 03:35:44PM +0100, Peter Maydell wrote: > On 31 March 2016 at 15:29, Daniel P. Berrange 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 > > --- > > 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 :|