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

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

[Nikita Ivanov]

[-- Attachment #1: Type: text/plain, Size: 5409 bytes --]

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(-)

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

-- 
2.37.1

[-- Attachment #2: Type: text/html, Size: 6680 bytes --]

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

[Christian Schoenebeck]

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

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

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

[Peter Maydell]

On Fri, 5 Aug 2022 at 12:27, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> On Fri, Aug 5, 2022 at 3:11 PM Christian Schoenebeck
> <qemu_oss@crudebyte.com> wrote:
> > 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 ?

At the risk of getting into bikeshedding, since glibc's unistd.h
defines a TEMP_FAILURE_RETRY() macro for this purpose, maybe we
should use that, with a thing in osdep.h for "provide this macro
if the system headers don't [ie musl, BSDs, Windows, etc]" ?

(There is a subtle difference between our TFR() and the glibc
TEMP_FAILURE_RETRY(): TEMP_FAILURE_RETRY() casts the result
of the expr to 'long int' before comparing for equality with -1.)

More generally, I think we should either use this macro rather
more widely, or get rid of it entirely. The current situation
where we use it in some of the net/tap code and a few chardevs
and basically nowhere else is not very satisfactory.

thanks
-- PMM

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

[Nikita Ivanov]

[-- Attachment #1: Type: text/plain, Size: 11836 bytes --]

Summing up the discussion above, I suggest the following patch for TFR()
macro refactoring. (The patch is sequential to the first one I introduced
in the start of the discussion).

From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00 2001
From: Nikita Ivanov <nivanov@cloudlinux.com>
Date: Mon, 8 Aug 2022 09:30:34 +0300
Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()

glibc's unistd.h header provides the same macro with the
subtle difference in type casting. Adjust macro name to the
common standard and define conditionally.

Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
---
 chardev/char-fd.c      |  2 +-
 chardev/char-pipe.c    | 12 +++++++++---
 hw/9pfs/9p-local.c     |  6 ++++--
 include/qemu/osdep.h   |  6 ++++--
 net/l2tpv3.c           |  8 +++++---
 net/tap-linux.c        |  2 +-
 net/tap.c              | 10 ++++++----
 os-posix.c             |  2 +-
 qga/commands-posix.c   |  2 +-
 tests/qtest/libqtest.c |  2 +-
 util/main-loop.c       |  2 +-
 util/osdep.c           |  2 +-
 12 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index cf78454841..7f5ed9aba3 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -198,7 +198,7 @@ int qmp_chardev_open_file_source(char *src, int flags,
Error **errp)
 {
     int fd = -1;

-    TFR(fd = qemu_open_old(src, flags, 0666));
+    TEMP_FAILURE_RETRY(fd = qemu_open_old(src, flags, 0666));
     if (fd == -1) {
         error_setg_file_open(errp, errno, src);
     }
diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index 66d3b85091..aed97e306b 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -131,8 +131,12 @@ static void qemu_chr_open_pipe(Chardev *chr,

     filename_in = g_strdup_printf("%s.in", filename);
     filename_out = g_strdup_printf("%s.out", filename);
-    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
-    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
+    TEMP_FAILURE_RETRY(
+        fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY)
+    );
+    TEMP_FAILURE_RETRY(
+        fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY)
+    );
     g_free(filename_in);
     g_free(filename_out);
     if (fd_in < 0 || fd_out < 0) {
@@ -142,7 +146,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
         if (fd_out >= 0) {
             close(fd_out);
         }
-        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
+        TEMP_FAILURE_RETRY(
+            fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY)
+        );
         if (fd_in < 0) {
             error_setg_file_open(errp, errno, filename);
             return;
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index c90ab947ba..e803c05d0c 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -470,7 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
V9fsPath *fs_path,
         if (fd == -1) {
             return -1;
         }
-        TFR(tsize = read(fd, (void *)buf, bufsz));
+        TEMP_FAILURE_RETRY(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)) {
@@ -906,7 +906,9 @@ static int local_symlink(FsContext *fs_ctx, const char
*oldpath,
         }
         /* Write the oldpath (target) to the file. */
         oldpath_size = strlen(oldpath);
-        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
+        TEMP_FAILURE_RETRY(
+            write_size = write(fd, (void *)oldpath, oldpath_size)
+        );
         close_preserve_errno(fd);

         if (write_size != oldpath_size) {
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index b1c161c035..55f2927d8b 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
 #if !defined(ESHUTDOWN)
 #define ESHUTDOWN 4099
 #endif
-
-#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
+#if !defined(TEMP_FAILURE_RETRY)
+#define TEMP_FAILURE_RETRY(expr) \
+    do { if ((expr) != -1) break; } while (errno == EINTR)
+#endif

 /* time_t may be either 32 or 64 bits depending on the host OS, and
  * can be either signed or unsigned, so we can't just hardcode a
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index adfdbdb84c..9594047ddb 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -240,7 +240,7 @@ static ssize_t
net_l2tpv3_receive_dgram_iov(NetClientState *nc,
     message.msg_control = NULL;
     message.msg_controllen = 0;
     message.msg_flags = 0;
-    TFR(ret = sendmsg(s->fd, &message, 0));
+    TEMP_FAILURE_RETRY(ret = sendmsg(s->fd, &message, 0));
     if (ret > 0) {
         ret -= s->offset;
     } else if (ret == 0) {
@@ -283,7 +283,7 @@ static ssize_t net_l2tpv3_receive_dgram(NetClientState
*nc,
     message.msg_control = NULL;
     message.msg_controllen = 0;
     message.msg_flags = 0;
-    TFR(ret = sendmsg(s->fd, &message, 0));
+    TEMP_FAILURE_RETRY(ret = sendmsg(s->fd, &message, 0));
     if (ret > 0) {
         ret -= s->offset;
     } else if (ret == 0) {
@@ -430,7 +430,9 @@ static void net_l2tpv3_send(void *opaque)

     msgvec = s->msgvec + s->queue_head;
     if (target_count > 0) {
-        TFR(count = recvmmsg(s->fd, msgvec, target_count, MSG_DONTWAIT,
NULL));
+        TEMP_FAILURE_RETRY(
+            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-linux.c b/net/tap-linux.c
index 304ff45071..e1a10c77af 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -45,7 +45,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     int len = sizeof(struct virtio_net_hdr);
     unsigned int features;

-    TFR(fd = open(PATH_NET_TUN, O_RDWR));
+    TEMP_FAILURE_RETRY(fd = open(PATH_NET_TUN, O_RDWR));
     if (fd < 0) {
         error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
         return -1;
diff --git a/net/tap.c b/net/tap.c
index b047eca8b5..9fa0a8fa21 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -102,7 +102,7 @@ static ssize_t tap_write_packet(TAPState *s, const
struct iovec *iov, int iovcnt
 {
     ssize_t len;

-    TFR(len = writev(s->fd, iov, iovcnt));
+    TEMP_FAILURE_RETRY(len = writev(s->fd, iov, iovcnt));

     if (len == -1 && errno == EAGAIN) {
         tap_write_poll(s, true);
@@ -575,7 +575,7 @@ static int net_bridge_run_helper(const char *helper,
const char *bridge,

         close(sv[1]);

-        TFR(fd = recv_fd(sv[0]));
+        TEMP_FAILURE_RETRY(fd = recv_fd(sv[0]));
         saved_errno = errno;

         close(sv[0]);
@@ -647,8 +647,10 @@ static int net_tap_init(const NetdevTapOptions *tap,
int *vnet_hdr,
         vnet_hdr_required = 0;
     }

-    TFR(fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
-                      mq_required, errp));
+    TEMP_FAILURE_RETRY(
+        fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
+                      mq_required, errp)
+    );
     if (fd < 0) {
         return -1;
     }
diff --git a/os-posix.c b/os-posix.c
index 321fc4bd13..e9918b9788 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -266,7 +266,7 @@ void os_setup_post(void)
             error_report("not able to chdir to /: %s", strerror(errno));
             exit(1);
         }
-        TFR(fd = qemu_open_old("/dev/null", O_RDWR));
+        TEMP_FAILURE_RETRY(fd = qemu_open_old("/dev/null", O_RDWR));
         if (fd == -1) {
             exit(1);
         }
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 90f83aa9b6..5dd2c4a3e5 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -68,7 +68,7 @@ static void ga_wait_child(pid_t pid, int *status, Error
**errp)

     *status = 0;

-    TFR(rpid = waitpid(pid, status, 0));
+    TEMP_FAILURE_RETRY(rpid = waitpid(pid, status, 0));

     if (rpid == -1) {
         error_setg_errno(errp, errno, "failed to wait for child (pid: %d)",
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 8c159eacf5..031f71cbba 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -140,7 +140,7 @@ void qtest_kill_qemu(QTestState *s)
     /* Skip wait if qtest_probe_child already reaped.  */
     if (pid != -1) {
         kill(pid, SIGTERM);
-        TFR(pid = waitpid(s->qemu_pid, &s->wstatus, 0));
+        TEMP_FAILURE_RETRY(pid = waitpid(s->qemu_pid, &s->wstatus, 0));
         assert(pid == s->qemu_pid);
         s->qemu_pid = -1;
     }
diff --git a/util/main-loop.c b/util/main-loop.c
index 58e14db2d4..3a21f39792 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -64,7 +64,7 @@ static void sigfd_handler(void *opaque)
     ssize_t len;

     while (1) {
-        TFR(len = read(fd, &info, sizeof(info)));
+        TEMP_FAILURE_RETRY(len = read(fd, &info, sizeof(info)));

         if (len == -1 && errno == EAGAIN) {
             break;
diff --git a/util/osdep.c b/util/osdep.c
index d35c473ac7..e5d2225738 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -244,7 +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();
-    TFR(ret = fcntl(fd, fcntl_op_setlk, &fl));
+    TEMP_FAILURE_RETRY(ret = fcntl(fd, fcntl_op_setlk, &fl));
     return ret == -1 ? -errno : 0;
 }

-- 
2.37.1


On Fri, Aug 5, 2022 at 2:40 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Fri, 5 Aug 2022 at 12:27, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> > On Fri, Aug 5, 2022 at 3:11 PM Christian Schoenebeck
> > <qemu_oss@crudebyte.com> wrote:
> > > 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 ?
>
> At the risk of getting into bikeshedding, since glibc's unistd.h
> defines a TEMP_FAILURE_RETRY() macro for this purpose, maybe we
> should use that, with a thing in osdep.h for "provide this macro
> if the system headers don't [ie musl, BSDs, Windows, etc]" ?
>
> (There is a subtle difference between our TFR() and the glibc
> TEMP_FAILURE_RETRY(): TEMP_FAILURE_RETRY() casts the result
> of the expr to 'long int' before comparing for equality with -1.)
>
> More generally, I think we should either use this macro rather
> more widely, or get rid of it entirely. The current situation
> where we use it in some of the net/tap code and a few chardevs
> and basically nowhere else is not very satisfactory.
>
> thanks
> -- PMM
>


-- 
Best Regards,
*Nikita Ivanov* | C developer
*Telephone:* +79140870696
*Telephone:* +79015053149

[-- Attachment #2: Type: text/html, Size: 14541 bytes --]

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

[Markus Armbruster]

Nikita Ivanov <nivanov@cloudlinux.com> writes:

> Summing up the discussion above, I suggest the following patch for TFR()
> macro refactoring. (The patch is sequential to the first one I introduced
> in the start of the discussion).
>
>>From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00 2001
> From: Nikita Ivanov <nivanov@cloudlinux.com>
> Date: Mon, 8 Aug 2022 09:30:34 +0300
> Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
>
> glibc's unistd.h header provides the same macro with the
> subtle difference in type casting. Adjust macro name to the
> common standard and define conditionally.
>
> Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
> ---
>  chardev/char-fd.c      |  2 +-
>  chardev/char-pipe.c    | 12 +++++++++---
>  hw/9pfs/9p-local.c     |  6 ++++--
>  include/qemu/osdep.h   |  6 ++++--
>  net/l2tpv3.c           |  8 +++++---
>  net/tap-linux.c        |  2 +-
>  net/tap.c              | 10 ++++++----
>  os-posix.c             |  2 +-
>  qga/commands-posix.c   |  2 +-
>  tests/qtest/libqtest.c |  2 +-
>  util/main-loop.c       |  2 +-
>  util/osdep.c           |  2 +-
>  12 files changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index cf78454841..7f5ed9aba3 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -198,7 +198,7 @@ int qmp_chardev_open_file_source(char *src, int flags,
> Error **errp)
>  {
>      int fd = -1;
>
> -    TFR(fd = qemu_open_old(src, flags, 0666));
> +    TEMP_FAILURE_RETRY(fd = qemu_open_old(src, flags, 0666));
>      if (fd == -1) {
>          error_setg_file_open(errp, errno, src);
>      }
> diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
> index 66d3b85091..aed97e306b 100644
> --- a/chardev/char-pipe.c
> +++ b/chardev/char-pipe.c
> @@ -131,8 +131,12 @@ static void qemu_chr_open_pipe(Chardev *chr,
>
>      filename_in = g_strdup_printf("%s.in", filename);
>      filename_out = g_strdup_printf("%s.out", filename);
> -    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
> -    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
> +    TEMP_FAILURE_RETRY(
> +        fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY)
> +    );
> +    TEMP_FAILURE_RETRY(
> +        fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY)
> +    );

Style question: do we want the ");" on its own line?  I think we
generally don't do that for function and function-like macro calls.

>      g_free(filename_in);
>      g_free(filename_out);
>      if (fd_in < 0 || fd_out < 0) {
> @@ -142,7 +146,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
>          if (fd_out >= 0) {
>              close(fd_out);
>          }
> -        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
> +        TEMP_FAILURE_RETRY(
> +            fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY)
> +        );
>          if (fd_in < 0) {
>              error_setg_file_open(errp, errno, filename);
>              return;
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index c90ab947ba..e803c05d0c 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -470,7 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
> V9fsPath *fs_path,
>          if (fd == -1) {
>              return -1;
>          }
> -        TFR(tsize = read(fd, (void *)buf, bufsz));
> +        TEMP_FAILURE_RETRY(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)) {
> @@ -906,7 +906,9 @@ static int local_symlink(FsContext *fs_ctx, const char
> *oldpath,
>          }
>          /* Write the oldpath (target) to the file. */
>          oldpath_size = strlen(oldpath);
> -        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
> +        TEMP_FAILURE_RETRY(
> +            write_size = write(fd, (void *)oldpath, oldpath_size)
> +        );
>          close_preserve_errno(fd);
>
>          if (write_size != oldpath_size) {
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index b1c161c035..55f2927d8b 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
>  #if !defined(ESHUTDOWN)
>  #define ESHUTDOWN 4099
>  #endif
> -
> -#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
> +#if !defined(TEMP_FAILURE_RETRY)
> +#define TEMP_FAILURE_RETRY(expr) \
> +    do { if ((expr) != -1) break; } while (errno == EINTR)
> +#endif

GLibc's version is

   # define TEMP_FAILURE_RETRY(expression) \
     (__extension__							      \
       ({ long int __result;						      \
          do __result = (long int) (expression);				      \
          while (__result == -1L && errno == EINTR);			      \
          __result; }))

The difference isn't just "type casting", it's also statement
vs. expression.

Is it a good idea to have the macro expand into a statement on some
hosts, and into an expression on others?  Sure, CI should catch any uses
as expression, but delaying compile errors to CI wastes developer time.

>
>  /* time_t may be either 32 or 64 bits depending on the host OS, and
>   * can be either signed or unsigned, so we can't just hardcode a

[...]

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

[Nikita Ivanov]

[-- Attachment #1: Type: text/plain, Size: 5933 bytes --]

Good point, thank you!
I think it's a bad idea to keep it like I proposed. Though, could I just
copy the definition that Markus has posted or there are any objections?

On Mon, Aug 8, 2022 at 11:06 AM Markus Armbruster <armbru@redhat.com> wrote:

> Nikita Ivanov <nivanov@cloudlinux.com> writes:
>
> > Summing up the discussion above, I suggest the following patch for TFR()
> > macro refactoring. (The patch is sequential to the first one I introduced
> > in the start of the discussion).
> >
> >>From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00 2001
> > From: Nikita Ivanov <nivanov@cloudlinux.com>
> > Date: Mon, 8 Aug 2022 09:30:34 +0300
> > Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
> >
> > glibc's unistd.h header provides the same macro with the
> > subtle difference in type casting. Adjust macro name to the
> > common standard and define conditionally.
> >
> > Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
> > ---
> >  chardev/char-fd.c      |  2 +-
> >  chardev/char-pipe.c    | 12 +++++++++---
> >  hw/9pfs/9p-local.c     |  6 ++++--
> >  include/qemu/osdep.h   |  6 ++++--
> >  net/l2tpv3.c           |  8 +++++---
> >  net/tap-linux.c        |  2 +-
> >  net/tap.c              | 10 ++++++----
> >  os-posix.c             |  2 +-
> >  qga/commands-posix.c   |  2 +-
> >  tests/qtest/libqtest.c |  2 +-
> >  util/main-loop.c       |  2 +-
> >  util/osdep.c           |  2 +-
> >  12 files changed, 35 insertions(+), 21 deletions(-)
> >
> > diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> > index cf78454841..7f5ed9aba3 100644
> > --- a/chardev/char-fd.c
> > +++ b/chardev/char-fd.c
> > @@ -198,7 +198,7 @@ int qmp_chardev_open_file_source(char *src, int
> flags,
> > Error **errp)
> >  {
> >      int fd = -1;
> >
> > -    TFR(fd = qemu_open_old(src, flags, 0666));
> > +    TEMP_FAILURE_RETRY(fd = qemu_open_old(src, flags, 0666));
> >      if (fd == -1) {
> >          error_setg_file_open(errp, errno, src);
> >      }
> > diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
> > index 66d3b85091..aed97e306b 100644
> > --- a/chardev/char-pipe.c
> > +++ b/chardev/char-pipe.c
> > @@ -131,8 +131,12 @@ static void qemu_chr_open_pipe(Chardev *chr,
> >
> >      filename_in = g_strdup_printf("%s.in", filename);
> >      filename_out = g_strdup_printf("%s.out", filename);
> > -    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
> > -    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
> > +    TEMP_FAILURE_RETRY(
> > +        fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY)
> > +    );
> > +    TEMP_FAILURE_RETRY(
> > +        fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY)
> > +    );
>
> Style question: do we want the ");" on its own line?  I think we
> generally don't do that for function and function-like macro calls.
>
> >      g_free(filename_in);
> >      g_free(filename_out);
> >      if (fd_in < 0 || fd_out < 0) {
> > @@ -142,7 +146,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
> >          if (fd_out >= 0) {
> >              close(fd_out);
> >          }
> > -        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR |
> O_BINARY));
> > +        TEMP_FAILURE_RETRY(
> > +            fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY)
> > +        );
> >          if (fd_in < 0) {
> >              error_setg_file_open(errp, errno, filename);
> >              return;
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index c90ab947ba..e803c05d0c 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -470,7 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
> > V9fsPath *fs_path,
> >          if (fd == -1) {
> >              return -1;
> >          }
> > -        TFR(tsize = read(fd, (void *)buf, bufsz));
> > +        TEMP_FAILURE_RETRY(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)) {
> > @@ -906,7 +906,9 @@ static int local_symlink(FsContext *fs_ctx, const
> char
> > *oldpath,
> >          }
> >          /* Write the oldpath (target) to the file. */
> >          oldpath_size = strlen(oldpath);
> > -        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
> > +        TEMP_FAILURE_RETRY(
> > +            write_size = write(fd, (void *)oldpath, oldpath_size)
> > +        );
> >          close_preserve_errno(fd);
> >
> >          if (write_size != oldpath_size) {
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index b1c161c035..55f2927d8b 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
> >  #if !defined(ESHUTDOWN)
> >  #define ESHUTDOWN 4099
> >  #endif
> > -
> > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
> > +#if !defined(TEMP_FAILURE_RETRY)
> > +#define TEMP_FAILURE_RETRY(expr) \
> > +    do { if ((expr) != -1) break; } while (errno == EINTR)
> > +#endif
>
> GLibc's version is
>
>    # define TEMP_FAILURE_RETRY(expression) \
>      (__extension__
>    \
>        ({ long int __result;
>     \
>           do __result = (long int) (expression);
>             \
>           while (__result == -1L && errno == EINTR);
>     \
>           __result; }))
>
> The difference isn't just "type casting", it's also statement
> vs. expression.
>
> Is it a good idea to have the macro expand into a statement on some
> hosts, and into an expression on others?  Sure, CI should catch any uses
> as expression, but delaying compile errors to CI wastes developer time.
>
> >
> >  /* time_t may be either 32 or 64 bits depending on the host OS, and
> >   * can be either signed or unsigned, so we can't just hardcode a
>
> [...]
>
>

-- 
Best Regards,
*Nikita Ivanov* | C developer
*Telephone:* +79140870696
*Telephone:* +79015053149

[-- Attachment #2: Type: text/html, Size: 8041 bytes --]

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

[Christian Schoenebeck]

On Montag, 8. August 2022 10:05:56 CEST Markus Armbruster wrote:
> Nikita Ivanov <nivanov@cloudlinux.com> writes:
> > Summing up the discussion above, I suggest the following patch for TFR()
> > macro refactoring. (The patch is sequential to the first one I introduced
> > in the start of the discussion).
> > 
> >>From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00 2001
> >>
> > From: Nikita Ivanov <nivanov@cloudlinux.com>
> > Date: Mon, 8 Aug 2022 09:30:34 +0300
> > Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
> > 
> > glibc's unistd.h header provides the same macro with the
> > subtle difference in type casting. Adjust macro name to the
> > common standard and define conditionally.
> > 
> > Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
> > ---
> > 
> >  chardev/char-fd.c      |  2 +-
> >  chardev/char-pipe.c    | 12 +++++++++---
> >  hw/9pfs/9p-local.c     |  6 ++++--
> >  include/qemu/osdep.h   |  6 ++++--
> >  net/l2tpv3.c           |  8 +++++---
> >  net/tap-linux.c        |  2 +-
> >  net/tap.c              | 10 ++++++----
> >  os-posix.c             |  2 +-
> >  qga/commands-posix.c   |  2 +-
> >  tests/qtest/libqtest.c |  2 +-
> >  util/main-loop.c       |  2 +-
> >  util/osdep.c           |  2 +-
> >  12 files changed, 35 insertions(+), 21 deletions(-)
> > 
> > diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> > index cf78454841..7f5ed9aba3 100644
> > --- a/chardev/char-fd.c
> > +++ b/chardev/char-fd.c
> > @@ -198,7 +198,7 @@ int qmp_chardev_open_file_source(char *src, int flags,
> > Error **errp)
> > 
> >  {
> >  
> >      int fd = -1;
> > 
> > -    TFR(fd = qemu_open_old(src, flags, 0666));
> > +    TEMP_FAILURE_RETRY(fd = qemu_open_old(src, flags, 0666));
> > 
> >      if (fd == -1) {
> >      
> >          error_setg_file_open(errp, errno, src);
> >      
> >      }
> > 
> > diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
> > index 66d3b85091..aed97e306b 100644
> > --- a/chardev/char-pipe.c
> > +++ b/chardev/char-pipe.c
> > @@ -131,8 +131,12 @@ static void qemu_chr_open_pipe(Chardev *chr,
> > 
> >      filename_in = g_strdup_printf("%s.in", filename);
> >      filename_out = g_strdup_printf("%s.out", filename);
> > 
> > -    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
> > -    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
> > +    TEMP_FAILURE_RETRY(
> > +        fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY)
> > +    );
> > +    TEMP_FAILURE_RETRY(
> > +        fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY)
> > +    );
> 
> Style question: do we want the ");" on its own line?  I think we
> generally don't do that for function and function-like macro calls.

So far scripts/checkpatch.pl doesn't complain, therefore I used this code 
style in QEMU before.

BTW, another exotic function call code style (not being compalained about yet) 
in approach:
https://lore.kernel.org/qemu-devel/E1oDQqv-0003d4-Hm@lizzy.crudebyte.com/

> >      g_free(filename_in);
> >      g_free(filename_out);
> >      if (fd_in < 0 || fd_out < 0) {
> > 
> > @@ -142,7 +146,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
> > 
> >          if (fd_out >= 0) {
> >          
> >              close(fd_out);
> >          
> >          }
> > 
> > -        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
> > +        TEMP_FAILURE_RETRY(
> > +            fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY)
> > +        );
> > 
> >          if (fd_in < 0) {
> >          
> >              error_setg_file_open(errp, errno, filename);
> >              return;
> > 
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index c90ab947ba..e803c05d0c 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -470,7 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
> > V9fsPath *fs_path,
> > 
> >          if (fd == -1) {
> >          
> >              return -1;
> >          
> >          }
> > 
> > -        TFR(tsize = read(fd, (void *)buf, bufsz));
> > +        TEMP_FAILURE_RETRY(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)) {
> > 
> > @@ -906,7 +906,9 @@ static int local_symlink(FsContext *fs_ctx, const char
> > *oldpath,
> > 
> >          }
> >          /* Write the oldpath (target) to the file. */
> >          oldpath_size = strlen(oldpath);
> > 
> > -        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
> > +        TEMP_FAILURE_RETRY(
> > +            write_size = write(fd, (void *)oldpath, oldpath_size)
> > +        );
> > 
> >          close_preserve_errno(fd);
> >          
> >          if (write_size != oldpath_size) {
> > 
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index b1c161c035..55f2927d8b 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
> > 
> >  #if !defined(ESHUTDOWN)
> >  #define ESHUTDOWN 4099
> >  #endif
> > 
> > -
> > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
> > +#if !defined(TEMP_FAILURE_RETRY)
> > +#define TEMP_FAILURE_RETRY(expr) \
> > +    do { if ((expr) != -1) break; } while (errno == EINTR)
> > +#endif
> 
> GLibc's version is
> 
>    # define TEMP_FAILURE_RETRY(expression) \
>      (__extension__						
	      \
>        ({ long int __result;					
	      \
>           do __result = (long int) (expression);			
	      \
>           while (__result == -1L && errno == EINTR);			
      \
>           __result; }))
> 
> The difference isn't just "type casting", it's also statement
> vs. expression.
> 
> Is it a good idea to have the macro expand into a statement on some
> hosts, and into an expression on others?  Sure, CI should catch any uses
> as expression, but delaying compile errors to CI wastes developer time.

For consistency and simplicity, I would define exactly one version (no ifdefs) 
of the macro with a different macro name than glibc's TEMP_FAILURE_RETRY(), 
and use that QEMU specific macro name in QEMU code everywhere.

As for statement vs. expression: The only advantage of the statement version 
is if you'd need __result as an rvalue, which is not needed ATM, right? So I 
would go for the expression version (with cast) for now.

The glibc history does not reveal why they chose the statement version.

Best regards,
Christian Schoenebeck

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

[Christian Schoenebeck]

On Montag, 8. August 2022 14:52:28 CEST Christian Schoenebeck wrote:
> On Montag, 8. August 2022 10:05:56 CEST Markus Armbruster wrote:
> > Nikita Ivanov <nivanov@cloudlinux.com> writes:
> > > Summing up the discussion above, I suggest the following patch for TFR()
> > > macro refactoring. (The patch is sequential to the first one I
> > > introduced
> > > in the start of the discussion).
> > > 
> > >>From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00 2001
> > >>
> > > From: Nikita Ivanov <nivanov@cloudlinux.com>
> > > Date: Mon, 8 Aug 2022 09:30:34 +0300
> > > Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
> > > 
> > > glibc's unistd.h header provides the same macro with the
> > > subtle difference in type casting. Adjust macro name to the
> > > common standard and define conditionally.
> > > 
> > > Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
> > > ---
> > > 
> > >  chardev/char-fd.c      |  2 +-
> > >  chardev/char-pipe.c    | 12 +++++++++---
> > >  hw/9pfs/9p-local.c     |  6 ++++--
> > >  include/qemu/osdep.h   |  6 ++++--
> > >  net/l2tpv3.c           |  8 +++++---
> > >  net/tap-linux.c        |  2 +-
> > >  net/tap.c              | 10 ++++++----
> > >  os-posix.c             |  2 +-
> > >  qga/commands-posix.c   |  2 +-
> > >  tests/qtest/libqtest.c |  2 +-
> > >  util/main-loop.c       |  2 +-
> > >  util/osdep.c           |  2 +-
> > >  12 files changed, 35 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> > > index cf78454841..7f5ed9aba3 100644
> > > --- a/chardev/char-fd.c
> > > +++ b/chardev/char-fd.c
> > > @@ -198,7 +198,7 @@ int qmp_chardev_open_file_source(char *src, int
> > > flags,
> > > Error **errp)
> > > 
> > >  {
> > >  
> > >      int fd = -1;
> > > 
> > > -    TFR(fd = qemu_open_old(src, flags, 0666));
> > > +    TEMP_FAILURE_RETRY(fd = qemu_open_old(src, flags, 0666));
> > > 
> > >      if (fd == -1) {
> > >      
> > >          error_setg_file_open(errp, errno, src);
> > >      
> > >      }
> > > 
> > > diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
> > > index 66d3b85091..aed97e306b 100644
> > > --- a/chardev/char-pipe.c
> > > +++ b/chardev/char-pipe.c
> > > @@ -131,8 +131,12 @@ static void qemu_chr_open_pipe(Chardev *chr,
> > > 
> > >      filename_in = g_strdup_printf("%s.in", filename);
> > >      filename_out = g_strdup_printf("%s.out", filename);
> > > 
> > > -    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
> > > -    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
> > > +    TEMP_FAILURE_RETRY(
> > > +        fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY)
> > > +    );
> > > +    TEMP_FAILURE_RETRY(
> > > +        fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY)
> > > +    );
> > 
> > Style question: do we want the ");" on its own line?  I think we
> > generally don't do that for function and function-like macro calls.
> 
> So far scripts/checkpatch.pl doesn't complain, therefore I used this code
> style in QEMU before.
> 
> BTW, another exotic function call code style (not being compalained about
> yet) in approach:
> https://lore.kernel.org/qemu-devel/E1oDQqv-0003d4-Hm@lizzy.crudebyte.com/
> 
> > >      g_free(filename_in);
> > >      g_free(filename_out);
> > >      if (fd_in < 0 || fd_out < 0) {
> > > 
> > > @@ -142,7 +146,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
> > > 
> > >          if (fd_out >= 0) {
> > >          
> > >              close(fd_out);
> > >          
> > >          }
> > > 
> > > -        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR |
> > > O_BINARY));
> > > +        TEMP_FAILURE_RETRY(
> > > +            fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY)
> > > +        );
> > > 
> > >          if (fd_in < 0) {
> > >          
> > >              error_setg_file_open(errp, errno, filename);
> > >              return;
> > > 
> > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > index c90ab947ba..e803c05d0c 100644
> > > --- a/hw/9pfs/9p-local.c
> > > +++ b/hw/9pfs/9p-local.c
> > > @@ -470,7 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
> > > V9fsPath *fs_path,
> > > 
> > >          if (fd == -1) {
> > >          
> > >              return -1;
> > >          
> > >          }
> > > 
> > > -        TFR(tsize = read(fd, (void *)buf, bufsz));
> > > +        TEMP_FAILURE_RETRY(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)) {
> > > 
> > > @@ -906,7 +906,9 @@ static int local_symlink(FsContext *fs_ctx, const
> > > char
> > > *oldpath,
> > > 
> > >          }
> > >          /* Write the oldpath (target) to the file. */
> > >          oldpath_size = strlen(oldpath);
> > > 
> > > -        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
> > > +        TEMP_FAILURE_RETRY(
> > > +            write_size = write(fd, (void *)oldpath, oldpath_size)
> > > +        );
> > > 
> > >          close_preserve_errno(fd);
> > >          
> > >          if (write_size != oldpath_size) {
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index b1c161c035..55f2927d8b 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
> > > 
> > >  #if !defined(ESHUTDOWN)
> > >  #define ESHUTDOWN 4099
> > >  #endif
> > > 
> > > -
> > > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno ==
> > > EINTR)
> > > +#if !defined(TEMP_FAILURE_RETRY)
> > > +#define TEMP_FAILURE_RETRY(expr) \
> > > +    do { if ((expr) != -1) break; } while (errno == EINTR)
> > > +#endif
> > 
> > GLibc's version is
> > 
> >    # define TEMP_FAILURE_RETRY(expression) \
> >    
> >      (__extension__
> 
> 	      \
> 
> >        ({ long int __result;
> 
> 	      \
> 
> >           do __result = (long int) (expression);
> 
> 	      \
> 
> >           while (__result == -1L && errno == EINTR);
> 
>       \
> 
> >           __result; }))
> > 
> > The difference isn't just "type casting", it's also statement
> > vs. expression.
> > 
> > Is it a good idea to have the macro expand into a statement on some
> > hosts, and into an expression on others?  Sure, CI should catch any uses
> > as expression, but delaying compile errors to CI wastes developer time.
> 
> For consistency and simplicity, I would define exactly one version (no
> ifdefs) of the macro with a different macro name than glibc's
> TEMP_FAILURE_RETRY(), and use that QEMU specific macro name in QEMU code
> everywhere.
> 
> As for statement vs. expression: The only advantage of the statement version
> is if you'd need __result as an rvalue, which is not needed ATM, right? So
> I would go for the expression version (with cast) for now.
> 
> The glibc history does not reveal why they chose the statement version.
> 
> Best regards,
> Christian Schoenebeck

Sorry: s/rvalue/lvalue/ i.e. if you need the memory address of result or if 
you need to take the result value of the last iteration in 'expression' into 
account.

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

[Markus Armbruster]

Christian Schoenebeck <qemu_oss@crudebyte.com> writes:

> On Montag, 8. August 2022 14:52:28 CEST Christian Schoenebeck wrote:
>> On Montag, 8. August 2022 10:05:56 CEST Markus Armbruster wrote:
>> > Nikita Ivanov <nivanov@cloudlinux.com> writes:
>> > > Summing up the discussion above, I suggest the following patch for TFR()
>> > > macro refactoring. (The patch is sequential to the first one I
>> > > introduced
>> > > in the start of the discussion).
>> > > 
>> > >>From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00 2001
>> > >>
>> > > From: Nikita Ivanov <nivanov@cloudlinux.com>
>> > > Date: Mon, 8 Aug 2022 09:30:34 +0300
>> > > Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
>> > > 
>> > > glibc's unistd.h header provides the same macro with the
>> > > subtle difference in type casting. Adjust macro name to the
>> > > common standard and define conditionally.
>> > > 
>> > > Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>

[...]

>> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> > > index b1c161c035..55f2927d8b 100644
>> > > --- a/include/qemu/osdep.h
>> > > +++ b/include/qemu/osdep.h
>> > > @@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
>> > > 
>> > >  #if !defined(ESHUTDOWN)
>> > >  #define ESHUTDOWN 4099
>> > >  #endif
>> > > 
>> > > -
>> > > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno ==
>> > > EINTR)
>> > > +#if !defined(TEMP_FAILURE_RETRY)
>> > > +#define TEMP_FAILURE_RETRY(expr) \
>> > > +    do { if ((expr) != -1) break; } while (errno == EINTR)

To avoid / reduce confusion: this macro expands into a statement, and ...

>> > > +#endif
>> > 
>> > GLibc's version is
>> > 
>> >    # define TEMP_FAILURE_RETRY(expression) \
>> >      (__extension__							      \
>> >        ({ long int __result;						      \
>> >           do __result = (long int) (expression);				      \
>> >           while (__result == -1L && errno == EINTR);			      \
>> >           __result; }))

... this one expands into an expression.  It uses GCC's "a compound
statement enclosed in parentheses may appear as an expression" extension.

>> > 
>> > The difference isn't just "type casting", it's also statement
>> > vs. expression.
>> > 
>> > Is it a good idea to have the macro expand into a statement on some
>> > hosts, and into an expression on others?  Sure, CI should catch any uses
>> > as expression, but delaying compile errors to CI wastes developer time.
>> 
>> For consistency and simplicity, I would define exactly one version (no
>> ifdefs) of the macro with a different macro name than glibc's
>> TEMP_FAILURE_RETRY(), and use that QEMU specific macro name in QEMU code
>> everywhere.

TFR()?  Can't resist closing the circle...

>> As for statement vs. expression: The only advantage of the statement version
>> is if you'd need __result as an rvalue, which is not needed ATM, right? So
>> I would go for the expression version (with cast) for now.

The expression-like macro is nicer where the return value matters.
Example (stolen from "The GNU C Library Reference Manual"):

    nbytes = TEMP_FAILURE_RETRY (write (desc, buffer, count));

With the statement-like macro, you have to write

    TEMP_FAILURE_RETRY (nbytes = write (desc, buffer, count));

>> The glibc history does not reveal why they chose the statement version.

The expression version, actually.

>> Best regards,
>> Christian Schoenebeck
>
> Sorry: s/rvalue/lvalue/ i.e. if you need the memory address of result or if 
> you need to take the result value of the last iteration in 'expression' into 
> account.

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

[Nikita Ivanov]

[-- Attachment #1: Type: text/plain, Size: 17127 bytes --]

Hi!
During our discussion, I found that I've missed a couple of places where
TFR() macro could be applied. Here is an updated first patch:

From 8a68f50aac4a8549f416b9350cf339cf0501a712 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>
---
 block/file-posix.c        | 39 ++++++++++++++++++---------------------
 chardev/char-pty.c        |  4 +---
 hw/9pfs/9p-local.c        |  8 ++------
 net/l2tpv3.c              | 15 +++------------
 net/socket.c              | 16 +++++++---------
 net/tap.c                 |  8 ++------
 qga/commands-posix.c      |  4 +---
 semihosting/syscalls.c    |  4 +---
 tests/qtest/libqtest.c    | 14 +++++++-------
 tests/vhost-user-bridge.c |  4 +---
 util/main-loop.c          |  4 +---
 util/osdep.c              |  4 +---
 util/vfio-helpers.c       | 12 ++++++------
 13 files changed, 51 insertions(+), 85 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 48cd096624..0786cb76f9 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1238,9 +1238,9 @@ static int hdev_get_max_segments(int fd, struct stat
*st)
         ret = -errno;
         goto out;
     }
-    do {
-        ret = read(sysfd, buf, sizeof(buf) - 1);
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = read(sysfd, buf, sizeof(buf) - 1)
+    );
     if (ret < 0) {
         ret = -errno;
         goto out;
@@ -1388,9 +1388,9 @@ static int handle_aiocb_ioctl(void *opaque)
     RawPosixAIOData *aiocb = opaque;
     int ret;

-    do {
-        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
+    );
     if (ret == -1) {
         return -errno;
     }
@@ -1472,18 +1472,17 @@ static ssize_t
handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
 {
     ssize_t len;

-    do {
-        if (aiocb->aio_type & QEMU_AIO_WRITE)
-            len = qemu_pwritev(aiocb->aio_fildes,
-                               aiocb->io.iov,
-                               aiocb->io.niov,
-                               aiocb->aio_offset);
-         else
-            len = qemu_preadv(aiocb->aio_fildes,
-                              aiocb->io.iov,
-                              aiocb->io.niov,
-                              aiocb->aio_offset);
-    } while (len == -1 && errno == EINTR);
+    TFR(
+        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
+            qemu_pwritev(aiocb->aio_fildes,
+                           aiocb->io.iov,
+                           aiocb->io.niov,
+                           aiocb->aio_offset) :
+            qemu_preadv(aiocb->aio_fildes,
+                          aiocb->io.iov,
+                          aiocb->io.niov,
+                          aiocb->aio_offset)
+    );

     if (len == -1) {
         return -errno;
@@ -1908,9 +1907,7 @@ static int allocate_first_block(int fd, size_t
max_size)
     buf = qemu_memalign(max_align, write_size);
     memset(buf, 0, write_size);

-    do {
-        n = pwrite(fd, buf, write_size, 0);
-    } while (n == -1 && errno == EINTR);
+    TFR(n = pwrite(fd, buf, write_size, 0));

     ret = (n == -1) ? -errno : 0;

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index 53f25c6bbd..b2f490bacf 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -93,9 +93,7 @@ static void pty_chr_update_read_handler(Chardev *chr)
     pfd.fd = fioc->fd;
     pfd.events = G_IO_OUT;
     pfd.revents = 0;
-    do {
-        rc = g_poll(&pfd, 1, 0);
-    } while (rc == -1 && errno == EINTR);
+    TFR(rc = g_poll(&pfd, 1, 0));
     assert(rc >= 0);

     if (pfd.revents & G_IO_HUP) {
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/socket.c b/net/socket.c
index bfd8596250..a3397660c2 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -117,15 +117,13 @@ static ssize_t
net_socket_receive_dgram(NetClientState *nc, const uint8_t *buf,
     NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
     ssize_t ret;

-    do {
-        if (s->dgram_dst.sin_family != AF_UNIX) {
-            ret = sendto(s->fd, buf, size, 0,
-                         (struct sockaddr *)&s->dgram_dst,
-                         sizeof(s->dgram_dst));
-        } else {
-            ret = send(s->fd, buf, size, 0);
-        }
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = s->dgram_dst.sin_family != AF_UNIX ?
+            sendto(s->fd, buf, size, 0,
+                     (struct sockaddr *)&s->dgram_dst,
+                     sizeof(s->dgram_dst)) :
+            send(s->fd, buf, size, 0)
+    );

     if (ret == -1 && errno == EAGAIN) {
         net_socket_write_poll(s, true);
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/semihosting/syscalls.c b/semihosting/syscalls.c
index 508a0ad88c..025c08147a 100644
--- a/semihosting/syscalls.c
+++ b/semihosting/syscalls.c
@@ -317,9 +317,7 @@ static void host_read(CPUState *cs,
gdb_syscall_complete_cb complete,
         complete(cs, -1, EFAULT);
         return;
     }
-    do {
-        ret = read(gf->hostfd, ptr, len);
-    } while (ret == -1 && errno == EINTR);
+    TFR(ret = read(gf->hostfd, ptr, len));
     if (ret == -1) {
         complete(cs, -1, errno);
         unlock_user(ptr, buf, 0);
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 8c159eacf5..d4022966b3 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -101,10 +101,10 @@ static int socket_accept(int sock)
         return -1;
     }

-    do {
-        addrlen = sizeof(addr);
-        ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
-    } while (ret == -1 && errno == EINTR);
+    addrlen = sizeof(addr);
+    TFR(
+        ret = accept(sock, (struct sockaddr *)&addr, &addrlen)
+    );
     if (ret == -1) {
         fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
     }
@@ -578,9 +578,9 @@ int qtest_socket_server(const char *socket_path)
     addr.sun_family = AF_UNIX;
     snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);

-    do {
-        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr))
+    );
     g_assert_cmpint(ret, !=, -1);
     ret = listen(sock, 1);
     g_assert_cmpint(ret, !=, -1);
diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index 9b1dab2f28..4d8f933ed5 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -331,9 +331,7 @@ vubr_backend_recv_cb(int sock, void *ctx)
             .msg_iovlen = num,
             .msg_flags = MSG_DONTWAIT,
         };
-        do {
-            ret = recvmsg(vubr->backend_udp_sock, &msg, 0);
-        } while (ret == -1 && (errno == EINTR));
+        TFR(ret = recvmsg(vubr->backend_udp_sock, &msg, 0));

         if (i == 0) {
             iov_restore_front(elem->in_sg, sg, hdrlen);
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;
 }

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 5ba01177bf..c878be1c5f 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -240,9 +240,9 @@ static int qemu_vfio_pci_read_config(QEMUVFIOState *s,
void *buf,
                                     s->config_region_info.offset,
                                     s->config_region_info.size);
     assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
-    do {
-        ret = pread(s->device, buf, size, s->config_region_info.offset +
ofs);
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = pread(s->device, buf, size, s->config_region_info.offset +
ofs)
+    );
     return ret == size ? 0 : -errno;
 }

@@ -254,9 +254,9 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s,
void *buf, int size, int
                                      s->config_region_info.offset,
                                      s->config_region_info.size);
     assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
-    do {
-        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
ofs);
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
ofs)
+    );
     return ret == size ? 0 : -errno;
 }

-- 
2.37.1


On Mon, Aug 8, 2022 at 5:22 PM Markus Armbruster <armbru@redhat.com> wrote:

> Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
>
> > On Montag, 8. August 2022 14:52:28 CEST Christian Schoenebeck wrote:
> >> On Montag, 8. August 2022 10:05:56 CEST Markus Armbruster wrote:
> >> > Nikita Ivanov <nivanov@cloudlinux.com> writes:
> >> > > Summing up the discussion above, I suggest the following patch for
> TFR()
> >> > > macro refactoring. (The patch is sequential to the first one I
> >> > > introduced
> >> > > in the start of the discussion).
> >> > >
> >> > >>From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00
> 2001
> >> > >>
> >> > > From: Nikita Ivanov <nivanov@cloudlinux.com>
> >> > > Date: Mon, 8 Aug 2022 09:30:34 +0300
> >> > > Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
> >> > >
> >> > > glibc's unistd.h header provides the same macro with the
> >> > > subtle difference in type casting. Adjust macro name to the
> >> > > common standard and define conditionally.
> >> > >
> >> > > Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
>
> [...]
>
> >> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> >> > > index b1c161c035..55f2927d8b 100644
> >> > > --- a/include/qemu/osdep.h
> >> > > +++ b/include/qemu/osdep.h
> >> > > @@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
> >> > >
> >> > >  #if !defined(ESHUTDOWN)
> >> > >  #define ESHUTDOWN 4099
> >> > >  #endif
> >> > >
> >> > > -
> >> > > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno ==
> >> > > EINTR)
> >> > > +#if !defined(TEMP_FAILURE_RETRY)
> >> > > +#define TEMP_FAILURE_RETRY(expr) \
> >> > > +    do { if ((expr) != -1) break; } while (errno == EINTR)
>
> To avoid / reduce confusion: this macro expands into a statement, and ...
>
> >> > > +#endif
> >> >
> >> > GLibc's version is
> >> >
> >> >    # define TEMP_FAILURE_RETRY(expression) \
> >> >      (__extension__
>             \
> >> >        ({ long int __result;
>            \
> >> >           do __result = (long int) (expression);
>            \
> >> >           while (__result == -1L && errno == EINTR);
>            \
> >> >           __result; }))
>
> ... this one expands into an expression.  It uses GCC's "a compound
> statement enclosed in parentheses may appear as an expression" extension.
>
> >> >
> >> > The difference isn't just "type casting", it's also statement
> >> > vs. expression.
> >> >
> >> > Is it a good idea to have the macro expand into a statement on some
> >> > hosts, and into an expression on others?  Sure, CI should catch any
> uses
> >> > as expression, but delaying compile errors to CI wastes developer
> time.
> >>
> >> For consistency and simplicity, I would define exactly one version (no
> >> ifdefs) of the macro with a different macro name than glibc's
> >> TEMP_FAILURE_RETRY(), and use that QEMU specific macro name in QEMU code
> >> everywhere.
>
> TFR()?  Can't resist closing the circle...
>
> >> As for statement vs. expression: The only advantage of the statement
> version
> >> is if you'd need __result as an rvalue, which is not needed ATM, right?
> So
> >> I would go for the expression version (with cast) for now.
>
> The expression-like macro is nicer where the return value matters.
> Example (stolen from "The GNU C Library Reference Manual"):
>
>     nbytes = TEMP_FAILURE_RETRY (write (desc, buffer, count));
>
> With the statement-like macro, you have to write
>
>     TEMP_FAILURE_RETRY (nbytes = write (desc, buffer, count));
>
> >> The glibc history does not reveal why they chose the statement version.
>
> The expression version, actually.
>
> >> Best regards,
> >> Christian Schoenebeck
> >
> > Sorry: s/rvalue/lvalue/ i.e. if you need the memory address of result or
> if
> > you need to take the result value of the last iteration in 'expression'
> into
> > account.
>
>

-- 
Best Regards,
*Nikita Ivanov* | C developer
*Telephone:* +79140870696
*Telephone:* +79015053149

[-- Attachment #2: Type: text/html, Size: 22167 bytes --]

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

[Nikita Ivanov]

[-- Attachment #1: Type: text/plain, Size: 36587 bytes --]

And summing up the discussion about TEMP_FAILURE_RETRY() usage examples,
I've come up with a new patch for TFR() to TEMP_FAILURE_RETRY()
refactoring. I've decided to stick to expression realisation.

From 94217dfacf12b3211cfab6e19d750e57d679e851 Mon Sep 17 00:00:00 2001
From: Nikita Ivanov <nivanov@cloudlinux.com>
Date: Mon, 8 Aug 2022 20:43:45 +0300
Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()

glibc's unistd.h header provides the same macro with the
subtle difference in type casting. Adjust macro name to the
common standard and define conditionally.

Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
---
 block/file-posix.c        | 14 +++++++-------
 chardev/char-fd.c         |  2 +-
 chardev/char-pipe.c       | 12 +++++++++---
 chardev/char-pty.c        |  2 +-
 hw/9pfs/9p-local.c        |  6 ++++--
 include/qemu/osdep.h      | 11 +++++++++--
 net/l2tpv3.c              |  8 +++++---
 net/socket.c              |  4 ++--
 net/tap-bsd.c             |  6 +++---
 net/tap-linux.c           |  2 +-
 net/tap-solaris.c         |  8 ++++----
 net/tap.c                 | 10 ++++++----
 os-posix.c                |  2 +-
 qga/commands-posix.c      |  2 +-
 semihosting/syscalls.c    |  2 +-
 tests/qtest/libqtest.c    | 10 +++++-----
 tests/vhost-user-bridge.c |  4 +++-
 util/main-loop.c          |  2 +-
 util/osdep.c              |  2 +-
 util/vfio-helpers.c       |  8 ++++----
 20 files changed, 69 insertions(+), 48 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 0786cb76f9..28ce649ab9 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1238,8 +1238,8 @@ static int hdev_get_max_segments(int fd, struct stat
*st)
         ret = -errno;
         goto out;
     }
-    TFR(
-        ret = read(sysfd, buf, sizeof(buf) - 1)
+    ret = TEMP_FAILURE_RETRY(
+        read(sysfd, buf, sizeof(buf) - 1)
     );
     if (ret < 0) {
         ret = -errno;
@@ -1388,8 +1388,8 @@ static int handle_aiocb_ioctl(void *opaque)
     RawPosixAIOData *aiocb = opaque;
     int ret;

-    TFR(
-        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
+    ret = TEMP_FAILURE_RETRY(
+        ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
     );
     if (ret == -1) {
         return -errno;
@@ -1472,8 +1472,8 @@ static ssize_t handle_aiocb_rw_vector(RawPosixAIOData
*aiocb)
 {
     ssize_t len;

-    TFR(
-        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
+    len = TEMP_FAILURE_RETRY(
+        (aiocb->aio_type & QEMU_AIO_WRITE) ?
             qemu_pwritev(aiocb->aio_fildes,
                            aiocb->io.iov,
                            aiocb->io.niov,
@@ -1907,7 +1907,7 @@ static int allocate_first_block(int fd, size_t
max_size)
     buf = qemu_memalign(max_align, write_size);
     memset(buf, 0, write_size);

-    TFR(n = pwrite(fd, buf, write_size, 0));
+    n = TEMP_FAILURE_RETRY(pwrite(fd, buf, write_size, 0));

     ret = (n == -1) ? -errno : 0;

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index cf78454841..1250a63236 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -198,7 +198,7 @@ int qmp_chardev_open_file_source(char *src, int flags,
Error **errp)
 {
     int fd = -1;

-    TFR(fd = qemu_open_old(src, flags, 0666));
+    fd = TEMP_FAILURE_RETRY(qemu_open_old(src, flags, 0666));
     if (fd == -1) {
         error_setg_file_open(errp, errno, src);
     }
diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index 66d3b85091..a59a055c87 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -131,8 +131,12 @@ static void qemu_chr_open_pipe(Chardev *chr,

     filename_in = g_strdup_printf("%s.in", filename);
     filename_out = g_strdup_printf("%s.out", filename);
-    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
-    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
+    fd_in = TEMP_FAILURE_RETRY(
+        qemu_open_old(filename_in, O_RDWR | O_BINARY)
+    );
+    fd_out = TEMP_FAILURE_RETRY(
+        qemu_open_old(filename_out, O_RDWR | O_BINARY)
+    );
     g_free(filename_in);
     g_free(filename_out);
     if (fd_in < 0 || fd_out < 0) {
@@ -142,7 +146,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
         if (fd_out >= 0) {
             close(fd_out);
         }
-        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
+        fd_in = fd_out = TEMP_FAILURE_RETRY(
+            qemu_open_old(filename, O_RDWR | O_BINARY)
+        );
         if (fd_in < 0) {
             error_setg_file_open(errp, errno, filename);
             return;
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index b2f490bacf..913a98250b 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -93,7 +93,7 @@ static void pty_chr_update_read_handler(Chardev *chr)
     pfd.fd = fioc->fd;
     pfd.events = G_IO_OUT;
     pfd.revents = 0;
-    TFR(rc = g_poll(&pfd, 1, 0));
+    rc = TEMP_FAILURE_RETRY(g_poll(&pfd, 1, 0));
     assert(rc >= 0);

     if (pfd.revents & G_IO_HUP) {
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index c90ab947ba..6a3b4fac4a 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -470,7 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
V9fsPath *fs_path,
         if (fd == -1) {
             return -1;
         }
-        TFR(tsize = read(fd, (void *)buf, bufsz));
+        tsize = TEMP_FAILURE_RETRY(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)) {
@@ -906,7 +906,9 @@ static int local_symlink(FsContext *fs_ctx, const char
*oldpath,
         }
         /* Write the oldpath (target) to the file. */
         oldpath_size = strlen(oldpath);
-        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
+        write_size = TEMP_FAILURE_RETRY(
+            write(fd, (void *)oldpath, oldpath_size)
+        );
         close_preserve_errno(fd);

         if (write_size != oldpath_size) {
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index b1c161c035..6cd0de8e38 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -242,8 +242,15 @@ void QEMU_ERROR("code path is reachable")
 #if !defined(ESHUTDOWN)
 #define ESHUTDOWN 4099
 #endif
-
-#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
+#if !defined(TEMP_FAILURE_RETRY)
+#define TEMP_FAILURE_RETRY(expr) \
+    (__extension__                                          \
+        ({ long int __result;                               \
+           do {                                             \
+                __result = (long int) (expression);         \
+           } while (__result == -1L && errno == EINTR);     \
+           __result; }))
+#endif

 /* time_t may be either 32 or 64 bits depending on the host OS, and
  * can be either signed or unsigned, so we can't just hardcode a
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index adfdbdb84c..cbde9332fe 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -240,7 +240,7 @@ static ssize_t
net_l2tpv3_receive_dgram_iov(NetClientState *nc,
     message.msg_control = NULL;
     message.msg_controllen = 0;
     message.msg_flags = 0;
-    TFR(ret = sendmsg(s->fd, &message, 0));
+    ret = TEMP_FAILURE_RETRY(sendmsg(s->fd, &message, 0));
     if (ret > 0) {
         ret -= s->offset;
     } else if (ret == 0) {
@@ -283,7 +283,7 @@ static ssize_t net_l2tpv3_receive_dgram(NetClientState
*nc,
     message.msg_control = NULL;
     message.msg_controllen = 0;
     message.msg_flags = 0;
-    TFR(ret = sendmsg(s->fd, &message, 0));
+    ret = TEMP_FAILURE_RETRY(sendmsg(s->fd, &message, 0));
     if (ret > 0) {
         ret -= s->offset;
     } else if (ret == 0) {
@@ -430,7 +430,9 @@ static void net_l2tpv3_send(void *opaque)

     msgvec = s->msgvec + s->queue_head;
     if (target_count > 0) {
-        TFR(count = recvmmsg(s->fd, msgvec, target_count, MSG_DONTWAIT,
NULL));
+        count = TEMP_FAILURE_RETRY(
+            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/socket.c b/net/socket.c
index a3397660c2..cfb893b183 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -117,8 +117,8 @@ static ssize_t net_socket_receive_dgram(NetClientState
*nc, const uint8_t *buf,
     NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
     ssize_t ret;

-    TFR(
-        ret = s->dgram_dst.sin_family != AF_UNIX ?
+    ret = TEMP_FAILURE_RETRY(
+        s->dgram_dst.sin_family != AF_UNIX ?
             sendto(s->fd, buf, size, 0,
                      (struct sockaddr *)&s->dgram_dst,
                      sizeof(s->dgram_dst)) :
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 005ce05c6e..720090244e 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -56,7 +56,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         } else {
             snprintf(dname, sizeof dname, "/dev/tap%d", i);
         }
-        TFR(fd = open(dname, O_RDWR));
+        fd = TEMP_FAILURE_RETRY(open(dname, O_RDWR));
         if (fd >= 0) {
             break;
         }
@@ -111,7 +111,7 @@ static int tap_open_clone(char *ifname, int
ifname_size, Error **errp)
     int fd, s, ret;
     struct ifreq ifr;

-    TFR(fd = open(PATH_NET_TAP, O_RDWR));
+    fd = TEMP_FAILURE_RETRY(open(PATH_NET_TAP, O_RDWR));
     if (fd < 0) {
         error_setg_errno(errp, errno, "could not open %s", PATH_NET_TAP);
         return -1;
@@ -159,7 +159,7 @@ int tap_open(char *ifname, int ifname_size, int
*vnet_hdr,
     if (ifname[0] != '\0') {
         char dname[100];
         snprintf(dname, sizeof dname, "/dev/%s", ifname);
-        TFR(fd = open(dname, O_RDWR));
+        fd = TEMP_FAILURE_RETRY(open(dname, O_RDWR));
         if (fd < 0 && errno != ENOENT) {
             error_setg_errno(errp, errno, "could not open %s", dname);
             return -1;
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 304ff45071..bf3ce44329 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -45,7 +45,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     int len = sizeof(struct virtio_net_hdr);
     unsigned int features;

-    TFR(fd = open(PATH_NET_TUN, O_RDWR));
+    fd = TEMP_FAILURE_RETRY(open(PATH_NET_TUN, O_RDWR));
     if (fd < 0) {
         error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
         return -1;
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index a44f8805c2..73e0e9f196 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -84,13 +84,13 @@ static int tap_alloc(char *dev, size_t dev_size, Error
**errp)
     if( ip_fd )
        close(ip_fd);

-    TFR(ip_fd = open("/dev/udp", O_RDWR, 0));
+    ip_fd = TEMP_FAILURE_RETRY(open("/dev/udp", O_RDWR, 0));
     if (ip_fd < 0) {
         error_setg(errp, "Can't open /dev/ip (actually /dev/udp)");
         return -1;
     }

-    TFR(tap_fd = open("/dev/tap", O_RDWR, 0));
+    tap_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
     if (tap_fd < 0) {
         error_setg(errp, "Can't open /dev/tap");
         return -1;
@@ -104,7 +104,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error
**errp)
     if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
         error_report("Can't assign new interface");

-    TFR(if_fd = open("/dev/tap", O_RDWR, 0));
+    if_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
     if (if_fd < 0) {
         error_setg(errp, "Can't open /dev/tap (2)");
         return -1;
@@ -137,7 +137,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error
**errp)
     if (ioctl (ip_fd, I_PUSH, "arp") < 0)
         error_report("Can't push ARP module (3)");
     /* Open arp_fd */
-    TFR(arp_fd = open ("/dev/tap", O_RDWR, 0));
+    arp_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
     if (arp_fd < 0)
         error_report("Can't open %s", "/dev/tap");

diff --git a/net/tap.c b/net/tap.c
index b047eca8b5..dfd220a184 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -102,7 +102,7 @@ static ssize_t tap_write_packet(TAPState *s, const
struct iovec *iov, int iovcnt
 {
     ssize_t len;

-    TFR(len = writev(s->fd, iov, iovcnt));
+    len = TEMP_FAILURE_RETRY(writev(s->fd, iov, iovcnt));

     if (len == -1 && errno == EAGAIN) {
         tap_write_poll(s, true);
@@ -575,7 +575,7 @@ static int net_bridge_run_helper(const char *helper,
const char *bridge,

         close(sv[1]);

-        TFR(fd = recv_fd(sv[0]));
+        fd = TEMP_FAILURE_RETRY(recv_fd(sv[0]));
         saved_errno = errno;

         close(sv[0]);
@@ -647,8 +647,10 @@ static int net_tap_init(const NetdevTapOptions *tap,
int *vnet_hdr,
         vnet_hdr_required = 0;
     }

-    TFR(fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
-                      mq_required, errp));
+    fd = TEMP_FAILURE_RETRY(
+        tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
+                      mq_required, errp)
+    );
     if (fd < 0) {
         return -1;
     }
diff --git a/os-posix.c b/os-posix.c
index 321fc4bd13..59cac65585 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -266,7 +266,7 @@ void os_setup_post(void)
             error_report("not able to chdir to /: %s", strerror(errno));
             exit(1);
         }
-        TFR(fd = qemu_open_old("/dev/null", O_RDWR));
+        fd = TEMP_FAILURE_RETRY(qemu_open_old("/dev/null", O_RDWR));
         if (fd == -1) {
             exit(1);
         }
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 90f83aa9b6..feb0fab807 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -68,7 +68,7 @@ static void ga_wait_child(pid_t pid, int *status, Error
**errp)

     *status = 0;

-    TFR(rpid = waitpid(pid, status, 0));
+    rpid = TEMP_FAILURE_RETRY(waitpid(pid, status, 0));

     if (rpid == -1) {
         error_setg_errno(errp, errno, "failed to wait for child (pid: %d)",
diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
index 025c08147a..0cbef7d22b 100644
--- a/semihosting/syscalls.c
+++ b/semihosting/syscalls.c
@@ -317,7 +317,7 @@ static void host_read(CPUState *cs,
gdb_syscall_complete_cb complete,
         complete(cs, -1, EFAULT);
         return;
     }
-    TFR(ret = read(gf->hostfd, ptr, len));
+    ret = TEMP_FAILURE_RETRY(read(gf->hostfd, ptr, len));
     if (ret == -1) {
         complete(cs, -1, errno);
         unlock_user(ptr, buf, 0);
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index d4022966b3..4a12824dc4 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -102,8 +102,8 @@ static int socket_accept(int sock)
     }

     addrlen = sizeof(addr);
-    TFR(
-        ret = accept(sock, (struct sockaddr *)&addr, &addrlen)
+    ret = TEMP_FAILURE_RETRY(
+        accept(sock, (struct sockaddr *)&addr, &addrlen)
     );
     if (ret == -1) {
         fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
@@ -140,7 +140,7 @@ void qtest_kill_qemu(QTestState *s)
     /* Skip wait if qtest_probe_child already reaped.  */
     if (pid != -1) {
         kill(pid, SIGTERM);
-        TFR(pid = waitpid(s->qemu_pid, &s->wstatus, 0));
+        pid = TEMP_FAILURE_RETRY(waitpid(s->qemu_pid, &s->wstatus, 0));
         assert(pid == s->qemu_pid);
         s->qemu_pid = -1;
     }
@@ -578,8 +578,8 @@ int qtest_socket_server(const char *socket_path)
     addr.sun_family = AF_UNIX;
     snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);

-    TFR(
-        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr))
+    ret = TEMP_FAILURE_RETRY(
+        bind(sock, (struct sockaddr *)&addr, sizeof(addr))
     );
     g_assert_cmpint(ret, !=, -1);
     ret = listen(sock, 1);
diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index 4d8f933ed5..624a211f3d 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -331,7 +331,9 @@ vubr_backend_recv_cb(int sock, void *ctx)
             .msg_iovlen = num,
             .msg_flags = MSG_DONTWAIT,
         };
-        TFR(ret = recvmsg(vubr->backend_udp_sock, &msg, 0));
+        ret = TEMP_FAILURE_RETRY(
+                recvmsg(vubr->backend_udp_sock, &msg, 0)
+        );

         if (i == 0) {
             iov_restore_front(elem->in_sg, sg, hdrlen);
diff --git a/util/main-loop.c b/util/main-loop.c
index 58e14db2d4..5f65f928bd 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -64,7 +64,7 @@ static void sigfd_handler(void *opaque)
     ssize_t len;

     while (1) {
-        TFR(len = read(fd, &info, sizeof(info)));
+        len = TEMP_FAILURE_RETRY(read(fd, &info, sizeof(info)));

         if (len == -1 && errno == EAGAIN) {
             break;
diff --git a/util/osdep.c b/util/osdep.c
index d35c473ac7..2c287e75e7 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -244,7 +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();
-    TFR(ret = fcntl(fd, fcntl_op_setlk, &fl));
+    ret = TEMP_FAILURE_RETRY(fcntl(fd, fcntl_op_setlk, &fl));
     return ret == -1 ? -errno : 0;
 }

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index c878be1c5f..e1422666eb 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -240,8 +240,8 @@ static int qemu_vfio_pci_read_config(QEMUVFIOState *s,
void *buf,
                                     s->config_region_info.offset,
                                     s->config_region_info.size);
     assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
-    TFR(
-        ret = pread(s->device, buf, size, s->config_region_info.offset +
ofs)
+    ret = TEMP_FAILURE_RETRY(
+        pread(s->device, buf, size, s->config_region_info.offset + ofs)
     );
     return ret == size ? 0 : -errno;
 }
@@ -254,8 +254,8 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s,
void *buf, int size, int
                                      s->config_region_info.offset,
                                      s->config_region_info.size);
     assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
-    TFR(
-        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
ofs)
+    ret = TEMP_FAILURE_RETRY(
+        pwrite(s->device, buf, size, s->config_region_info.offset + ofs)
     );
     return ret == size ? 0 : -errno;
 }
-- 
2.37.1


On Mon, Aug 8, 2022 at 9:00 PM Nikita Ivanov <nivanov@cloudlinux.com> wrote:

> Hi!
> During our discussion, I found that I've missed a couple of places where
> TFR() macro could be applied. Here is an updated first patch:
>
> From 8a68f50aac4a8549f416b9350cf339cf0501a712 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>
> ---
>  block/file-posix.c        | 39 ++++++++++++++++++---------------------
>  chardev/char-pty.c        |  4 +---
>  hw/9pfs/9p-local.c        |  8 ++------
>  net/l2tpv3.c              | 15 +++------------
>  net/socket.c              | 16 +++++++---------
>  net/tap.c                 |  8 ++------
>  qga/commands-posix.c      |  4 +---
>  semihosting/syscalls.c    |  4 +---
>  tests/qtest/libqtest.c    | 14 +++++++-------
>  tests/vhost-user-bridge.c |  4 +---
>  util/main-loop.c          |  4 +---
>  util/osdep.c              |  4 +---
>  util/vfio-helpers.c       | 12 ++++++------
>  13 files changed, 51 insertions(+), 85 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 48cd096624..0786cb76f9 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1238,9 +1238,9 @@ static int hdev_get_max_segments(int fd, struct stat
> *st)
>          ret = -errno;
>          goto out;
>      }
> -    do {
> -        ret = read(sysfd, buf, sizeof(buf) - 1);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = read(sysfd, buf, sizeof(buf) - 1)
> +    );
>      if (ret < 0) {
>          ret = -errno;
>          goto out;
> @@ -1388,9 +1388,9 @@ static int handle_aiocb_ioctl(void *opaque)
>      RawPosixAIOData *aiocb = opaque;
>      int ret;
>
> -    do {
> -        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd,
> aiocb->ioctl.buf);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
> +    );
>      if (ret == -1) {
>          return -errno;
>      }
> @@ -1472,18 +1472,17 @@ static ssize_t
> handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
>  {
>      ssize_t len;
>
> -    do {
> -        if (aiocb->aio_type & QEMU_AIO_WRITE)
> -            len = qemu_pwritev(aiocb->aio_fildes,
> -                               aiocb->io.iov,
> -                               aiocb->io.niov,
> -                               aiocb->aio_offset);
> -         else
> -            len = qemu_preadv(aiocb->aio_fildes,
> -                              aiocb->io.iov,
> -                              aiocb->io.niov,
> -                              aiocb->aio_offset);
> -    } while (len == -1 && errno == EINTR);
> +    TFR(
> +        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
> +            qemu_pwritev(aiocb->aio_fildes,
> +                           aiocb->io.iov,
> +                           aiocb->io.niov,
> +                           aiocb->aio_offset) :
> +            qemu_preadv(aiocb->aio_fildes,
> +                          aiocb->io.iov,
> +                          aiocb->io.niov,
> +                          aiocb->aio_offset)
> +    );
>
>      if (len == -1) {
>          return -errno;
> @@ -1908,9 +1907,7 @@ static int allocate_first_block(int fd, size_t
> max_size)
>      buf = qemu_memalign(max_align, write_size);
>      memset(buf, 0, write_size);
>
> -    do {
> -        n = pwrite(fd, buf, write_size, 0);
> -    } while (n == -1 && errno == EINTR);
> +    TFR(n = pwrite(fd, buf, write_size, 0));
>
>      ret = (n == -1) ? -errno : 0;
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index 53f25c6bbd..b2f490bacf 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -93,9 +93,7 @@ static void pty_chr_update_read_handler(Chardev *chr)
>      pfd.fd = fioc->fd;
>      pfd.events = G_IO_OUT;
>      pfd.revents = 0;
> -    do {
> -        rc = g_poll(&pfd, 1, 0);
> -    } while (rc == -1 && errno == EINTR);
> +    TFR(rc = g_poll(&pfd, 1, 0));
>      assert(rc >= 0);
>
>      if (pfd.revents & G_IO_HUP) {
> 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/socket.c b/net/socket.c
> index bfd8596250..a3397660c2 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -117,15 +117,13 @@ static ssize_t
> net_socket_receive_dgram(NetClientState *nc, const uint8_t *buf,
>      NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
>      ssize_t ret;
>
> -    do {
> -        if (s->dgram_dst.sin_family != AF_UNIX) {
> -            ret = sendto(s->fd, buf, size, 0,
> -                         (struct sockaddr *)&s->dgram_dst,
> -                         sizeof(s->dgram_dst));
> -        } else {
> -            ret = send(s->fd, buf, size, 0);
> -        }
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = s->dgram_dst.sin_family != AF_UNIX ?
> +            sendto(s->fd, buf, size, 0,
> +                     (struct sockaddr *)&s->dgram_dst,
> +                     sizeof(s->dgram_dst)) :
> +            send(s->fd, buf, size, 0)
> +    );
>
>      if (ret == -1 && errno == EAGAIN) {
>          net_socket_write_poll(s, true);
> 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/semihosting/syscalls.c b/semihosting/syscalls.c
> index 508a0ad88c..025c08147a 100644
> --- a/semihosting/syscalls.c
> +++ b/semihosting/syscalls.c
> @@ -317,9 +317,7 @@ static void host_read(CPUState *cs,
> gdb_syscall_complete_cb complete,
>          complete(cs, -1, EFAULT);
>          return;
>      }
> -    do {
> -        ret = read(gf->hostfd, ptr, len);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(ret = read(gf->hostfd, ptr, len));
>      if (ret == -1) {
>          complete(cs, -1, errno);
>          unlock_user(ptr, buf, 0);
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 8c159eacf5..d4022966b3 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -101,10 +101,10 @@ static int socket_accept(int sock)
>          return -1;
>      }
>
> -    do {
> -        addrlen = sizeof(addr);
> -        ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
> -    } while (ret == -1 && errno == EINTR);
> +    addrlen = sizeof(addr);
> +    TFR(
> +        ret = accept(sock, (struct sockaddr *)&addr, &addrlen)
> +    );
>      if (ret == -1) {
>          fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
>      }
> @@ -578,9 +578,9 @@ int qtest_socket_server(const char *socket_path)
>      addr.sun_family = AF_UNIX;
>      snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);
>
> -    do {
> -        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr))
> +    );
>      g_assert_cmpint(ret, !=, -1);
>      ret = listen(sock, 1);
>      g_assert_cmpint(ret, !=, -1);
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 9b1dab2f28..4d8f933ed5 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -331,9 +331,7 @@ vubr_backend_recv_cb(int sock, void *ctx)
>              .msg_iovlen = num,
>              .msg_flags = MSG_DONTWAIT,
>          };
> -        do {
> -            ret = recvmsg(vubr->backend_udp_sock, &msg, 0);
> -        } while (ret == -1 && (errno == EINTR));
> +        TFR(ret = recvmsg(vubr->backend_udp_sock, &msg, 0));
>
>          if (i == 0) {
>              iov_restore_front(elem->in_sg, sg, hdrlen);
> 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;
>  }
>
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 5ba01177bf..c878be1c5f 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -240,9 +240,9 @@ static int qemu_vfio_pci_read_config(QEMUVFIOState *s,
> void *buf,
>                                      s->config_region_info.offset,
>                                      s->config_region_info.size);
>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
> -    do {
> -        ret = pread(s->device, buf, size, s->config_region_info.offset +
> ofs);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = pread(s->device, buf, size, s->config_region_info.offset +
> ofs)
> +    );
>      return ret == size ? 0 : -errno;
>  }
>
> @@ -254,9 +254,9 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState
> *s, void *buf, int size, int
>                                       s->config_region_info.offset,
>                                       s->config_region_info.size);
>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
> -    do {
> -        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
> ofs);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
> ofs)
> +    );
>      return ret == size ? 0 : -errno;
>  }
>
> --
> 2.37.1
>
>
> On Mon, Aug 8, 2022 at 5:22 PM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
>>
>> > On Montag, 8. August 2022 14:52:28 CEST Christian Schoenebeck wrote:
>> >> On Montag, 8. August 2022 10:05:56 CEST Markus Armbruster wrote:
>> >> > Nikita Ivanov <nivanov@cloudlinux.com> writes:
>> >> > > Summing up the discussion above, I suggest the following patch for
>> TFR()
>> >> > > macro refactoring. (The patch is sequential to the first one I
>> >> > > introduced
>> >> > > in the start of the discussion).
>> >> > >
>> >> > >>From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00
>> 2001
>> >> > >>
>> >> > > From: Nikita Ivanov <nivanov@cloudlinux.com>
>> >> > > Date: Mon, 8 Aug 2022 09:30:34 +0300
>> >> > > Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
>> >> > >
>> >> > > glibc's unistd.h header provides the same macro with the
>> >> > > subtle difference in type casting. Adjust macro name to the
>> >> > > common standard and define conditionally.
>> >> > >
>> >> > > Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
>>
>> [...]
>>
>> >> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> >> > > index b1c161c035..55f2927d8b 100644
>> >> > > --- a/include/qemu/osdep.h
>> >> > > +++ b/include/qemu/osdep.h
>> >> > > @@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
>> >> > >
>> >> > >  #if !defined(ESHUTDOWN)
>> >> > >  #define ESHUTDOWN 4099
>> >> > >  #endif
>> >> > >
>> >> > > -
>> >> > > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno ==
>> >> > > EINTR)
>> >> > > +#if !defined(TEMP_FAILURE_RETRY)
>> >> > > +#define TEMP_FAILURE_RETRY(expr) \
>> >> > > +    do { if ((expr) != -1) break; } while (errno == EINTR)
>>
>> To avoid / reduce confusion: this macro expands into a statement, and ...
>>
>> >> > > +#endif
>> >> >
>> >> > GLibc's version is
>> >> >
>> >> >    # define TEMP_FAILURE_RETRY(expression) \
>> >> >      (__extension__
>>             \
>> >> >        ({ long int __result;
>>              \
>> >> >           do __result = (long int) (expression);
>>              \
>> >> >           while (__result == -1L && errno == EINTR);
>>              \
>> >> >           __result; }))
>>
>> ... this one expands into an expression.  It uses GCC's "a compound
>> statement enclosed in parentheses may appear as an expression" extension.
>>
>> >> >
>> >> > The difference isn't just "type casting", it's also statement
>> >> > vs. expression.
>> >> >
>> >> > Is it a good idea to have the macro expand into a statement on some
>> >> > hosts, and into an expression on others?  Sure, CI should catch any
>> uses
>> >> > as expression, but delaying compile errors to CI wastes developer
>> time.
>> >>
>> >> For consistency and simplicity, I would define exactly one version (no
>> >> ifdefs) of the macro with a different macro name than glibc's
>> >> TEMP_FAILURE_RETRY(), and use that QEMU specific macro name in QEMU
>> code
>> >> everywhere.
>>
>> TFR()?  Can't resist closing the circle...
>>
>> >> As for statement vs. expression: The only advantage of the statement
>> version
>> >> is if you'd need __result as an rvalue, which is not needed ATM,
>> right? So
>> >> I would go for the expression version (with cast) for now.
>>
>> The expression-like macro is nicer where the return value matters.
>> Example (stolen from "The GNU C Library Reference Manual"):
>>
>>     nbytes = TEMP_FAILURE_RETRY (write (desc, buffer, count));
>>
>> With the statement-like macro, you have to write
>>
>>     TEMP_FAILURE_RETRY (nbytes = write (desc, buffer, count));
>>
>> >> The glibc history does not reveal why they chose the statement version.
>>
>> The expression version, actually.
>>
>> >> Best regards,
>> >> Christian Schoenebeck
>> >
>> > Sorry: s/rvalue/lvalue/ i.e. if you need the memory address of result
>> or if
>> > you need to take the result value of the last iteration in 'expression'
>> into
>> > account.
>>
>>
>
> --
> Best Regards,
> *Nikita Ivanov* | C developer
> *Telephone:* +79140870696
> *Telephone:* +79015053149
>


-- 
Best Regards,
*Nikita Ivanov* | C developer
*Telephone:* +79140870696
*Telephone:* +79015053149

[-- Attachment #2: Type: text/html, Size: 45004 bytes --]

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

[Nikita Ivanov]

[-- Attachment #1: Type: text/plain, Size: 38401 bytes --]

Hi! Are there any updates? I have not received any comments since the last
email.

On Mon, Aug 8, 2022 at 9:03 PM Nikita Ivanov <nivanov@cloudlinux.com> wrote:

> And summing up the discussion about TEMP_FAILURE_RETRY() usage examples,
> I've come up with a new patch for TFR() to TEMP_FAILURE_RETRY()
> refactoring. I've decided to stick to expression realisation.
>
> From 94217dfacf12b3211cfab6e19d750e57d679e851 Mon Sep 17 00:00:00 2001
> From: Nikita Ivanov <nivanov@cloudlinux.com>
> Date: Mon, 8 Aug 2022 20:43:45 +0300
> Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
>
> glibc's unistd.h header provides the same macro with the
> subtle difference in type casting. Adjust macro name to the
> common standard and define conditionally.
>
> Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
> ---
>  block/file-posix.c        | 14 +++++++-------
>  chardev/char-fd.c         |  2 +-
>  chardev/char-pipe.c       | 12 +++++++++---
>  chardev/char-pty.c        |  2 +-
>  hw/9pfs/9p-local.c        |  6 ++++--
>  include/qemu/osdep.h      | 11 +++++++++--
>  net/l2tpv3.c              |  8 +++++---
>  net/socket.c              |  4 ++--
>  net/tap-bsd.c             |  6 +++---
>  net/tap-linux.c           |  2 +-
>  net/tap-solaris.c         |  8 ++++----
>  net/tap.c                 | 10 ++++++----
>  os-posix.c                |  2 +-
>  qga/commands-posix.c      |  2 +-
>  semihosting/syscalls.c    |  2 +-
>  tests/qtest/libqtest.c    | 10 +++++-----
>  tests/vhost-user-bridge.c |  4 +++-
>  util/main-loop.c          |  2 +-
>  util/osdep.c              |  2 +-
>  util/vfio-helpers.c       |  8 ++++----
>  20 files changed, 69 insertions(+), 48 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 0786cb76f9..28ce649ab9 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1238,8 +1238,8 @@ static int hdev_get_max_segments(int fd, struct stat
> *st)
>          ret = -errno;
>          goto out;
>      }
> -    TFR(
> -        ret = read(sysfd, buf, sizeof(buf) - 1)
> +    ret = TEMP_FAILURE_RETRY(
> +        read(sysfd, buf, sizeof(buf) - 1)
>      );
>      if (ret < 0) {
>          ret = -errno;
> @@ -1388,8 +1388,8 @@ static int handle_aiocb_ioctl(void *opaque)
>      RawPosixAIOData *aiocb = opaque;
>      int ret;
>
> -    TFR(
> -        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
> +    ret = TEMP_FAILURE_RETRY(
> +        ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
>      );
>      if (ret == -1) {
>          return -errno;
> @@ -1472,8 +1472,8 @@ static ssize_t
> handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
>  {
>      ssize_t len;
>
> -    TFR(
> -        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
> +    len = TEMP_FAILURE_RETRY(
> +        (aiocb->aio_type & QEMU_AIO_WRITE) ?
>              qemu_pwritev(aiocb->aio_fildes,
>                             aiocb->io.iov,
>                             aiocb->io.niov,
> @@ -1907,7 +1907,7 @@ static int allocate_first_block(int fd, size_t
> max_size)
>      buf = qemu_memalign(max_align, write_size);
>      memset(buf, 0, write_size);
>
> -    TFR(n = pwrite(fd, buf, write_size, 0));
> +    n = TEMP_FAILURE_RETRY(pwrite(fd, buf, write_size, 0));
>
>      ret = (n == -1) ? -errno : 0;
>
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index cf78454841..1250a63236 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -198,7 +198,7 @@ int qmp_chardev_open_file_source(char *src, int flags,
> Error **errp)
>  {
>      int fd = -1;
>
> -    TFR(fd = qemu_open_old(src, flags, 0666));
> +    fd = TEMP_FAILURE_RETRY(qemu_open_old(src, flags, 0666));
>      if (fd == -1) {
>          error_setg_file_open(errp, errno, src);
>      }
> diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
> index 66d3b85091..a59a055c87 100644
> --- a/chardev/char-pipe.c
> +++ b/chardev/char-pipe.c
> @@ -131,8 +131,12 @@ static void qemu_chr_open_pipe(Chardev *chr,
>
>      filename_in = g_strdup_printf("%s.in", filename);
>      filename_out = g_strdup_printf("%s.out", filename);
> -    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
> -    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
> +    fd_in = TEMP_FAILURE_RETRY(
> +        qemu_open_old(filename_in, O_RDWR | O_BINARY)
> +    );
> +    fd_out = TEMP_FAILURE_RETRY(
> +        qemu_open_old(filename_out, O_RDWR | O_BINARY)
> +    );
>      g_free(filename_in);
>      g_free(filename_out);
>      if (fd_in < 0 || fd_out < 0) {
> @@ -142,7 +146,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
>          if (fd_out >= 0) {
>              close(fd_out);
>          }
> -        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
> +        fd_in = fd_out = TEMP_FAILURE_RETRY(
> +            qemu_open_old(filename, O_RDWR | O_BINARY)
> +        );
>          if (fd_in < 0) {
>              error_setg_file_open(errp, errno, filename);
>              return;
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index b2f490bacf..913a98250b 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -93,7 +93,7 @@ static void pty_chr_update_read_handler(Chardev *chr)
>      pfd.fd = fioc->fd;
>      pfd.events = G_IO_OUT;
>      pfd.revents = 0;
> -    TFR(rc = g_poll(&pfd, 1, 0));
> +    rc = TEMP_FAILURE_RETRY(g_poll(&pfd, 1, 0));
>      assert(rc >= 0);
>
>      if (pfd.revents & G_IO_HUP) {
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index c90ab947ba..6a3b4fac4a 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -470,7 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
> V9fsPath *fs_path,
>          if (fd == -1) {
>              return -1;
>          }
> -        TFR(tsize = read(fd, (void *)buf, bufsz));
> +        tsize = TEMP_FAILURE_RETRY(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)) {
> @@ -906,7 +906,9 @@ static int local_symlink(FsContext *fs_ctx, const char
> *oldpath,
>          }
>          /* Write the oldpath (target) to the file. */
>          oldpath_size = strlen(oldpath);
> -        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
> +        write_size = TEMP_FAILURE_RETRY(
> +            write(fd, (void *)oldpath, oldpath_size)
> +        );
>          close_preserve_errno(fd);
>
>          if (write_size != oldpath_size) {
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index b1c161c035..6cd0de8e38 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -242,8 +242,15 @@ void QEMU_ERROR("code path is reachable")
>  #if !defined(ESHUTDOWN)
>  #define ESHUTDOWN 4099
>  #endif
> -
> -#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
> +#if !defined(TEMP_FAILURE_RETRY)
> +#define TEMP_FAILURE_RETRY(expr) \
> +    (__extension__                                          \
> +        ({ long int __result;                               \
> +           do {                                             \
> +                __result = (long int) (expression);         \
> +           } while (__result == -1L && errno == EINTR);     \
> +           __result; }))
> +#endif
>
>  /* time_t may be either 32 or 64 bits depending on the host OS, and
>   * can be either signed or unsigned, so we can't just hardcode a
> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
> index adfdbdb84c..cbde9332fe 100644
> --- a/net/l2tpv3.c
> +++ b/net/l2tpv3.c
> @@ -240,7 +240,7 @@ static ssize_t
> net_l2tpv3_receive_dgram_iov(NetClientState *nc,
>      message.msg_control = NULL;
>      message.msg_controllen = 0;
>      message.msg_flags = 0;
> -    TFR(ret = sendmsg(s->fd, &message, 0));
> +    ret = TEMP_FAILURE_RETRY(sendmsg(s->fd, &message, 0));
>      if (ret > 0) {
>          ret -= s->offset;
>      } else if (ret == 0) {
> @@ -283,7 +283,7 @@ static ssize_t net_l2tpv3_receive_dgram(NetClientState
> *nc,
>      message.msg_control = NULL;
>      message.msg_controllen = 0;
>      message.msg_flags = 0;
> -    TFR(ret = sendmsg(s->fd, &message, 0));
> +    ret = TEMP_FAILURE_RETRY(sendmsg(s->fd, &message, 0));
>      if (ret > 0) {
>          ret -= s->offset;
>      } else if (ret == 0) {
> @@ -430,7 +430,9 @@ static void net_l2tpv3_send(void *opaque)
>
>      msgvec = s->msgvec + s->queue_head;
>      if (target_count > 0) {
> -        TFR(count = recvmmsg(s->fd, msgvec, target_count, MSG_DONTWAIT,
> NULL));
> +        count = TEMP_FAILURE_RETRY(
> +            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/socket.c b/net/socket.c
> index a3397660c2..cfb893b183 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -117,8 +117,8 @@ static ssize_t net_socket_receive_dgram(NetClientState
> *nc, const uint8_t *buf,
>      NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
>      ssize_t ret;
>
> -    TFR(
> -        ret = s->dgram_dst.sin_family != AF_UNIX ?
> +    ret = TEMP_FAILURE_RETRY(
> +        s->dgram_dst.sin_family != AF_UNIX ?
>              sendto(s->fd, buf, size, 0,
>                       (struct sockaddr *)&s->dgram_dst,
>                       sizeof(s->dgram_dst)) :
> diff --git a/net/tap-bsd.c b/net/tap-bsd.c
> index 005ce05c6e..720090244e 100644
> --- a/net/tap-bsd.c
> +++ b/net/tap-bsd.c
> @@ -56,7 +56,7 @@ int tap_open(char *ifname, int ifname_size, int
> *vnet_hdr,
>          } else {
>              snprintf(dname, sizeof dname, "/dev/tap%d", i);
>          }
> -        TFR(fd = open(dname, O_RDWR));
> +        fd = TEMP_FAILURE_RETRY(open(dname, O_RDWR));
>          if (fd >= 0) {
>              break;
>          }
> @@ -111,7 +111,7 @@ static int tap_open_clone(char *ifname, int
> ifname_size, Error **errp)
>      int fd, s, ret;
>      struct ifreq ifr;
>
> -    TFR(fd = open(PATH_NET_TAP, O_RDWR));
> +    fd = TEMP_FAILURE_RETRY(open(PATH_NET_TAP, O_RDWR));
>      if (fd < 0) {
>          error_setg_errno(errp, errno, "could not open %s", PATH_NET_TAP);
>          return -1;
> @@ -159,7 +159,7 @@ int tap_open(char *ifname, int ifname_size, int
> *vnet_hdr,
>      if (ifname[0] != '\0') {
>          char dname[100];
>          snprintf(dname, sizeof dname, "/dev/%s", ifname);
> -        TFR(fd = open(dname, O_RDWR));
> +        fd = TEMP_FAILURE_RETRY(open(dname, O_RDWR));
>          if (fd < 0 && errno != ENOENT) {
>              error_setg_errno(errp, errno, "could not open %s", dname);
>              return -1;
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index 304ff45071..bf3ce44329 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -45,7 +45,7 @@ int tap_open(char *ifname, int ifname_size, int
> *vnet_hdr,
>      int len = sizeof(struct virtio_net_hdr);
>      unsigned int features;
>
> -    TFR(fd = open(PATH_NET_TUN, O_RDWR));
> +    fd = TEMP_FAILURE_RETRY(open(PATH_NET_TUN, O_RDWR));
>      if (fd < 0) {
>          error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
>          return -1;
> diff --git a/net/tap-solaris.c b/net/tap-solaris.c
> index a44f8805c2..73e0e9f196 100644
> --- a/net/tap-solaris.c
> +++ b/net/tap-solaris.c
> @@ -84,13 +84,13 @@ static int tap_alloc(char *dev, size_t dev_size, Error
> **errp)
>      if( ip_fd )
>         close(ip_fd);
>
> -    TFR(ip_fd = open("/dev/udp", O_RDWR, 0));
> +    ip_fd = TEMP_FAILURE_RETRY(open("/dev/udp", O_RDWR, 0));
>      if (ip_fd < 0) {
>          error_setg(errp, "Can't open /dev/ip (actually /dev/udp)");
>          return -1;
>      }
>
> -    TFR(tap_fd = open("/dev/tap", O_RDWR, 0));
> +    tap_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
>      if (tap_fd < 0) {
>          error_setg(errp, "Can't open /dev/tap");
>          return -1;
> @@ -104,7 +104,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error
> **errp)
>      if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
>          error_report("Can't assign new interface");
>
> -    TFR(if_fd = open("/dev/tap", O_RDWR, 0));
> +    if_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
>      if (if_fd < 0) {
>          error_setg(errp, "Can't open /dev/tap (2)");
>          return -1;
> @@ -137,7 +137,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error
> **errp)
>      if (ioctl (ip_fd, I_PUSH, "arp") < 0)
>          error_report("Can't push ARP module (3)");
>      /* Open arp_fd */
> -    TFR(arp_fd = open ("/dev/tap", O_RDWR, 0));
> +    arp_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
>      if (arp_fd < 0)
>          error_report("Can't open %s", "/dev/tap");
>
> diff --git a/net/tap.c b/net/tap.c
> index b047eca8b5..dfd220a184 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -102,7 +102,7 @@ static ssize_t tap_write_packet(TAPState *s, const
> struct iovec *iov, int iovcnt
>  {
>      ssize_t len;
>
> -    TFR(len = writev(s->fd, iov, iovcnt));
> +    len = TEMP_FAILURE_RETRY(writev(s->fd, iov, iovcnt));
>
>      if (len == -1 && errno == EAGAIN) {
>          tap_write_poll(s, true);
> @@ -575,7 +575,7 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
>
>          close(sv[1]);
>
> -        TFR(fd = recv_fd(sv[0]));
> +        fd = TEMP_FAILURE_RETRY(recv_fd(sv[0]));
>          saved_errno = errno;
>
>          close(sv[0]);
> @@ -647,8 +647,10 @@ static int net_tap_init(const NetdevTapOptions *tap,
> int *vnet_hdr,
>          vnet_hdr_required = 0;
>      }
>
> -    TFR(fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
> -                      mq_required, errp));
> +    fd = TEMP_FAILURE_RETRY(
> +        tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
> +                      mq_required, errp)
> +    );
>      if (fd < 0) {
>          return -1;
>      }
> diff --git a/os-posix.c b/os-posix.c
> index 321fc4bd13..59cac65585 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -266,7 +266,7 @@ void os_setup_post(void)
>              error_report("not able to chdir to /: %s", strerror(errno));
>              exit(1);
>          }
> -        TFR(fd = qemu_open_old("/dev/null", O_RDWR));
> +        fd = TEMP_FAILURE_RETRY(qemu_open_old("/dev/null", O_RDWR));
>          if (fd == -1) {
>              exit(1);
>          }
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 90f83aa9b6..feb0fab807 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -68,7 +68,7 @@ static void ga_wait_child(pid_t pid, int *status, Error
> **errp)
>
>      *status = 0;
>
> -    TFR(rpid = waitpid(pid, status, 0));
> +    rpid = TEMP_FAILURE_RETRY(waitpid(pid, status, 0));
>
>      if (rpid == -1) {
>          error_setg_errno(errp, errno, "failed to wait for child (pid:
> %d)",
> diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
> index 025c08147a..0cbef7d22b 100644
> --- a/semihosting/syscalls.c
> +++ b/semihosting/syscalls.c
> @@ -317,7 +317,7 @@ static void host_read(CPUState *cs,
> gdb_syscall_complete_cb complete,
>          complete(cs, -1, EFAULT);
>          return;
>      }
> -    TFR(ret = read(gf->hostfd, ptr, len));
> +    ret = TEMP_FAILURE_RETRY(read(gf->hostfd, ptr, len));
>      if (ret == -1) {
>          complete(cs, -1, errno);
>          unlock_user(ptr, buf, 0);
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index d4022966b3..4a12824dc4 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -102,8 +102,8 @@ static int socket_accept(int sock)
>      }
>
>      addrlen = sizeof(addr);
> -    TFR(
> -        ret = accept(sock, (struct sockaddr *)&addr, &addrlen)
> +    ret = TEMP_FAILURE_RETRY(
> +        accept(sock, (struct sockaddr *)&addr, &addrlen)
>      );
>      if (ret == -1) {
>          fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
> @@ -140,7 +140,7 @@ void qtest_kill_qemu(QTestState *s)
>      /* Skip wait if qtest_probe_child already reaped.  */
>      if (pid != -1) {
>          kill(pid, SIGTERM);
> -        TFR(pid = waitpid(s->qemu_pid, &s->wstatus, 0));
> +        pid = TEMP_FAILURE_RETRY(waitpid(s->qemu_pid, &s->wstatus, 0));
>          assert(pid == s->qemu_pid);
>          s->qemu_pid = -1;
>      }
> @@ -578,8 +578,8 @@ int qtest_socket_server(const char *socket_path)
>      addr.sun_family = AF_UNIX;
>      snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);
>
> -    TFR(
> -        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr))
> +    ret = TEMP_FAILURE_RETRY(
> +        bind(sock, (struct sockaddr *)&addr, sizeof(addr))
>      );
>      g_assert_cmpint(ret, !=, -1);
>      ret = listen(sock, 1);
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 4d8f933ed5..624a211f3d 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -331,7 +331,9 @@ vubr_backend_recv_cb(int sock, void *ctx)
>              .msg_iovlen = num,
>              .msg_flags = MSG_DONTWAIT,
>          };
> -        TFR(ret = recvmsg(vubr->backend_udp_sock, &msg, 0));
> +        ret = TEMP_FAILURE_RETRY(
> +                recvmsg(vubr->backend_udp_sock, &msg, 0)
> +        );
>
>          if (i == 0) {
>              iov_restore_front(elem->in_sg, sg, hdrlen);
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 58e14db2d4..5f65f928bd 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -64,7 +64,7 @@ static void sigfd_handler(void *opaque)
>      ssize_t len;
>
>      while (1) {
> -        TFR(len = read(fd, &info, sizeof(info)));
> +        len = TEMP_FAILURE_RETRY(read(fd, &info, sizeof(info)));
>
>          if (len == -1 && errno == EAGAIN) {
>              break;
> diff --git a/util/osdep.c b/util/osdep.c
> index d35c473ac7..2c287e75e7 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -244,7 +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();
> -    TFR(ret = fcntl(fd, fcntl_op_setlk, &fl));
> +    ret = TEMP_FAILURE_RETRY(fcntl(fd, fcntl_op_setlk, &fl));
>      return ret == -1 ? -errno : 0;
>  }
>
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index c878be1c5f..e1422666eb 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -240,8 +240,8 @@ static int qemu_vfio_pci_read_config(QEMUVFIOState *s,
> void *buf,
>                                      s->config_region_info.offset,
>                                      s->config_region_info.size);
>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
> -    TFR(
> -        ret = pread(s->device, buf, size, s->config_region_info.offset +
> ofs)
> +    ret = TEMP_FAILURE_RETRY(
> +        pread(s->device, buf, size, s->config_region_info.offset + ofs)
>      );
>      return ret == size ? 0 : -errno;
>  }
> @@ -254,8 +254,8 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState
> *s, void *buf, int size, int
>                                       s->config_region_info.offset,
>                                       s->config_region_info.size);
>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
> -    TFR(
> -        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
> ofs)
> +    ret = TEMP_FAILURE_RETRY(
> +        pwrite(s->device, buf, size, s->config_region_info.offset + ofs)
>      );
>      return ret == size ? 0 : -errno;
>  }
> --
> 2.37.1
>
>
> On Mon, Aug 8, 2022 at 9:00 PM Nikita Ivanov <nivanov@cloudlinux.com>
> wrote:
>
>> Hi!
>> During our discussion, I found that I've missed a couple of places where
>> TFR() macro could be applied. Here is an updated first patch:
>>
>> From 8a68f50aac4a8549f416b9350cf339cf0501a712 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>
>> ---
>>  block/file-posix.c        | 39 ++++++++++++++++++---------------------
>>  chardev/char-pty.c        |  4 +---
>>  hw/9pfs/9p-local.c        |  8 ++------
>>  net/l2tpv3.c              | 15 +++------------
>>  net/socket.c              | 16 +++++++---------
>>  net/tap.c                 |  8 ++------
>>  qga/commands-posix.c      |  4 +---
>>  semihosting/syscalls.c    |  4 +---
>>  tests/qtest/libqtest.c    | 14 +++++++-------
>>  tests/vhost-user-bridge.c |  4 +---
>>  util/main-loop.c          |  4 +---
>>  util/osdep.c              |  4 +---
>>  util/vfio-helpers.c       | 12 ++++++------
>>  13 files changed, 51 insertions(+), 85 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 48cd096624..0786cb76f9 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1238,9 +1238,9 @@ static int hdev_get_max_segments(int fd, struct
>> stat *st)
>>          ret = -errno;
>>          goto out;
>>      }
>> -    do {
>> -        ret = read(sysfd, buf, sizeof(buf) - 1);
>> -    } while (ret == -1 && errno == EINTR);
>> +    TFR(
>> +        ret = read(sysfd, buf, sizeof(buf) - 1)
>> +    );
>>      if (ret < 0) {
>>          ret = -errno;
>>          goto out;
>> @@ -1388,9 +1388,9 @@ static int handle_aiocb_ioctl(void *opaque)
>>      RawPosixAIOData *aiocb = opaque;
>>      int ret;
>>
>> -    do {
>> -        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd,
>> aiocb->ioctl.buf);
>> -    } while (ret == -1 && errno == EINTR);
>> +    TFR(
>> +        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd,
>> aiocb->ioctl.buf)
>> +    );
>>      if (ret == -1) {
>>          return -errno;
>>      }
>> @@ -1472,18 +1472,17 @@ static ssize_t
>> handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
>>  {
>>      ssize_t len;
>>
>> -    do {
>> -        if (aiocb->aio_type & QEMU_AIO_WRITE)
>> -            len = qemu_pwritev(aiocb->aio_fildes,
>> -                               aiocb->io.iov,
>> -                               aiocb->io.niov,
>> -                               aiocb->aio_offset);
>> -         else
>> -            len = qemu_preadv(aiocb->aio_fildes,
>> -                              aiocb->io.iov,
>> -                              aiocb->io.niov,
>> -                              aiocb->aio_offset);
>> -    } while (len == -1 && errno == EINTR);
>> +    TFR(
>> +        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
>> +            qemu_pwritev(aiocb->aio_fildes,
>> +                           aiocb->io.iov,
>> +                           aiocb->io.niov,
>> +                           aiocb->aio_offset) :
>> +            qemu_preadv(aiocb->aio_fildes,
>> +                          aiocb->io.iov,
>> +                          aiocb->io.niov,
>> +                          aiocb->aio_offset)
>> +    );
>>
>>      if (len == -1) {
>>          return -errno;
>> @@ -1908,9 +1907,7 @@ static int allocate_first_block(int fd, size_t
>> max_size)
>>      buf = qemu_memalign(max_align, write_size);
>>      memset(buf, 0, write_size);
>>
>> -    do {
>> -        n = pwrite(fd, buf, write_size, 0);
>> -    } while (n == -1 && errno == EINTR);
>> +    TFR(n = pwrite(fd, buf, write_size, 0));
>>
>>      ret = (n == -1) ? -errno : 0;
>>
>> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
>> index 53f25c6bbd..b2f490bacf 100644
>> --- a/chardev/char-pty.c
>> +++ b/chardev/char-pty.c
>> @@ -93,9 +93,7 @@ static void pty_chr_update_read_handler(Chardev *chr)
>>      pfd.fd = fioc->fd;
>>      pfd.events = G_IO_OUT;
>>      pfd.revents = 0;
>> -    do {
>> -        rc = g_poll(&pfd, 1, 0);
>> -    } while (rc == -1 && errno == EINTR);
>> +    TFR(rc = g_poll(&pfd, 1, 0));
>>      assert(rc >= 0);
>>
>>      if (pfd.revents & G_IO_HUP) {
>> 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/socket.c b/net/socket.c
>> index bfd8596250..a3397660c2 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -117,15 +117,13 @@ static ssize_t
>> net_socket_receive_dgram(NetClientState *nc, const uint8_t *buf,
>>      NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
>>      ssize_t ret;
>>
>> -    do {
>> -        if (s->dgram_dst.sin_family != AF_UNIX) {
>> -            ret = sendto(s->fd, buf, size, 0,
>> -                         (struct sockaddr *)&s->dgram_dst,
>> -                         sizeof(s->dgram_dst));
>> -        } else {
>> -            ret = send(s->fd, buf, size, 0);
>> -        }
>> -    } while (ret == -1 && errno == EINTR);
>> +    TFR(
>> +        ret = s->dgram_dst.sin_family != AF_UNIX ?
>> +            sendto(s->fd, buf, size, 0,
>> +                     (struct sockaddr *)&s->dgram_dst,
>> +                     sizeof(s->dgram_dst)) :
>> +            send(s->fd, buf, size, 0)
>> +    );
>>
>>      if (ret == -1 && errno == EAGAIN) {
>>          net_socket_write_poll(s, true);
>> 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/semihosting/syscalls.c b/semihosting/syscalls.c
>> index 508a0ad88c..025c08147a 100644
>> --- a/semihosting/syscalls.c
>> +++ b/semihosting/syscalls.c
>> @@ -317,9 +317,7 @@ static void host_read(CPUState *cs,
>> gdb_syscall_complete_cb complete,
>>          complete(cs, -1, EFAULT);
>>          return;
>>      }
>> -    do {
>> -        ret = read(gf->hostfd, ptr, len);
>> -    } while (ret == -1 && errno == EINTR);
>> +    TFR(ret = read(gf->hostfd, ptr, len));
>>      if (ret == -1) {
>>          complete(cs, -1, errno);
>>          unlock_user(ptr, buf, 0);
>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>> index 8c159eacf5..d4022966b3 100644
>> --- a/tests/qtest/libqtest.c
>> +++ b/tests/qtest/libqtest.c
>> @@ -101,10 +101,10 @@ static int socket_accept(int sock)
>>          return -1;
>>      }
>>
>> -    do {
>> -        addrlen = sizeof(addr);
>> -        ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
>> -    } while (ret == -1 && errno == EINTR);
>> +    addrlen = sizeof(addr);
>> +    TFR(
>> +        ret = accept(sock, (struct sockaddr *)&addr, &addrlen)
>> +    );
>>      if (ret == -1) {
>>          fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
>>      }
>> @@ -578,9 +578,9 @@ int qtest_socket_server(const char *socket_path)
>>      addr.sun_family = AF_UNIX;
>>      snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);
>>
>> -    do {
>> -        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));
>> -    } while (ret == -1 && errno == EINTR);
>> +    TFR(
>> +        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr))
>> +    );
>>      g_assert_cmpint(ret, !=, -1);
>>      ret = listen(sock, 1);
>>      g_assert_cmpint(ret, !=, -1);
>> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
>> index 9b1dab2f28..4d8f933ed5 100644
>> --- a/tests/vhost-user-bridge.c
>> +++ b/tests/vhost-user-bridge.c
>> @@ -331,9 +331,7 @@ vubr_backend_recv_cb(int sock, void *ctx)
>>              .msg_iovlen = num,
>>              .msg_flags = MSG_DONTWAIT,
>>          };
>> -        do {
>> -            ret = recvmsg(vubr->backend_udp_sock, &msg, 0);
>> -        } while (ret == -1 && (errno == EINTR));
>> +        TFR(ret = recvmsg(vubr->backend_udp_sock, &msg, 0));
>>
>>          if (i == 0) {
>>              iov_restore_front(elem->in_sg, sg, hdrlen);
>> 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;
>>  }
>>
>> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
>> index 5ba01177bf..c878be1c5f 100644
>> --- a/util/vfio-helpers.c
>> +++ b/util/vfio-helpers.c
>> @@ -240,9 +240,9 @@ static int qemu_vfio_pci_read_config(QEMUVFIOState
>> *s, void *buf,
>>                                      s->config_region_info.offset,
>>                                      s->config_region_info.size);
>>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
>> -    do {
>> -        ret = pread(s->device, buf, size, s->config_region_info.offset +
>> ofs);
>> -    } while (ret == -1 && errno == EINTR);
>> +    TFR(
>> +        ret = pread(s->device, buf, size, s->config_region_info.offset +
>> ofs)
>> +    );
>>      return ret == size ? 0 : -errno;
>>  }
>>
>> @@ -254,9 +254,9 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState
>> *s, void *buf, int size, int
>>                                       s->config_region_info.offset,
>>                                       s->config_region_info.size);
>>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
>> -    do {
>> -        ret = pwrite(s->device, buf, size, s->config_region_info.offset
>> + ofs);
>> -    } while (ret == -1 && errno == EINTR);
>> +    TFR(
>> +        ret = pwrite(s->device, buf, size, s->config_region_info.offset
>> + ofs)
>> +    );
>>      return ret == size ? 0 : -errno;
>>  }
>>
>> --
>> 2.37.1
>>
>>
>> On Mon, Aug 8, 2022 at 5:22 PM Markus Armbruster <armbru@redhat.com>
>> wrote:
>>
>>> Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
>>>
>>> > On Montag, 8. August 2022 14:52:28 CEST Christian Schoenebeck wrote:
>>> >> On Montag, 8. August 2022 10:05:56 CEST Markus Armbruster wrote:
>>> >> > Nikita Ivanov <nivanov@cloudlinux.com> writes:
>>> >> > > Summing up the discussion above, I suggest the following patch
>>> for TFR()
>>> >> > > macro refactoring. (The patch is sequential to the first one I
>>> >> > > introduced
>>> >> > > in the start of the discussion).
>>> >> > >
>>> >> > >>From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00
>>> 2001
>>> >> > >>
>>> >> > > From: Nikita Ivanov <nivanov@cloudlinux.com>
>>> >> > > Date: Mon, 8 Aug 2022 09:30:34 +0300
>>> >> > > Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
>>> >> > >
>>> >> > > glibc's unistd.h header provides the same macro with the
>>> >> > > subtle difference in type casting. Adjust macro name to the
>>> >> > > common standard and define conditionally.
>>> >> > >
>>> >> > > Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
>>>
>>> [...]
>>>
>>> >> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>>> >> > > index b1c161c035..55f2927d8b 100644
>>> >> > > --- a/include/qemu/osdep.h
>>> >> > > +++ b/include/qemu/osdep.h
>>> >> > > @@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
>>> >> > >
>>> >> > >  #if !defined(ESHUTDOWN)
>>> >> > >  #define ESHUTDOWN 4099
>>> >> > >  #endif
>>> >> > >
>>> >> > > -
>>> >> > > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno ==
>>> >> > > EINTR)
>>> >> > > +#if !defined(TEMP_FAILURE_RETRY)
>>> >> > > +#define TEMP_FAILURE_RETRY(expr) \
>>> >> > > +    do { if ((expr) != -1) break; } while (errno == EINTR)
>>>
>>> To avoid / reduce confusion: this macro expands into a statement, and ...
>>>
>>> >> > > +#endif
>>> >> >
>>> >> > GLibc's version is
>>> >> >
>>> >> >    # define TEMP_FAILURE_RETRY(expression) \
>>> >> >      (__extension__
>>>               \
>>> >> >        ({ long int __result;
>>>              \
>>> >> >           do __result = (long int) (expression);
>>>              \
>>> >> >           while (__result == -1L && errno == EINTR);
>>>              \
>>> >> >           __result; }))
>>>
>>> ... this one expands into an expression.  It uses GCC's "a compound
>>> statement enclosed in parentheses may appear as an expression" extension.
>>>
>>> >> >
>>> >> > The difference isn't just "type casting", it's also statement
>>> >> > vs. expression.
>>> >> >
>>> >> > Is it a good idea to have the macro expand into a statement on some
>>> >> > hosts, and into an expression on others?  Sure, CI should catch any
>>> uses
>>> >> > as expression, but delaying compile errors to CI wastes developer
>>> time.
>>> >>
>>> >> For consistency and simplicity, I would define exactly one version (no
>>> >> ifdefs) of the macro with a different macro name than glibc's
>>> >> TEMP_FAILURE_RETRY(), and use that QEMU specific macro name in QEMU
>>> code
>>> >> everywhere.
>>>
>>> TFR()?  Can't resist closing the circle...
>>>
>>> >> As for statement vs. expression: The only advantage of the statement
>>> version
>>> >> is if you'd need __result as an rvalue, which is not needed ATM,
>>> right? So
>>> >> I would go for the expression version (with cast) for now.
>>>
>>> The expression-like macro is nicer where the return value matters.
>>> Example (stolen from "The GNU C Library Reference Manual"):
>>>
>>>     nbytes = TEMP_FAILURE_RETRY (write (desc, buffer, count));
>>>
>>> With the statement-like macro, you have to write
>>>
>>>     TEMP_FAILURE_RETRY (nbytes = write (desc, buffer, count));
>>>
>>> >> The glibc history does not reveal why they chose the statement
>>> version.
>>>
>>> The expression version, actually.
>>>
>>> >> Best regards,
>>> >> Christian Schoenebeck
>>> >
>>> > Sorry: s/rvalue/lvalue/ i.e. if you need the memory address of result
>>> or if
>>> > you need to take the result value of the last iteration in
>>> 'expression' into
>>> > account.
>>>
>>>
>>
>> --
>> Best Regards,
>> *Nikita Ivanov* | C developer
>> *Telephone:* +79140870696
>> *Telephone:* +79015053149
>>
>
>
> --
> Best Regards,
> *Nikita Ivanov* | C developer
> *Telephone:* +79140870696
> *Telephone:* +79015053149
>


-- 
Best Regards,
*Nikita Ivanov* | C developer
*Telephone:* +79140870696
*Telephone:* +79015053149

[-- Attachment #2: Type: text/html, Size: 45747 bytes --]

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

[Peter Maydell]

On Wed, 17 Aug 2022 at 15:06, Nikita Ivanov <nivanov@cloudlinux.com> wrote:
>
> Hi! Are there any updates? I have not received any comments since the last email.

Looking at the thread, I don't think we (yet) have consensus on the
right thing to do here...

thanks
-- PMM

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

[Nikita Ivanov]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

Well...

What exactly is still under discussion? In my perspective, the main
pitfalls have been resolved:

0. All possible places where TFR() macro could be applied are covered.
1. Macro has been renamed in order to be more transparent. The name has
been chosen in comparison with a similar glibc macro.
2. The macro itself has been refactored, in order to replace it entirely
with glibc alternative.
3. Problems with statement/expressions differences in qemu and glibc
implementation have been resolved.

Is there any room for improvement?


On Wed, Aug 17, 2022 at 5:17 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Wed, 17 Aug 2022 at 15:06, Nikita Ivanov <nivanov@cloudlinux.com>
> wrote:
> >
> > Hi! Are there any updates? I have not received any comments since the
> last email.
>
> Looking at the thread, I don't think we (yet) have consensus on the
> right thing to do here...
>
> thanks
> -- PMM
>


-- 
Best Regards,
*Nikita Ivanov* | C developer
*Telephone:* +79140870696
*Telephone:* +79015053149

[-- Attachment #2: Type: text/html, Size: 1595 bytes --]

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

[Peter Maydell]

On Wed, 17 Aug 2022 at 15:49, Nikita Ivanov <nivanov@cloudlinux.com> wrote:
>
> Well...
>
> What exactly is still under discussion? In my perspective, the main pitfalls have been resolved:
>
> 0. All possible places where TFR() macro could be applied are covered.
> 1. Macro has been renamed in order to be more transparent. The name has been chosen in comparison with a similar glibc macro.
> 2. The macro itself has been refactored, in order to replace it entirely with glibc alternative.
> 3. Problems with statement/expressions differences in qemu and glibc implementation have been resolved.
>
> Is there any room for improvement?

(a) do we want the statement version or the expression version?
(b) do we want "use the glibc one, with same-semantics version for
compatibility", or do we want "we have our own thing"?

I would have voted for following glibc, except that it does
that cast-to-long thing, which is incorrect behaviour when
long is 32 bits and the return value from the function being
tested is 64 bits.

thanks
-- PMM

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

[Christian Schoenebeck]

On Mittwoch, 17. August 2022 17:55:24 CEST Peter Maydell wrote:
> On Wed, 17 Aug 2022 at 15:49, Nikita Ivanov <nivanov@cloudlinux.com> wrote:
> > Well...
> > 
> > What exactly is still under discussion? In my perspective, the main
> > pitfalls have been resolved:
> > 
> > 0. All possible places where TFR() macro could be applied are covered.
> > 1. Macro has been renamed in order to be more transparent. The name has
> > been chosen in comparison with a similar glibc macro. 2. The macro itself
> > has been refactored, in order to replace it entirely with glibc
> > alternative. 3. Problems with statement/expressions differences in qemu
> > and glibc implementation have been resolved.
> > 
> > Is there any room for improvement?
> 
> (a) do we want the statement version or the expression version?

I think the tendency was in favour for the expression version? Markus made it 
clear that the glibc version indeed may evaluate as an expression (GCC 
extension).

> (b) do we want "use the glibc one, with same-semantics version for
> compatibility", or do we want "we have our own thing"?
> 
> I would have voted for following glibc, except that it does
> that cast-to-long thing, which is incorrect behaviour when
> long is 32 bits and the return value from the function being
> tested is 64 bits.

Then simply int64_t as a type instead, and as "our own thing"?

Best regards,
Christian Schoenebeck

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

[Peter Maydell]

On Thu, 18 Aug 2022 at 15:07, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> On Mittwoch, 17. August 2022 17:55:24 CEST Peter Maydell wrote:
> > On Wed, 17 Aug 2022 at 15:49, Nikita Ivanov <nivanov@cloudlinux.com> wrote:
> > > Well...
> > >
> > > What exactly is still under discussion? In my perspective, the main
> > > pitfalls have been resolved:
> > >
> > > 0. All possible places where TFR() macro could be applied are covered.
> > > 1. Macro has been renamed in order to be more transparent. The name has
> > > been chosen in comparison with a similar glibc macro. 2. The macro itself
> > > has been refactored, in order to replace it entirely with glibc
> > > alternative. 3. Problems with statement/expressions differences in qemu
> > > and glibc implementation have been resolved.
> > >
> > > Is there any room for improvement?
> >
> > (a) do we want the statement version or the expression version?
>
> I think the tendency was in favour for the expression version? Markus made it
> clear that the glibc version indeed may evaluate as an expression (GCC
> extension).

Sounds good to me.

> > (b) do we want "use the glibc one, with same-semantics version for
> > compatibility", or do we want "we have our own thing"?
> >
> > I would have voted for following glibc, except that it does
> > that cast-to-long thing, which is incorrect behaviour when
> > long is 32 bits and the return value from the function being
> > tested is 64 bits.
>
> Then simply int64_t as a type instead, and as "our own thing"?

I think this is probably what I would go for, except that we
should use typeof() rather than a specific type.

Then we get to bikeshed the macro name again :-)

-- PMM

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

[Nikita Ivanov]

[-- Attachment #1: Type: text/plain, Size: 33316 bytes --]

Hi!
Sorry for such a long absence, I've been resolving some other issues in my
life for a while. I've adjusted the patch according to your latest
comments. Could you check it out, please?

From 5389c5ccc8789f8f666ab99e50d38af728bd2c9c Mon Sep 17 00:00:00 2001
From: Nikita Ivanov <nivanov@cloudlinux.com>
Date: Wed, 3 Aug 2022 12:54:00 +0300
Subject: [PATCH 1/2] 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>
---
 block/file-posix.c        | 39 ++++++++++++++++++---------------------
 chardev/char-pty.c        |  4 +---
 hw/9pfs/9p-local.c        |  8 ++------
 net/l2tpv3.c              | 15 +++------------
 net/socket.c              | 16 +++++++---------
 net/tap.c                 |  8 ++------
 qga/commands-posix.c      |  4 +---
 semihosting/syscalls.c    |  4 +---
 tests/qtest/libqtest.c    | 14 +++++++-------
 tests/vhost-user-bridge.c |  4 +---
 util/main-loop.c          |  4 +---
 util/osdep.c              |  4 +---
 util/vfio-helpers.c       | 12 ++++++------
 13 files changed, 51 insertions(+), 85 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 66fdb07820..7892bdea31 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1238,9 +1238,9 @@ static int hdev_get_max_segments(int fd, struct stat
*st)
         ret = -errno;
         goto out;
     }
-    do {
-        ret = read(sysfd, buf, sizeof(buf) - 1);
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = read(sysfd, buf, sizeof(buf) - 1)
+    );
     if (ret < 0) {
         ret = -errno;
         goto out;
@@ -1388,9 +1388,9 @@ static int handle_aiocb_ioctl(void *opaque)
     RawPosixAIOData *aiocb = opaque;
     int ret;

-    do {
-        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
+    );
     if (ret == -1) {
         return -errno;
     }
@@ -1472,18 +1472,17 @@ static ssize_t
handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
 {
     ssize_t len;

-    do {
-        if (aiocb->aio_type & QEMU_AIO_WRITE)
-            len = qemu_pwritev(aiocb->aio_fildes,
-                               aiocb->io.iov,
-                               aiocb->io.niov,
-                               aiocb->aio_offset);
-         else
-            len = qemu_preadv(aiocb->aio_fildes,
-                              aiocb->io.iov,
-                              aiocb->io.niov,
-                              aiocb->aio_offset);
-    } while (len == -1 && errno == EINTR);
+    TFR(
+        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
+            qemu_pwritev(aiocb->aio_fildes,
+                           aiocb->io.iov,
+                           aiocb->io.niov,
+                           aiocb->aio_offset) :
+            qemu_preadv(aiocb->aio_fildes,
+                          aiocb->io.iov,
+                          aiocb->io.niov,
+                          aiocb->aio_offset)
+    );

     if (len == -1) {
         return -errno;
@@ -1908,9 +1907,7 @@ static int allocate_first_block(int fd, size_t
max_size)
     buf = qemu_memalign(max_align, write_size);
     memset(buf, 0, write_size);

-    do {
-        n = pwrite(fd, buf, write_size, 0);
-    } while (n == -1 && errno == EINTR);
+    TFR(n = pwrite(fd, buf, write_size, 0));

     ret = (n == -1) ? -errno : 0;

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index 53f25c6bbd..b2f490bacf 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -93,9 +93,7 @@ static void pty_chr_update_read_handler(Chardev *chr)
     pfd.fd = fioc->fd;
     pfd.events = G_IO_OUT;
     pfd.revents = 0;
-    do {
-        rc = g_poll(&pfd, 1, 0);
-    } while (rc == -1 && errno == EINTR);
+    TFR(rc = g_poll(&pfd, 1, 0));
     assert(rc >= 0);

     if (pfd.revents & G_IO_HUP) {
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/socket.c b/net/socket.c
index bfd8596250..a3397660c2 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -117,15 +117,13 @@ static ssize_t
net_socket_receive_dgram(NetClientState *nc, const uint8_t *buf,
     NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
     ssize_t ret;

-    do {
-        if (s->dgram_dst.sin_family != AF_UNIX) {
-            ret = sendto(s->fd, buf, size, 0,
-                         (struct sockaddr *)&s->dgram_dst,
-                         sizeof(s->dgram_dst));
-        } else {
-            ret = send(s->fd, buf, size, 0);
-        }
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = s->dgram_dst.sin_family != AF_UNIX ?
+            sendto(s->fd, buf, size, 0,
+                     (struct sockaddr *)&s->dgram_dst,
+                     sizeof(s->dgram_dst)) :
+            send(s->fd, buf, size, 0)
+    );

     if (ret == -1 && errno == EAGAIN) {
         net_socket_write_poll(s, true);
diff --git a/net/tap.c b/net/tap.c
index e203d07a12..281b0ec9b9 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 eea819cff0..19a03ce6d8 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/semihosting/syscalls.c b/semihosting/syscalls.c
index 508a0ad88c..025c08147a 100644
--- a/semihosting/syscalls.c
+++ b/semihosting/syscalls.c
@@ -317,9 +317,7 @@ static void host_read(CPUState *cs,
gdb_syscall_complete_cb complete,
         complete(cs, -1, EFAULT);
         return;
     }
-    do {
-        ret = read(gf->hostfd, ptr, len);
-    } while (ret == -1 && errno == EINTR);
+    TFR(ret = read(gf->hostfd, ptr, len));
     if (ret == -1) {
         complete(cs, -1, errno);
         unlock_user(ptr, buf, 0);
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 4f4b2d6477..60de1d2fcb 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -101,10 +101,10 @@ static int socket_accept(int sock)
         return -1;
     }

-    do {
-        addrlen = sizeof(addr);
-        ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
-    } while (ret == -1 && errno == EINTR);
+    addrlen = sizeof(addr);
+    TFR(
+        ret = accept(sock, (struct sockaddr *)&addr, &addrlen)
+    );
     if (ret == -1) {
         fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
     }
@@ -582,9 +582,9 @@ int qtest_socket_server(const char *socket_path)
     addr.sun_family = AF_UNIX;
     snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);

-    do {
-        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr))
+    );
     g_assert_cmpint(ret, !=, -1);
     ret = listen(sock, 1);
     g_assert_cmpint(ret, !=, -1);
diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index fecdf915e7..66d1ddbd7a 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -331,9 +331,7 @@ vubr_backend_recv_cb(int sock, void *ctx)
             .msg_iovlen = num,
             .msg_flags = MSG_DONTWAIT,
         };
-        do {
-            ret = recvmsg(vubr->backend_udp_sock, &msg, 0);
-        } while (ret == -1 && (errno == EINTR));
+        TFR(ret = recvmsg(vubr->backend_udp_sock, &msg, 0));

         if (i == 0) {
             iov_restore_front(elem->in_sg, sg, hdrlen);
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;
 }

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 5ba01177bf..c878be1c5f 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -240,9 +240,9 @@ static int qemu_vfio_pci_read_config(QEMUVFIOState *s,
void *buf,
                                     s->config_region_info.offset,
                                     s->config_region_info.size);
     assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
-    do {
-        ret = pread(s->device, buf, size, s->config_region_info.offset +
ofs);
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = pread(s->device, buf, size, s->config_region_info.offset +
ofs)
+    );
     return ret == size ? 0 : -errno;
 }

@@ -254,9 +254,9 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s,
void *buf, int size, int
                                      s->config_region_info.offset,
                                      s->config_region_info.size);
     assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
-    do {
-        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
ofs);
-    } while (ret == -1 && errno == EINTR);
+    TFR(
+        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
ofs)
+    );
     return ret == size ? 0 : -errno;
 }

-- 
2.37.3


From 7a9fccf00ec2d1c6b30b2ed1cb98398b49ddb0bc Mon Sep 17 00:00:00 2001
From: Nikita Ivanov <nivanov@cloudlinux.com>
Date: Mon, 8 Aug 2022 20:43:45 +0300
Subject: [PATCH 2/2] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()

glibc's unistd.h header provides the same macro with the
subtle difference in type casting. Adjust macro name to the
common standard and refactor it to expression.

Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
---
 block/file-posix.c        | 14 +++++++-------
 chardev/char-fd.c         |  2 +-
 chardev/char-pipe.c       | 12 +++++++++---
 chardev/char-pty.c        |  2 +-
 hw/9pfs/9p-local.c        |  6 ++++--
 include/qemu/osdep.h      |  8 +++++++-
 net/l2tpv3.c              |  8 +++++---
 net/socket.c              |  4 ++--
 net/tap-bsd.c             |  6 +++---
 net/tap-linux.c           |  2 +-
 net/tap-solaris.c         |  8 ++++----
 net/tap.c                 | 10 ++++++----
 os-posix.c                |  2 +-
 qga/commands-posix.c      |  2 +-
 semihosting/syscalls.c    |  2 +-
 tests/qtest/libqtest.c    | 10 +++++-----
 tests/vhost-user-bridge.c |  4 +++-
 util/main-loop.c          |  2 +-
 util/osdep.c              |  2 +-
 util/vfio-helpers.c       |  8 ++++----
 20 files changed, 67 insertions(+), 47 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 7892bdea31..ee7f60c78a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1238,8 +1238,8 @@ static int hdev_get_max_segments(int fd, struct stat
*st)
         ret = -errno;
         goto out;
     }
-    TFR(
-        ret = read(sysfd, buf, sizeof(buf) - 1)
+    ret = TEMP_FAILURE_RETRY(
+        read(sysfd, buf, sizeof(buf) - 1)
     );
     if (ret < 0) {
         ret = -errno;
@@ -1388,8 +1388,8 @@ static int handle_aiocb_ioctl(void *opaque)
     RawPosixAIOData *aiocb = opaque;
     int ret;

-    TFR(
-        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
+    ret = TEMP_FAILURE_RETRY(
+        ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
     );
     if (ret == -1) {
         return -errno;
@@ -1472,8 +1472,8 @@ static ssize_t handle_aiocb_rw_vector(RawPosixAIOData
*aiocb)
 {
     ssize_t len;

-    TFR(
-        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
+    len = TEMP_FAILURE_RETRY(
+        (aiocb->aio_type & QEMU_AIO_WRITE) ?
             qemu_pwritev(aiocb->aio_fildes,
                            aiocb->io.iov,
                            aiocb->io.niov,
@@ -1907,7 +1907,7 @@ static int allocate_first_block(int fd, size_t
max_size)
     buf = qemu_memalign(max_align, write_size);
     memset(buf, 0, write_size);

-    TFR(n = pwrite(fd, buf, write_size, 0));
+    n = TEMP_FAILURE_RETRY(pwrite(fd, buf, write_size, 0));

     ret = (n == -1) ? -errno : 0;

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index cf78454841..1250a63236 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -198,7 +198,7 @@ int qmp_chardev_open_file_source(char *src, int flags,
Error **errp)
 {
     int fd = -1;

-    TFR(fd = qemu_open_old(src, flags, 0666));
+    fd = TEMP_FAILURE_RETRY(qemu_open_old(src, flags, 0666));
     if (fd == -1) {
         error_setg_file_open(errp, errno, src);
     }
diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index 66d3b85091..a59a055c87 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -131,8 +131,12 @@ static void qemu_chr_open_pipe(Chardev *chr,

     filename_in = g_strdup_printf("%s.in", filename);
     filename_out = g_strdup_printf("%s.out", filename);
-    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
-    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
+    fd_in = TEMP_FAILURE_RETRY(
+        qemu_open_old(filename_in, O_RDWR | O_BINARY)
+    );
+    fd_out = TEMP_FAILURE_RETRY(
+        qemu_open_old(filename_out, O_RDWR | O_BINARY)
+    );
     g_free(filename_in);
     g_free(filename_out);
     if (fd_in < 0 || fd_out < 0) {
@@ -142,7 +146,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
         if (fd_out >= 0) {
             close(fd_out);
         }
-        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
+        fd_in = fd_out = TEMP_FAILURE_RETRY(
+            qemu_open_old(filename, O_RDWR | O_BINARY)
+        );
         if (fd_in < 0) {
             error_setg_file_open(errp, errno, filename);
             return;
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index b2f490bacf..913a98250b 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -93,7 +93,7 @@ static void pty_chr_update_read_handler(Chardev *chr)
     pfd.fd = fioc->fd;
     pfd.events = G_IO_OUT;
     pfd.revents = 0;
-    TFR(rc = g_poll(&pfd, 1, 0));
+    rc = TEMP_FAILURE_RETRY(g_poll(&pfd, 1, 0));
     assert(rc >= 0);

     if (pfd.revents & G_IO_HUP) {
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index c90ab947ba..6a3b4fac4a 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -470,7 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
V9fsPath *fs_path,
         if (fd == -1) {
             return -1;
         }
-        TFR(tsize = read(fd, (void *)buf, bufsz));
+        tsize = TEMP_FAILURE_RETRY(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)) {
@@ -906,7 +906,9 @@ static int local_symlink(FsContext *fs_ctx, const char
*oldpath,
         }
         /* Write the oldpath (target) to the file. */
         oldpath_size = strlen(oldpath);
-        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
+        write_size = TEMP_FAILURE_RETRY(
+            write(fd, (void *)oldpath, oldpath_size)
+        );
         close_preserve_errno(fd);

         if (write_size != oldpath_size) {
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index b1c161c035..6e244f15fa 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -243,7 +243,13 @@ void QEMU_ERROR("code path is reachable")
 #define ESHUTDOWN 4099
 #endif

-#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
+#define TEMP_FAILURE_RETRY(expr) \
+    (__extension__                                          \
+        ({ typeof(int64_t) __result;                               \
+           do {                                             \
+                __result = (typeof(int64_t)) (expression);         \
+           } while (__result == -1L && errno == EINTR);     \
+           __result; }))

 /* time_t may be either 32 or 64 bits depending on the host OS, and
  * can be either signed or unsigned, so we can't just hardcode a
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index adfdbdb84c..cbde9332fe 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -240,7 +240,7 @@ static ssize_t
net_l2tpv3_receive_dgram_iov(NetClientState *nc,
     message.msg_control = NULL;
     message.msg_controllen = 0;
     message.msg_flags = 0;
-    TFR(ret = sendmsg(s->fd, &message, 0));
+    ret = TEMP_FAILURE_RETRY(sendmsg(s->fd, &message, 0));
     if (ret > 0) {
         ret -= s->offset;
     } else if (ret == 0) {
@@ -283,7 +283,7 @@ static ssize_t net_l2tpv3_receive_dgram(NetClientState
*nc,
     message.msg_control = NULL;
     message.msg_controllen = 0;
     message.msg_flags = 0;
-    TFR(ret = sendmsg(s->fd, &message, 0));
+    ret = TEMP_FAILURE_RETRY(sendmsg(s->fd, &message, 0));
     if (ret > 0) {
         ret -= s->offset;
     } else if (ret == 0) {
@@ -430,7 +430,9 @@ static void net_l2tpv3_send(void *opaque)

     msgvec = s->msgvec + s->queue_head;
     if (target_count > 0) {
-        TFR(count = recvmmsg(s->fd, msgvec, target_count, MSG_DONTWAIT,
NULL));
+        count = TEMP_FAILURE_RETRY(
+            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/socket.c b/net/socket.c
index a3397660c2..cfb893b183 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -117,8 +117,8 @@ static ssize_t net_socket_receive_dgram(NetClientState
*nc, const uint8_t *buf,
     NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
     ssize_t ret;

-    TFR(
-        ret = s->dgram_dst.sin_family != AF_UNIX ?
+    ret = TEMP_FAILURE_RETRY(
+        s->dgram_dst.sin_family != AF_UNIX ?
             sendto(s->fd, buf, size, 0,
                      (struct sockaddr *)&s->dgram_dst,
                      sizeof(s->dgram_dst)) :
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 005ce05c6e..720090244e 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -56,7 +56,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         } else {
             snprintf(dname, sizeof dname, "/dev/tap%d", i);
         }
-        TFR(fd = open(dname, O_RDWR));
+        fd = TEMP_FAILURE_RETRY(open(dname, O_RDWR));
         if (fd >= 0) {
             break;
         }
@@ -111,7 +111,7 @@ static int tap_open_clone(char *ifname, int
ifname_size, Error **errp)
     int fd, s, ret;
     struct ifreq ifr;

-    TFR(fd = open(PATH_NET_TAP, O_RDWR));
+    fd = TEMP_FAILURE_RETRY(open(PATH_NET_TAP, O_RDWR));
     if (fd < 0) {
         error_setg_errno(errp, errno, "could not open %s", PATH_NET_TAP);
         return -1;
@@ -159,7 +159,7 @@ int tap_open(char *ifname, int ifname_size, int
*vnet_hdr,
     if (ifname[0] != '\0') {
         char dname[100];
         snprintf(dname, sizeof dname, "/dev/%s", ifname);
-        TFR(fd = open(dname, O_RDWR));
+        fd = TEMP_FAILURE_RETRY(open(dname, O_RDWR));
         if (fd < 0 && errno != ENOENT) {
             error_setg_errno(errp, errno, "could not open %s", dname);
             return -1;
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 304ff45071..bf3ce44329 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -45,7 +45,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     int len = sizeof(struct virtio_net_hdr);
     unsigned int features;

-    TFR(fd = open(PATH_NET_TUN, O_RDWR));
+    fd = TEMP_FAILURE_RETRY(open(PATH_NET_TUN, O_RDWR));
     if (fd < 0) {
         error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
         return -1;
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index a44f8805c2..73e0e9f196 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -84,13 +84,13 @@ static int tap_alloc(char *dev, size_t dev_size, Error
**errp)
     if( ip_fd )
        close(ip_fd);

-    TFR(ip_fd = open("/dev/udp", O_RDWR, 0));
+    ip_fd = TEMP_FAILURE_RETRY(open("/dev/udp", O_RDWR, 0));
     if (ip_fd < 0) {
         error_setg(errp, "Can't open /dev/ip (actually /dev/udp)");
         return -1;
     }

-    TFR(tap_fd = open("/dev/tap", O_RDWR, 0));
+    tap_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
     if (tap_fd < 0) {
         error_setg(errp, "Can't open /dev/tap");
         return -1;
@@ -104,7 +104,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error
**errp)
     if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
         error_report("Can't assign new interface");

-    TFR(if_fd = open("/dev/tap", O_RDWR, 0));
+    if_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
     if (if_fd < 0) {
         error_setg(errp, "Can't open /dev/tap (2)");
         return -1;
@@ -137,7 +137,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error
**errp)
     if (ioctl (ip_fd, I_PUSH, "arp") < 0)
         error_report("Can't push ARP module (3)");
     /* Open arp_fd */
-    TFR(arp_fd = open ("/dev/tap", O_RDWR, 0));
+    arp_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
     if (arp_fd < 0)
         error_report("Can't open %s", "/dev/tap");

diff --git a/net/tap.c b/net/tap.c
index 281b0ec9b9..592d6c2c1a 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -102,7 +102,7 @@ static ssize_t tap_write_packet(TAPState *s, const
struct iovec *iov, int iovcnt
 {
     ssize_t len;

-    TFR(len = writev(s->fd, iov, iovcnt));
+    len = TEMP_FAILURE_RETRY(writev(s->fd, iov, iovcnt));

     if (len == -1 && errno == EAGAIN) {
         tap_write_poll(s, true);
@@ -575,7 +575,7 @@ static int net_bridge_run_helper(const char *helper,
const char *bridge,

         close(sv[1]);

-        TFR(fd = recv_fd(sv[0]));
+        fd = TEMP_FAILURE_RETRY(recv_fd(sv[0]));
         saved_errno = errno;

         close(sv[0]);
@@ -647,8 +647,10 @@ static int net_tap_init(const NetdevTapOptions *tap,
int *vnet_hdr,
         vnet_hdr_required = 0;
     }

-    TFR(fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
-                      mq_required, errp));
+    fd = TEMP_FAILURE_RETRY(
+        tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
+                      mq_required, errp)
+    );
     if (fd < 0) {
         return -1;
     }
diff --git a/os-posix.c b/os-posix.c
index 321fc4bd13..59cac65585 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -266,7 +266,7 @@ void os_setup_post(void)
             error_report("not able to chdir to /: %s", strerror(errno));
             exit(1);
         }
-        TFR(fd = qemu_open_old("/dev/null", O_RDWR));
+        fd = TEMP_FAILURE_RETRY(qemu_open_old("/dev/null", O_RDWR));
         if (fd == -1) {
             exit(1);
         }
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 19a03ce6d8..1cb214c7e2 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -68,7 +68,7 @@ static void ga_wait_child(pid_t pid, int *status, Error
**errp)

     *status = 0;

-    TFR(rpid = waitpid(pid, status, 0));
+    rpid = TEMP_FAILURE_RETRY(waitpid(pid, status, 0));

     if (rpid == -1) {
         error_setg_errno(errp, errno, "failed to wait for child (pid: %d)",
diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
index 025c08147a..0cbef7d22b 100644
--- a/semihosting/syscalls.c
+++ b/semihosting/syscalls.c
@@ -317,7 +317,7 @@ static void host_read(CPUState *cs,
gdb_syscall_complete_cb complete,
         complete(cs, -1, EFAULT);
         return;
     }
-    TFR(ret = read(gf->hostfd, ptr, len));
+    ret = TEMP_FAILURE_RETRY(read(gf->hostfd, ptr, len));
     if (ret == -1) {
         complete(cs, -1, errno);
         unlock_user(ptr, buf, 0);
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 60de1d2fcb..56863a56b9 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -102,8 +102,8 @@ static int socket_accept(int sock)
     }

     addrlen = sizeof(addr);
-    TFR(
-        ret = accept(sock, (struct sockaddr *)&addr, &addrlen)
+    ret = TEMP_FAILURE_RETRY(
+        accept(sock, (struct sockaddr *)&addr, &addrlen)
     );
     if (ret == -1) {
         fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
@@ -140,7 +140,7 @@ void qtest_kill_qemu(QTestState *s)
     /* Skip wait if qtest_probe_child already reaped.  */
     if (pid != -1) {
         kill(pid, SIGTERM);
-        TFR(pid = waitpid(s->qemu_pid, &s->wstatus, 0));
+        pid = TEMP_FAILURE_RETRY(waitpid(s->qemu_pid, &s->wstatus, 0));
         assert(pid == s->qemu_pid);
         s->qemu_pid = -1;
     }
@@ -582,8 +582,8 @@ int qtest_socket_server(const char *socket_path)
     addr.sun_family = AF_UNIX;
     snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);

-    TFR(
-        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr))
+    ret = TEMP_FAILURE_RETRY(
+        bind(sock, (struct sockaddr *)&addr, sizeof(addr))
     );
     g_assert_cmpint(ret, !=, -1);
     ret = listen(sock, 1);
diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index 66d1ddbd7a..392432259f 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -331,7 +331,9 @@ vubr_backend_recv_cb(int sock, void *ctx)
             .msg_iovlen = num,
             .msg_flags = MSG_DONTWAIT,
         };
-        TFR(ret = recvmsg(vubr->backend_udp_sock, &msg, 0));
+        ret = TEMP_FAILURE_RETRY(
+                recvmsg(vubr->backend_udp_sock, &msg, 0)
+        );

         if (i == 0) {
             iov_restore_front(elem->in_sg, sg, hdrlen);
diff --git a/util/main-loop.c b/util/main-loop.c
index 58e14db2d4..5f65f928bd 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -64,7 +64,7 @@ static void sigfd_handler(void *opaque)
     ssize_t len;

     while (1) {
-        TFR(len = read(fd, &info, sizeof(info)));
+        len = TEMP_FAILURE_RETRY(read(fd, &info, sizeof(info)));

         if (len == -1 && errno == EAGAIN) {
             break;
diff --git a/util/osdep.c b/util/osdep.c
index d35c473ac7..2c287e75e7 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -244,7 +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();
-    TFR(ret = fcntl(fd, fcntl_op_setlk, &fl));
+    ret = TEMP_FAILURE_RETRY(fcntl(fd, fcntl_op_setlk, &fl));
     return ret == -1 ? -errno : 0;
 }

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index c878be1c5f..e1422666eb 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -240,8 +240,8 @@ static int qemu_vfio_pci_read_config(QEMUVFIOState *s,
void *buf,
                                     s->config_region_info.offset,
                                     s->config_region_info.size);
     assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
-    TFR(
-        ret = pread(s->device, buf, size, s->config_region_info.offset +
ofs)
+    ret = TEMP_FAILURE_RETRY(
+        pread(s->device, buf, size, s->config_region_info.offset + ofs)
     );
     return ret == size ? 0 : -errno;
 }
@@ -254,8 +254,8 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s,
void *buf, int size, int
                                      s->config_region_info.offset,
                                      s->config_region_info.size);
     assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
-    TFR(
-        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
ofs)
+    ret = TEMP_FAILURE_RETRY(
+        pwrite(s->device, buf, size, s->config_region_info.offset + ofs)
     );
     return ret == size ? 0 : -errno;
 }
-- 
2.37.3


On Thu, Aug 18, 2022 at 5:12 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 18 Aug 2022 at 15:07, Christian Schoenebeck
> <qemu_oss@crudebyte.com> wrote:
> >
> > On Mittwoch, 17. August 2022 17:55:24 CEST Peter Maydell wrote:
> > > On Wed, 17 Aug 2022 at 15:49, Nikita Ivanov <nivanov@cloudlinux.com>
> wrote:
> > > > Well...
> > > >
> > > > What exactly is still under discussion? In my perspective, the main
> > > > pitfalls have been resolved:
> > > >
> > > > 0. All possible places where TFR() macro could be applied are
> covered.
> > > > 1. Macro has been renamed in order to be more transparent. The name
> has
> > > > been chosen in comparison with a similar glibc macro. 2. The macro
> itself
> > > > has been refactored, in order to replace it entirely with glibc
> > > > alternative. 3. Problems with statement/expressions differences in
> qemu
> > > > and glibc implementation have been resolved.
> > > >
> > > > Is there any room for improvement?
> > >
> > > (a) do we want the statement version or the expression version?
> >
> > I think the tendency was in favour for the expression version? Markus
> made it
> > clear that the glibc version indeed may evaluate as an expression (GCC
> > extension).
>
> Sounds good to me.
>
> > > (b) do we want "use the glibc one, with same-semantics version for
> > > compatibility", or do we want "we have our own thing"?
> > >
> > > I would have voted for following glibc, except that it does
> > > that cast-to-long thing, which is incorrect behaviour when
> > > long is 32 bits and the return value from the function being
> > > tested is 64 bits.
> >
> > Then simply int64_t as a type instead, and as "our own thing"?
>
> I think this is probably what I would go for, except that we
> should use typeof() rather than a specific type.
>
> Then we get to bikeshed the macro name again :-)
>
> -- PMM
>


-- 
Best Regards,
*Nikita Ivanov* | C developer
*Telephone:* +79140870696
*Telephone:* +79015053149

[-- Attachment #2: Type: text/html, Size: 41082 bytes --]

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

[Christian Schoenebeck]

On Freitag, 7. Oktober 2022 13:44:28 CEST Nikita Ivanov wrote:
> Hi!

Hi Nikita!

> Sorry for such a long absence, I've been resolving some other issues in my
> life for a while. I've adjusted the patch according to your latest
> comments. Could you check it out, please?

Sorry for the drill, but I fear this has to follow common form for patch 
submissions:

  * one email as cover letter

  * two (additional) separate emails for the two patches, each referencing the 
    cover letter email

  * bumping the version in subject line

https://www.qemu.org/docs/master/devel/submitting-a-patch.html

One more comment on patch 2 below ...

> From 5389c5ccc8789f8f666ab99e50d38af728bd2c9c Mon Sep 17 00:00:00 2001
> From: Nikita Ivanov <nivanov@cloudlinux.com>
> Date: Wed, 3 Aug 2022 12:54:00 +0300
> Subject: [PATCH 1/2] 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>
> ---
>  block/file-posix.c        | 39 ++++++++++++++++++---------------------
>  chardev/char-pty.c        |  4 +---
>  hw/9pfs/9p-local.c        |  8 ++------
>  net/l2tpv3.c              | 15 +++------------
>  net/socket.c              | 16 +++++++---------
>  net/tap.c                 |  8 ++------
>  qga/commands-posix.c      |  4 +---
>  semihosting/syscalls.c    |  4 +---
>  tests/qtest/libqtest.c    | 14 +++++++-------
>  tests/vhost-user-bridge.c |  4 +---
>  util/main-loop.c          |  4 +---
>  util/osdep.c              |  4 +---
>  util/vfio-helpers.c       | 12 ++++++------
>  13 files changed, 51 insertions(+), 85 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 66fdb07820..7892bdea31 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1238,9 +1238,9 @@ static int hdev_get_max_segments(int fd, struct stat
> *st)
>          ret = -errno;
>          goto out;
>      }
> -    do {
> -        ret = read(sysfd, buf, sizeof(buf) - 1);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = read(sysfd, buf, sizeof(buf) - 1)
> +    );
>      if (ret < 0) {
>          ret = -errno;
>          goto out;
> @@ -1388,9 +1388,9 @@ static int handle_aiocb_ioctl(void *opaque)
>      RawPosixAIOData *aiocb = opaque;
>      int ret;
> 
> -    do {
> -        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
> +    );
>      if (ret == -1) {
>          return -errno;
>      }
> @@ -1472,18 +1472,17 @@ static ssize_t
> handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
>  {
>      ssize_t len;
> 
> -    do {
> -        if (aiocb->aio_type & QEMU_AIO_WRITE)
> -            len = qemu_pwritev(aiocb->aio_fildes,
> -                               aiocb->io.iov,
> -                               aiocb->io.niov,
> -                               aiocb->aio_offset);
> -         else
> -            len = qemu_preadv(aiocb->aio_fildes,
> -                              aiocb->io.iov,
> -                              aiocb->io.niov,
> -                              aiocb->aio_offset);
> -    } while (len == -1 && errno == EINTR);
> +    TFR(
> +        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
> +            qemu_pwritev(aiocb->aio_fildes,
> +                           aiocb->io.iov,
> +                           aiocb->io.niov,
> +                           aiocb->aio_offset) :
> +            qemu_preadv(aiocb->aio_fildes,
> +                          aiocb->io.iov,
> +                          aiocb->io.niov,
> +                          aiocb->aio_offset)
> +    );
> 
>      if (len == -1) {
>          return -errno;
> @@ -1908,9 +1907,7 @@ static int allocate_first_block(int fd, size_t
> max_size)
>      buf = qemu_memalign(max_align, write_size);
>      memset(buf, 0, write_size);
> 
> -    do {
> -        n = pwrite(fd, buf, write_size, 0);
> -    } while (n == -1 && errno == EINTR);
> +    TFR(n = pwrite(fd, buf, write_size, 0));
> 
>      ret = (n == -1) ? -errno : 0;
> 
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index 53f25c6bbd..b2f490bacf 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -93,9 +93,7 @@ static void pty_chr_update_read_handler(Chardev *chr)
>      pfd.fd = fioc->fd;
>      pfd.events = G_IO_OUT;
>      pfd.revents = 0;
> -    do {
> -        rc = g_poll(&pfd, 1, 0);
> -    } while (rc == -1 && errno == EINTR);
> +    TFR(rc = g_poll(&pfd, 1, 0));
>      assert(rc >= 0);
> 
>      if (pfd.revents & G_IO_HUP) {
> 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/socket.c b/net/socket.c
> index bfd8596250..a3397660c2 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -117,15 +117,13 @@ static ssize_t
> net_socket_receive_dgram(NetClientState *nc, const uint8_t *buf,
>      NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
>      ssize_t ret;
> 
> -    do {
> -        if (s->dgram_dst.sin_family != AF_UNIX) {
> -            ret = sendto(s->fd, buf, size, 0,
> -                         (struct sockaddr *)&s->dgram_dst,
> -                         sizeof(s->dgram_dst));
> -        } else {
> -            ret = send(s->fd, buf, size, 0);
> -        }
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = s->dgram_dst.sin_family != AF_UNIX ?
> +            sendto(s->fd, buf, size, 0,
> +                     (struct sockaddr *)&s->dgram_dst,
> +                     sizeof(s->dgram_dst)) :
> +            send(s->fd, buf, size, 0)
> +    );
> 
>      if (ret == -1 && errno == EAGAIN) {
>          net_socket_write_poll(s, true);
> diff --git a/net/tap.c b/net/tap.c
> index e203d07a12..281b0ec9b9 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 eea819cff0..19a03ce6d8 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/semihosting/syscalls.c b/semihosting/syscalls.c
> index 508a0ad88c..025c08147a 100644
> --- a/semihosting/syscalls.c
> +++ b/semihosting/syscalls.c
> @@ -317,9 +317,7 @@ static void host_read(CPUState *cs,
> gdb_syscall_complete_cb complete,
>          complete(cs, -1, EFAULT);
>          return;
>      }
> -    do {
> -        ret = read(gf->hostfd, ptr, len);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(ret = read(gf->hostfd, ptr, len));
>      if (ret == -1) {
>          complete(cs, -1, errno);
>          unlock_user(ptr, buf, 0);
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 4f4b2d6477..60de1d2fcb 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -101,10 +101,10 @@ static int socket_accept(int sock)
>          return -1;
>      }
> 
> -    do {
> -        addrlen = sizeof(addr);
> -        ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
> -    } while (ret == -1 && errno == EINTR);
> +    addrlen = sizeof(addr);
> +    TFR(
> +        ret = accept(sock, (struct sockaddr *)&addr, &addrlen)
> +    );
>      if (ret == -1) {
>          fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
>      }
> @@ -582,9 +582,9 @@ int qtest_socket_server(const char *socket_path)
>      addr.sun_family = AF_UNIX;
>      snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);
> 
> -    do {
> -        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr))
> +    );
>      g_assert_cmpint(ret, !=, -1);
>      ret = listen(sock, 1);
>      g_assert_cmpint(ret, !=, -1);
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index fecdf915e7..66d1ddbd7a 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -331,9 +331,7 @@ vubr_backend_recv_cb(int sock, void *ctx)
>              .msg_iovlen = num,
>              .msg_flags = MSG_DONTWAIT,
>          };
> -        do {
> -            ret = recvmsg(vubr->backend_udp_sock, &msg, 0);
> -        } while (ret == -1 && (errno == EINTR));
> +        TFR(ret = recvmsg(vubr->backend_udp_sock, &msg, 0));
> 
>          if (i == 0) {
>              iov_restore_front(elem->in_sg, sg, hdrlen);
> 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;
>  }
> 
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 5ba01177bf..c878be1c5f 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -240,9 +240,9 @@ static int qemu_vfio_pci_read_config(QEMUVFIOState *s,
> void *buf,
>                                      s->config_region_info.offset,
>                                      s->config_region_info.size);
>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
> -    do {
> -        ret = pread(s->device, buf, size, s->config_region_info.offset +
> ofs);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = pread(s->device, buf, size, s->config_region_info.offset +
> ofs)
> +    );
>      return ret == size ? 0 : -errno;
>  }
> 
> @@ -254,9 +254,9 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s,
> void *buf, int size, int
>                                       s->config_region_info.offset,
>                                       s->config_region_info.size);
>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
> -    do {
> -        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
> ofs);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
> ofs)
> +    );
>      return ret == size ? 0 : -errno;
>  }
> 
> --
> 2.37.3
> 
> 
> From 7a9fccf00ec2d1c6b30b2ed1cb98398b49ddb0bc Mon Sep 17 00:00:00 2001
> From: Nikita Ivanov <nivanov@cloudlinux.com>
> Date: Mon, 8 Aug 2022 20:43:45 +0300
> Subject: [PATCH 2/2] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
> 
> glibc's unistd.h header provides the same macro with the
> subtle difference in type casting. Adjust macro name to the
> common standard and refactor it to expression.
> 
> Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
> ---
>  block/file-posix.c        | 14 +++++++-------
>  chardev/char-fd.c         |  2 +-
>  chardev/char-pipe.c       | 12 +++++++++---
>  chardev/char-pty.c        |  2 +-
>  hw/9pfs/9p-local.c        |  6 ++++--
>  include/qemu/osdep.h      |  8 +++++++-
>  net/l2tpv3.c              |  8 +++++---
>  net/socket.c              |  4 ++--
>  net/tap-bsd.c             |  6 +++---
>  net/tap-linux.c           |  2 +-
>  net/tap-solaris.c         |  8 ++++----
>  net/tap.c                 | 10 ++++++----
>  os-posix.c                |  2 +-
>  qga/commands-posix.c      |  2 +-
>  semihosting/syscalls.c    |  2 +-
>  tests/qtest/libqtest.c    | 10 +++++-----
>  tests/vhost-user-bridge.c |  4 +++-
>  util/main-loop.c          |  2 +-
>  util/osdep.c              |  2 +-
>  util/vfio-helpers.c       |  8 ++++----
>  20 files changed, 67 insertions(+), 47 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 7892bdea31..ee7f60c78a 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1238,8 +1238,8 @@ static int hdev_get_max_segments(int fd, struct stat
> *st)
>          ret = -errno;
>          goto out;
>      }
> -    TFR(
> -        ret = read(sysfd, buf, sizeof(buf) - 1)
> +    ret = TEMP_FAILURE_RETRY(
> +        read(sysfd, buf, sizeof(buf) - 1)
>      );
>      if (ret < 0) {
>          ret = -errno;
> @@ -1388,8 +1388,8 @@ static int handle_aiocb_ioctl(void *opaque)
>      RawPosixAIOData *aiocb = opaque;
>      int ret;
> 
> -    TFR(
> -        ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
> +    ret = TEMP_FAILURE_RETRY(
> +        ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
>      );
>      if (ret == -1) {
>          return -errno;
> @@ -1472,8 +1472,8 @@ static ssize_t handle_aiocb_rw_vector(RawPosixAIOData
> *aiocb)
>  {
>      ssize_t len;
> 
> -    TFR(
> -        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
> +    len = TEMP_FAILURE_RETRY(
> +        (aiocb->aio_type & QEMU_AIO_WRITE) ?
>              qemu_pwritev(aiocb->aio_fildes,
>                             aiocb->io.iov,
>                             aiocb->io.niov,
> @@ -1907,7 +1907,7 @@ static int allocate_first_block(int fd, size_t
> max_size)
>      buf = qemu_memalign(max_align, write_size);
>      memset(buf, 0, write_size);
> 
> -    TFR(n = pwrite(fd, buf, write_size, 0));
> +    n = TEMP_FAILURE_RETRY(pwrite(fd, buf, write_size, 0));
> 
>      ret = (n == -1) ? -errno : 0;
> 
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index cf78454841..1250a63236 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -198,7 +198,7 @@ int qmp_chardev_open_file_source(char *src, int flags,
> Error **errp)
>  {
>      int fd = -1;
> 
> -    TFR(fd = qemu_open_old(src, flags, 0666));
> +    fd = TEMP_FAILURE_RETRY(qemu_open_old(src, flags, 0666));
>      if (fd == -1) {
>          error_setg_file_open(errp, errno, src);
>      }
> diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
> index 66d3b85091..a59a055c87 100644
> --- a/chardev/char-pipe.c
> +++ b/chardev/char-pipe.c
> @@ -131,8 +131,12 @@ static void qemu_chr_open_pipe(Chardev *chr,
> 
>      filename_in = g_strdup_printf("%s.in", filename);
>      filename_out = g_strdup_printf("%s.out", filename);
> -    TFR(fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY));
> -    TFR(fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY));
> +    fd_in = TEMP_FAILURE_RETRY(
> +        qemu_open_old(filename_in, O_RDWR | O_BINARY)
> +    );
> +    fd_out = TEMP_FAILURE_RETRY(
> +        qemu_open_old(filename_out, O_RDWR | O_BINARY)
> +    );
>      g_free(filename_in);
>      g_free(filename_out);
>      if (fd_in < 0 || fd_out < 0) {
> @@ -142,7 +146,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
>          if (fd_out >= 0) {
>              close(fd_out);
>          }
> -        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
> +        fd_in = fd_out = TEMP_FAILURE_RETRY(
> +            qemu_open_old(filename, O_RDWR | O_BINARY)
> +        );
>          if (fd_in < 0) {
>              error_setg_file_open(errp, errno, filename);
>              return;
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index b2f490bacf..913a98250b 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -93,7 +93,7 @@ static void pty_chr_update_read_handler(Chardev *chr)
>      pfd.fd = fioc->fd;
>      pfd.events = G_IO_OUT;
>      pfd.revents = 0;
> -    TFR(rc = g_poll(&pfd, 1, 0));
> +    rc = TEMP_FAILURE_RETRY(g_poll(&pfd, 1, 0));
>      assert(rc >= 0);
> 
>      if (pfd.revents & G_IO_HUP) {
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index c90ab947ba..6a3b4fac4a 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -470,7 +470,7 @@ static ssize_t local_readlink(FsContext *fs_ctx,
> V9fsPath *fs_path,
>          if (fd == -1) {
>              return -1;
>          }
> -        TFR(tsize = read(fd, (void *)buf, bufsz));
> +        tsize = TEMP_FAILURE_RETRY(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)) {
> @@ -906,7 +906,9 @@ static int local_symlink(FsContext *fs_ctx, const char
> *oldpath,
>          }
>          /* Write the oldpath (target) to the file. */
>          oldpath_size = strlen(oldpath);
> -        TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
> +        write_size = TEMP_FAILURE_RETRY(
> +            write(fd, (void *)oldpath, oldpath_size)
> +        );
>          close_preserve_errno(fd);
> 
>          if (write_size != oldpath_size) {
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index b1c161c035..6e244f15fa 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -243,7 +243,13 @@ void QEMU_ERROR("code path is reachable")
>  #define ESHUTDOWN 4099
>  #endif
> 
> -#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
> +#define TEMP_FAILURE_RETRY(expr) \
> +    (__extension__                                          \
> +        ({ typeof(int64_t) __result;                               \

I think Peter meant `typeof(expr)` here. :)

> +           do {                                             \
> +                __result = (typeof(int64_t)) (expression);         \

Same here.

> +           } while (__result == -1L && errno == EINTR);     \
> +           __result; }))
> 
>  /* time_t may be either 32 or 64 bits depending on the host OS, and
>   * can be either signed or unsigned, so we can't just hardcode a
> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
> index adfdbdb84c..cbde9332fe 100644
> --- a/net/l2tpv3.c
> +++ b/net/l2tpv3.c
> @@ -240,7 +240,7 @@ static ssize_t
> net_l2tpv3_receive_dgram_iov(NetClientState *nc,
>      message.msg_control = NULL;
>      message.msg_controllen = 0;
>      message.msg_flags = 0;
> -    TFR(ret = sendmsg(s->fd, &message, 0));
> +    ret = TEMP_FAILURE_RETRY(sendmsg(s->fd, &message, 0));
>      if (ret > 0) {
>          ret -= s->offset;
>      } else if (ret == 0) {
> @@ -283,7 +283,7 @@ static ssize_t net_l2tpv3_receive_dgram(NetClientState
> *nc,
>      message.msg_control = NULL;
>      message.msg_controllen = 0;
>      message.msg_flags = 0;
> -    TFR(ret = sendmsg(s->fd, &message, 0));
> +    ret = TEMP_FAILURE_RETRY(sendmsg(s->fd, &message, 0));
>      if (ret > 0) {
>          ret -= s->offset;
>      } else if (ret == 0) {
> @@ -430,7 +430,9 @@ static void net_l2tpv3_send(void *opaque)
> 
>      msgvec = s->msgvec + s->queue_head;
>      if (target_count > 0) {
> -        TFR(count = recvmmsg(s->fd, msgvec, target_count, MSG_DONTWAIT,
> NULL));
> +        count = TEMP_FAILURE_RETRY(
> +            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/socket.c b/net/socket.c
> index a3397660c2..cfb893b183 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -117,8 +117,8 @@ static ssize_t net_socket_receive_dgram(NetClientState
> *nc, const uint8_t *buf,
>      NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
>      ssize_t ret;
> 
> -    TFR(
> -        ret = s->dgram_dst.sin_family != AF_UNIX ?
> +    ret = TEMP_FAILURE_RETRY(
> +        s->dgram_dst.sin_family != AF_UNIX ?
>              sendto(s->fd, buf, size, 0,
>                       (struct sockaddr *)&s->dgram_dst,
>                       sizeof(s->dgram_dst)) :
> diff --git a/net/tap-bsd.c b/net/tap-bsd.c
> index 005ce05c6e..720090244e 100644
> --- a/net/tap-bsd.c
> +++ b/net/tap-bsd.c
> @@ -56,7 +56,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
> } else {
>              snprintf(dname, sizeof dname, "/dev/tap%d", i);
>          }
> -        TFR(fd = open(dname, O_RDWR));
> +        fd = TEMP_FAILURE_RETRY(open(dname, O_RDWR));
>          if (fd >= 0) {
>              break;
>          }
> @@ -111,7 +111,7 @@ static int tap_open_clone(char *ifname, int
> ifname_size, Error **errp)
>      int fd, s, ret;
>      struct ifreq ifr;
> 
> -    TFR(fd = open(PATH_NET_TAP, O_RDWR));
> +    fd = TEMP_FAILURE_RETRY(open(PATH_NET_TAP, O_RDWR));
>      if (fd < 0) {
>          error_setg_errno(errp, errno, "could not open %s", PATH_NET_TAP);
>          return -1;
> @@ -159,7 +159,7 @@ int tap_open(char *ifname, int ifname_size, int
> *vnet_hdr,
>      if (ifname[0] != '\0') {
>          char dname[100];
>          snprintf(dname, sizeof dname, "/dev/%s", ifname);
> -        TFR(fd = open(dname, O_RDWR));
> +        fd = TEMP_FAILURE_RETRY(open(dname, O_RDWR));
>          if (fd < 0 && errno != ENOENT) {
>              error_setg_errno(errp, errno, "could not open %s", dname);
>              return -1;
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index 304ff45071..bf3ce44329 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -45,7 +45,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
> int len = sizeof(struct virtio_net_hdr);
>      unsigned int features;
> 
> -    TFR(fd = open(PATH_NET_TUN, O_RDWR));
> +    fd = TEMP_FAILURE_RETRY(open(PATH_NET_TUN, O_RDWR));
>      if (fd < 0) {
>          error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
>          return -1;
> diff --git a/net/tap-solaris.c b/net/tap-solaris.c
> index a44f8805c2..73e0e9f196 100644
> --- a/net/tap-solaris.c
> +++ b/net/tap-solaris.c
> @@ -84,13 +84,13 @@ static int tap_alloc(char *dev, size_t dev_size, Error
> **errp)
>      if( ip_fd )
>         close(ip_fd);
> 
> -    TFR(ip_fd = open("/dev/udp", O_RDWR, 0));
> +    ip_fd = TEMP_FAILURE_RETRY(open("/dev/udp", O_RDWR, 0));
>      if (ip_fd < 0) {
>          error_setg(errp, "Can't open /dev/ip (actually /dev/udp)");
>          return -1;
>      }
> 
> -    TFR(tap_fd = open("/dev/tap", O_RDWR, 0));
> +    tap_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
>      if (tap_fd < 0) {
>          error_setg(errp, "Can't open /dev/tap");
>          return -1;
> @@ -104,7 +104,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error
> **errp)
>      if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
>          error_report("Can't assign new interface");
> 
> -    TFR(if_fd = open("/dev/tap", O_RDWR, 0));
> +    if_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
>      if (if_fd < 0) {
>          error_setg(errp, "Can't open /dev/tap (2)");
>          return -1;
> @@ -137,7 +137,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error
> **errp)
>      if (ioctl (ip_fd, I_PUSH, "arp") < 0)
>          error_report("Can't push ARP module (3)");
>      /* Open arp_fd */
> -    TFR(arp_fd = open ("/dev/tap", O_RDWR, 0));
> +    arp_fd = TEMP_FAILURE_RETRY(open("/dev/tap", O_RDWR, 0));
>      if (arp_fd < 0)
>          error_report("Can't open %s", "/dev/tap");
> 
> diff --git a/net/tap.c b/net/tap.c
> index 281b0ec9b9..592d6c2c1a 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -102,7 +102,7 @@ static ssize_t tap_write_packet(TAPState *s, const
> struct iovec *iov, int iovcnt
>  {
>      ssize_t len;
> 
> -    TFR(len = writev(s->fd, iov, iovcnt));
> +    len = TEMP_FAILURE_RETRY(writev(s->fd, iov, iovcnt));
> 
>      if (len == -1 && errno == EAGAIN) {
>          tap_write_poll(s, true);
> @@ -575,7 +575,7 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
> 
>          close(sv[1]);
> 
> -        TFR(fd = recv_fd(sv[0]));
> +        fd = TEMP_FAILURE_RETRY(recv_fd(sv[0]));
>          saved_errno = errno;
> 
>          close(sv[0]);
> @@ -647,8 +647,10 @@ static int net_tap_init(const NetdevTapOptions *tap,
> int *vnet_hdr,
>          vnet_hdr_required = 0;
>      }
> 
> -    TFR(fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
> -                      mq_required, errp));
> +    fd = TEMP_FAILURE_RETRY(
> +        tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
> +                      mq_required, errp)
> +    );
>      if (fd < 0) {
>          return -1;
>      }
> diff --git a/os-posix.c b/os-posix.c
> index 321fc4bd13..59cac65585 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -266,7 +266,7 @@ void os_setup_post(void)
>              error_report("not able to chdir to /: %s", strerror(errno));
>              exit(1);
>          }
> -        TFR(fd = qemu_open_old("/dev/null", O_RDWR));
> +        fd = TEMP_FAILURE_RETRY(qemu_open_old("/dev/null", O_RDWR));
>          if (fd == -1) {
>              exit(1);
>          }
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 19a03ce6d8..1cb214c7e2 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -68,7 +68,7 @@ static void ga_wait_child(pid_t pid, int *status, Error
> **errp)
> 
>      *status = 0;
> 
> -    TFR(rpid = waitpid(pid, status, 0));
> +    rpid = TEMP_FAILURE_RETRY(waitpid(pid, status, 0));
> 
>      if (rpid == -1) {
>          error_setg_errno(errp, errno, "failed to wait for child (pid: %d)",
> diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
> index 025c08147a..0cbef7d22b 100644
> --- a/semihosting/syscalls.c
> +++ b/semihosting/syscalls.c
> @@ -317,7 +317,7 @@ static void host_read(CPUState *cs,
> gdb_syscall_complete_cb complete,
>          complete(cs, -1, EFAULT);
>          return;
>      }
> -    TFR(ret = read(gf->hostfd, ptr, len));
> +    ret = TEMP_FAILURE_RETRY(read(gf->hostfd, ptr, len));
>      if (ret == -1) {
>          complete(cs, -1, errno);
>          unlock_user(ptr, buf, 0);
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 60de1d2fcb..56863a56b9 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -102,8 +102,8 @@ static int socket_accept(int sock)
>      }
> 
>      addrlen = sizeof(addr);
> -    TFR(
> -        ret = accept(sock, (struct sockaddr *)&addr, &addrlen)
> +    ret = TEMP_FAILURE_RETRY(
> +        accept(sock, (struct sockaddr *)&addr, &addrlen)
>      );
>      if (ret == -1) {
>          fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
> @@ -140,7 +140,7 @@ void qtest_kill_qemu(QTestState *s)
>      /* Skip wait if qtest_probe_child already reaped. */
>      if (pid != -1) {
>          kill(pid, SIGTERM);
> -        TFR(pid = waitpid(s->qemu_pid, &s->wstatus, 0));
> +        pid = TEMP_FAILURE_RETRY(waitpid(s->qemu_pid, &s->wstatus, 0));
>          assert(pid == s->qemu_pid);
>          s->qemu_pid = -1;
>      }
> @@ -582,8 +582,8 @@ int qtest_socket_server(const char *socket_path)
>      addr.sun_family = AF_UNIX;
>      snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);
> 
> -    TFR(
> -        ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr))
> +    ret = TEMP_FAILURE_RETRY(
> +        bind(sock, (struct sockaddr *)&addr, sizeof(addr))
>      );
>      g_assert_cmpint(ret, !=, -1);
>      ret = listen(sock, 1);
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 66d1ddbd7a..392432259f 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -331,7 +331,9 @@ vubr_backend_recv_cb(int sock, void *ctx)
>              .msg_iovlen = num,
>              .msg_flags = MSG_DONTWAIT,
>          };
> -        TFR(ret = recvmsg(vubr->backend_udp_sock, &msg, 0));
> +        ret = TEMP_FAILURE_RETRY(
> +                recvmsg(vubr->backend_udp_sock, &msg, 0)
> +        );
> 
>          if (i == 0) {
>              iov_restore_front(elem->in_sg, sg, hdrlen);
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 58e14db2d4..5f65f928bd 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -64,7 +64,7 @@ static void sigfd_handler(void *opaque)
>      ssize_t len;
> 
>      while (1) {
> -        TFR(len = read(fd, &info, sizeof(info)));
> +        len = TEMP_FAILURE_RETRY(read(fd, &info, sizeof(info)));
> 
>          if (len == -1 && errno == EAGAIN) {
>              break;
> diff --git a/util/osdep.c b/util/osdep.c
> index d35c473ac7..2c287e75e7 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -244,7 +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();
> -    TFR(ret = fcntl(fd, fcntl_op_setlk, &fl));
> +    ret = TEMP_FAILURE_RETRY(fcntl(fd, fcntl_op_setlk, &fl));
>      return ret == -1 ? -errno : 0;
>  }
> 
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index c878be1c5f..e1422666eb 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -240,8 +240,8 @@ static int qemu_vfio_pci_read_config(QEMUVFIOState *s,
> void *buf,
>                                      s->config_region_info.offset,
>                                      s->config_region_info.size);
>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
> -    TFR(
> -        ret = pread(s->device, buf, size, s->config_region_info.offset +
> ofs)
> +    ret = TEMP_FAILURE_RETRY(
> +        pread(s->device, buf, size, s->config_region_info.offset + ofs)
>      );
>      return ret == size ? 0 : -errno;
>  }
> @@ -254,8 +254,8 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s,
> void *buf, int size, int
>                                       s->config_region_info.offset,
>                                       s->config_region_info.size);
>      assert(QEMU_IS_ALIGNED(s->config_region_info.offset + ofs, size));
> -    TFR(
> -        ret = pwrite(s->device, buf, size, s->config_region_info.offset +
> ofs)
> +    ret = TEMP_FAILURE_RETRY(
> +        pwrite(s->device, buf, size, s->config_region_info.offset + ofs)
>      );
>      return ret == size ? 0 : -errno;
>  }
> --
> 2.37.3
> 
> 
> On Thu, Aug 18, 2022 at 5:12 PM Peter Maydell <peter.maydell@linaro.org>
> 
> wrote:
> > On Thu, 18 Aug 2022 at 15:07, Christian Schoenebeck
> > 
> > <qemu_oss@crudebyte.com> wrote:
> > > On Mittwoch, 17. August 2022 17:55:24 CEST Peter Maydell wrote:
> > > > On Wed, 17 Aug 2022 at 15:49, Nikita Ivanov <nivanov@cloudlinux.com>
> > 
> > wrote:
> > > > > Well...
> > > > > 
> > > > > What exactly is still under discussion? In my perspective, the main
> > > > > pitfalls have been resolved:
> > > > > 
> > > > > 0. All possible places where TFR() macro could be applied are
> > 
> > covered.
> > 
> > > > > 1. Macro has been renamed in order to be more transparent. The name
> > 
> > has
> > 
> > > > > been chosen in comparison with a similar glibc macro. 2. The macro
> > 
> > itself
> > 
> > > > > has been refactored, in order to replace it entirely with glibc
> > > > > alternative. 3. Problems with statement/expressions differences in
> > 
> > qemu
> > 
> > > > > and glibc implementation have been resolved.
> > > > > 
> > > > > Is there any room for improvement?
> > > > 
> > > > (a) do we want the statement version or the expression version?
> > > 
> > > I think the tendency was in favour for the expression version? Markus
> > 
> > made it
> > 
> > > clear that the glibc version indeed may evaluate as an expression (GCC
> > > extension).
> > 
> > Sounds good to me.
> > 
> > > > (b) do we want "use the glibc one, with same-semantics version for
> > > > compatibility", or do we want "we have our own thing"?
> > > > 
> > > > I would have voted for following glibc, except that it does
> > > > that cast-to-long thing, which is incorrect behaviour when
> > > > long is 32 bits and the return value from the function being
> > > > tested is 64 bits.
> > > 
> > > Then simply int64_t as a type instead, and as "our own thing"?
> > 
> > I think this is probably what I would go for, except that we
> > should use typeof() rather than a specific type.
> > 
> > Then we get to bikeshed the macro name again :-)
> > 
> > -- PMM
> 
> --
> Best Regards,
> *Nikita Ivanov* | C developer
> *Telephone:* +79140870696
> *Telephone:* +79015053149

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

[Peter Maydell]

On Fri, 7 Oct 2022 at 12:44, Nikita Ivanov <nivanov@cloudlinux.com> wrote:
>
> Hi!
> Sorry for such a long absence, I've been resolving some other issues in my life for a while. I've adjusted the patch according to your latest comments. Could you check it out, please?

Hi; thanks for coming back to this. (I'd been meaning to re-read the
thread but hadn't found time to do so; sorry.) As Christian says,
you should send the patches as a proper new patchset thread of their
own, but for the moment:

> From 5389c5ccc8789f8f666ab99e50d38af728bd2c9c Mon Sep 17 00:00:00 2001
> From: Nikita Ivanov <nivanov@cloudlinux.com>
> Date: Wed, 3 Aug 2022 12:54:00 +0300
> Subject: [PATCH 1/2] 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>

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 66fdb07820..7892bdea31 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1238,9 +1238,9 @@ static int hdev_get_max_segments(int fd, struct stat *st)
>          ret = -errno;
>          goto out;
>      }
> -    do {
> -        ret = read(sysfd, buf, sizeof(buf) - 1);
> -    } while (ret == -1 && errno == EINTR);
> +    TFR(
> +        ret = read(sysfd, buf, sizeof(buf) - 1)
> +    );

I think this patch is doing things in the wrong order. Instead of
converting code to use the old macro that we don't like and then
updating it again in patch 2 to use the new macro, we should
first introduce the new macro, and then after that we can update
code that's currently not using a macro at all to use the new one.
This makes code review easier because we don't have to look at a
change to this code which is then going to be rewritten anyway.


> From 7a9fccf00ec2d1c6b30b2ed1cb98398b49ddb0bc Mon Sep 17 00:00:00 2001
> From: Nikita Ivanov <nivanov@cloudlinux.com>
> Date: Mon, 8 Aug 2022 20:43:45 +0300
> Subject: [PATCH 2/2] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
>
> glibc's unistd.h header provides the same macro with the
> subtle difference in type casting. Adjust macro name to the
> common standard and refactor it to expression.
>
> Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>


> diff --git a/block/file-posix.c b/block/file-posix.c
> index 7892bdea31..ee7f60c78a 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1238,8 +1238,8 @@ static int hdev_get_max_segments(int fd, struct stat *st)
>          ret = -errno;
>          goto out;
>      }
> -    TFR(
> -        ret = read(sysfd, buf, sizeof(buf) - 1)
> +    ret = TEMP_FAILURE_RETRY(
> +        read(sysfd, buf, sizeof(buf) - 1)
>      );

This doesn't need these newlines in it. If the whole thing fits on
a single line, that's easier to read.

>      if (ret < 0) {
>          ret = -errno;


> @@ -1472,8 +1472,8 @@ static ssize_t handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
>  {
>      ssize_t len;
>
> -    TFR(
> -        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
> +    len = TEMP_FAILURE_RETRY(
> +        (aiocb->aio_type & QEMU_AIO_WRITE) ?
>              qemu_pwritev(aiocb->aio_fildes,
>                             aiocb->io.iov,
>                             aiocb->io.niov,

I'm not sure why you've put the TEMP_FAILURE_RETRY on the outside here
rather than just on the individual function calls.


> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index b1c161c035..6e244f15fa 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -243,7 +243,13 @@ void QEMU_ERROR("code path is reachable")
>  #define ESHUTDOWN 4099
>  #endif
>
> -#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
> +#define TEMP_FAILURE_RETRY(expr) \

We can't call the macro this, because the glibc system headers already
may define a macro of that name, so the compiler will complain if they're
both defined at the same time, and depending on header ordering it might
not be clear which version you're getting.

> +    (__extension__                                          \
> +        ({ typeof(int64_t) __result;                               \

As Christian says, the point of the typeof is to use the type
of the expression. "typeof(int64_t)" is always just "int64_t".
You want "typeof(expr) __result;".

> +           do {                                             \
> +                __result = (typeof(int64_t)) (expression);         \

Then you don't need this cast, because both __result and expr
are the same type anyway.

Also, how did this compile? 'expression' isn't the name of the macro argument.

> +           } while (__result == -1L && errno == EINTR);     \

I think you don't need the 'L' suffix here.

> +           __result; }))

thanks
-- PMM

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

[Nikita Ivanov]

[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]

Hi! Thanks for your notes. I'll try to send updated patches by the end of
the day.

On Fri, Oct 7, 2022 at 6:32 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> I think this patch is doing things in the wrong order. Instead of
> converting code to use the old macro that we don't like and then
> updating it again in patch 2 to use the new macro, we should
> first introduce the new macro, and then after that we can update
> code that's currently not using a macro at all to use the new one.
> This makes code review easier because we don't have to look at a
> change to this code which is then going to be rewritten anyway.


Sounds smooth. I'll refactor patches accordingly.


> >      if (ret < 0) {
> >          ret = -errno;
>
>
> > @@ -1472,8 +1472,8 @@ static ssize_t
> handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
> >  {
> >      ssize_t len;
> >
> > -    TFR(
> > -        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
> > +    len = TEMP_FAILURE_RETRY(
> > +        (aiocb->aio_type & QEMU_AIO_WRITE) ?
> >              qemu_pwritev(aiocb->aio_fildes,
> >                             aiocb->io.iov,
> >                             aiocb->io.niov,
>
> I'm not sure why you've put the TEMP_FAILURE_RETRY on the outside here
> rather than just on the individual function calls.
>

The original code contained both branches in one while loop, so I was
afraid that
value `aiocb->aio_type & QEMU_AIO_WRITE` would change somehow during the
loop.
If you'll say that this is impossible, I'll adjust the code as you propose.

> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index b1c161c035..6e244f15fa 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -243,7 +243,13 @@ void QEMU_ERROR("code path is reachable")
> >  #define ESHUTDOWN 4099
> >  #endif
> >
> > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
> > +#define TEMP_FAILURE_RETRY(expr) \
>
> We can't call the macro this, because the glibc system headers already
> may define a macro of that name, so the compiler will complain if they're
> both defined at the same time, and depending on header ordering it might
> not be clear which version you're getting.
>

Sorry, my fault. I will rename it to "RETRY_ON_EINTR" as it was proposed
earlier in this thread.
-- 
Best Regards,
*Nikita Ivanov* | C developer

[-- Attachment #2: Type: text/html, Size: 3491 bytes --]

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

[Peter Maydell]

On Mon, 10 Oct 2022 at 09:34, Nikita Ivanov <nivanov@cloudlinux.com> wrote:
>
> Hi! Thanks for your notes. I'll try to send updated patches by the end of the day.
>
> On Fri, Oct 7, 2022 at 6:32 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> I'm not sure why you've put the TEMP_FAILURE_RETRY on the outside here
>> rather than just on the individual function calls.
>
>
> The original code contained both branches in one while loop, so I was afraid that
> value `aiocb->aio_type & QEMU_AIO_WRITE` would change somehow during the loop.
> If you'll say that this is impossible, I'll adjust the code as you propose.

Oh, yes, I see. No, nothing should be changing that during the loop,
I think it's just written the way it is now because it seemed neater
when you write out the retry longhand.

-- PMM