Struct allow named arguments, which seems easier to maintain instead of
a function with many arguments. Also, adding a new parameter does not
require changes to most of the callers.
The real advantage of this is that we encode all the search parameters
in one argument. And we can add that argument to
_match_section_infos_lookup(), alongside lookup by NMDevice or
NMPlatformLink.
All callers eventually want a boolean instead of a NMMatchSpecMatchType.
I think the NMMatchSpecMatchType enum still has value at the lower
layers, where the enum values are clearer (when reading the code). So
don't drop NMMatchSpecMatchType entirely.
However, let's add nm_match_spec_match_type_to_bool() to convert the
match-type to a boolean to avoid duplicating the code.
Arguably, a kernel link is needed for DHCP and so the interface name
univocally identifies a device (for example, the OVS interface). But
for consistency and clarity, store the device type to be used for
logging.
When logging, messages include the interface name to specify what
device they refer to. In most case the interface name is unique.
There are some devices that don't have a kernel link associated, and
their interface name is not guaranteed to be unique. This is currently
the case for OVS bridges and OVS ports. When reading a log with
duplicate interface names, it is difficult to understand what is
happening. And this is made worse by the fact that it is common
practice to assign the same name to all devices in a OVS hierarchy
(bridge, port, interface).
To make logs unambiguous, we want to print the device type together
with the name; however we don't want to *always* print the type
because in most cases it's not useful and it would consume valuable
real estate on the screen. Adopt a simple heuristic of showing the
type only for OVS devices.
This commit adds a helper function to return the device type to show
in logs, when it is needed.
It's not obvious, why we couldn't have a pending dever action
at that point. Maybe we cannot, but just to be explicit about it,
handle that we potentially might.
For example, we tend to schedule the timeout priv->carrier_defer_source
only from within nm_device_set_carrier() if `priv->carrier` is FALSE.
At the same time, nm_device_set_carrier() does nothing `if
(priv->carrier == carrier)`. So probably there is no problem.
However, we also set priv->carrier directly in
nm_device_set_carrier_from_platform() without clearing the timer. It's
hard to imagine whether there can be a case where we might have two
timeouts pending.
commit_option() was used in the past to set both bridge and bridge port
options using sysfs. Currently it is only used for bridge port options.
This patch removes the dead code for bridge options and unify it on
commit_port_options(). This is simplifying the work needed to support
bridge port option through netlink.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1643
When a fixed address is assigned by the P2P group owner, then the code
would set the IPv4 configuration method to DISABLED internally. However,
this causes issues, because it means that IPv4 is considered to not have
come up internally which can cause the connection to later time out even
though it was configured properly.
As such, map this method to MANUAL in this case. The AUTO mapping
becomes then:
* MANUAL: If the remote part is the GO and assigned an IP address
* DHCP: If the remote part is the GO and did not assign an address
* SHARED: If we are the GO
This fixes an issue where the connection established by GNOME Network
Displays would fail once IPv6 configuration also times out.
See-also: https://gitlab.gnome.org/GNOME/gnome-network-displays/-/issues/279https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1636
This is the version shipped in Fedora 38. As Fedora 38 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. This change
in isolation will break the "check-tree" test.
In constructed(), NMDevice starts watching the D-Bus name owner or
monitoring the unix socket, and so it is always aware if teamd is
running. When it is, NMDevice connects to it and initializes
priv->tdc.
It is not useful to try to connect to teamd in update_connection()
because warnings will be generated by NM and by libteam if teamd is
not running. As explained above the connection is always initialized
when teamd is available, and so we can just check priv->tdc.
Fixes: ab586236e3 ('core: implement update_connection() for Team')
https://bugzilla.redhat.com/show_bug.cgi?id=2182029https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1631
According to systemd, IPv6 forwarding is special anyway, and they only
enable forwarding for "net.ipv6.conf.all.forwarding" ([1]).
Since commit 46e63e03af ('device: announce the managed IPv6
configuration with ipv6.method=shared') we support "ipv6.method=shared"
and enable forwarding for IPv6, on the interface. Whether that makes
sense is questionable, given [1] and the claim that setting it
per-interface is not useful.
Anyway, since that change we always reset the "forwarding" sysctl to
zero, when we don't enable shared mode. That is not right, because the
user didn't explicitly ask for that (and there is no configuration
option like systemd-networkd's "IPForward=" setting to control that).
What we instead should do, not touch/reset the sysctl, unless we really
want to.
No longer set "forwarding" to zero by default. And only restore the
previous value (_dev_sysctl_save_ip6_properties()) if we actually
changed the value to "1".
[1] b8fba0cded/src/network/networkd-sysctl.c (L79)https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/923
Fixes: 46e63e03af ('device: announce the managed IPv6 configuration with ipv6.method=shared')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1616
Add per port priority support for bond active port re-selection during
failover. A higher number means a higher priority in selection. The
primary port still has the highest priority. This option is only
compatible with active-backup, balance-tlb and balance-alb modes.
sysfs is deprecated and kernel will not add new bond port options to
sysfs. Netlink is a stable API and therefore is the right method to
communicate with kernel in order to set the link options.
Before commit a42682d44f ('device: take reference to device object
before 'delete_on_deactivate''), we used a weak pointer to track the
idle action.
As we now use a strong reference, we can store all data about the idle
action in NMDevice itself. Drop DeleteOnDeactivateData.
Hook the information for tracking the activation of a device, to the
NMDevice itself. Sure, that slightly couples the NMPolicy closer to
NMDevice, but the result is still simpler code because we don't need a
separate ActivateData.
It also means we can immediately tell whether the auto activation check
for NMDevice is already scheduled and don't need to search through the
list.
It's the better name. Especially since there is no more signal involved,
the term "emit" doesn't match.
Note also how the previous approach using a signal tried to abstract
what is happening. So we were no longer rechecking-autoconnect, instead,
we were emitting-a-signal-to-recheck-autoconnect. Just be plain about
what it is doing and don't go through a layer of signal.
GObject signals don't make the code easier to understand, on the
contrary. They may have their purpose, when objects truly must/should
not be aware of each other, and need to be composed very loosely. That
is not the case here.
There really is only one subscriber to NM_DEVICE_RECHECK_AUTO_ACTIVATE
signal, and it only makes sense this way. Instead of going through a
signal invocation, just call the well known method directly. It becomes
clearer who calls this code (and it has a lower overhead).
When using cscope/ctags it also is easier to follow the code because the
tools understand function calls.
The delete_on_deactivate_link_delete() handler may be called after the
device was already removed from NMManager. Don't allow that.
Check whether the device is still exported on D-Bus as indication.
NM_reboot_openvswitch_vlan_configuration_var2 test exposes a race. What
the test does, is to create OVS profiles and repeatedly restart
NetworkManager, checking that those profiles autoconnect and the OVS
configuration gets created.
There is a race, where:
- the OVS interface exists, and an NMDeviceOvsInterface is created
- first ovsdb cleans up old interfaces, sending a json request.
- OVS deletes the interface, and NetworkManager first picks up the
platform signal (there is a race here, usually the ovsdb request
completes first, which will cleanup the NMDeviceOvsInterface in
a different way).
- when the device gets unrealized, we don't schedule a
check-autoactivate, so the device stays down.
See https://bugzilla.redhat.com/show_bug.cgi?id=2152864#c5 for a log
file with more details.
What should instead happen, is to autoactivate the OVS interface, which
then also fully configures the port and bridge interfaces.
Explicitly schedule an autoactivate when unrealizing devices.
Note that there are now several cases, where NetworkManager autoconnects
more eagerly. This even affects some CI tests and user-visible behavior.
But I think relying on "just don't call nm_device_emit_recheck_auto_activate()
to hope that autoconnect doesn't happen is wrong. It must always be
possible to trigger an autoconnect check, and the right thing must
happen. We only don't trigger autoconnect checks *all* the time, because
it would be a waste of CPU resources, but whenever we slightly suspect
that an autoconnect may happen, we must be allowed to trigger a check.
If a device is in a condition where it previously did not autoconnect,
and it also *should* not autoconnect, then we need to fix the code that
evaluates whether an autoconnect may happen (not avoid triggering a
check).
https://bugzilla.redhat.com/show_bug.cgi?id=2152864
Fixes-test: @NM_reboot_openvswitch_vlan_configuration_var2
Currently, when we delete a device then autoconnect does not kick in
right away. But that is only, because we happen not to schedule a
"autoactivate" recheck.
What should be happen, is that rechecking whether to autoconnect is
always allowed, and that we have the necessary state to know that
autoconnect currently should not work.
Instead, block autoconnect of the involved profile. That makes sense,
because clearly we don't want to autoconnect right again after `nmcli
device delete $iface`.
<error> is mostly about "really should not happen" scenarios. It's
closer to an assertion failure, and something that NetworkManager should
not happen.
Of course, things can go wrong, but <warn> is a sufficient. When ovsdb
gives unexpected communication, it's just a warning. At least, that's
also what all the similar cases in "nm-ovsdb.c" already do
GSocketConnection/GOutputStream/GInputStream seems rather unnecessary.
Maybe they make sense when you want to write portable code (for
Windows). Otherwise, watching a file descriptor and reading/writing it
directly is simpler (and also more efficient).
For example, we passed no GCancellable to g_input_stream_read_async().
What does that mean w.r.t. destroying the NMOvsdb instance? I suspect
it's wrong, but it's hard to say, because there are so many layers of
code.
Note that we anyway keep state in NMOvsdb, namely the data we want to
send (output_buf) and the data we partially received (input_buf). All we
need, are poll notifications when the file descriptor is ready. To
those, we hook up the read/write callbacks. Also before was the code
async, and there were callbacks when when read/write was done. That does
not simplify the code in any way.
- we no longer use separate NMOvsdbPrivate.buf and NMOvsdbPrivate.input
buffers. There is just a NMOvsdbPrivate.input_buf that can we can fill
directly.
The "priv->bufp" offset is only used while parsing a message at a time.
It's unnecessary to track it in NMOvsdbPrivate and keep it between
parsing messages. Tracking the state in NMOvsdbPrivate makes it more
complicated to understand, because one needs to reason at which times
the state is used (when it really is not used).
Also, move the parsing to a separate function.
The device shouldn't change state from DEACTIVATING to DISCONNECTED
until its detached from its controller; otherwise, the port detach
that is in progress can conflict with the following activation.
This changes the signature of detach_port() to be asynchronous,
similarly to attach_port(). The implementation can return TRUE/FALSE
on immediate completion.
Current implementations return immediately and so there is no change
in behavior for now.
