On Fri, 7 Oct 2022 at 12:44, Nikita Ivanov <niva...@cloudlinux.com> wrote: > > Hi! > Sorry for such a long absence, I've been resolving some other issues in my > life for a while. I've adjusted the patch according to your latest comments. > Could you check it out, please?
Hi; thanks for coming back to this. (I'd been meaning to re-read the thread but hadn't found time to do so; sorry.) As Christian says, you should send the patches as a proper new patchset thread of their own, but for the moment: > From 5389c5ccc8789f8f666ab99e50d38af728bd2c9c Mon Sep 17 00:00:00 2001 > From: Nikita Ivanov <niva...@cloudlinux.com> > Date: Wed, 3 Aug 2022 12:54:00 +0300 > Subject: [PATCH 1/2] error handling: Use TFR() macro where applicable > > There is a defined TFR() macro in qemu/osdep.h which > handles the same while loop. > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/415 > > Signed-off-by: Nikita Ivanov <niva...@cloudlinux.com> > diff --git a/block/file-posix.c b/block/file-posix.c > index 66fdb07820..7892bdea31 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1238,9 +1238,9 @@ static int hdev_get_max_segments(int fd, struct stat > *st) > ret = -errno; > goto out; > } > - do { > - ret = read(sysfd, buf, sizeof(buf) - 1); > - } while (ret == -1 && errno == EINTR); > + TFR( > + ret = read(sysfd, buf, sizeof(buf) - 1) > + ); I think this patch is doing things in the wrong order. Instead of converting code to use the old macro that we don't like and then updating it again in patch 2 to use the new macro, we should first introduce the new macro, and then after that we can update code that's currently not using a macro at all to use the new one. This makes code review easier because we don't have to look at a change to this code which is then going to be rewritten anyway. > From 7a9fccf00ec2d1c6b30b2ed1cb98398b49ddb0bc Mon Sep 17 00:00:00 2001 > From: Nikita Ivanov <niva...@cloudlinux.com> > Date: Mon, 8 Aug 2022 20:43:45 +0300 > Subject: [PATCH 2/2] Refactoring: rename TFR() to TEMP_FAILURE_RETRY() > > glibc's unistd.h header provides the same macro with the > subtle difference in type casting. Adjust macro name to the > common standard and refactor it to expression. > > Signed-off-by: Nikita Ivanov <niva...@cloudlinux.com> > diff --git a/block/file-posix.c b/block/file-posix.c > index 7892bdea31..ee7f60c78a 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1238,8 +1238,8 @@ static int hdev_get_max_segments(int fd, struct stat > *st) > ret = -errno; > goto out; > } > - TFR( > - ret = read(sysfd, buf, sizeof(buf) - 1) > + ret = TEMP_FAILURE_RETRY( > + read(sysfd, buf, sizeof(buf) - 1) > ); This doesn't need these newlines in it. If the whole thing fits on a single line, that's easier to read. > if (ret < 0) { > ret = -errno; > @@ -1472,8 +1472,8 @@ static ssize_t handle_aiocb_rw_vector(RawPosixAIOData > *aiocb) > { > ssize_t len; > > - TFR( > - len = (aiocb->aio_type & QEMU_AIO_WRITE) ? > + len = TEMP_FAILURE_RETRY( > + (aiocb->aio_type & QEMU_AIO_WRITE) ? > qemu_pwritev(aiocb->aio_fildes, > aiocb->io.iov, > aiocb->io.niov, I'm not sure why you've put the TEMP_FAILURE_RETRY on the outside here rather than just on the individual function calls. > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index b1c161c035..6e244f15fa 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -243,7 +243,13 @@ void QEMU_ERROR("code path is reachable") > #define ESHUTDOWN 4099 > #endif > > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR) > +#define TEMP_FAILURE_RETRY(expr) \ We can't call the macro this, because the glibc system headers already may define a macro of that name, so the compiler will complain if they're both defined at the same time, and depending on header ordering it might not be clear which version you're getting. > + (__extension__ \ > + ({ typeof(int64_t) __result; \ As Christian says, the point of the typeof is to use the type of the expression. "typeof(int64_t)" is always just "int64_t". You want "typeof(expr) __result;". > + do { \ > + __result = (typeof(int64_t)) (expression); \ Then you don't need this cast, because both __result and expr are the same type anyway. Also, how did this compile? 'expression' isn't the name of the macro argument. > + } while (__result == -1L && errno == EINTR); \ I think you don't need the 'L' suffix here. > + __result; })) thanks -- PMM
