Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 08/29] lib/utils: add unit test for async-signal-safe assert()
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.
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(-)
diff --git a/configure.ac b/configure.ac
index 893e82ac6fea..3549188ec5fb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -129,11 +129,16 @@ dnl Check for various libc functions, all optional.
dnl
dnl posix_fadvise helps to optimise linear reads and writes.
dnl
+dnl When /proc/sys/kernel/core_pattern starts with a pipe (|) symbol, Linux
+dnl ignores "ulimit -c" and (equivalent) setrlimit(RLIMIT_CORE)
actions, for
+dnl disabling core dumping. Only prctl() can be used then, for that purpose.
+dnl
dnl strerrordesc_np (glibc only) is preferred over sys_errlist:
dnl https://lists.fedoraproject.org/archives/list/glibc at
lists.fedoraproject.org/thread/WJHGG2OO7ABNAYICGA5WQZ2Q34Q2FEHU/
AC_CHECK_FUNCS([\
posix_fadvise \
posix_memalign \
+ prctl \
strerrordesc_np \
valloc])
diff --git a/lib/test-fork-safe-assert.c b/lib/test-fork-safe-assert.c
new file mode 100644
index 000000000000..131cbaa0abca
--- /dev/null
+++ b/lib/test-fork-safe-assert.c
@@ -0,0 +1,63 @@
+/* nbd client library in userspace
+ * Copyright (C) 2013-2023 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#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
+
+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;
+ }
+#endif
+
+ NBD_INTERNAL_FORK_SAFE_ASSERT (TRUE);
+ NBD_INTERNAL_FORK_SAFE_ASSERT (FALSE);
+ return 0;
+}
diff --git a/lib/Makefile.am b/lib/Makefile.am
index ece50770326e..1f6501ceb49a 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -25,15 +25,30 @@ generator_built = \
unlocked.h \
$(NULL)
+CLEANFILES += \
+ test-fork-safe-assert.err \
+ $(NULL)
+
EXTRA_DIST = \
$(generator_built) \
libnbd.syms \
+ test-fork-safe-assert.sh \
$(NULL)
lib_LTLIBRARIES = libnbd.la
BUILT_SOURCES = $(generator_built)
+AM_CPPFLAGS = \
+ -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
+ -I$(top_srcdir)/common/utils \
+ $(NULL)
+
+AM_CFLAGS = \
+ $(WARNINGS_CFLAGS) \
+ $(NULL)
+
libnbd_la_SOURCES = \
aio.c \
api.c \
@@ -60,13 +75,11 @@ libnbd_la_SOURCES = \
utils.c \
$(NULL)
libnbd_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
- -I$(top_srcdir)/common/include \
- -I$(top_srcdir)/common/utils \
+ $(AM_CPPFLAGS) \
-Dsysconfdir=\"$(sysconfdir)\" \
$(NULL)
libnbd_la_CFLAGS = \
- $(WARNINGS_CFLAGS) \
+ $(AM_CFLAGS) \
$(PTHREAD_CFLAGS) \
$(GNUTLS_CFLAGS) \
$(LIBXML2_CFLAGS) \
@@ -86,3 +99,20 @@ libnbd_la_LDFLAGS = \
pkgconfigdir = $(libdir)/pkgconfig
pkgconfig_DATA = libnbd.pc
+
+# Unit tests.
+
+TESTS = \
+ test-fork-safe-assert.sh \
+ $(NULL)
+
+check_PROGRAMS = \
+ test-fork-safe-assert \
+ $(NULL)
+
+test_fork_safe_assert_SOURCES = \
+ $(top_srcdir)/common/utils/vector.c \
+ errors.c \
+ test-fork-safe-assert.c \
+ utils.c \
+ $(NULL)
diff --git a/lib/test-fork-safe-assert.sh b/lib/test-fork-safe-assert.sh
new file mode 100755
index 000000000000..88fc553b484d
--- /dev/null
+++ b/lib/test-fork-safe-assert.sh
@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+# nbd client library in userspace
+# Copyright (C) 2013-2023 Red Hat Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+set +e
+
+./test-fork-safe-assert 2>test-fork-safe-assert.err
+exit_status=$?
+
+set -e
+
+test $exit_status -gt 128
+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\\.\$"
+grep -E -q -- "$ptrn" test-fork-safe-assert.err
diff --git a/.gitignore b/.gitignore
index f42737131d8c..cd27a4ed7557 100644
--- a/.gitignore
+++ b/.gitignore
@@ -124,6 +124,8 @@ Makefile.in
/lib/states-run.c
/lib/states.c
/lib/states.h
+/lib/test-fork-safe-assert
+/lib/test-fork-safe-assert.err
/lib/unlocked.h
/libtool
/ltmain.sh
Eric Blake
2023-Feb-15 21:10 UTC
[Libguestfs] [libnbd PATCH v3 08/29] lib/utils: add unit test for async-signal-safe assert()
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 0Any 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