Re: [PATCH] error handling: Use TFR() macro where applicable
On Mon, 10 Oct 2022 at 09:34, Nikita Ivanov 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 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
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
On Fri, 7 Oct 2022 at 12:44, Nikita Ivanov 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
> 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
> 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
> 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
> 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
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
> 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 =
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 = write(fd, (void *)oldpath, oldpath_size);
-} while (write_size == -1 && errno == EINTR);
+TFR(write_size = write(fd, (void *)oldpath, oldpath_size));
Re: [PATCH] error handling: Use TFR() macro where applicable
On Thu, 18 Aug 2022 at 15:07, Christian Schoenebeck wrote: > > On Mittwoch, 17. August 2022 17:55:24 CEST Peter Maydell wrote: > > On Wed, 17 Aug 2022 at 15:49, Nikita Ivanov 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
On Mittwoch, 17. August 2022 17:55:24 CEST Peter Maydell wrote: > On Wed, 17 Aug 2022 at 15:49, Nikita Ivanov 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
On Wed, 17 Aug 2022 at 15:49, Nikita Ivanov 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
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
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
Re: [PATCH] error handling: Use TFR() macro where applicable
_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 *), )
> +ret = TEMP_FAILURE_RETRY(
> +accept(sock, (struct sockaddr *), )
> );
> 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, >wstatus, 0));
> +pid = TEMP_FAILURE_RETRY(waitpid(s->qemu_pid, >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 *), sizeof(addr))
> +ret = TEMP_FAILURE_RETRY(
> +bind(sock, (struct sockaddr *), 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, , 0));
> +ret = TEMP_FAILURE_RETRY(
> +recvmsg(vubr->backend_udp_sock, , 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, , sizeof(info)));
> +len = TEMP_FAILURE_RETRY(read(fd, , 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, ));
> +ret = TEMP_FAILURE_RETRY(fcntl(fd, fcntl_op
Re: [PATCH] error handling: Use TFR() macro where applicable
t.c
@@ -102,8 +102,8 @@ static int socket_accept(int sock)
}
addrlen = sizeof(addr);
-TFR(
-ret = accept(sock, (struct sockaddr *), )
+ret = TEMP_FAILURE_RETRY(
+accept(sock, (struct sockaddr *), )
);
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, >wstatus, 0));
+pid = TEMP_FAILURE_RETRY(waitpid(s->qemu_pid, >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 *), sizeof(addr))
+ret = TEMP_FAILURE_RETRY(
+bind(sock, (struct sockaddr *), 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, , 0));
+ret = TEMP_FAILURE_RETRY(
+recvmsg(vubr->backend_udp_sock, , 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, , sizeof(info)));
+len = TEMP_FAILURE_RETRY(read(fd, , 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, ));
+ret = TEMP_FAILURE_RETRY(fcntl(fd, fcntl_op_setlk, ));
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 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
> 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/osd
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
Christian Schoenebeck 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 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
[...]
>> > > 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
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 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.
>
> 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...@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)
>
Re: [PATCH] error handling: Use TFR() macro where applicable
On Montag, 8. August 2022 10:05:56 CEST 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.
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...@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")
> >
>
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)
> > +);
> > 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
Re: [PATCH] error handling: Use TFR() macro where applicable
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)
> +);
> 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.
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 = sendmsg(s->fd, , 0));
+TEMP_FAILURE_RETRY(ret = sendmsg(s->fd, , 0));
if
Re: [PATCH] error handling: Use TFR() macro where applicable
On Fri, 5 Aug 2022 at 12:27, Marc-André Lureau wrote: > On Fri, Aug 5, 2022 at 3:11 PM Christian Schoenebeck > wrote: > > I was thinking the same as Marc-André before: > > > > commit 1dacd88ddcf33eb6ed044c4080e3ef5e3de4b6b6 > > Author: Marc-André Lureau > > 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 > > Message-Id: <20220323155743.1585078-17-marcandre.lur...@redhat.com> > > Signed-off-by: Paolo Bonzini > > > > 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
Hi
On Fri, Aug 5, 2022 at 3:11 PM Christian Schoenebeck
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
> > 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(-)
>
> I was thinking the same as Marc-André before:
>
> commit 1dacd88ddcf33eb6ed044c4080e3ef5e3de4b6b6
> Author: Marc-André Lureau
> 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
> Message-Id: <20220323155743.1585078-17-marcandre.lur...@redhat.com>
> Signed-off-by: Paolo Bonzini
>
> 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, , 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,
> &
Re: [PATCH] error handling: Use TFR() macro where applicable
On Donnerstag, 4. August 2022 09:25:17 CEST Nikita Ivanov wrote:
> 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(-)
I was thinking the same as Marc-André before:
commit 1dacd88ddcf33eb6ed044c4080e3ef5e3de4b6b6
Author: Marc-André Lureau
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
Message-Id: <20220323155743.1585078-17-marcandre.lur...@redhat.com>
Signed-off-by: Paolo Bonzini
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, , 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, t
[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) {
