2:21 a.m.
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 <lersek(a)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 <eblake(a)redhat.com>
Thanks!
Laszlo