On 2/15/23 22:10, Eric Blake wrote:
> On Wed, Feb 15, 2023 at 03:11:37PM +0100, Laszlo Ersek wrote:
>> Don't try to test async-signal-safety, only that
>> NBD_INTERNAL_FORK_SAFE_ASSERT() works similarly to assert():
>>
>> - it prints diagnostics to stderr,
>>
>> - it calls abort().
>>
>> Some unfortunate gymnastics are necessary to avoid littering the system
>> with unwanted core dumps.
>
> Wow - impressive work for a unit test.
>
>>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>> configure.ac | 5 ++
>> lib/test-fork-safe-assert.c | 63 ++++++++++++++++++++
>> lib/Makefile.am | 38 ++++++++++--
>> lib/test-fork-safe-assert.sh | 31 ++++++++++
>> .gitignore | 2 +
>> 5 files changed, 135 insertions(+), 4 deletions(-)
>>
>
>> +++ b/lib/test-fork-safe-assert.c
>
>> +
>> +#include <config.h>
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#ifdef HAVE_PRCTL
>> +#include <sys/prctl.h>
>> +#endif
>> +#include <sys/resource.h>
>> +
>> +#undef NDEBUG
>> +
>> +#include "internal.h"
>> +
>> +#define TRUE 1
>> +#define FALSE 0
>
> Any reason for defining these, other than to test that our macro
> properly stringifies them into the resulting assertion text?
>
> /me reads ahead
>
> Yep, your .sh script does check that a literal "`FALSE'" is part of
> the expected output string. A comment here might be worthwhile?
Right, I'll add a comment.
>
>> +
>> +int
>> +main (void)
>> +{
>> + struct rlimit rlimit;
>> +
>> + /* The standard approach for disabling core dumping. Has no effect on
>> Linux if
>> + * /proc/sys/kernel/core_pattern starts with a pipe (|) symbol.
>> + */
>> + if (getrlimit (RLIMIT_CORE, &rlimit) == -1) {
>> + perror ("getrlimit");
>> + return EXIT_FAILURE;
>> + }
>> + rlimit.rlim_cur = 0;
>> + if (setrlimit (RLIMIT_CORE, &rlimit) == -1) {
>> + perror ("setrlimit");
>> + return EXIT_FAILURE;
>> + }
>> +
>> +#ifdef HAVE_PRCTL
>> + if (prctl (PR_SET_DUMPABLE, 0, 0, 0, 0) == -1) {
>> + perror ("prctl");
>> + return EXIT_FAILURE;
>
> Would 'return 77' (ie 'skipped test', in automake terminology) be
> better if we can't pre-set the environment to our liking? [1]
My thinking went like this: on Linux, we'll surely have prctl(), so we
expect it to work. On OpenBSD and FreeBSD (the other two platforms we
support), we might not have prctl(), but on those platforms, I actually
expect setrlimit() to suffice for disabling core dumps. So I don't see a
case where we should skip the test.
>
>> + }
>> +#endif
>> +
>> + NBD_INTERNAL_FORK_SAFE_ASSERT (TRUE);
>> + NBD_INTERNAL_FORK_SAFE_ASSERT (FALSE);
>> + return 0;
>
>> +++ b/lib/test-fork-safe-assert.sh
>> @@ -0,0 +1,31 @@
>
>> +
>> +set +e
>> +
>> +./test-fork-safe-assert 2>test-fork-safe-assert.err
>> +exit_status=$?
>> +
>> +set -e
>> +
>> +test $exit_status -gt 128
>
> [1] Hmm - if you do return 77 when we can't avoid the core dump, this
> would need to forward that on.
>
>> +signal_name=$(kill -l $exit_status)
>> +test "x$signal_name" = xABRT || test "x$signal_name" = xSIGABRT
>> +
>> +ptrn="^test-fork-safe-assert\\.c:[0-9]+: main: Assertion \`FALSE'
>> failed\\.\$"
>
> Would it be any easier writing ptrn with '' instead of "" shell quoting?
I considered that, but it doesn't do the whole job: there's an
apostrophe in the expected output, so minimally I'd have to write
ptrn='that'\''s still messy'
and then I figured double quotes are just more "consequent".
>
> Do we also want to test that the pattern "`TRUE'" is NOT present in
> the output file (that is, our passing assertion does not generate
> unexpected output)?
For TRUE to be present together with FALSE, the
NBD_INTERNAL_FORK_SAFE_ASSERT (TRUE) macro invocation in the C code
would have to produce output and not abort() at the same time. I thought
that that didn't need checking (seemed too outlandish), but yes, I can
add a "grep -v" too.
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
>
Thanks!
Laszlo
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
