Re: [PATCH v2 1/2] Refactoring: refactor TFR() macro to RETRY_ON_EINTR()

2022-10-13 Thread Markus Armbruster 
Christian Schoenebeck  writes:

> On Mittwoch, 12. Oktober 2022 14:28:23 CEST Nikita Ivanov wrote:
>> Rename macro name to more transparent one and refactor
>> it to expression.
>> 
>> Signed-off-by: Nikita Ivanov 

[...]

>> 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); \
>
> Not a big deal, but as Peter already pointed out in previous version: you 
> could drop the type cast in this particular form here.

Yes, please.

> glibc's TEMP_FAILURE_RETRY() version needs the cast as it uses `long int` as 
> hard coded type for the result variable, whereas this version here uses a 
> generic approach by declaring the result variable already exactly with the 
> type the passed expression evaluates to, so the cast is redundant in this 
> version here.
>
>> +   } while (__result == -1L && errno == EINTR); \
>> +   __result; }))
>> 
>>  /* time_t may be either 32 or 64 bits depending on the host OS, and
>>   * can be either signed or unsigned, so we can't just hardcode a

[...]

Re: [PATCH v2 1/2] Refactoring: refactor TFR() macro to RETRY_ON_EINTR()

2022-10-12 Thread Christian Schoenebeck 
On Mittwoch, 12. Oktober 2022 14:28:23 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 
>  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); \

Not a big deal, but as Peter already pointed out in previous version: you 
could drop the type cast in this particular form here.

glibc's TEMP_FAILURE_RETRY() version needs the cast as it uses `long int` as 
hard coded type for the result variable, whereas this version here uses a 
generic approach by declaring the result variable already exactly with the 
type the passed expression evaluates to, so the cast is redundant in this 
version here.

> +   } while (__result == -1L && errno == EINTR); \
> +   __result; }))
> 
>  /* time_t may be either 32 or 64 bits depending on the host OS, and
>   * can be either signed or unsigned, so we can't just hardcode a
> diff --git a/net/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 =

Re: [PATCH v2 1/2] Refactoring: refactor TFR() macro to RETRY_ON_EINTR()

2022-10-12 Thread Marc-André Lureau 
On Wed, Oct 12, 2022 at 4:29 PM Nikita Ivanov 
wrote:

> Rename macro name to more transparent one and refactor
> it to expression.
>
> Signed-off-by: Nikita Ivanov 
>

Reviewed-by: Marc-André Lureau 



> ---
>  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,

[PATCH v2 1/2] Refactoring: refactor TFR() macro to RETRY_ON_EINTR()

2022-10-12 Thread Nikita Ivanov 
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) {
 error_setg(errp, "Can't open /dev/ip (actually /dev/udp)");
 return -1;
 }

-TFR(tap_fd = open("/dev/tap", O_RDWR, 0));
+tap_fd =