Re: [Libguestfs] [libnbd PATCH v3 07/29] lib/utils: add async-signal-safe assert()
On Mon, Feb 20, 2023 at 07:21:05PM +0100, Laszlo Ersek wrote:
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.
Oh, I was confused. I went back and re-read your macro; I guess I was
thinking it was something like:
do if (!(expression)) nbd_internal_fork_safe_assert (args); while (0)
where the function is only called on failure, but you instead wrote it
as:
nbd_internal_fork_safe_assert ((expression) != 0), args)
So I stand corrected - we do need the early exit here, because we are
unconditionally calling the function (when assertions are enabled) at
least in the current spelling of the macro.
...
> 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!
My R-b still stands, and I think you cleared up my confusion without
having to change any code on your part.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org