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.
The strength of CList is of course to use it as a stack of queue,
and only append/remove from the front/tail.
However, since this is an intrusive list, it can also be useful to
just use it to track elements, and -- when necessary -- sort them
via c_list_sort().
If we have a sorted list, we might want to insert a new element
honoring the sort order. This function achieves that.
We have our own copy of linux kernel headers, and we must never
directly include the corresponding versions from the system.
Avoid that, by only including the clones via "libnm-std-aux/nm-linux-compat.h"
and by including the compat wrapper header before other system headers.
NM_CMP_DIRECT_MEMCMP() gets called by NM_CMP_FIELD_MEMCMP_LEN().
For example, if you want to compare a NMIPAddr, it seems sensible
to call
NM_CMP_FIELD_MEMCMP_LEN(obj1, obj2, addr, nm_utils_addr_family_to_size(obj1->addr_family));
Granted, nm_utils_addr_family_to_size() asserts that addr_family is
either AF_INET or AF_INET6. However, if the assertion fails, we don't
want yet another undefined behavior here and do the sensible thing
about n zero.
In general, for a low-level function that uses memcmp(), it's non
obvious to ensure that the caller does not accidentally invoke undefined
behavior. nm_memcmp() avoids that.
- add code comments explaining some things.
- for NM_CMP_FIELD*() variants have a corresponding NM_CMP_DIRECT*()
macro and use it (aside the "memcmp" variants, which don't translate
directly).
NM_STR_BUF_INIT() and nm_str_buf_init() were pretty much redundant. Drop one of
them.
Usually our pattern is that we don't have functions that return structs.
But NM_STR_BUF_INIT() returns a struct, because it's convenient to use
with
nm_auto_str_buf NMStrBuf strbuf = NM_STR_BUF_INIT(...);
So use that variant instead.
Clang 14 has a new warning "-Wbitwise-instead-of-logical", and it warns
about our usage with NM_IN_SET_SE()/NM_IN_STRSET_SE(). It complains that we
are using '|' with boolean operands. Which is true (and intended), as we bitwise-or
the result of the '==' comparisons.
Work around the warning by casting the operands to "int". Note that
in C, the comparison operators have already a type "int", so this cast
should not result in any changes in the compiled code.
../src/libnm-core-impl/tests/test-general.c:9415:17: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
_ASSERT(2, !NM_IN_SET_SE(-1, G(1), G(2)));
~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/libnm-std-aux/nm-std-aux.h:800:30: note: expanded from macro 'NM_IN_SET_SE'
#define NM_IN_SET_SE(x, ...) _NM_IN_SET(|, typeof(x), x, __VA_ARGS__)
^
../src/libnm-std-aux/nm-std-aux.h:789:39: note: expanded from macro '_NM_IN_SET'
!!(NM_VA_ARGS_FOREACH(, , op, _NM_IN_SET_OP, __VA_ARGS__)); \
^
../src/libnm-std-aux/nm-std-aux.h:772:20: note: expanded from macro 'NM_VA_ARGS_FOREACH'
op, \
^
note: (skipping 7 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
../src/libnm-glib-aux/nm-macros-internal.h:1603:47: note: expanded from macro '_G_BOOLEAN_EXPR'
#define _G_BOOLEAN_EXPR(expr) NM_BOOLEAN_EXPR(expr)
~~~~~~~~~~~~~~~~^~~~~
../src/libnm-std-aux/nm-std-aux.h:167:62: note: expanded from macro 'NM_BOOLEAN_EXPR'
#define NM_BOOLEAN_EXPR(expr) _NM_BOOLEAN_EXPR_IMPL(NM_UNIQ, expr)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
../src/libnm-std-aux/nm-std-aux.h:161:13: note: expanded from macro '_NM_BOOLEAN_EXPR_IMPL'
if (expr) \
^~~~
../src/libnm-core-impl/tests/test-general.c:9415:17: note: cast one or both operands to int to silence this warning
../src/libnm-std-aux/nm-std-aux.h:800:30: note: expanded from macro 'NM_IN_SET_SE'
#define NM_IN_SET_SE(x, ...) _NM_IN_SET(|, typeof(x), x, __VA_ARGS__)
^
../src/libnm-std-aux/nm-std-aux.h:789:39: note: expanded from macro '_NM_IN_SET'
!!(NM_VA_ARGS_FOREACH(, , op, _NM_IN_SET_OP, __VA_ARGS__)); \
^
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
LLD 13 adds -z start-stop-gc and makes it the default, resulting in:
CCLD src/core/NetworkManager-all-sym
ld.lld: error: undefined symbol: __stop_connection_defaults
>>> referenced by nm-config.c:0 (src/core/nm-config.c:0)
>>> libNetworkManager_la-nm-config.o:(read_config) in archive src/core/.libs/libNetworkManager.a
>>> referenced by nm-config-data.c:1598 (src/core/nm-config-data.c:1598)
>>> libNetworkManager_la-nm-config-data.o:(nm_config_data_get_connection_default) in archive src/core/.libs/libNetworkManager.a
>>> referenced by nm-config-data.c:0 (src/core/nm-config-data.c:0)
>>> libNetworkManager_la-nm-config-data.o:(nm_config_data_get_connection_default) in archive src/core/.libs/libNetworkManager.a
ld.lld: error: undefined symbol: __start_connection_defaults
>>> referenced by nm-config.c:0 (src/core/nm-config.c:0)
>>> libNetworkManager_la-nm-config.o:(read_config) in archive src/core/.libs/libNetworkManager.a
>>> referenced by nm-config.c:0 (src/core/nm-config.c:0)
>>> libNetworkManager_la-nm-config.o:(read_config) in archive src/core/.libs/libNetworkManager.a
>>> referenced by nm-config.c:0 (src/core/nm-config.c:0)
>>> libNetworkManager_la-nm-config.o:(read_config) in archive src/core/.libs/libNetworkManager.a
>>> referenced 2 more times
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Add __attribute__((__retain__)) to prevent GC of the connection
defaults.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1008
We use the cleanup attribute heavily. It's useful for deferring
deallocation. For example, we have code like:
gs_unref_object NMBluezManager *self_keep_alive = g_object_ref(self);
where we don't use the variable otherwise, except for owning (and
freeing) the reference. This already lead to a compiler warning about
unused variable, which we would workaround with
_nm_unused gs_unref_object NMBluezManager *self_keep_alive = g_object_ref(self);
With clang 13.0.0~rc1-1.fc35, this got worse. Now for example also
static inline void
nm_strvarray_set_strv(GArray **array, const char *const *strv)
{
gs_unref_array GArray *array_old = NULL;
array_old = g_steal_pointer(array);
if (!strv || !strv[0])
return;
nm_strvarray_ensure(array);
for (; strv[0]; strv++)
nm_strvarray_add(*array, strv[0]);
}
leads to a warning
./src/libnm-glib-aux/nm-shared-utils.h:3078:28: error: variable array_old set but not used [-Werror,-Wunused-but-set-variable]
gs_unref_array GArray *array_old = NULL;
^
This is really annoying. We don't want to plaster our code with _nm_unused,
because that might hide actual issues. But we also want to keep using this
pattern and need to avoid the warning.
A problem is also that GCC usually does not warn about truly unused
variables with cleanup attribute. Clang was very useful here to flag
such variables. But now clang warns about cases which are no bugs, which
is a problem. So this does loose some useful warnings. On the other hand,
a truly unused variable (with cleanup attribute) is ugly, but not an actual
problem.
Now, with clang 13, automatically mark nm_auto() variables as _nm_unused
as workaround.
