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 <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.

thanks
-- PMM