"nm-device.c" is large and complicated. It's hard to find relevant places
that modify the ifindex,ip_ifindex,iface,ip_iface fields.
Mark them as const, to make that easier.
When NM fails to connect to teamd during an activation, it sets the
device state to FAILED. Eventually the device will become DISCONNECTED
and will call the ->deactivate() method that will perform the cleanup
of timers, teamd process and teamdctl instance.
However, in this way, when the device is DISCONNECTED timers are still
armed and can be triggered in the wrong state. Instead, perform the
cleanup immediately on failure.
https://bugzilla.redhat.com/show_bug.cgi?id=1856723
When using VRF devices we must pre-generate dependent local
routes in the VRF's table otherwise they will be incorrectly added
to the local table instead.
https://bugzilla.redhat.com/show_bug.cgi?id=1857133
Fixes: a199cd2a7d ('core: add dependent local routes configured by kernel')
Kernel will reject setting "active_slave", if the interface is not enslaved or not
up. We already handle that by setting the option whenever we enslave an interface.
However, we also must not set it initially, otherwise we get an ugly error log message:
NetworkManager[939]: <debug> [1594709143.7459] platform-linux: sysctl: setting net:/sys/class/net/bond99/bonding/active_slave to eth1 (current value is )
NetworkManager[939]: <error> [1594709143.7459] platform-linux: sysctl: failed to set bonding/active_slave to eth1: (22) Invalid argument
NetworkManager[939]: <warn> [1594709143.7460] device (bond99): failed to set bonding attribute active_slave to eth1
...
kernel: bond99: (slave eth1): Device is not bonding slave
kernel: bond99: option active_slave: invalid value (eth1)
See-also: https://bugzilla.redhat.com/show_bug.cgi?id=1856640https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/577
- the arp_ip_target option in the settings might not have normalized
IP addresses or duplicates. If there would be duplicates, setting
them twice would fail with EINVAL. Hence, first normalize them
and make them unique.
- if what we want to set is identical to what is already set, don't
do anything.
When dispose() is called, there can't be any pending operation because
they keep a reference to the device. Instead, there can be a a queued
operation not yet executed. Destroy it.
NM will now sync all tables when a connection has specified
at least 1 local route in 'ipv[4|6].routes' to correctly
reconcile local routes when reapplying connections on a device.
If the connection has no local routes only the main table will be
taken into account preserving the previous NM's behaviour.
https://bugzilla.redhat.com/show_bug.cgi?id=1821787
NetworkManager can't control the name of the PPP interface name
created by pppd; so it has to wait for the interface to appear and
then rename it. This happens in nm_device_take_over_link() called by
nm-device-ppp.c:ppp_ifindex_set() when pppd tells NM the ifindex of
the interface that was created.
However, sometimes the initial interface name is already correct, for
example when the connection.interface-name is ppp0 and this is the
first PPP interface created.
When this happens, nm_device_update_from_platform_link() is called on
the NMDevicePPP and it sets the device ifindex. Later, when pppd
notifies NM, nm_device_take_over_link() fails because the ifindex is
already set:
nm_device_take_over_link: assertion 'priv->ifindex <= 0' failed
Make nm_device_take_over_link() more robust to cope with this
situation.
https://bugzilla.redhat.com/show_bug.cgi?id=1849386
nm_device_cleanup() can be called when the device no longer has an
ifindex. In such case, don't try to reset the MAC address as that
would lead to an assertion failure.
We already set the MAC of OVS interfaces in the ovsdb. Unfortunately,
vswitchd doesn't create the interface with the given MAC from the
beginning, but first creates it with a random MAC and then changes it.
This causes a race condition: as soon as NM sees the new link, it
starts IP configuration on it and (possibly later) vswitchd will
change the MAC.
To avoid this, also set the desired MAC via netlink before starting IP
configuration.
https://bugzilla.redhat.com/show_bug.cgi?id=1852106https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/483
When a user creates a ovs-interface with the same name of the parent
ovs-bridge, openvswitch considers the interface as the "local
interface" [1] and assigns the MAC address of the bridge to the
interface [2].
This is confusing for users, as the cloned MAC property is ignored in
some cases, depending on the ovs-interface name.
Instead, detect when the interface is local and set the MAC from the
ovs-interface connection in the bridge table.
[1] https://github.com/openvswitch/ovs/blob/v2.13.0/vswitchd/vswitch.xml#L2546
[2] https://github.com/openvswitch/ovs/blob/v2.13.0/vswitchd/bridge.c#L4744
On Ubuntu 20.10, we build against ModemManager 1.14.0 and get a compiler warning:
../src/devices/wwan/nm-modem-broadband.c: In function 'try_create_connect_properties':
../src/devices/wwan/nm-modem-broadband.c:492:2: error: 'MMModemCapabilityDeprecated' is deprecated [-Werror=deprecated-declarations]
492 | if (MODEM_CAPS_3GPP (ctx->caps)) {
| ^~
Suppress it.
An alternative would be to drop the flag entirely. It seems the flag
was never used (and never will be used). But if that's true, there is
little harm done checking it. If it's not true, we better keep checking
for older versions.
0cd76bf1c4
There are some APs that require a DHCP transaction before allowing
other traffic. This is meant to improve security by preventing the use
of static addresses. Currently we don't renew DHCP after roaming to a
new AP and this can lead to broken connectivity with APs that
implement the check described above. Also, even if unlikely, the new
AP could be in a different layer 3 network and so the old address
could be no longer valid.
Renew dynamic IP configuration after we detect the supplicant decided
to roam to a new AP. Note that we only trigger a DHCP client restart;
the DHCP client already implements the logic to renew the previous
address and fall back to a full request in case of NAK or timeout.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/449
Do what systemd does with sd_lldp_neighbor_get_chassis_id_as_string()
and sd_lldp_neighbor_get_port_id_as_string(). Maybe we should use the
systemd functions directly, however that is not done because the way
how we convert the values to string is part of our stable API. Let's not
rely on systemd for that.
Also, support SD_LLDP_CHASSIS_SUBTYPE_NETWORK_ADDRESS and SD_LLDP_PORT_SUBTYPE_NETWORK_ADDRESS
types. Use the same formatting scheme as systemd ([1]) and lldpd ([2]).
[1] a07e962549/src/libsystemd-network/lldp-neighbor.c (L422)
[2] d21599d2e6/src/lib/atoms/chassis.c (L125)
Also, in case we don't support the type or the type contains unexpected
data, fallback to still expose the LLDP neighbor, and convert the value
to a hex string (like systemd does). This means, lldp_neighbor_new()
in practice can no longer fail and the error handling for that can be
dropped.
There is one tiny problem: now as fallback we expose the
chassis-id/port-id as hex string. That means, if we in the future
recognize a new type, we will have to change API for those types.
The alternative would be to either hide the neighbor completely from the
D-Bus API (as previously done), or not expose the hex strings on D-Bus.
Neither seems very attractive, so expose the value (and reserve the
right to change API in the future).
For the ID of LLDP neighbors follow what systemd does (w.r.t. what it
consideres equality of two neighbors).
Note that previously we did almost the same thing. Except, we compared
priv->chassis_id and priv->port_id, but these values are string
representations of the original (binary value). Don't use the pretty
strings as ID but the original binary value.
An invalid destination address doesn't need to break the LLDL neighbor entirely.
In fact, systemd will already filter out such addresses. So in practice,
the neighbor always has a valid destination address.
There is thus no need to parse it already during lldp_neighbor_new().
When the instance is not running (after creation or after stop), there
is no need to keep the GHashTable around.
Create it when needed (during start) and clear it during stop. This
makes it slightly cheaper to keep a NMLldpListener instance around,
if it's currently not running.
NMDevice already keeps the NMLldpListener around, even after stopping
it. It's not clear whether the instance will be started again, so also
clear the GHashTable. Also, one effect is that if you initially were in
a network with many LLDP neibors, after stop and start, the GHashTable
now gets recreated and may not need to allocate a large internal array
as before.
We already rate limit change events by two seconds. When we notice
that something changed, we call data_changed_schedule().
Previously, that would immediately issue the change notification,
if ratelimiting currently was not in effect. That means, if we happen
go receive two LLDP neighbor events in short succession, then the
first one will trigger the change notification right away, while
the second will be rate limited.
Avoid that by always issue scheduling the change notification in
the background. And if we currently are not rate limited, with
an idle handler with low priority.
This changes the order to what the code did previously, before switching
from GVariantDict to GVariantBuilder. But it changes the actually
serialized order in the variant.
GVariantDict is basically a GHashTable, and during g_variant_dict_end()
it uses a GVariantBuilder to create the variant.
This is totally unnecessary in this case. It's probably unnecessary in
most use cases, because commonly we construct variants in a determined series
of steps and don't need to add/remove keys.
Aside the overhead, GHashTable also does not give a stable sort order,
which seems a pretty bad property in this case.
Note that the code changes the order in which we call
g_variant_builder_add() for the fields in code, to preserve the previous
order that GVariantDict actually created (at least, with my version of
glib).
The intermediate parsing step serves very little purpose.
The only use is to ensure that we always add the keys in a stable
order, but we can easily ensure that otherwise.
We only need to parse them to construct the GVariant. There is
no need to keep them around otherwise.
We still keep LldpAttrs array and don't construct the GVariant right
away. The benefit is that this way while parsing we set the array
fields, and afterwards, when we generate the string dictionary, the
keys are sorted.
Move the parsing of the LLDP attributes to a separate function.
In the next step, we will no longer keep all attribute around
and no longer parse them during lldp_neighbor_new().
One effect is that we can no longer (easily) declare the LLDP message as
invalid, if parsing the attributes fails. That makes IMO more sense,
because we should try to expose what little we could parse, and not
be forgiving to unexpected data. If we wanted, we still could hide such
neighbors entirely from being exposed, but that is not done, because
it seems better to expose the parts that were valid.
We actually only need to parse the attributes while constructing
the GVariant. In a first step decouple the tracking of the parsed
attributes from LldpNeighbor struct. More will follow.
Also, track sd_lldp_neighbor instance directly.
sd_lldp_neighbor is a perfectly reasonable container for keeping
track of the LLDP neighbor information. Just keep a reference to
it, and don't clone the data. Especially since the LLDP library
keeps a reference to this instance as well.
Also, to compare whether two neighbors are the same, it is sufficient
to only consider the raw data. Everything else depends on these fields
anyway.
This is only possible and useful becuase sd_lldp_neighbor is of course
immutable. It wouldn't make sense otherwise, but it also would be bad
design to mutate the sd_lldp_neighbor instances.
This couples our code slightly more to the systemd code, which we usually
try to avoid. But when we move away in the future from systemd LLDP library,
we anyway need to rework this heavily (and then too, we wouldn't want
to clone the data, when we could just share the reference).
Allocating and growing the buffer with realloc isn't really
complicated. Do that instead of using a CList.
Also, if there is only one element, then we can track it in-place.
NM_UTILS_LOOKUP_STR_DEFINE() is implemented via a switch statement.
You'd expect that the compiler could optimize that to plain lookup,
since all indexes are consecutive numbers. Anyway, my compiler doesn't,
so use the array ourself.
Note that NM_UTILS_LOOKUP_STR_DEFINE() is exactly intended to lookup
by enum/integer, if the enum values are not consecutive numbers. It may
not be best, when you can directly use the numbers as lookup index.
VARDICT sounds like it would be a variant of "a{sv}" type. But it
can be really any GVariant. Rename to make the type more generic.
This will be used to also hold a binary variant of type "ay".
