We want to move platform code to "shared/nm-platform". However, platform
code uses the logging infrastructure from the daemon, there is thus
an odd circular dependency.
Solve that by moving the "src/nm-logging.[hc]" to a new helper library
in "shared/nm-log-core".
"src/nm-logging.c" should be independent of libnm-core. It almost
is, except the error domain and code.
Move NM_MANAGER_ERROR to "nm-glib-aux/nm-shared-utils.h" so that
"nm-logging.c" is independent of libnm-core.
NetworkManager core is huge. We should try to split out
parts that are independent.
Platform code is already mostly independent. But due to having it
under "src/", there is no strict separation/layering which determines
the parts that can work independently. So, while the code is mostly
independent (in practice), that is not obvious from looking at the
source tree. It thus still contributes to cognitive load.
Add a shared library "shared/nm-platform", which should have no
dependencies on libnm-core or NetworkManager core.
In a first step, move the netlink code there. More should follow.
This is the same as libnm's nm_utils_hwaddr_aton(), which however
is public API.
We want to use this function also without libnm(-core). Hence add
the helper to "shared/nm-glib-aux".
Enums can also be negative (contrary to Flags). Fix the parsing.
$ nmcli connection modify "$PROFILE" connection.llmnr -1
Error: failed to modify connection.llmnr: invalid option '-1', use one of [default,no,resolve,yes].
In the vast majority of cases is the string for _nm_utils_enum_from_str_full()
short. As we duplicate it for stripping, prefer to clone it on the stack
with nm_strdup_maybe_a().
The BOOTIF MAC address can be prefixed with a hardware address
type. Typically it is 01 (for ethernet), but the legacy network module
accepts (and strips) any byte value.
It seems wrong to take any address type without validation. In
addition to "01", also accept a zero type which, according to the
bugzilla below, is used in some configurations to mean "undefined".
While at it, also accept ':' as separator for the first byte.
https://bugzilla.redhat.com/show_bug.cgi?id=1904099https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/713
This assert sometimes fails during copr builds. But the way
the assert was, it was hard to see what the actual problem
was.
Restructure the assert (again) to get the errno in the
test logs.
RFCs actually expect to honor the lifetime. See for example [1].
This is just not right, and totally arbitrary. It was added
when our libndp based implementation was added, but unclear
why this was done (beyond the code comment).
[1] page 204, v6LC.2.2.25: Processing Router Advertisement DNS (Host
only) at https://ipv6ready.org/docs/Core_Conformance_5_0_0.pdf
There is no actual change in behavior, because "struct nd_opt_hdr"
as two uint8_t, so in practice this struct was always packed already.
But make it explicit, because it's clear that we use these structs
to set the binary message and they need a well defined (packed) memory
layout.
The endpoints of WireGuard peers can be configured as DNS name, which
NetworkManager will resolve.
Since activating a profile might affect now names get resolved, we must
first resolve names before completing the activation of the WireGuard
device (and before reconfiguring DNS accordingly).
For example, if you configure exclusive DNS resolution via the WireGuard
device, and if the peer needs to be resolved via DNS, then resolving the
peer name must happen before the reconfiguration of DNS. Otherwise the
new DNS configuration will be broken due to being unable to reach the
WireGuard peer.
Fix that by waiting.
There is still an unfixed problem. If resolving any peers fails,
activation silently proceeds -- again possibly breaking the network
setup. Of course, NetworkManager will repeatedly try to re-resolve
the name, but that may never succeed if DNS would be resolved via
the VPN itself.
That is different from `wg set` which resolves hostnames and fails.
Consequently `wg-quick up` would also fail. But these are both one shot
applications, they are not around and basically let the user handle the
error (by reading the log and invoking the command again). NetworkManager
can do something different and proceed activation (as it will also
periodically re-resolve the hostnames again). Note that it's also valid
to activate a WireGuard device without any peers (and to modify the
activated device later with Reapply()). As such, having no peers (or
being unable to resolve a hostname) may be a valid configuration.
I think we should add an option/flag that when enabled will cause
the activation to fail of names cannot be resolved.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/535https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/616https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/721
With LTO and gcc-10.2.1-9.fc33.s390x we get:
src/platform/nm-platform.c:3325:1: error: link_duplex may be used uninitialized in this function [-Werror=maybe-uninitialized]
3325 | NM_UTILS_LOOKUP_STR_DEFINE(nm_platform_link_duplex_type_to_string,
| ^
src/devices/nm-device-ethernet.c:899: note: link_duplex was declared here
899 | NMPlatformLinkDuplexType link_duplex;
|
With gcc-10.2.1-9.fc33.s390x we get a (false positive) warning:
src/platform/tests/test-common.c: In function nmtstp_acd_defender_new:
src/platform/tests/test-common.c:2688:15: error: probe may be used uninitialized in this function [-Werror=maybe-uninitialized]
2688 | *defender = (NMTstpAcdDefender){
| ^
src/platform/tests/test-common.c:2656:56: note: probe was declared here
2656 | NAcdProbe * probe;
| ^
- accept directory names in the command line. In that case,
still honor the excluded files. That is a major improvement
for me, because I usually only want to reformat a directory
that I know has changed and it is fast to only process some
directories.
- pass all files at once to clang-format. For me that gives
a significant speed improvement (about 3 times faster), although
clang-format is only single threaded. Possibly clang-format could
even be faster by checking files in parallel.
In case of a style error, the script still falls back to
iterate over all files to find the first bad file and print
the full diff. But that is considered an unusual case.
- make it correctly work from calling it from a subdirectory.
In that case, we only check files inside that directory --
but still correctly honor the excluded files.
With LTO, the compiler can see that some code paths return without
initializing the variable. But it fails to see that those are code
paths after an assertion fail. Still that can lead to
"-Wmaybe-uninitialized" warnings in the caller.
Avoid that by not using g_return_if_fail() but nm_assert().
src/nm-ip6-config.c: In function '_nmtst_ip6_config_get_address':
./shared/nm-glib-aux/nm-dedup-multi.h:337:8: error: 'iter._next' may be used uninitialized in this function [-Werror=maybe-uninitialized]
337 | if (!iter->_next)
| ^
src/nm-ip6-config.c:1622:33: note: 'iter._next' was declared here
1622 | NMDedupMultiIter iter;
| ^
./shared/nm-glib-aux/nm-dedup-multi.h:343:8: error: 'iter._head' may be used uninitialized in this function [-Werror=maybe-uninitialized]
343 | if (iter->_next->next == iter->_head)
| ^
src/nm-ip6-config.c:1622:33: note: 'iter._head' was declared here
1622 | NMDedupMultiIter iter;
| ^
and more.
When compiling with LTO, the compiler can think that an
assertion failure (g_return*()) is regular code path, and
thus that the output variable is not set.
This can lead to "-Wmaybe-uninitialized" warnings in the
caller, despite this not happening in non-bug case.
Work aound that by setting the out argument.
Warning with LTO enabled and gcc-10.2.1-9.fc33.s390x:
src/nm-config-data.c: In function nm_config_data_get_device_config:
src/devices/nm-device.c:17454:9: error: is_fake may be used uninitialized in this function [-Werror=maybe-uninitialized]
17454 | m = nm_match_spec_device(specs,
| ^
src/devices/nm-device.c:17444:26: note: is_fake was declared here
17444 | gboolean is_fake;
| ^
src/nm-config-data.c: In function nm_config_data_get_connection_default:
src/devices/nm-device.c:17454:9: error: is_fake may be used uninitialized in this function [-Werror=maybe-uninitialized]
17454 | m = nm_match_spec_device(specs,
| ^
src/devices/nm-device.c:17444:26: note: is_fake was declared here
17444 | gboolean is_fake;
| ^
src/devices/nm-device.c: In function nm_device_check_unrealized_device_managed:
src/devices/nm-device.c:17454:9: error: is_fake may be used uninitialized in this function [-Werror=maybe-uninitialized]
17454 | m = nm_match_spec_device(specs,
| ^
src/devices/nm-lldp-listener.c: In function 'lldp_neighbor_to_variant':
./shared/nm-glib-aux/nm-shared-utils.h:1271:5: error: 'raw_len' may be used uninitialized in this function [-Werror=maybe-uninitialized]
1271 | g_variant_builder_add(builder,
| ^
src/devices/nm-lldp-listener.c:107:19: note: 'raw_len' was declared here
107 | gsize raw_len;
| ^
./shared/nm-glib-aux/nm-shared-utils.h:1271:5: error: 'raw_data' may be used uninitialized in this function [-Werror=maybe-uninitialized]
1271 | g_variant_builder_add(builder,
| ^
src/devices/nm-lldp-listener.c:106:19: note: 'raw_data' was declared here
106 | gconstpointer raw_data;
| ^
With LTO build on s390x (Fedora 33) we get a compiler warning:
libnm-core/nm-setting-ethtool.c: In function 'nm_setting_ethtool_get_optnames':
libnm-core/nm-setting-ethtool.c:263:60: error: 'len' may be used uninitialized in this function [-Werror=maybe-uninitialized]
263 | return len > 0 ? nm_memdup(names, sizeof(names[0]) * (((gsize) len) + 1u)) : NULL;
| ^
libnm-core/nm-setting-ethtool.c:257:24: note: 'len' was declared here
257 | guint len;
| ^
libnm-core/nm-setting-ethtool.c: In function 'nm_setting_ethtool_get_optnames':
libnm-core/nm-setting-ethtool.c:263:60: error: 'len' may be used uninitialized in this function [-Werror=maybe-uninitialized]
263 | return len > 0 ? nm_memdup(names, sizeof(names[0]) * (((gsize) len) + 1u)) : NULL;
| ^
libnm-core/nm-setting-ethtool.c:257:24: note: 'len' was declared here
257 | guint len;
| ^
The change broke unit tests on 32 bit systems.
Change the code again to make it more similar to what it was
before. Now only on 64 bit systems there is any difference compared
to before. That makes it easier about reasoning for how the unit test
should be (in most cases, it is unchanged).
Fixes: 040c86f15c ('shared: avoid compiler warning for nm_utils_get_next_realloc_size() returning huge sizes')
With LTO enabled, the compiler might think that "len" is not initialized.
That is even a correct assumption, if the compiler does not understand the
API of sendmsg() and that sendmsg() is supposed to set a negative errno.
Work around by initializing the variable.
shared/n-dhcp4/src/n-dhcp4-c-connection.c: In function n_dhcp4_c_connection_send_request:
shared/n-dhcp4/src/n-dhcp4-socket.c:368:19: error: len may be used uninitialized in this function [-Werror=maybe-uninitialized]
} else if (len != n_buf) {
^
shared/n-dhcp4/src/n-dhcp4-socket.c:351:23: note: len was declared here
size_t n_buf, len;
^
