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 <ler...@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 <ebl...@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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
