Optimally we want stateless, pure code. Obviously, NMDhcpClient needs to
keep state to know what it's doing. However, we should well encapsulate
the state inside NMDhcpClient, and only accept events/notifications that
mutate the internal state according to certain rules.
Having a function public set_state(self, new_state) means that other
components (subclasses of NMDhcpClient) can directly mangle the state.
That means, you no longer need to only reason about the internal state
of NMDhcpClient (and the events/notifications/state-changes that it
implements). You also need to reason that other components take part of
maintaining that internal state.
Rename nm_dhcp_client_set_state() to nm_dhcp_client_notify(). Also, add
a new enum NMDhcpClientEventType with notification/event types.
In practice, this is only renaming. But naming is important, because it
suggests the reader how to think about the code.
(cherry picked from commit 97e65e4b50)
(cherry picked from commit 2dba874c5a)
The "noop" state is almost unused, however, nm_dhcp_set_state()
has a check "if (new_state >= NM_DHCP_STATE_TIMEOUT)", so the order
of the NOOP state matters.
Fix that by reordering.
Also, just return right away from NOOP.
(cherry picked from commit 9761e38f7e)
(cherry picked from commit 62ae5c0d0d)
NMDhcpState is very tied to events from dhclient. But most of these
states we don't care about, and NMDhcpClient definitely should abstract
and hide them.
We should repurpose NMDhcpState to simpler state. For that, first drop
the state from nm_dhcp_client_handle_event().
This is only the first step (which arguably makes the code more
complicated, because reason_to_state() gets spread out and the logic
happens more than once). That will be addressed next.
(cherry picked from commit f102051a29)
(cherry picked from commit 2c7f74ad94)
g_bytes_ref() does not accept NULL. But doing so can be convenient,
add a helper for that.
Note that g_bytes_unref() does accept NULL, so there is no corresponding
helper.
(cherry picked from commit 222f404928)
(cherry picked from commit 01bcde8ab0)
- use NM_IN_STRSET_ASCII_CASE().
- don't use else block after we return.
- don't accept the "iface" argument just for logging. The caller
can do the logging, if they wish.
(cherry picked from commit 9b9c07530c)
(cherry picked from commit ea7ad68ed2)
The "take" parameter of _set_effective_client_id() was always "FALSE". Drop it.
Also, drop _set_effective_client_id() and just call nm_dhcp_client_set_effective_client_id()
directly.
(cherry picked from commit 6b191d6ea7)
In nm_dns_manager_set_ip_config() we try to avoid calling update_dns()
unless something changes, because updating DNS is expensive and can
trigger other actions such as a new hostname resolution.
When we add a new ip_data, even if the new element is equivalent to
the old one that was removed, we need to sort the list again.
Fixes: ce0a36d20f ('dns: better track l3cd changes')
https://bugzilla.redhat.com/show_bug.cgi?id=2098574
(cherry picked from commit 3cc7801779)
(cherry picked from commit db4c55c8d3)
(cherry picked from commit 6ac62a746f)
Fixes: eb883e34a5 ('core: Add option to AddAndActivateConnection2 to bind the lifetime')
(cherry picked from commit 88f5e7518a)
(cherry picked from commit afe53b902f)
(cherry picked from commit 659ee74d8d)
There is no reason to disallow resetting the state.
(cherry picked from commit 607a9544cb)
(cherry picked from commit 6af0233a21)
(cherry picked from commit aebfb3461e)
Sync/blocking methods are ugly. Their name should highlight this.
Also, we may have an async variant, so we will need the "good" name
for apply() and apply_finish().
(cherry picked from commit dc66fb7d04)
(cherry picked from commit 558bcd5aae)
(cherry picked from commit 5235dce259)
Blocking calls are ugly. Rename those to have a "_sync()" suffix.
Also, split from _fw_nft_set_shared() the part that constructs the
stdin for nft.
(cherry picked from commit 7362ad6266)
(cherry picked from commit bbf3d01e82)
(cherry picked from commit 61ed013e7b)
In practice there is little difference.
Previously, "strbuf" would own the string until the end of the function,
when the "nm_auto_str_buf" cleanup attribute destroys it. In the
meantime, we would pass it on to _fw_nft_call_sync(), which in fact
won't access the string after returning.
Instead, we can just transfer ownership to the GBytes instance. That seems
more logical and safer than aliasing the buffer owned by NMStrBuf with
a g_bytes_new_static(). That way, we don't add a non-obvious restriction
on the lifetime of the string. The lifetime is now guarded by the GBytes
instance, which, could be referenced and kept alive longer.
There is also no runtime/memory overhead in doing this.
(cherry picked from commit 6a04bcc59d)
(cherry picked from commit c598f0ff0f)
NMStrBuf can also contains NUL characters. We thus cannot use g_strndup(),
which uses strncpy() and truncates at the first NUL.
Fixes: 13d25f9d0b ('glib-aux: add support for starting with stack-allocated buffer in NMStrBuf')
(cherry picked from commit 520411623d)
(cherry picked from commit 7a3de841b8)
(cherry picked from commit 51b9f0ad4c)
It's wrong, and it breaks certain uses.
Fixes: 13d25f9d0b ('glib-aux: add support for starting with stack-allocated buffer in NMStrBuf')
(cherry picked from commit c5ec4ebd77)
(cherry picked from commit 7b487e6951)
(cherry picked from commit fed6e78d05)
NM_MORE_ASSERT is a compile time constant. The compiler can optimize
it away just fine.
(cherry picked from commit 560feecb4c)
(cherry picked from commit de6da97e9d)
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.
(cherry picked from commit 532f3e34a8)
(cherry picked from commit 90255a8aa8)
Allow to initialize NMStrBuf with an externally allocated array.
Usually a stack buffer. If the NMStrBuf grows beyond the size of
that initial buffer, then it would switch using malloc.
The idea is to support the common case where the result is small enough
to fit on the stack.
I always wanted to do such optimization because the main purpose of
NMStrBuf is to put it on the stack and ad-hoc construct a string.
I just figured, it would complicate the implementation and add
a runtime overhead. But turns out, it doesn't really.
The biggest question is how NMStrBuf should behave with a pre-allocated
buffer? Turns out, most choices can be made in a rather obvious way.
The only non-obvious thing is that nm_str_buf_finalize() would malloc()
a buffer, but that too seems consistent and what a user would probably
expect. As such, this doesn't seem to add unexpected semantics to the API.
(cherry picked from commit 13d25f9d0b)
(cherry picked from commit 51393413b4)
We've been outright ignoring master-slave checks if the link ended up
without a master since commit 2e22880894 ('device: don't remove the
device from master if its link has no master').
This was done to deal with OpenVSwitch port-interface relationship,
where the interface's platform link lacked an actual master in platform
(what matters there is the OVSDB entry), but the fix was too wide.
Let's limit the special case to devices whose were not enslaved to
masters that lack a platform link, which pretty much happens for
OpenVSwitch only.
Morale: Write better commit messages of future you is going to be upset
Fixes: 2e22880894 ('device: don't remove the device from master if its link has no master')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1358
(cherry picked from commit a1de6810df)
(cherry picked from commit dc2d2da9db)
(cherry picked from commit 0946610c54)
NMPGlobalTracker allows to track objects for independent users/callers.
That is, callers that are not aware whether another caller tracks the
same/similar object. It thus groups all objects by their nmp_object_id_equal()
(as `TrackObjData` struct), while keeping a list of each individually tracked
object (as `TrackData` struct which honors the object and the user-tag parameter).
When the same caller (based on the user-tag) tracks the same object again, then
NMPGlobalTracker will only track it once and combine the objects. That is done by
also having a dictionary for the `TrackData` entries (`self->by_data`).
This latter dictionary lookup wrongly considered nmp_object_id_equal().
Instead, it needs to consider all minor differences of the objects, and
use nmp_object_equal().
For example, for NMPlatformMptcpAddress, only the "address" is part of
the ID. Other fields, like the MPTCP flags are not. Imagine a profile is
active with MPTCP endpoints configured with flags "subflow". During reapply,
the user can only update the MPTCP flags (e.g. to "signal"). When that happens,
the caller (NML3Cfg) would track a new NMPlatformMptcpAddress instance, that only
differs by MPTCP flags. In this case, we need to track the new address for the
differences that it has according to nmp_object_equal(), and not
nmp_object_id_equal().
Due to this bug, reapply might not work correctly. For other supported types (routing
rules and routes) this bug may have been harder to reproduce, because most attributes
of rules/routes are also part of the ID and because it's uncommon to reapply a minor
change to a rule/route.
https://bugzilla.redhat.com/show_bug.cgi?id=2120471
Fixes: b8398b9e79 ('platform: add NMPRulesManager for syncing routing rules')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1375
(cherry picked from commit d8aacba3b2)
(cherry picked from commit c456bfa7c4)
(cherry picked from commit 06e720f7b2)
It is allowed to have a connection with empty connection.slave-type
and a NMSettingBondPort; the property will be set automatically during
normalization if a master is set, otherwise the setting will be removed.
With this change, it becomes possible to remove a port from a bond
from nmcli, turning it into a non-slave connection. Before, this used
to fail with:
$ nmcli connection add type ethernet ifname test con-name test+ connection.master bond0 connection.slave-type bond
$ nmcli connection modify test+ connection.master '' connection.slave-type ''
Error: Failed to modify connection 'test+': connection.slave-type: A connection with a 'bond-port' setting must have the slave-type set to 'bond'
https://bugzilla.redhat.com/show_bug.cgi?id=2126262https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1382
Fixes: 9958510f28 ('bond: add support of queue_id of bond port')
(cherry picked from commit 23ce9cff99)
(cherry picked from commit 30366e5b3a)
(cherry picked from commit 3e15e55b9b)
When disposing NMPolicy all the devices in the devices hash-table should
be unregistered and removed from the hash-table.
Fixes: 7e3d090acb ('policy: refactor tracking of registered devices')
(cherry picked from commit 5a87683b14)
(cherry picked from commit d23c6040f8)
(cherry picked from commit 962ecdd3eb)
When only one of those connection.{lldp,mdns,llmnr,dns-over-tls}
settings changes, we still need to do a full restart of the IP
configuration to reapply the changes.
Fixes: 58287cbcc0 ('core: rework IP configuration in NetworkManager using layer 3 configuration')
(cherry picked from commit f4b128c63b)
(cherry picked from commit 6eaee2b13f)
The @dracut_NM_vlan_over_team_no_boot sometimes fails, among other
things, because it fails to assume an indicated connection after a
restart.
That seems to happen because after the decision to activate the
indicated connection, the device does not move from DISCONNECTED state
quickly enough. Another assumption recheck runs in between and decides
to generate a connection, because the assume state was already reset
in between.
First start, creates and activates b3a61b68-f744-4a4c-a513-61399c154a67
on vlan0017:
NetworkManager (version 1.41.1-30921.55767cf5.el9) is starting...
(asserts:10000, boot:caf7301a-19cd-498b-b5ba-5d36ee939ffe)
...
settings: update[b3a61b68-f744-4a4c-a513-61399c154a67]: adding connection "vlan0017"
(45113870df0a4cfb/keyfile)
Second start:
NetworkManager (version 1.41.1-30921.55767cf5.el9) is starting...
(after a restart, asserts:10000, boot:caf7301a-19cd-498b-b5ba-5d36ee939ffe)
Assumption attempt successfully picks the right connection and thus
proceeds to reset the assume state:
manager: (vlan0017): assume: will attempt to assume matching connection 'vlan0017'
(b3a61b68-f744-4a4c-a513-61399c154a67) (indicated)
device[c7c5101cf0b73f5f] (vlan0017): assume-state: set guess-assume=0, connection=(null)
Everything great so far, activation of the right connection is enqueued
and the device moves away from unavailable state. However, the
activation can't proceed immediately:
device (vlan0017): state change: unmanaged -> unavailable
(reason 'connection-assumed', sys-iface-state: 'assume')
device (vlan0017): state change: unavailable -> disconnected
(reason 'connection-assumed', sys-iface-state: 'assume')
active-connection[0x55ba1162f1c0]: set device "vlan0017" [0x55ba1163c4f0]
device[c7c5101cf0b73f5f] (vlan0017): queue activation request waiting for carrier
Now another assumption attempt is done. The original assume state is
gone, so a connection is generated:
platform-linux: UDEV event: action 'add' subsys 'net' device 'vlan0017' (6); seqnum=1959
device[c7c5101cf0b73f5f] (vlan0017): queued link change for ifindex 6
manager: (vlan0017): assume: generated connection 'vlan0017' (57627119-8c20-4f9e-bf4d-4fc427b4a6a9)
keyfile: commit: 57627119-8c20-4f9e-bf4d-4fc427b4a6a9 (vlan0017) added as
"/run/NetworkManager/system-connections/vlan0017-57627119-8c20-4f9e-bf4d-4fc427b4a6a9.nmconnection"
(nm-generated,volatile,external)
I think this shouldn't have happened. We've picked the correct
connection already and it's enqueued for activation!
Change the check in nm_device_emit_recheck_assume() to also consider
any queued activation.
Fixes-test: @dracut_NM_vlan_over_team_no_boot
Co-authored-by: Lubomir Rintel <lkundrak@v3.sk>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1351
(cherry picked from commit 9eb8cbca76)
(cherry picked from commit bdaba47a68)
(cherry picked from commit f2925801f2)
