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 <niva...@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 <niva...@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.mayd...@linaro.org> wrote: > On Fri, 5 Aug 2022 at 12:27, Marc-André Lureau > <marcandre.lur...@redhat.com> wrote: > > On Fri, Aug 5, 2022 at 3:11 PM Christian Schoenebeck > > <qemu_...@crudebyte.com> wrote: > > > I was thinking the same as Marc-André before: > > > > > > commit 1dacd88ddcf33eb6ed044c4080e3ef5e3de4b6b6 > > > Author: Marc-André Lureau <marcandre.lur...@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.lur...@redhat.com> > > > Message-Id: <20220323155743.1585078-17-marcandre.lur...@redhat.com > > > > > Signed-off-by: Paolo Bonzini <pbonz...@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