12:21 p.m.
On 2/15/23 21:57, Eric Blake wrote:
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(a)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,
I either don't understand, or I disagree. With NDEBUG not defined, the
NBD_INTERNAL_FORK_SAFE_ASSERT macro is always expanded to a call to
nbd_internal_fork_safe_assert(). And the expression to check is
evaluated in the first argument of that function call:
nbd_internal_fork_safe_assert ((expression) != 0, ...
So I think the early return is definitely necessary here.
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.
Any single buffer presents the problem of sizing the buffer
appropriately, which we can't do in this context :)
> + 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.
Personally I dislike `', and like '' (and ""), but I specifically
modeled the message on the standard assert() failure message on RHEL9.
> + 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.
What I did was, I checked POSIX on assert(), and the spec there simply
says that assert() calls abort():
"When it is executed, if expression (which shall have a scalar type) is
false (that is, compares equal to 0), assert() shall write information
about the particular call that failed on stderr and shall call abort()."
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(a)redhat.com>
Thanks!
Laszlo