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.
This is the version shipped in Fedora 37. As Fedora 37 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. The change
in isolation will break the "check-tree" test.
This test is inherently fragile, as it depends on starting processes,
wait for something and kill the process. There are timings involved
that are out of control of the test. Try to adjust the timing.
# NetworkManager-DEBUG: <debug> [1668755976.9741] kill child process test-s-4 (111487): sending SIGKILL...
# NetworkManager-DEBUG: <debug> [1668755976.9753] kill child process test-s-4 (111487): waiting for process to terminate after sending SIGTERM (15) and SIGKILL...
# NetworkManager-DEBUG: <debug> [1668755976.9758] kill child process test-s-4 (111487): after sending SIGTERM (15) and SIGKILL, process 111487 exited by signal 9 (5759 usec elapsed)
Bail out! GLib:ERROR:../src/core/tests/test-core-with-expect.c:154:test_nm_utils_kill_child_sync_do: Did not see expected message NetworkManager-DEBUG: *<debug> [*] kill child process test-s-4 (*): waiting up to 1 milliseconds for process to terminate normally after sending SIGTERM (15)...
Bail out! nm:ERROR:../src/core/tests/test-core-with-expect.c:457:test_nm_utils_kill_child: assertion failed (exit_status == 0): (6 == 0)
--- stderr ---
**
GLib:ERROR:../src/core/tests/test-core-with-expect.c:154:test_nm_utils_kill_child_sync_do: Did not see expected message NetworkManager-DEBUG: *<debug> [*] kill child process test-s-4 (*): waiting up to 1 milliseconds for process to terminate normally after sending SIGTERM (15)...
**
nm:ERROR:../src/core/tests/test-core-with-expect.c:457:test_nm_utils_kill_child: assertion failed (exit_status == 0): (6 == 0)
/builds/NetworkManager/NetworkManager/tools/run-nm-test.sh: line 337: 110662 Aborted "${NMTST_DBUS_RUN_SESSION[@]}" "${NMTST_LIBTOOL[@]}" "$NMTST_VALGRIND" --quiet --error-exitcode=$VALGRIND_ERROR --leak-check=full --gen-suppressions=all "${NMTST_SUPPRESSIONS[@]}" --num-callers=100 --log-file="$LOGFILE" "$TEST" "${TEST_ARGV[@]}"
We already have src/linux-headers, where we have complete copies of linux
user space headers. Of course that exists, because we want to use certain
features and don't depend on the installed kernel headers. Which works
well, because kernel user space API is stable, and we anyway want to
support compiling against a newer kernel and run against an older (e.g.
in a container). So having our copy of newer kernel headers is merely
as if we compiled against as newer kernel.
Add "src/nm-compat-headers" which has a similar purpose, but a different
approach. Instead of replacing the included header entirely, include
the system header and patch it with #define.
Use this for "linux/if_addr.h". Of course, the approach here is that we
no longer include <linux/if_addr.h> directly, but instead include
"nm-compat-headers/linux/if_addr.h".
These variants provide additional nm_assert() checks, and are thus
preferable.
Note that we cannot just blindly replace &g_array_index() with
&nm_g_array_index(), because the latter would not allow getting a
pointer at index [arr->len]. That might be a valid (though uncommon)
usecase. The correct replacement of &g_array_index() is thus
nm_g_array_index_p().
I checked the code manually and replaced uses of nm_g_array_index_p()
with &nm_g_array_index(), if that was a safe thing to do. The latter
seems preferable, because it is familar to &g_array_index().
- name things related to `in_addr_t`, `struct in6_addr`, `NMIPAddr` as
`nm_ip4_addr_*()`, `nm_ip6_addr_*()`, `nm_ip_addr_*()`, respectively.
- we have a wrapper `nm_inet_ntop()` for `inet_ntop()`. This name
of our wrapper is chosen to be familiar with the libc underlying
function. With this, also name functions that are about string
representations of addresses `nm_inet_*()`, `nm_inet4_*()`,
`nm_inet6_*()`. For example, `nm_inet_parse_str()`,
`nm_inet_is_normalized()`.
<<<<
R() {
git grep -l "$1" | xargs sed -i "s/\<$1\>/$2/g"
}
R NM_CMP_DIRECT_IN4ADDR_SAME_PREFIX NM_CMP_DIRECT_IP4_ADDR_SAME_PREFIX
R NM_CMP_DIRECT_IN6ADDR_SAME_PREFIX NM_CMP_DIRECT_IP6_ADDR_SAME_PREFIX
R NM_UTILS_INET_ADDRSTRLEN NM_INET_ADDRSTRLEN
R _nm_utils_inet4_ntop nm_inet4_ntop
R _nm_utils_inet6_ntop nm_inet6_ntop
R _nm_utils_ip4_get_default_prefix nm_ip4_addr_get_default_prefix
R _nm_utils_ip4_get_default_prefix0 nm_ip4_addr_get_default_prefix0
R _nm_utils_ip4_netmask_to_prefix nm_ip4_addr_netmask_to_prefix
R _nm_utils_ip4_prefix_to_netmask nm_ip4_addr_netmask_from_prefix
R nm_utils_inet4_ntop_dup nm_inet4_ntop_dup
R nm_utils_inet6_ntop_dup nm_inet6_ntop_dup
R nm_utils_inet_ntop nm_inet_ntop
R nm_utils_inet_ntop_dup nm_inet_ntop_dup
R nm_utils_ip4_address_clear_host_address nm_ip4_addr_clear_host_address
R nm_utils_ip4_address_is_link_local nm_ip4_addr_is_link_local
R nm_utils_ip4_address_is_loopback nm_ip4_addr_is_loopback
R nm_utils_ip4_address_is_zeronet nm_ip4_addr_is_zeronet
R nm_utils_ip4_address_same_prefix nm_ip4_addr_same_prefix
R nm_utils_ip4_address_same_prefix_cmp nm_ip4_addr_same_prefix_cmp
R nm_utils_ip6_address_clear_host_address nm_ip6_addr_clear_host_address
R nm_utils_ip6_address_same_prefix nm_ip6_addr_same_prefix
R nm_utils_ip6_address_same_prefix_cmp nm_ip6_addr_same_prefix_cmp
R nm_utils_ip6_is_ula nm_ip6_addr_is_ula
R nm_utils_ip_address_same_prefix nm_ip_addr_same_prefix
R nm_utils_ip_address_same_prefix_cmp nm_ip_addr_same_prefix_cmp
R nm_utils_ip_is_site_local nm_ip_addr_is_site_local
R nm_utils_ipaddr_is_normalized nm_inet_is_normalized
R nm_utils_ipaddr_is_valid nm_inet_is_valid
R nm_utils_ipx_address_clear_host_address nm_ip_addr_clear_host_address
R nm_utils_parse_inaddr nm_inet_parse_str
R nm_utils_parse_inaddr_bin nm_inet_parse_bin
R nm_utils_parse_inaddr_bin_full nm_inet_parse_bin_full
R nm_utils_parse_inaddr_prefix nm_inet_parse_with_prefix_str
R nm_utils_parse_inaddr_prefix_bin nm_inet_parse_with_prefix_bin
R test_nm_utils_ip6_address_same_prefix test_nm_ip_addr_same_prefix
./contrib/scripts/nm-code-format.sh -F
In the past, nmp_lookup_init_object() could both lookup all object for a
certain ifindex, and lookup all objects of a type. That fallback path
already leads to an assertion failure fora while now, so nobody should
be using this function to lookup all objects of a certain type (for
what, we have nmp_lookup_init_obj_type()).
Now, remove the fallback path, and rename the function to what it really
does.
ASSUME is causing more troubles than benefits it provides. This patch is
dropping NM_L3_CFG_COMMIT_TYPE_ASSUME and assume_config_once. NM3LCfg
will commit as if the sys-iface-state is MANAGED.
This patch is part of the effort to remove ASSUME from NetworkManager.
After ASSUME is dropped when starting NetworkManager it will take full
control of the interface, re-configuring it. The interface will be
managed from the start instead of assumed and then managed.
This will solve the situations where an interface is half-up and then a
restart happens. When NetworkManager is back it won't add the missing
addresses (which is what assume does) so the interface will fail during
the activation and will require a full activation.
https://bugzilla.redhat.com/show_bug.cgi?id=2050216https://bugzilla.redhat.com/show_bug.cgi?id=2077605https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1196
On glibc, HOST_NAME_MAX is defined as 64. Also, Linux'
sethostname() enforces that limit (__NEW_UTS_LEN). Also,
`man gethostname` comments that HOST_NAME_MAX on Linux is
64.
However, when building against musl, HOST_NAME_MAX is defined as 255.
That seems wrong. We use this limit to validate the hostname, and that
should not depend on the libc or on the compilation.
Hardcode the value to 64.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1197
This was only for unit testing, to check whether our reader
for "/etc/machine-id" agrees with systemd's.
That unit test was anyway flawed, because it actually accesses
the machine-id on the test system.
Anyway. Drop this. Most likely our parser is good enough, and
if we get a bug report with a defect, we can unit test against
that.
This is long replaced by nettools' n-dhcp4 client.
Drop it.
We still require NMDhcpSystemd for the DHCPv6 client.
Note that "[main].dhcp=systemd" now falls back to the internal client.
But this option was undocumented and internal anyway.
test_machine_id_read() is a flawed unit test, as it reads the machine-id
of the machine where it's running. That means the test depends on the
test machine, which is obviously a problem.
If you had no /etc/machine-id but a /var/lib/dbus/machine-id, then previously
the test would fail. If the file exists, assume we are able to read a
valid machine-id.
On test systems that have a bogus /etc/machine-id or /var/lib/dbus/machine-id,
the test would still fail. Just don't do that.
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
Completely rework IP configuration in the daemon. Use NML3Cfg as layer 3
manager for the IP configuration of an interface. Use NML3ConfigData as
pieces of configuration that the various components collect and
configure. NMDevice is managing most of the IP configuration at a higher
level, that is, it starts DHCP and other IP methods. Rework the state
handling there.
This is a huge rework of how NetworkManager daemon handles IP
configuration. Some fallout is to be expected.
It appears the patch deletes many lines of code. That is not accurate, because
you also have to count the files `src/core/nm-l3*`, which were unused previously.
Co-authored-by: Beniamino Galvani <bgalvani@redhat.com>
- add "pre-commit" signal.
- fix assertion in nm_l3_config_data_get_ip6_privacy().
- set IPv6 privacy in _init_from_connection_ip() from profile.
- fix leaking "os_zombie_lst" in _obj_state_data_free().
- remove wrong assertion about VRF.
- fix _routes_temporary_not_available_update() to honor only the
requested object type. Otherwise, we always prune unrelated objects
too.
We have nm_l3cfg_commit(), however that is synchronous and triggers an
avalanche of side effects. So it should be avoided if a component is
not aware of the current circumstances in which it gets called (most of them).
The alternative is nm_l3cfg_commit_on_idle_schedule(), but previously
that only supported the auto type.
Two changes:
- add a commit_type parameter to nm_l3cfg_commit_on_idle_schedule().
This allows to explicitly select a type for the next commit.
Previously, if the caller wanted for example to trigger a reapply
once, they had to register a handle, trigger the commit and unregister
the handle again. This basically allows to specify an ad-hoc commit
type that is only used once.
- if an explicit commit type is requested, then still always combine
it with auto. That means, we always use the "maximum" of what is
requested and what is registered.
NML3ConfigData is supposed to be immutable. It can be initialized from a
NMConnection, and its DNS priority property might be zero.
For the DNS priority, the value can be overwritten by global defaults.
We thus need to inject the default value at the right place.
This helper class is supposed to encapsulate most logic about
configuring IPv6 link local addresses and exposes a simpler API in order
to simplify NMDevice. Currently this logic is spread out in NMDevice.
Also, NML3IPv6LL directly uses NML3Cfg, thereby freeing NMDevice to care
about that too much.
For several reasons, NML3IPv6LL works different than NML3IPv4LL.
For one, with IPv6 we need to configure the address in kernel, which does
DAD for us. So, NML3IPv6LL will tell NML3Cfg to configure those
addresses that it wants to probe. For IPv4, it only tells NML3Cfg to do
ACD, without configuring anything yet. That is left to the caller.
It's great to have functions that cannot fail, because it allows to
skip any error handling.
_set_stable_privacy() as it was could not fail, so the only reason why
nm_utils_ipv6_addr_set_stable_privacy() could fail is because the DAD
counter exhausted.
Also, it will be useful to have a function that does not do the counter
check, where the caller wants to handle that differently.
Rename some functions, and make the core nm_utils_ipv6_addr_set_stable_privacy()
not failable.
The "only_dirty" parameter to a remove-all() function is odd.
For one, the function is called remove-all, but depending on a parameter
it does not remove all.
Also, setting remove-all(only_dirty=TRUE) means it will remove not
everything, so passing TRUE will remove only parts. That logic seems
confusing.
Avoid that, by removing the parameter from nm_l3cfg_remove_config_all()
and add nm_l3cfg_remove_config_all_dirty().
Each NML3ConfigData should have a source set, and in fact most callers
would call nm_l3_config_data_set_source() right after creating the
instance.
Move the source parameter to the new() constructor function. Also remove
the setter, making the source of an instance immutable.
As every l3cfg instance generally has a clear purpose, the source should
always be known from the start and doesn't need to change.
The "utils" part does not seem useful in the name.
Note that we also have NMStrBuf, which is named nm_str_buf_*().
There is an unfortunate similarity between the two, but it's still
distinct enough (in particular, because one takes an NMStrBuf and
the other not).
I missed that we already have a gettid() wrapper. Drop the duplicated
again and use nm_utils_gettid().
Fixes: e874c5bf6b ('random: Provide missing gettid() declaration')
NMConfigData is immutable and with the previous commit are the strings
already cached internally. There is no need to clone it.
Of course, the callers must not assume that the string stays alive after
a config reload (SIGHUP), where the NMConfigData might change. So they
are not always alive, but long enough for all callers to avoid cloning
the string.
Coverity says:
Error: ALLOC_FREE_MISMATCH (CWE-762):
NetworkManager-1.31.3/src/core/tests/test-systemd.c:261: alloc: Allocation of memory which must be freed using "free".
NetworkManager-1.31.3/src/core/tests/test-systemd.c:274: free: Calling "_nm_auto_g_free" frees "exp2_arr" using "g_free" but it should have been freed using "free".
# 272| g_assert_cmpmem(expected_arr, expected_len, exp3_arr, exp3_len);
# 273| }
# 274|-> }
# 275|
# 276| #define _test_unbase64mem(base64, expected_str) \
Error: ALLOC_FREE_MISMATCH (CWE-762):
NetworkManager-1.31.3/src/core/tests/test-systemd.c:270: alloc: Allocation of memory which must be freed using "free".
NetworkManager-1.31.3/src/core/tests/test-systemd.c:274: free: Calling "_nm_auto_g_free" frees "exp3_arr" using "g_free" but it should have been freed using "free".
# 272| g_assert_cmpmem(expected_arr, expected_len, exp3_arr, exp3_len);
# 273| }
# 274|-> }
# 275|
# 276| #define _test_unbase64mem(base64, expected_str) \
Fixes: 0298d54078 ('systemd: expose unbase64mem() as nm_sd_utils_unbase64mem()')
Having two functions like link_set_x() and link_set_nox() it is not a
good idea. This patch is introducing nm_platform_link_change_flags().
This allow flag modification directly, so the developer does not need to
define the virtual functions all the time everywhere.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
