Re: [PATCH v4 0/2] Refactoring: expand usage of TFR() macro
Hi! Is there any update on this? I haven't received any comments. On Sun, Oct 23, 2022 at 12:04 PM Nikita Ivanov wrote: > At the moment, TFR() macro has a vague name and is not used > where it possibly could be. In order to make it more transparent > and useful, it was decided to refactor it to make it closer to > the similar one in glibc: TEMP_FAILURE_RETRY(). Now, macro > evaluates into an expression and is named RETRY_ON_EINTR(). All the > places where RETRY_ON_EINTR() macro code be applied were covered. > > Nikita Ivanov (2): > Refactoring: refactor TFR() macro to RETRY_ON_EINTR() > error handling: Use RETRY_ON_EINTR() macro where applicable > > block/file-posix.c| 37 - > chardev/char-fd.c | 2 +- > chardev/char-pipe.c | 8 +--- > chardev/char-pty.c| 4 +--- > hw/9pfs/9p-local.c| 8 ++-- > include/qemu/osdep.h | 8 +++- > net/l2tpv3.c | 17 + > net/socket.c | 16 +++- > 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 | 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 ++-- > 20 files changed, 73 insertions(+), 101 deletions(-) > > -- > 2.37.3 > > -- Best Regards, *Nikita Ivanov* | C developer *Telephone:* +79140870696 *Telephone:* +79015053149
[PATCH v4 1/2] Refactoring: refactor TFR() macro to RETRY_ON_EINTR()
Rename macro name to more transparent one and refactor
it to expression.
Signed-off-by: Nikita Ivanov
---
chardev/char-fd.c | 2 +-
chardev/char-pipe.c| 8 +---
include/qemu/osdep.h | 8 +++-
net/tap-bsd.c | 6 +++---
net/tap-linux.c| 2 +-
net/tap-solaris.c | 8
net/tap.c | 2 +-
os-posix.c | 2 +-
tests/qtest/libqtest.c | 2 +-
9 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index cf78454841..d2c4923359 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 = RETRY_ON_EINTR(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..5ad30bcc59 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -131,8 +131,8 @@ 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 = RETRY_ON_EINTR(qemu_open_old(filename_in, O_RDWR | O_BINARY));
+fd_out = RETRY_ON_EINTR(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 +142,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 = RETRY_ON_EINTR(
+qemu_open_old(filename, O_RDWR | O_BINARY)
+);
if (fd_in < 0) {
error_setg_file_open(errp, errno, filename);
return;
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index b1c161c035..25c35b9a0e 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 RETRY_ON_EINTR(expr) \
+(__extension__ \
+({ typeof(expr) __result; \
+ do { \
+__result = (expr); \
+ } while (__result == -1 && 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/tap-bsd.c b/net/tap-bsd.c
index 005ce05c6e..4c98fdd337 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 = RETRY_ON_EINTR(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 = RETRY_ON_EINTR(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 = RETRY_ON_EINTR(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..f54f308d35 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 = RETRY_ON_EINTR(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..38e15028bf 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 = RETRY_ON_EINTR(open(&quo
[PATCH v4 0/2] Refactoring: expand usage of TFR() macro
At the moment, TFR() macro has a vague name and is not used where it possibly could be. In order to make it more transparent and useful, it was decided to refactor it to make it closer to the similar one in glibc: TEMP_FAILURE_RETRY(). Now, macro evaluates into an expression and is named RETRY_ON_EINTR(). All the places where RETRY_ON_EINTR() macro code be applied were covered. Nikita Ivanov (2): Refactoring: refactor TFR() macro to RETRY_ON_EINTR() error handling: Use RETRY_ON_EINTR() macro where applicable block/file-posix.c| 37 - chardev/char-fd.c | 2 +- chardev/char-pipe.c | 8 +--- chardev/char-pty.c| 4 +--- hw/9pfs/9p-local.c| 8 ++-- include/qemu/osdep.h | 8 +++- net/l2tpv3.c | 17 + net/socket.c | 16 +++- 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 | 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 ++-- 20 files changed, 73 insertions(+), 101 deletions(-) -- 2.37.3
[PATCH v4 2/2] error handling: Use RETRY_ON_EINTR() macro where applicable
There is a defined RETRY_ON_EINTR() 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
---
block/file-posix.c| 37 -
chardev/char-pty.c| 4 +---
hw/9pfs/9p-local.c| 8 ++--
net/l2tpv3.c | 17 +
net/socket.c | 16 +++-
net/tap.c | 8 ++--
qga/commands-posix.c | 4 +---
semihosting/syscalls.c| 4 +---
tests/qtest/libqtest.c| 12 +---
tests/vhost-user-bridge.c | 4 +---
util/main-loop.c | 4 +---
util/osdep.c | 4 +---
util/vfio-helpers.c | 12 ++--
13 files changed, 49 insertions(+), 85 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 23acffb9a4..8f7a22e3e4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1229,9 +1229,7 @@ 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);
+ret = RETRY_ON_EINTR(read(sysfd, buf, sizeof(buf) - 1));
if (ret < 0) {
ret = -errno;
goto out;
@@ -1379,9 +1377,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);
+ret = RETRY_ON_EINTR(
+ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
+);
if (ret == -1) {
return -errno;
}
@@ -1463,18 +1461,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);
+len = RETRY_ON_EINTR(
+(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;
@@ -1899,9 +1896,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);
+n = RETRY_ON_EINTR(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..92fd33c854 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(, 1, 0);
-} while (rc == -1 && errno == EINTR);
+rc = RETRY_ON_EINTR(g_poll(, 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..bb3187244f 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);
+tsize = RETRY_ON_EINTR(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);
+write_size = RETRY_ON_EINTR(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..e0726f5f8
Re: [PATCH v3 2/2] error handling: Use RETRY_ON_EINTR() macro where applicable
Hi!
Thanks for clarification! Corrected it in v4.
On Wed, Oct 19, 2022 at 6:24 PM Christian Schoenebeck <
qemu_...@crudebyte.com> wrote:
> On Tuesday, October 18, 2022 10:43:41 AM CEST Nikita Ivanov wrote:
> > There is a defined RETRY_ON_EINTR() 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
> > ---
> > block/file-posix.c| 37 -
> > chardev/char-pty.c| 4 +---
> > hw/9pfs/9p-local.c| 8 ++--
> > net/l2tpv3.c | 17 +
> > net/socket.c | 16 +++-
> > net/tap.c | 8 ++--
> > qga/commands-posix.c | 4 +---
> > semihosting/syscalls.c| 4 +---
> > tests/qtest/libqtest.c| 12 +---
> > tests/vhost-user-bridge.c | 4 +---
> > util/main-loop.c | 4 +---
> > util/osdep.c | 4 +---
> > util/vfio-helpers.c | 12 ++--
> > 13 files changed, 49 insertions(+), 85 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 23acffb9a4..8f7a22e3e4 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1229,9 +1229,7 @@ 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);
> > +ret = RETRY_ON_EINTR(read(sysfd, buf, sizeof(buf) - 1));
> > if (ret < 0) {
> > ret = -errno;
> > goto out;
> > @@ -1379,9 +1377,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);
> > +ret = RETRY_ON_EINTR(
> > +ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
> > +);
> > if (ret == -1) {
> > return -errno;
> > }
> > @@ -1463,18 +1461,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);
> > +len = RETRY_ON_EINTR(
> > +(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;
> > @@ -1899,9 +1896,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);
> > +n = RETRY_ON_EINTR(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..92fd33c854 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(, 1, 0);
> > -} while (rc == -1 && errno == EINTR);
> > +rc = RETRY_ON_EINTR(g_poll(, 1, 0));
> > assert(rc >= 0);
> >
> &
Re: [PATCH v3 1/2] Refactoring: refactor TFR() macro to RETRY_ON_EINTR()
Hi!
Thanks for mentioning the issue. Corrected it in v4.
On Wed, Oct 19, 2022 at 6:40 PM Christian Schoenebeck <
qemu_...@crudebyte.com> wrote:
> On Dienstag, 18. Oktober 2022 10:43:40 CEST Nikita Ivanov wrote:
> > Rename macro name to more transparent one and refactor
> > it to expression.
> >
> > Signed-off-by: Nikita Ivanov
> > ---
> > chardev/char-fd.c | 2 +-
> > chardev/char-pipe.c| 8 +---
> > include/qemu/osdep.h | 8 +++-
> > net/tap-bsd.c | 6 +++---
> > net/tap-linux.c| 2 +-
> > net/tap-solaris.c | 8
> > net/tap.c | 2 +-
> > os-posix.c | 2 +-
> > tests/qtest/libqtest.c | 2 +-
> > 9 files changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> > index cf78454841..d2c4923359 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 = RETRY_ON_EINTR(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..5ad30bcc59 100644
> > --- a/chardev/char-pipe.c
> > +++ b/chardev/char-pipe.c
> > @@ -131,8 +131,8 @@ 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 = RETRY_ON_EINTR(qemu_open_old(filename_in, O_RDWR |
> O_BINARY));
> > +fd_out = RETRY_ON_EINTR(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 +142,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 = RETRY_ON_EINTR(
> > +qemu_open_old(filename, O_RDWR | O_BINARY)
> > +);
> > if (fd_in < 0) {
> > error_setg_file_open(errp, errno, filename);
> > return;
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index b1c161c035..45fcf5f2dc 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 RETRY_ON_EINTR(expr) \
> > +(__extension__ \
> > +({ typeof(expr) __result; \
> > + do { \
> > +__result = (typeof(expr)) (expr); \
>
> You forgot to drop the redundant type cast here. With that dropped:
>
> Reviewed-by: Christian Schoenebeck
>
> > + } while (__result == -1 && 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/tap-bsd.c b/net/tap-bsd.c
> > index 005ce05c6e..4c98fdd337 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 = RETRY_ON_EINTR(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 = RETRY_ON_EINTR(open(PATH_NET_TAP, O_RDWR));
> > if (fd < 0) {
> > error_setg_errno(errp, errno, "could not open %s",
> PATH
[PATCH v3 2/2] error handling: Use RETRY_ON_EINTR() macro where applicable
There is a defined RETRY_ON_EINTR() 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
---
block/file-posix.c| 37 -
chardev/char-pty.c| 4 +---
hw/9pfs/9p-local.c| 8 ++--
net/l2tpv3.c | 17 +
net/socket.c | 16 +++-
net/tap.c | 8 ++--
qga/commands-posix.c | 4 +---
semihosting/syscalls.c| 4 +---
tests/qtest/libqtest.c| 12 +---
tests/vhost-user-bridge.c | 4 +---
util/main-loop.c | 4 +---
util/osdep.c | 4 +---
util/vfio-helpers.c | 12 ++--
13 files changed, 49 insertions(+), 85 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 23acffb9a4..8f7a22e3e4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1229,9 +1229,7 @@ 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);
+ret = RETRY_ON_EINTR(read(sysfd, buf, sizeof(buf) - 1));
if (ret < 0) {
ret = -errno;
goto out;
@@ -1379,9 +1377,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);
+ret = RETRY_ON_EINTR(
+ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
+);
if (ret == -1) {
return -errno;
}
@@ -1463,18 +1461,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);
+len = RETRY_ON_EINTR(
+(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;
@@ -1899,9 +1896,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);
+n = RETRY_ON_EINTR(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..92fd33c854 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(, 1, 0);
-} while (rc == -1 && errno == EINTR);
+rc = RETRY_ON_EINTR(g_poll(, 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..bb3187244f 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);
+tsize = RETRY_ON_EINTR(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);
+write_size = RETRY_ON_EINTR(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..e0726f5f8
[PATCH v3 1/2] Refactoring: refactor TFR() macro to RETRY_ON_EINTR()
Rename macro name to more transparent one and refactor
it to expression.
Signed-off-by: Nikita Ivanov
---
chardev/char-fd.c | 2 +-
chardev/char-pipe.c| 8 +---
include/qemu/osdep.h | 8 +++-
net/tap-bsd.c | 6 +++---
net/tap-linux.c| 2 +-
net/tap-solaris.c | 8
net/tap.c | 2 +-
os-posix.c | 2 +-
tests/qtest/libqtest.c | 2 +-
9 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index cf78454841..d2c4923359 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 = RETRY_ON_EINTR(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..5ad30bcc59 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -131,8 +131,8 @@ 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 = RETRY_ON_EINTR(qemu_open_old(filename_in, O_RDWR | O_BINARY));
+fd_out = RETRY_ON_EINTR(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 +142,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 = RETRY_ON_EINTR(
+qemu_open_old(filename, O_RDWR | O_BINARY)
+);
if (fd_in < 0) {
error_setg_file_open(errp, errno, filename);
return;
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index b1c161c035..45fcf5f2dc 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 RETRY_ON_EINTR(expr) \
+(__extension__ \
+({ typeof(expr) __result; \
+ do { \
+__result = (typeof(expr)) (expr); \
+ } while (__result == -1 && 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/tap-bsd.c b/net/tap-bsd.c
index 005ce05c6e..4c98fdd337 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 = RETRY_ON_EINTR(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 = RETRY_ON_EINTR(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 = RETRY_ON_EINTR(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..f54f308d35 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 = RETRY_ON_EINTR(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..38e15028bf 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 = R
[PATCH v3 0/2] Refactoring: expand usage of TFR() macro
At the moment, TFR() macro has a vague name and is not used where it possibly could be. In order to make it more transparent and useful, it was decided to refactor it to make it closer to the similar one in glibc: TEMP_FAILURE_RETRY(). Now, macro evaluates into an expression and is named RETRY_ON_EINTR(). All the places where RETRY_ON_EINTR() macro code be applied were covered. Nikita Ivanov (2): Refactoring: refactor TFR() macro to RETRY_ON_EINTR() error handling: Use RETRY_ON_EINTR() macro where applicable block/file-posix.c| 37 - chardev/char-fd.c | 2 +- chardev/char-pipe.c | 8 +--- chardev/char-pty.c| 4 +--- hw/9pfs/9p-local.c| 8 ++-- include/qemu/osdep.h | 8 +++- net/l2tpv3.c | 17 + net/socket.c | 16 +++- 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 | 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 ++-- 20 files changed, 73 insertions(+), 101 deletions(-) -- 2.37.3
Re: [PATCH v2 2/2] error handling: Use RETRY_ON_EINTR() macro where applicable
Hi! Execuse me, my fault. Overlooked TFR occurrences in second patch. I will correct it. ср, 12 окт. 2022 г., 18:43 Christian Schoenebeck : > On Mittwoch, 12. Oktober 2022 17:17:46 CEST Bin Meng wrote: > > Hi, > > > > On Wed, Oct 12, 2022 at 8:32 PM Nikita Ivanov > wrote: > > > There is a defined RETRY_ON_EINTR() 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 > > > --- > > > > > > block/file-posix.c| 37 - > > > chardev/char-pty.c| 4 +--- > > > hw/9pfs/9p-local.c| 8 ++-- > > > net/l2tpv3.c | 17 + > > > net/socket.c | 16 +++- > > > net/tap.c | 12 > > > 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, 52 insertions(+), 88 deletions(-) > > > > This patch has to be squashed into patch 1 for bisectability, as TFR > > is already removed in patch 1. > > They are intentionally separated: 1st patch replaces occurrences of TFR, > whereas 2nd patch introduces use of macro at locations where not used yet. > > Nikita, could you please move those 2 hunks that still had TFR() > occurrence to > patch 1? > > And please use git's --thread option next time, so that individual patch > emails are linked to cover letter email (which adds appropriate > `References:` > and `In-Reply-To:` email headers). > > Best regards, > Christian Schoenebeck > > >
[PATCH v2 2/2] error handling: Use RETRY_ON_EINTR() macro where applicable
There is a defined RETRY_ON_EINTR() 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
---
block/file-posix.c| 37 -
chardev/char-pty.c| 4 +---
hw/9pfs/9p-local.c| 8 ++--
net/l2tpv3.c | 17 +
net/socket.c | 16 +++-
net/tap.c | 12
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, 52 insertions(+), 88 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 66fdb07820..c589cb489b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1238,9 +1238,7 @@ 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);
+ret = RETRY_ON_EINTR(read(sysfd, buf, sizeof(buf) - 1));
if (ret < 0) {
ret = -errno;
goto out;
@@ -1388,9 +1386,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);
+ret = RETRY_ON_EINTR(
+ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf)
+);
if (ret == -1) {
return -errno;
}
@@ -1472,18 +1470,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);
+len = RETRY_ON_EINTR(
+(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 +1905,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);
+n = RETRY_ON_EINTR(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..92fd33c854 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(, 1, 0);
-} while (rc == -1 && errno == EINTR);
+rc = RETRY_ON_EINTR(g_poll(, 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..bb3187244f 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);
+tsize = RETRY_ON_EINTR(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);
+write_size = RETRY_ON_EINTR(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..e0726f5f
[PATCH v2 1/2] Refactoring: refactor TFR() macro to RETRY_ON_EINTR()
Rename macro name to more transparent one and refactor
it to expression.
Signed-off-by: Nikita Ivanov
---
chardev/char-fd.c| 2 +-
chardev/char-pipe.c | 8 +---
include/qemu/osdep.h | 8 +++-
net/tap-bsd.c| 6 +++---
net/tap-linux.c | 2 +-
net/tap-solaris.c| 8
os-posix.c | 2 +-
7 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index cf78454841..d2c4923359 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 = RETRY_ON_EINTR(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..5ad30bcc59 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -131,8 +131,8 @@ 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 = RETRY_ON_EINTR(qemu_open_old(filename_in, O_RDWR | O_BINARY));
+fd_out = RETRY_ON_EINTR(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 +142,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 = RETRY_ON_EINTR(
+qemu_open_old(filename, O_RDWR | O_BINARY)
+);
if (fd_in < 0) {
error_setg_file_open(errp, errno, filename);
return;
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index b1c161c035..a470905475 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 RETRY_ON_EINTR(expr) \
+(__extension__ \
+({ typeof(expr) __result; \
+ do { \
+__result = (typeof(expr)) (expr); \
+ } 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/tap-bsd.c b/net/tap-bsd.c
index 005ce05c6e..4c98fdd337 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 = RETRY_ON_EINTR(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 = RETRY_ON_EINTR(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 = RETRY_ON_EINTR(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..f54f308d35 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 = RETRY_ON_EINTR(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..38e15028bf 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 = RETRY_ON_EINTR(open("/dev/udp", O_RDWR, 0));
if (ip_fd < 0) {
erro
[PATCH v2 0/2] Refactoring: expand usage of TFR() macro
At the moment, TFR() macro has a vague name and is not used where it possibly could be. In order to make it more transparent and useful, it was decided to refactor it to make it closer to the similar one in glibc: TEMP_FAILURE_RETRY(). Now, macro evaluates into an expression and is named RETRY_ON_EINTR(). All the places where RETRY_ON_EINTR() macro code be applied were covered. Nikita Ivanov (2): Refactoring: refactor TFR() macro to RETRY_ON_EINTR() error handling: Use RETRY_ON_EINTR() macro where applicable block/file-posix.c| 37 - chardev/char-fd.c | 2 +- chardev/char-pipe.c | 8 +--- chardev/char-pty.c| 4 +--- hw/9pfs/9p-local.c| 8 ++-- include/qemu/osdep.h | 8 +++- net/l2tpv3.c | 17 + net/socket.c | 16 +++- net/tap-bsd.c | 6 +++--- net/tap-linux.c | 2 +- net/tap-solaris.c | 8 net/tap.c | 12 os-posix.c| 2 +- 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 ++-- 20 files changed, 74 insertions(+), 102 deletions(-) -- 2.37.3
Re: [PATCH] error handling: Use TFR() macro where applicable
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
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
Re: [PATCH] error handling: Use TFR() macro where applicable
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
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
---
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(, 1, 0);
-} while (rc == -1 && errno == EINTR);
+TFR(rc = g_poll(, 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 = writ
Re: [PATCH] error handling: Use TFR() macro where applicable
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 wrote: > On Wed, 17 Aug 2022 at 15:06, Nikita Ivanov > 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
Re: [PATCH] error handling: Use TFR() macro where applicable
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 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
> 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
> ---
> 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) {
>
Re: [PATCH] error handling: Use TFR() macro where applicable
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
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
---
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(, 1, 0));
+rc = TEMP_FAILURE_RETRY(g_poll(, 1, 0));
assert(rc >= 0);
if
Re: [PATCH] error handling: Use TFR() macro where applicable
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
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
---
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(, 1, 0);
-} while (rc == -1 && errno == EINTR);
+TFR(rc = g_poll(, 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_s
Re: [PATCH] error handling: Use TFR() macro where applicable
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 wrote:
> Nikita Ivanov 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
> > 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
> > ---
> > 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)
> &
Re: [PATCH] error handling: Use TFR() macro where applicable
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
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
---
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, , 0));
+TEMP_FAILURE_RETRY(ret = sendmsg(s->fd, , 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 =
[PATCH] error handling: Use TFR() macro where applicable
>From 0ceb04ada1ed5a863914f4449469d7572d3443ed Mon Sep 17 00:00:00 2001
From: Nikita Ivanov
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
---
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, , 0);
-} while ((ret == -1) && (errno == EINTR));
+TFR(ret = sendmsg(s->fd, , 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, , 0);
-} while ((ret == -1) && (errno == EINTR));
+TFR(ret = sendmsg(s->fd, , 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, , sizeof(info));
-} while (len == -1 && errno == EINTR);
+TFR(len = read(fd, , sizeof(info)));
if (len == -1 && errno == EAGAIN) {
