Historically, the NMSetting types were in public headers. Theoretically,
that allowed users to subtype our classes. However in practice that was
impossible because they lacked required access to internal functions to
fully create an NMSetting class outside of libnm. And it also was not
useful, because you simply cannot extend libnm by subtyping a libnm
class. And supporting such a use case would be hard and limit what we can
do in libnm.
Having GObject structs in public headers also require that we don't
change it's layout. The ABI of those structs must not change, if anybody
out there was actually subclassing our GObjects.
In libnm 1.34 (commit e46d484fae ('libnm: hide NMSetting types from
public headers')) we moved the structs from headers to internal.
This would have caused a compiler error if anybody was using those
struct definitions. However, we still didn't change the ABI/layout so
that we didn't break users who relied on it (for whatever reason).
It doesn't seem there were any affected user. We waited long enough.
Change internal ABI.
No longer use g_type_class_add_private(). Instead, embed the private
structs directly (_NM_GET_PRIVATE()) or indirectly
(_NM_GET_PRIVATE_PTR()) in the object.
The main benefit is for debugging in the debugger, where we can now
easily find the private data. Previously that was so cumbersome to be
effectively impossible.
It's also the fastest possible way, since NM_SETTING_*_GET_PRIVATE()
literally resolves to "&self->_priv" (plus static asserts and
nm_assert() for type checking).
_NM_GET_PRIVATE() also propagates constness and requires that the
argument is a compatible pointer type (at compile time).
Note that g_type_class_add_private() is also deprecated in glib 2.58 and
replaced by G_ADD_PRIVATE(). For one, we still don't rely on 2.58. Also,
G_ADD_PRIVATE() is a worse solution as it supports a usecase that we
don't care for (public structs in headers). _NM_GET_PRIVATE() is still
faster, works with older glib and most importantly: is better for
debugging as you can find the private data from an object pointer.
For NMSettingIPConfig this is rather awkward, because all direct
properties require a common "klass->private_offset". This was however
the case before this change. Nothing new here. And if you ever touch
this and do something wrong, many unit tests will fail. It's almost
impossible to get wrong, albeit it can be confusing to understand.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1773
If a ovs interface has the cloned-mac-address property set, we pass
the desired MAC to ovsdb when creating the db entry, and openvswitch
will eventually assign it to the interface.
Note that usually the link will not have the desired MAC when it's
created. Therefore, currently we also change the MAC via netlink
before proceeding with IP configuration. This is important to make
sure that ARP announcements, DHCP client-id, etc. will use the correct
MAC address.
This doesn't work when using the "netdev" (userspace) datapath, as the
attempts to change the MAC of the tun interface via netlink fail,
leading to an activation failure.
To properly handle both cases in the same way, adopt a different
strategy: now we don't set the MAC address explicitly via netlink but
we only wait until ovs does that.
The code to determine if we are using the netdev datapath is logically
separated from the code to start IP configuration; move it to its own
function to make the code easier to follow.
The deactivation can happen while we are waiting for the ifindex, and
it can happen via two code paths, depending on the state. For a
regular deactivation, method deactivate_async() is called. Otherwise,
if the device goes directly to UNMANAGED or UNAVAILABLE, deactivate()
is called. We need to make sure that signal and source handlers are
disconnected, so that they are not called at the wrong time.
Fixes: 99a6c6eda6 ('ovs, dpdk: fix creating ovs-interface when the ovs-bridge is netdev')
nm_g_array_index() performs additional nm_assert() checks for
correctness.
In this case, it's pretty clear that the assertion will hold and that
the code is correct. Note that most of the time when having assertions,
we expect that they hold. Since nm_assert() is disabled in release
build, arguing that an assertion holds is not a strong argument against
having the assert (they are always supposed to hold, quite obviously so!).
The reason to change is that we should use the wrappers that perform
additional checks. Especially when the additional checks are nm_assert()
or static-asserts, as they are not present in release builds. To find
how well we are doing in this regard we can check `git grep -w
g_array_index`. If that gives many uses of the unchecked function, then
we cannot manually check them all to be really obviously correct.
Instead, we should not use g_array_index() and trivially see that all
array accesses are guarded by assertions.
"checkpatch.pl" also recommends against g_array_index().
If the device is being disconnected for a user request, at the moment
the active connection goes to state DEACTIVATED through the following
transitions, independently of the reason for the disconnection:
- state: DEACTIVATING, reason: UNKNOWN
- state: DEACTIVATED, reason: DEVICE_DISCONNECTED
For VPNs, a disconnection is always user-initiated, and the active
connection states emitted are:
- state: DEACTIVATING, reason: USER_DISCONNECTED
- state: DEACTIVATED, reason: USER_DISCONNECTED
This difference poses problems for clients that want to handle device
and VPNs in the same way, especially because WireGuard is implemented
as a device, but is logically a VPN.
Let NMActRequest translate the USER_REQUESTED device state reason to
USER_DISCONNECTED active connection state reason, in case of
disconnection.
This is an API change, but the previous behavior of reporting generic
uninformative reasons seems a bug. See for example
nmc_activation_get_effective_state(), which inspects the AC state
reason and in case it's generic (DEVICE_DISCONNECTED), it considers
the device state instead.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1405
NMActiveConnection implements method device_state_changed() that
re-emits device state changes as convenience for subclasses. Add the
reason for the state change to the handler, as it will be used in the
next commit.
This reverts commit 05c31da4d9.
In the linked commit the callback was made repeating on the assumption
that forward progress would result in the callback getting canceled in
cb_data_complete. However, this assumption does not hold since a timeout
callback does not guarantee completion (or error out) of a request.
curl tweaked some internals in v8.4.0 and started giving 0 timeouts, and
a repeating callback is firing back-to-back without making any progress
in doing so.
Revert the change and make the callback non-repeating again.
Fixes: 05c31da4d9 ('connectivity: don't cancel curl timerfunction from timeout')
"skip_repeated" sounds as if the function would only drop duplicate
elements that follow each other (in which case, the operation would be
O(n)). But it does search the entire array to prevent duplicates (resulting
in O(n^2)). Rename the argument "skip_repeated" to "no_duplicates"
to make that clearer.
Also, rename "skip_{empty,duplicates}" to "no_{empty,duplicates}". The
function removes those elements from the list, so "skip" is a bit
misleading too.
nm_strv_find_first() is useful (and used) to find the first index (if
any). I can thus also used to check for membership.
However, we also have nm_strv_contains(), which seems better for
readability, when we check for membership. Use it.
When IPv6 is disabled in kernel but ipv6.method is set to auto, NetworkManager repeatedly attempts
IPv6 configuration internally, resulting in unnecessary warning messages being output infinitely.
platform-linux: do-add-ip6-address[2: fe80::5054:ff:fe7c:4293]: failure 95 (Operation not supported)
ipv6ll[e898db403d9b5099,ifindex=2]: changed: no IPv6 link local address to retry after Duplicate Address Detection failures (back off)
platform-linux: do-add-ip6-address[2: fe80::5054:ff:fe7c:4293]: failure 95 (Operation not supported)
ipv6ll[e898db403d9b5099,ifindex=2]: changed: no IPv6 link local address to retry after Duplicate Address Detection failures (back off)
platform-linux: do-add-ip6-address[2: fe80::5054:ff:fe7c:4293]: failure 95 (Operation not supported)
ipv6ll[e898db403d9b5099,ifindex=2]: changed: no IPv6 link local address to retry after Duplicate Address Detection failures (back off)
To prevent this issue, let's disable IPv6 in NetworkManager when it is disabled in the kernel.
In order to do it in activate_stage3_ip_config() only once during activation,
the firewall initialization needed to be moved earlier. Otherwise, the IPv6 disablement could occur
twice during activation because activate_stage3_ip_config() is also executed from subsequent of fw_change_zone().
nm_streq() is better for readability. Prefer it over strcmp(). Note that
nm_streq() will be inlined, so it should make no difference performance
wise.
While at it, drop wrong comment.
ethtool "channels" parameters can be used to configure multiple queues
for a NIC, which helps to improve performances. Until now, users had
to use dispatcher scripts to change those parameters. Introduce native
support in NetworkManager by adding the following properties:
- ethtool.channels-rx
- ethtool.channels-tx
- ethtool.channels-other
- ethtool.channels-combined
`_ethtool_*_reset()` functions already check that the state is not
NULL, no need to check it before. The only exception was for "feature"
settings, where the check was missing.
Convert the open-coded conditions to a switch/case so that the
compilation will fail if a new ethtool type is added and is not
handled in various places.
The old src/libnm-client-impl/nm-settings-docs-gir.xml has been splitted
into different files specific for nmcli, keyfile, ifcfg-rh and dbus.
Ignore these files that are autogenerated.
We don't expect to add more variants, but better to use a wilcard to
cover this possibility. Also for the nm-property-infos-*.xml files.
Fixes: f4fbc59a16 ('man nm-setting-*: proper format for gtkdoc constants')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1748
If the client-id has been set to "none", the DHCP client-id option
(option 61) mustn't be sent. Honor this when the dhclient plugin is
used.
If dhclient has been called with the -i option (Use a DUID with DHCPv4
clients), it will send a Client-ID even without setting one in dhclient.conf.
In this case, this option needs to be explicitly overwritten with:
send dhcp-client-identifier = "";
At least in RHEL 8, dhclient is launched with `-i` turned on by default.
The function merge_dhclient_config was called only once from
create_dhclient_config. The content of both of them is short and simple,
so moving the content from merge_dhclient_config to the caller
improves the readability and makes the functions call chain easier to
follow. Also, both functions takes a long list of arguments which are
almost the same, so we can avoid having to pass them over and over in a
long call chain.
Sending the client-identifier (DHCP Option 61) is not mandatory,
although it's recommended, and there are some weird cases where
clients need not to send it.
Allow not to send it by leaving client_id unset.
Sending a client-id is not mandatory according to RFC2131. It is
mandatory according to RFC4361 that superseedes it.
Some weird DHCP servers conforming RFC2131 can get confused and break
existing DHCP leases if they start receiving a client-id when it was not
being previously received. Users that were using other DHCP client like
dhclient, but want to use NetworkManager's internal DHCP client, can
suffer this problem.
Add "none" as accepted value in ipv4.dhcp-client-id to specify that
client-id must not be sent. Note that this is generally not recommended
unless it's explicitly needed for some reason like the explained above.
Client-id is mandatory in DHCPv6.
This commit allow to set the "none" value and properly parse it in the
NMDhcpClientConfig struct. Next commits will modify the different DHCP
plugins to honor it.
