Older gcc versions don't like this. The _Pragam() itself is to workaround a
-Wnonnull-compare warning with gcc 14.
After all, we use compiler warning extensively. They are our linters and have
necessarily false positives. To make them usable across a wide range of
compilers, is a constant effort.
Here is another one.
The error:
./src/libnm-std-aux/nm-std-aux.h: In function ‘nm_utils_addr_family_other’:
./src/libnm-std-aux/nm-std-aux.h:230:36: error: expected expression before ‘#pragma’
230 | #define NM_PRAGMA_DIAGNOSTICS_PUSH _Pragma("GCC diagnostic push")
| ^~~~~~~
./src/libnm-std-aux/nm-std-aux.h:232:5: note: in expansion of macro ‘NM_PRAGMA_DIAGNOSTICS_PUSH’
232 | NM_PRAGMA_DIAGNOSTICS_PUSH _Pragma(_NM_PRAGMA_WARNING_DO(warning))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:291:9: note: in expansion of macro ‘NM_PRAGMA_WARNING_DISABLE’
291 | NM_PRAGMA_WARNING_DISABLE("-Wnonnull-compare"); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:953:9: note: in expansion of macro ‘nm_assert’
953 | nm_assert(true || NM_UNIQ_T(xx, uniq) == (x)); \
| ^~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:961:27: note: in expansion of macro ‘_NM_IN_SET’
961 | #define NM_IN_SET(x, ...) _NM_IN_SET(NM_UNIQ, ||, typeof(x), x, __VA_ARGS__)
| ^~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1493:15: note: in expansion of macro ‘NM_IN_SET’
1493 | nm_assert(NM_IN_SET((addr_family), NM_AF_INET, NM_AF_INET6))
| ^~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1502:9: note: in expansion of macro ‘nm_assert_addr_family’
1502 | nm_assert_addr_family(NM_UNIQ_T(_addr_family, uniq)); \
| ^~~~~~~~~~~~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1510:33: note: in expansion of macro ‘_NM_IS_IPv4’
1510 | #define NM_IS_IPv4(addr_family) _NM_IS_IPv4(NM_UNIQ, addr_family)
| ^~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1515:12: note: in expansion of macro ‘NM_IS_IPv4’
1515 | return NM_IS_IPv4(addr_family) ? NM_AF_INET6 : NM_AF_INET;
| ^~~~~~~~~~
Fixes: 62c1745f62 ('std-aux: suppress "-Wnonnull-compare" warning in nm_assert()')
(cherry picked from commit e4b154e1b0)
When we use a "static" array declarator to a function, we understand
and tell the compiler that the argument must not be NULL.
But now gcc-14.0.1-0.2.fc40 starts warning about NULL checks for
such arguments.
static void foo(char args[static 10]) {
nm_assert(args);
sprintf(args, "hi");
}
Granted, the compiler is right, and we know that this condition is not
supposed to be violated. A logical thing would be just to drop the
assertion.
Instead, suppress "-Wnonnull-compare" warnings inside a nm_assert(). An
nm_assert() is more than a run time check, it's an additional
self-documenting code of the invariants.
It's fine to assert for something that is true. Actually, all the
conditions that we assert against, hold. The compiler telling us that
the condition that we assert against is valid, is not useful.
(cherry picked from commit 62c1745f62)
Glib's MIN()/MAX() should not be used, in favor of NM_MIN()/NM_MAX().
That's because the NM variants
- evaluate arguments only once
- have a static assertion that the signedness of the arguments matches
However, previously those macros never evaluated to a compile time
constant. Unlike the glib variants, which do so when the arguments are
compile time constants. That is sometimes important when using the
macros in a context that requires a constant.
Extend NM_MIN()/NM_MAX() to be a compile time constant, when possible.
Note that there are still a few places where NM_MIN()/NM_MAX() cannot be
used due to the expression statement. For those cases, there is
NM_MIN_CONST()/NM_MAX_CONST().
Remove all the code that was added for the CSME coexistence.
The Intel WiFi team can't commit on when, if at all, this feature will
be completely integrated and tested in the NetworkManager.
The preferred solution for now is the solution that involves the kernel
only.
Remove the code that was merged so far.
We have nm_strerror_native_r(), which is the wrapper around strerror_r() that
we want to use in glib components (it also will ensure that the string is valid
UTF-8). However, it's not usable from non-glib components.
Move the part that abstracts strerror_r() out to libnm-std-aux as _nm_strerror_r().
The purpose is that non-glib componenent can use the thread-safe wrapper around
strerror_r().
The macros NM_MIN()/NM_MAX()/NM_CLAMP() use typeof() to accept any
integer type as argument. Internally, they rely on standard C integral
conversions of the <> operators and the ternary operator for evaluating
the comparison and the result(type).
That works mostly great. Except, comparing signed and unsigned values in
C leads to oddities and the caller should explicitly take care of that.
Add static assertions to check that the compared arguments have the same
signedness.
This is the version shipped in Fedora 38. As Fedora 38 is now out, the
core developers switch to it. Our gitlab-ci will also use that as base
image for the check-{patch.tree} tests and to generate the pages. There
is a need that everybody agrees on which clang-format version to use,
and that version should be the one of the currently used Fedora release.
Also update the used Fedora image in "contrib/scripts/nm-code-format-container.sh"
script.
The gitlab-ci still needs update in the following commit. This change
in isolation will break the "check-tree" test.
Having a list with only one element is often interesting to know. For
example, if you are about to unlink an element, you may want to check
whether afterwards the list is empty.
Add c_list_is_empty_or_single() for that. It is probably more efficient than
plain c_list_length_is(list, 1) and also a better name.
We also do that with g_return*() macros. These strings increase the
binary size for little use. Drop them, unless we build with
more asserts enabled.
This affects nm_assert() messages.
glibc defines __assert_fail as:
extern void __assert_fail (const char *__assertion, const char *__file,
unsigned int __line, const char *__function)
__THROW __attribute__ ((__noreturn__));
but musl as:
_Noreturn void __assert_fail (const char *, const char *, int, const char *);
(note the difference in the type for the line argument).
This cannot be made to work, unless we would detect the used type at configure
time, which seems too much effort.
Drop this again.
This reverts commit 1ce29e120b.
Fixes: 1ce29e120b ('std-aux: drop assertion and function name from assert() in release mode')
G_TYPE_CHECK_INSTANCE_CAST() can trigger a "-Wcast-align":
src/core/devices/nm-device-macvlan.c: In function 'parent_changed_notify':
/usr/include/glib-2.0/gobject/gtype.h:2421:42: error: cast increases required alignment of target type [-Werror=cast-align]
2421 | # define _G_TYPE_CIC(ip, gt, ct) ((ct*) ip)
| ^
/usr/include/glib-2.0/gobject/gtype.h:501:66: note: in expansion of macro '_G_TYPE_CIC'
501 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) (_G_TYPE_CIC ((instance), (g_type), c_type))
| ^~~~~~~~~~~
src/core/devices/nm-device-macvlan.h:13:6: note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST'
13 | (G_TYPE_CHECK_INSTANCE_CAST((obj), NM_TYPE_DEVICE_MACVLAN, NMDeviceMacvlan))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
Avoid that by using _NM_G_TYPE_CHECK_INSTANCE_CAST().
This can only be done for our internal usages. The public headers
of libnm are not changed.
- with nm_assert(), if the argument is a compile time constant
always check it (regardless of NDEBUG, G_DISABLE_ASSERT)
and mark the failure as _nm_unreachable_code(). We do this,
even if we usually would not evaluate run time checks with
NDEBUG/G_DISABLE_ASSERT.
- with nm_assert_se(), if assertions are disabled with NDEBUG
and G_DISABLE_ASSERT, still mark the path as _nm_unreachable_code().
This is useful, because it can avoid compiler warnings that are
emitted if the compiler things that the code can be reached.
_nm_assert_fail() can clearly never be reached (unless a bug happens).
When compiling we can disable assertion checks with
NDEBUG/G_DISABLE_ASSERT, but if we know that an assertion must not be
hit (for example with nm_assert_not_reached()) then we still want to
mark the path as unreachable, even if assert() does not abort the
process.
This allows the compiler to see that nm_assert(0) is unreachable code.
That is because nm_assert(0) calls NM_LIKELY(0), which calls
NM_BOOLEAN_EXPR(0). The latter was a statement expression, which
to the compiler was not a constant expression. Hence, this may trigger
compiler warnings about uninitialized variables.
Let NM_BOOLEAN_EXPR() to be constant, if the arguments are.
This can avoid compiler warnings in some cases.
Note that __builtin_choose_expr(__builtin_constant_p(...), ...) does
not properly work with gcc 4.8 ([1]). Hence only do macro shenanigans
with a newer gcc. Then entire point of NM_BOOLEAN_EXPR() is anyway
to preserve the "-Wparentheses" warning (while only evaluating the
argument once, being safe with nested invocations, propagate constness).
If we don't care about "-Wparentheses", it should be the same as
(!!(expr)). We can ignore that on non-recent gcc.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
The implementation for static asserts with (sizeof(char[(cond) ? 1 : -1]))
silently fails if the condition is not a compile time constant, because
it results in a VLA which is evaluated at runtime. Well, for that reason
we build with "-Wvla" to catch accidentally using a non-const expression
in a static assert. But still, we can do better. Use instead bitfields
to trigger the compiler error. This works only with static expressions
and also without "-Wvla".
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1468
Fix this compile error when "defined(__GNUC__) && (__GNUC__ > 2) &&
defined(__OPTIMIZE__)" doesn't match:
In file included from ../src/libnm-std-aux/nm-default-std.h:102,
from ../src/libnm-std-aux/nm-std-utils.c:3:
../src/libnm-std-aux/nm-std-aux.h: In function ‘NM_ALIGN_TO’:
../src/libnm-std-aux/nm-std-aux.h:160:6: error: expected expression before ‘{’ token
160 | ({ \
| ^
../src/libnm-std-aux/nm-std-aux.h:169:31: note: in expansion of macro ‘_NM_BOOLEAN_EXPR_IMPL’
169 | #define NM_BOOLEAN_EXPR(expr) _NM_BOOLEAN_EXPR_IMPL(NM_UNIQ, expr)
| ^~~~~~~~~~~~~~~~~~~~~
../src/libnm-std-aux/nm-std-aux.h:175:27: note: in expansion of macro ‘NM_BOOLEAN_EXPR’
175 | #define NM_LIKELY(expr) NM_BOOLEAN_EXPR(expr)
| ^~~~~~~~~~~~~~~
../src/libnm-std-aux/nm-std-aux.h:238:19: note: in expansion of macro ‘NM_LIKELY’
238 | } else if NM_LIKELY (cond) { \
| ^~~~~~~~~
../src/libnm-std-aux/nm-std-aux.h:449:5: note: in expansion of macro ‘nm_assert’
449 | nm_assert(nm_utils_is_power_of_two(ali));
| ^~~~~~~~~
The NM_LIKELY()/NM_UNLIKELY() macros should be alwas called enclosed
in parentheses like in:
if (NM_LIKELY(i == 1)) ...
and should be expanded with only a single pair of parentheses so that
expressions like (i = 1) generate a compiler warning.
Fixes: 030d68aef7 ('shared: add nm_assert() to "nm-std-aux.h"')
Previously, nm_close() accepted EBADF, as long as fd was negative.
Getting EBADF for a non-negative file descriptor is a serious bug,
because in multi threaded applications there is a race to close
an unrelated file descriptor. Also, it indicates that somebody
messed up the tracking of the resource, which indicates a bug.
Getting EBADF on a negative FD is less of a problem.
But it still indicates that the caller does something they likely
don't intend to.
Assert against any EBADF.
Note that nm_clear_fd() and nm_auto_close take already care to not
close negative "file descriptors". So, if you use those, you are
not affected.
If you want a close function that doesn't care about whether fd is set,
then maybe use `nm_clear_fd(&fd)` instead.
Cleanup the handling of close().
First of all, closing an invalid (non-negative) file descriptor (EBADF) is
always a serious bug. We want to catch that. Hence, we should use nm_close()
(or nm_close_with_error()) which asserts against such bugs. Don't ever use
close() directly, to get that additional assertion.
Also, our nm_close() handles EINTR internally and correctly. Recent
POSIX defines that on EINTR the close should be retried. On Linux,
that is never correct. After close() returns, the file descriptor is
always closed (or invalid). nm_close() gets this right, and pretends
that EINTR is a success (without retrying).
The majority of our file descriptors are sockets, etc. That means,
often an error from close isn't something that we want to handle. Adjust
nm_close() to return no error and preserve the caller's errno. That is
the appropriate reaction to error (ignoring it) in most of our cases.
And error from close may mean that there was an IO error (except EINTR
and EBADF). In a few cases, we may want to handle that. For those
cases we have nm_close_with_error().
TL;DR: use almost always nm_close(). Unless you want to handle the error
code, then use nm_close_with_error(). Never use close() directly.
There is much reading on the internet about handling errors of close and
in particular EINTR. See the following links:
https://lwn.net/Articles/576478/https://askcodes.net/coding/what-to-do-if-a-posix-close-call-fails-https://www.austingroupbugs.net/view.php?id=529https://sourceware.org/bugzilla/show_bug.cgi?id=14627https://news.ycombinator.com/item?id=3363819https://peps.python.org/pep-0475/
For g_assert() and g_return*() we already do the same, in
"src/libnm-glib-aux/nm-gassert-patch.h"
Also patch __assert_fail() so that it omits the condition text and the
function name in production builds.
Note that this is a bit ugly, for two reasons:
- again, we make assumptions that __assert_fail() exists. In practice,
this is the case for glibc and musl.
- <assert.h> can be included multiple times, while also forward
declaring __assert_fail(). That means, we cannot add a macro
#define __assert_fail(...)
because that would break the forward declaration. Instead,
just `#define __assert_fail _nm_assert_fail_internal`
Of course, this only affects direct calls to assert(), which we have
few. nm_assert() is not affected, because that anyway doesn't do
anything, unless NM_MORE_ASSERTS is enabled.
1) ensure the compiler always sees the condition (even if
it's unreachable). That is important, to avoid warnings
about unused variables and to ensure the condition compiles.
Previously, if NM_MORE_ASSERTS was enabled and NDEBUG (or
G_DISABLE_ASSERT) was defined, the condition was not seen by
the compiler.
2) to achieve point 1, we evaluate the expression now always in
nm_assert() macro directly. This also has the benefit that we
control exactly what is done there, and the implementation is
guaranteed to not use any code that is not async-signal-safe
(unless the assertion fails, of course).
3) add NM_MORE_ASSERTS_EFFECTIVE.
When using no glib (libnm-std-aux), the assert is implemented
by C89 assert(), while libnm-glib-aux redirects that to g_assert().
Note that these assertions are only in effect, if both NM_MORE_ASSERTS
and NDEBUG/G_DISABLE_ASSERT say so. NM_MORE_ASSERTS_EFFECTIVE
is thus the effectively used level for nm_assert().
4) use the proper __assert_fail() and g_assertion_message_expr()
calls. __assert_fail() is not standard, but it is there for glibc
and musl. So relying on this is probably fine. Otherwise, we will
get a compilation error and notice it.
Clang 15 ([1], [2]) added
Added the -Wunreachable-code-generic-assoc diagnostic flag (grouped
under the -Wunreachable-code flag) which is enabled by default and warns
the user about _Generic selection associations which are unreachable
because the type specified is an array type or a qualified type.
This causes compiler warnings with various uses of _Generic():
../src/libnm-glib-aux/nm-shared-utils.h:2489:12: error: due to lvalue conversion of the controlling expression, association of type 'const char *const *const' will never be selected becaus
e it is qualified [-Werror,-Wunreachable-code-generic-assoc]
return nm_strv_find_first((const char *const *) strv->pdata, strv->len, str);
^
../src/libnm-glib-aux/nm-shared-utils.h:475:25: note: expanded from macro 'nm_strv_find_first'
_nm_strv_find_first(NM_CAST_STRV_CC(list), (len), (needle))
^
../src/libnm-glib-aux/nm-macros-internal.h:397:22: note: expanded from macro 'NM_CAST_STRV_CC'
const char *const*const: (const char *const*) (value), \
^
Clang is correct.
[1] https://releases.llvm.org/15.0.0/tools/clang/docs/ReleaseNotes.html#improvements-to-clang-s-diagnostics
[2] https://reviews.llvm.org/D125259
LTO without assertion enabled, thinks that certain code paths
result in uninitialized code. Technically, it's not wrong, in practice
those are only in cases where we already failed an assertion.
In function 'nm_ip_addr_is_null',
inlined from 'canonicalize_ip_binary' at src/libnm-core-impl/nm-setting-ip-config.c:67:21,
inlined from 'nm_ip_route_set_next_hop_binary' at src/libnm-core-impl/nm-setting-ip-config.c:1062:23:
./src/libnm-glib-aux/nm-inet-utils.h:80:12: error: 'a' may be used uninitialized [-Werror=maybe-uninitialized]
80 | return IN6_IS_ADDR_UNSPECIFIED(&a.addr6);
| ^
src/libnm-core-impl/nm-setting-ip-config.c: In function 'nm_ip_route_set_next_hop_binary':
./src/libnm-glib-aux/nm-inet-utils.h:73:14: note: 'a' declared here
73 | NMIPAddr a;
| ^
Try to workaround that by letting nm_utils_addr_family_to_size() always
return a non-zero size. This is ugly, because in the assertion case fail
we might now also get an additional memory corruption that could have
been avoided by returning zero. However, it probably doesn't matter, because
in this scenario we are already in a bad situation.
Fixes: b02aeaf2f3 ('glib-aux: fix various nm_ip_addr_*() functions for unaligned addresses')
If you do:
nm_assert_addr_family(NMP_OBJECT_CAST_MPTCP_ADDR(obj)->addr_family));
then there are two nested NM_IN_SET() macro invocations. Once,
NMP_OBJECT_CAST_MPTCP_ADDR() checks that the object type is one of
a few selected (using NM_IN_SET()). Then, that is passed to
nm_assert_addr_family(), which checks NM_IN_SET(addr_family, AF_INET,
AF_INET6).
In general, it's easy to end up in a situation like this.
And it mostly works just fine. The only problem was that NM_IN_SET()
uses an internal, local variable "_x". The compiler will emit a very
verbose failure about the shadowed variable:
./src/libnm-std-aux/nm-std-aux.h:802:14: error: declaration of '_x' shadows a previous local [-Werror=shadow]
802 | type _x = (x); \
NM_UNIQ_T() exists for this purpose. Use it. NM_IN_SET() is
popular enough to warrant a special treatment to avoid this pitfall.
