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

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 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,
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 at redhat.com>
> 
Thanks!
Laszlo

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

On 2/20/23 19:21, 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:
>>> +
>>> +  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 :)
Actually, we *can* do better:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/writev.html

What we're doing here is the textbook use case for scatter-gather (well,
in this case, gather). It's strange that it has taken me one night to
realize this (it occurred to me before falling asleep last night), given
that I heavily used scatter-gather in other, not-so-old, code. (Namely
the edk2 virtiofs driver.)

I'll attempt to replace xwrite() with xwritev().

Laszlo