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".
Currently the LLDP parsing code uses GVariantBuild, which possibly does not
ensure a stable order of elements. Thus the test may not be stable.
However, that will be fixed very soon.
When a device is not marked as unmanaged, but also not actively managed
by NetworkManager, then NetworkManager will generate an in-memory
profile to represent the active state, if the device is up and
configured (with an IP address).
Such profiles are commonly named like "eth0", and they are utterly
confusing to users, because they look as if NetworkManager actually
manages the device, when it really just shows that somebody else configures
the device.
We should express this better in the UI, hence add flags to indicate
that.
In practice, such profiles are UNSAVED, NM_GENERATED, and VOLATILE. But
add an explicit flag to represent that.
https://bugzilla.redhat.com/show_bug.cgi?id=1816202
When there are two patch ports connected, each of them must reference
the other; however they can't be created in a single transaction
because they are part of different bridges (so, different
connections). Therefore, the first patch that gets activated will
always fail with "No usable peer $x exists in 'system' datapath" until
the second patch exists.
In theory we could also match the error message, however this doesn't
seem very robust as the message may slightly change in the future.
Commit 37e7fa38c2 ("nm-supplicant-interface: enable OWE security
when transition mode is available") adds the OWE security flag in
case a valid OWE transtition mode IE is present on the beacon.
It also removes the OWE security flag in case the Iinformation elements
of a beacon are updated and a OWE transition mode IE can't be found.
When a pure OWE AP updates it's Information Elements (e.g. BSS Load
Element), the OWE security flag is falsely removed.
Introduce a new NM_802_11_AP_SEC_KEY_MGMT_OWE_TM security flag and use
it exclusively for OWE transition mode. Don't use the
M_802_11_AP_SEC_KEY_MGMT_OWE security flag on transition-mode APs.
Signed-off-by: David Bauer <mail@david-bauer.net>
Systemd's LLDP client also internally tracks all neighbours, and it thus
already needs a maximum already. For systemd, that is currently 128.
We don't need to implement our own rate limiting on top of that,
because if we wouldn't trust the LLDP client to get this right,
it would be DoS-able already.
Also decrease the number of maximum neighbours from 4k to 128.
Note that already previously we wouldn't ever get more than 128
entries.
As commit ccfe5fec8d ('wifi: don't autoconnect to networks that have
never been successful') explains, Wi-Fi profiles only autoconnected
if they had no or a positive timestamp.
The problem that tried to solve is when a user accidentally clicks on a
Wi-Fi network in applet. Then the created profile may not be usable
(because of wrong credentials). To avoid indefinitely to autoconnect,
uch a profile will have a timestamp of 0, which prevents further
auto activations.
However, I find that problematic.
An important use case is pre-deploying profiles. In that case, the user
cannot set the timestamp, because the timestamp cache
/var/lib/NetworkManager/timestamps is internal, undocumented API. Also,
ifcfg-rh doesn't support the timestamp and anyway, does the timestamp
of the NMConnection does not get honored (only the one from the
timestamps file).
Maybe that could be an alternative solution here, to allow the user to
mark profiles as "I really want it to autoconnect". But that seems
unnecessary and wrong to me.
The problem really is that the user cannot do anything to ensure that
autoconnect will work tomorrow (short of editing the timestamps
database). The problem is that the property of whether a profile
every connected successfully is not in direct control of the user (it
depends on external conditions).
If the user has bogus profiles configured, those profiles should be
deleted (or autoconnect disabled) and not keep autoconnect blocked.
Also note that if you are at home and accidentally click on your
neighbour's Wi-Fi network, then you presumably still also have a working
profile to your own network. That usable profile will have a more
recent timestamp and be preferred during autoconnect already.
This reverts commit ccfe5fec8d.
https://bugzilla.redhat.com/show_bug.cgi?id=1781253
The autoneg/speed ethtool settings are important. If they are wrong,
the device might not get any carrier. Having no carrier means that
you may be unable to activate a profile (because depending on
configuration, carrier is required to activate a profile).
Since activating profiles are the means to configure the link settings
in NetworkManager, and activating a profile can be hampered by wrong link
settings, it's important to reset the "correct" settings, when deactivating
a profile.
"Correct" in this case means to restore the settings that were present
before NM changed the settings. Presumably, these are the right once.
Beyond that, in the future it might make sense to support configuring
the default link settings per device. So that NM will always restore a
defined, configured, working state. The problem is that per-device
settings currently are only available via NetworkManager.conf, which
is rather inflexible.
Also, when you restart NetworkManager service, it leaves the interface
up but forgets the previous setting. That possibly could be fixed by
persisting the previous link state in /run. However, it's not
implemented yet.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/356https://bugzilla.redhat.com/show_bug.cgi?id=1807171
nm_platform_ethtool_init_ring() only has one caller. It's simpler to
drop the function and implement it at the only place where it is needed.
Maybe there could be a place for a function to initialize NMEthtoolRingState,
one option after the other. However, at the moment there is only one
user, so don't implement it.
This fixes various minor issues:
- the function had a NMPlatform argument, although the argument
is not used. Thus function merely operates on a NMEthtoolRingState
instance and shouldn't have a nm_platform_*() name.
- nm_platform_ethtool_init_ring() returned a boolean, but all
code paths (except assertion failures) returned success.
- as the function returned an error status, the caller was compelled
to handle an error that could never happen.
- the option was specified by name, although we already have a more
efficient way to express the option: the NMEthtoolID. Also, the
caller already needed to resolve the name to the NMEthtoolID, so
there was no need to again lookup the ID by name.
Imagine you have a veth device. That device supports certain offload features
(like "ethtool.feature-rx-checksum") but doesn't support any ring
options. Even trying to read the current ring settings will fail.
If you try to activate that profile, NMDevice previously would always
try to fetch the ring options and log a warning and extra debugging
messages:
<trace> [1590511552.3943] ethtool[31]: ETHTOOL_GRINGPARAM, v: failed: Operation not supported
<trace> [1590511552.3944] ethtool[31]: get-ring: failure getting ring settings
<warn> [1590511552.3944] device (v): ethtool: failure getting ring settings (cannot read)
It does so, although you didn't specify any ring settings and there
was no need to fetch the ring settings to begin with.
Avoid this extra logging by only fetching the ring option when they
are actually required.
Setting the kernel token is not strictly necessary as the IPv6 address
is generated in userspace by NetworkManager. However it is convenient
for users to see that the value set in the profile is also set in the
kernel, to confirm that everything is working as expected.
The kernel allows setting a token only when 'accept_ra' is 1:
temporarily flip it if necessary. Unfortunately this will also
generate an additional Router Solicitation from kernel, but this is
not a big issue.
When a team device is assumed, we skip stage1 and imply that teamd is
already running. If this doesn't happen (for example because teamd was
manually stopped or because the interface was created in the initrd),
the team interface will continue processing traffic but will not react
to changes in the environment (e.g. carrier changes). Ensure that
teamd is running for assumed devices.
Usually stage1 is skipped for external or assumed devices. Add a
mechanism to call stage1 for all devices, similarly to what was
already done for stage2.
We are going to expose some of this API in libnm.
The name "gendata" (for "generic data") is not very suited. Instead,
call the public API nm_setting_option_*(). This also brings no naming
conflict, because currently no API exists with such naming.
Rename the internal API, so that it matches the API that we are going
to expose next.
We already have NMEthtoolID to handle coalesce options in a way that is
convenient programmatically. That is, we can iterate over all valid
coalesce options (it's just an integer) and use that in a more generic
way.
If NMEthtoolCoalesceState names all fields explicitly, we need explicit
code that names each coalesce option. Especially since NMEthtoolCoalesceState
is an internal and intermediate data structure, this is cumbersome
and unnecessary.
Thereby it also fixes the issue that nm_platform_ethtool_init_coalesce() has a
NMPlatform argument without actually needing it.
nm_platform_ethtool_init_coalesce() does not operate on a NMPlatform
instance, and should not have the appearance of being a method of
NMPlatform.
nm-device now applies ethtool ring settings during stage 2 "device
config" of the connection activation.
ring settings will be then restored (according to what the state
was before the connection got activated on the device) when the
connection is deactivated during the device cleanup.
One thing to be noted is that unset ring settings (in the profile)
will not be touched at all by NetworkManager so that if the NIC driver
sets some default values these will be preserved unless specifically
overridden by the connection profile.
https://bugzilla.redhat.com/show_bug.cgi?id=1614700
Only in one moment we need the old and requested settings together:
during _ethtool_coalesce_set(). But for that we shouldn't track both
states in "NMEthtoolCoalesceState".
Simplify "NMEthtoolCoalesceState" to only contain one set of options.
By tracking less state, the code becomes simpler, because you don't
need to wonder where the old and requested state is used.
When the interface is in IPv4 or IPv6 shared mode and the user didn't
specify an explicit zone, use the nm-shared one.
Note that masquerade is still done through iptables direct calls
because at the moment it is not possible for a firewalld zone to do
masquerade based on the input interface.
The firewalld zone is needed on systems where firewalld is using the
nftables backend and the 'iptables' binary uses the iptables API
(instead of the nftables one). On such systems, even if the traffic is
allowed in iptables by our direct rules, it can still be dropped in
nftables by firewalld.
