[Libguestfs] [libnbd PATCH v3 08/29] lib/utils: add unit test for async-signal-safe assert()

[Eric Blake]

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

> +
> +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]

> +  }
> +#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?

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)?

Reviewed-by: Eric Blake <eblake at redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org