Remove assert() instances from libkmod

[evelikov]

As mentioned in #255, ideally we shouldn't be having assert() in a shared library.

This series removes them, leaving the ones in depmod as-is. Most instances are unreachable and error handling is added otherwise. Where possible, call sites are annotated as _nonnull_ so the compiler flags future issues.

As a final cleanup, our assert_cc macro becomes a wrapper around the C11 static_assert (should we nuke our wrapper?) and header inclusions are purged.

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
libkmod/libkmod-module.c	25.00%	1 Missing and 5 partials ⚠️
testsuite/init_module.c	42.85%	2 Missing and 2 partials ⚠️

Files with missing lines	Coverage Δ
libkmod/libkmod-elf.c	51.50% <ø> (ø)
libkmod/libkmod-index.c	56.08% <ø> (ø)
libkmod/libkmod.c	50.60% <ø> (ø)
shared/array.c	91.66% <ø> (ø)
shared/macro.h	100.00% <ø> (ø)
shared/strbuf.c	87.03% <ø> (ø)
shared/strbuf.h	75.00% <ø> (ø)
shared/util.c	65.84% <ø> (ø)
testsuite/delete_module.c	70.47% <ø> (ø)
testsuite/path.c	43.58% <ø> (ø)
... and 4 more

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[evelikov]

As an alternative, we could keep the assert() that are used to track internal misuse and where _nonnull_ is not sufficient.

In that case, we should probably check:

• fail for -D build-tests=true and -DNDEBUG is set (by meson or otherwise), and/or
• fail for -D build-tests=false and -UNDEBUG

Although in that case, any distribution which wants to build and run tests against the same build will be stuffed.
So I'm sort of leaning that purging them all is the better option.

[lucasdemarchi]

mixed feelings with these assert() removals. They were actually added for programming mistakes. Not for input errors handling. For this case, the input control is in do_init_module().

The assert here is because the function only handles KMOD_INSERT_FORCE_MODVERSION and KMOD_INSERT_FORCE_VERMAGIC. If we passed flags that doesn't contain those, it's not the end of the world: the function will continue to do its job, but it's likely wrong: it would mean we probably added another flag and forgot to add its handling in this function. By removing it we are making it silent rather than simply crash the program/test during development.

So IMO the statement "we shouldn't have asserts in a shared library" is wrong. It shouldn't be used for input control or error handling, but it's still useful to catch program programming mistakes.

[lucasdemarchi]

And true, some of them shouldn't be there and we should add the error handling, but I will need to review case by case.

[evelikov]

Would it make sense to keep some for the short run, until we get more extensive tests to cover the code-paths, or you prefer to keep them even in that case?

As a whole, it feels slightly awkward having them available (potentially reachable) in distribution builds. Perhaps they can be "enabled" with debug-messages or another meson toggle?

Fwiw most distributions are not* be using debug-messages these days.

• Fedora should stop using it any day now. While Gentoo/Yocto have it accessible with their modular build setups.

[lucasdemarchi]

For enabling/disabling maybe we should have this instead?

diff --git a/meson.build b/meson.build
index 5740e13..75343ed 100644
--- a/meson.build
+++ b/meson.build
@@ -7,6 +7,7 @@ project(
   default_options : [
     'c_std=gnu11',
     'b_pie=true',
+    'b_ndebug=if-release',
     'warning_level=2',
     'prefix=/usr',
     'sysconfdir=/etc',

... to be merged after the wrong asserts are removed, so we still pass the testsuite tests.

Distros can also control it with -Db_ndebug=...

[evelikov]

If I'm correctly parsing the meson documentation this is the default already. The issue is that due to $reasons, at least some distributions are building with an explicit --build-type plain.

At a glance we can easily swap the assert(static literal...) checks for assert_cc()` as below. It seems like the better option IMHO since it's a compile-time check. What do you think?

diff --git a/shared/array.c b/shared/array.c
index 85f8955e..da40d36e 100644
--- a/shared/array.c
+++ b/shared/array.c
@@ -36,7 +36,7 @@ static void array_trim(struct array *array)
 	}
 }
 
-void array_init(struct array *array, size_t step)
+void __array_init(struct array *array, size_t step)
 {
 	array->array = NULL;
 	array->count = 0;
diff --git a/shared/array.h b/shared/array.h
index 8facc52d..1cdcaed0 100644
--- a/shared/array.h
+++ b/shared/array.h
@@ -13,7 +13,10 @@ struct array {
 	size_t step;
 };
 
-void array_init(struct array *array, size_t step);
+void __array_init(struct array *array, size_t step);
+#define array_init(a, s) \
+	assert_cc(s > 0); \
+	__array_init(a, s);
 int array_append(struct array *array, const void *element);
 int array_append_unique(struct array *array, const void *element);
 void array_pop(struct array *array);