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

[Marc-André Lureau]

Hi

On Fri, Aug 5, 2022 at 3:11 PM Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> 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?

Apparently TFR often stands for "Temp Failure Retry" (looking at
github code search)

LOOP_WHILE_EINTR ?

>
> 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;
> >  }
>
>