Re: [PATCH] error handling: Use TFR() macro where applicable

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 4. August 2022 09:25:17 CEST Nikita Ivanov wrote:
> From 0ceb04ada1ed5a863914f4449469d7572d3443ed Mon Sep 17 00:00:00 2001
> From: Nikita Ivanov <nivanov@cloudlinux.com>
> Date: Wed, 3 Aug 2022 12:54:00 +0300
> Subject: [PATCH] error handling: Use TFR() macro where applicable
> 
> There is a defined TFR() macro in qemu/osdep.h which
> handles the same while loop.
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/415
> 
> Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
> ---
>  hw/9pfs/9p-local.c   |  8 ++------
>  net/l2tpv3.c         | 15 +++------------
>  net/tap.c            |  8 ++------
>  qga/commands-posix.c |  4 +---
>  util/main-loop.c     |  4 +---
>  util/osdep.c         |  4 +---
>  6 files changed, 10 insertions(+), 33 deletions(-)

I was thinking the same as Marc-André before:

commit 1dacd88ddcf33eb6ed044c4080e3ef5e3de4b6b6
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Wed Mar 23 19:57:27 2022 +0400

    include: move TFR to osdep.h
    
    The macro requires EINTR, which has its header included in osdep.h.
    
    (Not sure what TFR stands for, perhaps "Test For Retry". Rename it ?)
    
    Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
    Message-Id: <20220323155743.1585078-17-marcandre.lureau@redhat.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Wouldn't it make sense to first rename TFR() to something like 
RETRY_ON_EINTR() and then doing this consolidation here on top?

Best regards,
Christian Schoenebeck

> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index d42ce6d8b8..c90ab947ba 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -470,9 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
> V9fsPath *fs_path,
>          if (fd == -1) {
>              return -1;
>          }
> -        do {
> -            tsize = read(fd, (void *)buf, bufsz);
> -        } while (tsize == -1 && errno == EINTR);
> +        TFR(tsize = read(fd, (void *)buf, bufsz));
>          close_preserve_errno(fd);
>      } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
>                 (fs_ctx->export_flags & V9FS_SM_NONE)) {
> @@ -908,9 +906,7 @@ static int local_symlink(FsContext *fs_ctx, const char
> *oldpath,
>          }
>          /* Write the oldpath (target) to the file. */
>          oldpath_size = strlen(oldpath);
> -        do {
> -            write_size = write(fd, (void *)oldpath, oldpath_size);
> -        } while (write_size == -1 && errno == EINTR);
> +        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
>          close_preserve_errno(fd);
> 
>          if (write_size != oldpath_size) {
> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
> index af373e5c30..adfdbdb84c 100644
> --- a/net/l2tpv3.c
> +++ b/net/l2tpv3.c
> @@ -240,9 +240,7 @@ static ssize_t
> net_l2tpv3_receive_dgram_iov(NetClientState *nc,
>      message.msg_control = NULL;
>      message.msg_controllen = 0;
>      message.msg_flags = 0;
> -    do {
> -        ret = sendmsg(s->fd, &message, 0);
> -    } while ((ret == -1) && (errno == EINTR));
> +    TFR(ret = sendmsg(s->fd, &message, 0));
>      if (ret > 0) {
>          ret -= s->offset;
>      } else if (ret == 0) {
> @@ -285,9 +283,7 @@ static ssize_t net_l2tpv3_receive_dgram(NetClientState
> *nc,
>      message.msg_control = NULL;
>      message.msg_controllen = 0;
>      message.msg_flags = 0;
> -    do {
> -        ret = sendmsg(s->fd, &message, 0);
> -    } while ((ret == -1) && (errno == EINTR));
> +    TFR(ret = sendmsg(s->fd, &message, 0));
>      if (ret > 0) {
>          ret -= s->offset;
>      } else if (ret == 0) {
> @@ -434,12 +430,7 @@ static void net_l2tpv3_send(void *opaque)
> 
>      msgvec = s->msgvec + s->queue_head;
>      if (target_count > 0) {
> -        do {
> -            count = recvmmsg(
> -                s->fd,
> -                msgvec,
> -                target_count, MSG_DONTWAIT, NULL);
> -        } while ((count == -1) && (errno == EINTR));
> +        TFR(count = recvmmsg(s->fd, msgvec, target_count, MSG_DONTWAIT,
> NULL));
>          if (count < 0) {
>              /* Recv error - we still need to flush packets here,
>               * (re)set queue head to current position
> diff --git a/net/tap.c b/net/tap.c
> index b3ddfd4a74..b047eca8b5 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -102,9 +102,7 @@ static ssize_t tap_write_packet(TAPState *s, const
> struct iovec *iov, int iovcnt
>  {
>      ssize_t len;
> 
> -    do {
> -        len = writev(s->fd, iov, iovcnt);
> -    } while (len == -1 && errno == EINTR);
> +    TFR(len = writev(s->fd, iov, iovcnt));
> 
>      if (len == -1 && errno == EAGAIN) {
>          tap_write_poll(s, true);
> @@ -577,9 +575,7 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
> 
>          close(sv[1]);
> 
> -        do {
> -            fd = recv_fd(sv[0]);
> -        } while (fd == -1 && errno == EINTR);
> +        TFR(fd = recv_fd(sv[0]));
>          saved_errno = errno;
> 
>          close(sv[0]);
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 954efed01b..90f83aa9b6 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -68,9 +68,7 @@ static void ga_wait_child(pid_t pid, int *status, Error
> **errp)
> 
>      *status = 0;
> 
> -    do {
> -        rpid = waitpid(pid, status, 0);
> -    } while (rpid == -1 && errno == EINTR);
> +    TFR(rpid = waitpid(pid, status, 0));
> 
>      if (rpid == -1) {
>          error_setg_errno(errp, errno, "failed to wait for child (pid: %d)",
> diff --git a/util/main-loop.c b/util/main-loop.c
> index f00a25451b..58e14db2d4 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -64,9 +64,7 @@ static void sigfd_handler(void *opaque)
>      ssize_t len;
> 
>      while (1) {
> -        do {
> -            len = read(fd, &info, sizeof(info));
> -        } while (len == -1 && errno == EINTR);
> +        TFR(len = read(fd, &info, sizeof(info)));
> 
>          if (len == -1 && errno == EAGAIN) {
>              break;
> diff --git a/util/osdep.c b/util/osdep.c
> index 60fcbbaebe..d35c473ac7 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -244,9 +244,7 @@ static int qemu_lock_fcntl(int fd, int64_t start,
> int64_t len, int fl_type)
>          .l_type   = fl_type,
>      };
>      qemu_probe_lock_ops();
> -    do {
> -        ret = fcntl(fd, fcntl_op_setlk, &fl);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(ret = fcntl(fd, fcntl_op_setlk, &fl));
>      return ret == -1 ? -errno : 0;
>  }