Libguestfs - Feb 2023 - [libnbd PATCH v3 07/29] lib/utils: add async-signal-safe assert()

Add an assert() variant that we may call between fork() and exec*().

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    context:-U13

 lib/internal.h | 13 ++++++++++++
 lib/utils.c    | 22 ++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/lib/internal.h b/lib/internal.h
index 0b5f793011b8..9c9021ed366e 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -517,14 +517,27 @@ extern char *nbd_internal_printable_string (const char
*str)
 extern char *nbd_internal_printable_string_list (char **list)
   LIBNBD_ATTRIBUTE_ALLOC_DEALLOC (free);
 
 /* These are wrappers around socket(2) and socketpair(2).  They
  * always set SOCK_CLOEXEC.  nbd_internal_socket can set SOCK_NONBLOCK
  * according to the nonblock parameter.
  */
 extern int nbd_internal_socket (int domain, int type, int protocol,
                                 bool nonblock);
 extern int nbd_internal_socketpair (int domain, int type, int protocol,
                                     int *fds)
   LIBNBD_ATTRIBUTE_NONNULL (4);
 
+extern void nbd_internal_fork_safe_assert (int result, const char *file,
+                                           long line, const char *func,
+                                           const char *assertion)
+  LIBNBD_ATTRIBUTE_NONNULL (2, 4, 5);
+
+#ifdef NDEBUG
+#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression) ((void)0)
+#else
+#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression)                        \
+  (nbd_internal_fork_safe_assert ((expression) != 0, __FILE__, __LINE__, \
+                                  __func__, #expression))
+#endif
+
 #endif /* LIBNBD_INTERNAL_H */
diff --git a/lib/utils.c b/lib/utils.c
index 7fb16f6402d1..5dd00f82e073 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -431,13 +431,35 @@ nbd_internal_socketpair (int domain, int type, int
protocol, int *fds)
   if (ret == 0) {
     for (i = 0; i < 2; i++) {
       if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
         close (fds[0]);
         close (fds[1]);
         return -1;
       }
     }
   }
 #endif
 
   return ret;
 }
+
+void
+nbd_internal_fork_safe_assert (int result, const char *file, long line,
+                               const char *func, const char *assertion)
+{
+  const char *line_out;
+  char line_buf[32];
+
+  if (result)
+    return;
+
+  xwrite (STDERR_FILENO, file, strlen (file));
+  xwrite (STDERR_FILENO, ":", 1);
+  line_out = nbd_internal_fork_safe_itoa (line, line_buf, sizeof line_buf);
+  xwrite (STDERR_FILENO, line_out, strlen (line_out));
+  xwrite (STDERR_FILENO, ": ", 2);
+  xwrite (STDERR_FILENO, func, strlen (func));
+  xwrite (STDERR_FILENO, ": Assertion `", 13);
+  xwrite (STDERR_FILENO, assertion, strlen (assertion));
+  xwrite (STDERR_FILENO, "' failed.\n", 10);
+  abort ();
+}

On Wed, Feb 15, 2023 at 03:11:36PM +0100, Laszlo Ersek
wrote:
> Add an assert() variant that we may call between fork() and exec*().
> 
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
> 
> +++ b/lib/internal.h
> +
> +#ifdef NDEBUG
> +#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression) ((void)0)
May be affected by outcome of side discussion on 01/29 about whether
we want space after cast.
> +#else
> +#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression)                        \
> +  (nbd_internal_fork_safe_assert ((expression) != 0, __FILE__, __LINE__, \
> +                                  __func__, #expression))
> +#endif
> +
>  #endif /* LIBNBD_INTERNAL_H */
> +++ b/lib/utils.c
> @@ -431,13 +431,35 @@ nbd_internal_socketpair (int domain, int type, int
protocol, int *fds)
>    if (ret == 0) {
>      for (i = 0; i < 2; i++) {
>        if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
>          close (fds[0]);
>          close (fds[1]);
>          return -1;
>        }
>      }
>    }
>  #endif
>  
>    return ret;
>  }
> +
> +void
> +nbd_internal_fork_safe_assert (int result, const char *file, long line,
> +                               const char *func, const char *assertion)
> +{
> +  const char *line_out;
> +  char line_buf[32];
> +
> +  if (result)
> +    return;
Since no code should ever directly call
nbd_internal_fork_safe_assert(0,...), but instead go through our macro
that has already filtered on expression's value, this conditional is
technically dead code; but I'm okay whether it stays in or gets
removed.
> +
> +  xwrite (STDERR_FILENO, file, strlen (file));
> +  xwrite (STDERR_FILENO, ":", 1);
Presumably, if our first best-effort xwrite() fails to produce full
output, all later xwrite() will hopefully hit the same error condition
so that we aren't producing even more-mangled output where later
syscalls succeed despite missing context earlier in the overall
output.  If it were something we truly wanted to worry about, the
solution would be pre-loading the entire message into a single buffer,
then calling xwrite() just once - but that's far more effort for
something we don't anticipate hitting in normal usage anyways.  I'm
happy if you ignore this whole paragraph of mine.
> +  line_out = nbd_internal_fork_safe_itoa (line, line_buf, sizeof
line_buf);
> +  xwrite (STDERR_FILENO, line_out, strlen (line_out));
> +  xwrite (STDERR_FILENO, ": ", 2);
> +  xwrite (STDERR_FILENO, func, strlen (func));
> +  xwrite (STDERR_FILENO, ": Assertion `", 13);
> +  xwrite (STDERR_FILENO, assertion, strlen (assertion));
> +  xwrite (STDERR_FILENO, "' failed.\n", 10);
These days, the quoting style `' seems to be disappearing in favor of
''.  But a quick test showed me that at least glibc 2.36 is still
producing the message with the quotes as you have spelled it here.
> +  abort ();
Huh. I checked the source to glibc's assert.c, where it includes a
comment about having to work cleanly even if a second assertion ends
up being raised from within a SIGABRT handler active during the first
attempt to abort().  But POSIX says that abort() shall attempt to
override any SIGABRT handler in place, so I wonder if glibc's
implementation is a bit too defensive.  At any rate, your
implementation looks reasonable, and more to the point, satisfies your
desire of being async-signal-safe and thus appropriate between fork()
and exec().

Reviewed-by: Eric Blake <eblake at redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org