The DHCP client has potentially a large number of options,
including boolean options (flags). It is cumbersome to implement
them one by one. Instead, make more prominent use of NMDhcpClientFlags.
Previously, we used nm_udev_utils_property_as_boolean(), which was
taken from g_udev_device_get_property_as_boolean(). That function
accepts "1" and "true" (with ASCII case insensitive).
When we parse a flag, there is no need to reject "no", "yes" or
"on"/"off" as invalid (and thus return FALSE). We have a boolean
parse method _nm_utils_ascii_str_to_bool(), which parses everything
that nm_udev_utils_property_as_boolean() accepts, and more.
Be liberal in what we accept, so use our general parse function.
g_key_file_set_comment() has slightly weird API that will fail to set a
comment if it doesn't find the group. This is the case here since we
haven't set any strings under the [Security] group yet.
Fixing this is kind of ugly, so for now just don't add that comment in
the case where we don't have the [Security] group.
There are cases where the settings didn't actually change and we just
want to ensure NM and iwd settings are in sync (one such case would be
when a setting was changed while iwd wasn't running, here we want to
synchronize all settings when starting up iwd).
We're already doing this and calling sett_conn_changed() from
mirror_connection() on all connections after adding an interface, the
actual settings synchronization code doesn't get executed though because
we're passing 0 as update_reason, which means we bail out early from
sett_conn_changed().
To make sure we actually update the iwd network configurations in that
case, too, pass UPDATE_REASON_UPDATE_NON_SECRET as the update reason to
sett_conn_changed(), which is the correct update reason in this case.
Quite obviously, we want to update the AutoConnect setting of the iwd
network in case the NM and iwd settings differ, not if they are the
same. So check for unequality here instead of equality, which fixes the
AutoConnect setting's synchronization.
Fixes: 4229c97012 ('iwd: Mirror NM connections to IWD network config files'):
Apparently moving secrets between priv->connection and
priv->system_secrets in the various places throughout
NMSettingsConnection is no longer needed and has no effect on the
state of the D-Bus object or the gobject visible from outside. It
seems that it was needed for the secrets handling in NMDevice
subclasses before the introduction of the applied connection concept
but now nm_connection_need_secrets() is called in those subclasses
directly on the applied connection object and the secrets obtained
from multiple nm_settings_connection_get_secrets calls are also
collected directly in the applied connection's settings.
Drop the system secrets cache mechanism as a cause of a minor memory
overhead, some code overhead and also a source of some unneeded gobject
signals as the connection settings were being updated.
Note: the NM_SETTINGS_CONNECTION_UPDATE_REASON_CLEAR_SYSTEM_SECRETS and
NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS flags in the
SettingsConnection update signals appear to only have been used by the
SettingsConnection internally to keep priv->system_secrets up to date.
They can have potential other uses in the handlers of those signals so I
kept them. Unlike some of the other NMSettingsConnectionUpdateReason
values these are actual update *reasons* and not flags telling the
settings backends how to handle a specific change in the settings.
All usages of nm_settings_connection_clear_secrets() outside of the
NMSettingsConnection implementation were setting the
clear_cached_system_secrets parameter to FALSE which meant that the
operation was a no-op since the system-secrets cache kept a copy of the
secrets being cleared and any access to the SettingsConnection through
the D-Bus API or the class methods would behave the same as without the
call, with the exception of directly reading the settings from the
result of nm_settings_connection_get_connection(). The calls would
still generate D-Bus and gobject signals however, which were redundant.
Drop the method and its calls from the rest of NM code as not needed and
potentially confusing. The comments preceding these calls implied that
they were needed so that the next activation attempt would be forced to
use nm_settings_connection_get_secrets() but this was the case probably
only before the applied connection concept was introduced.
Also drop two nm_active_connection_clear_secrets() uses in
NMVpnConnection, right before the teardown of the active connection,
that could only possibly have any effect if they affected the
NMSettingsConnection, but as mentioned earlier the
nm_settings_connection_clear_secrets() use inside
nm_active_connection_clear_secrets() didn't do anything and is now
removed.
The one internal use of nm_active_connection_clear_secrets() in the
D-Bus ClearSecrets() implementation is inlined.
This patch is introducing the wired setting accept-all-mac-addresses
property. The value corresponds to the kernel flag IFF_PROMISC.
When accept-all-mac-address is enabled, the interface will accept all
the packets without checking the destination mac address.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Having two functions like link_set_x() and link_set_nox() it is not a
good idea. This patch is introducing nm_platform_link_change_flags().
This allow flag modification directly, so the developer does not need to
define the virtual functions all the time everywhere.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
This patch is introducing NM_DEVICE_INTERFACE_FLAG_PROMISC in
interface_flags. The flag represents IFF_PROMISC kernel flag.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Allow IP prefixes of 0 for routing rules, which are used for specifying a
route for all traffic. (e.g. route all traffic by default with separate
rules to exclude specific IP ranges)
#711
When the link goes away the manager keeps software devices alive as
unrealized because there is still a connection for them.
If the device is software and has a NM-generated connection, keeping
the device alive means that also the generated connection stays
alive. The result is that both stick around forever even if there is
no longer a kernel link.
Add a check to avoid this situation.
https://bugzilla.redhat.com/show_bug.cgi?id=1945282
Fixes: cd0cf9229d ('veth: add support to configure veth interfaces')
We need to handle the case that kernel mangles the configured values. We
already do, but there was a left over nm_platform_lnk_bridge_cmp() that
is still wrong.
Related: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/665
Fixes: ce9211500e ('platform/tests: work around rounding errors for bridge values in unit tests')
This ensures that the argument is some pointer to pointer. This gives a
bit of additional compile time safety, but in general, it still casts
any pointer to pointer (because that's what we require, as most arguments
won't be of type NMDBusObject to begin with).
NM should have been creating the IWD network config files with 0600
permission bits from the beginning since they can contain secrets.
g_key_file_save_to_file() uses 0666 which shouldn't be used even for the
temporary file before setting the final permissions.
Also try to preserve the last modification timestamp of the original
file because it is currently used by IWD when ranking networks for
autoconnect and updating it everytime NM rewrites the file could
potentially affect autoconnect priorities.
Extend nm_utils_file_set_contents to be able to optionally set the last
access + last modification times on the file being created, in addition
to the mode.
There was an attempt in the code to allow using existing system-owned
secrets based on whether the connection had ever succeeded before but
this wasn't implemented properly. Now decide whether existing secrets
are allowed and whether to pass the REQUEST_NEW flag to the secrets
request based on the last connection timestamp and on the network
security type (PSK vs. 802.1X) to align the policy with the policy
inside IWD.
Drop a useless nm_connection_clear_secrets call on the applied
connection just before failing the connection attempt and thus
destroying the applied connection.
Avoid saving agent-owned secrets when converting settings connections
to IWD config files and avoid reacting to NMSettingsConnection updates
that don't seem to touch any non-secret or system-owned-secret settings.
Along with NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS
and NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_AGENT_SECRETS, which can
be used in the NMSettingConnection's "updated" handlers to track secrets
updates, add NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET so
that the handlers can tell when something other than secrets has been
updated in the connection.
It can also potentially be used in _connection_changed_update in
src/core/settings/nm-settings.c to stop emitting the
NetworkManager.Settings.Connection.Updated() dbus signal if only secrets
are being updated (on agent queries etc.) if it is deemed to be correct.
NMLldpListener API was a (refcounted) GObject with start/stop methods.
That means, a listener instance itself had state, namely whether it was
running and which ifindex was used. And this was not only internal
state, but the user had to care about this.
That is all entirely unnecessary. Beside requiring more code and having
more overhead (of a GObject), it is also harder to use. NMDevice not
only need to care whether priv->listener is set, it also needs to care
whether it is running.
Simplify this. The NMLldpListener is no longer ref-counted. As such, the
notify callback is set in the constructor, and the user will stop
receiving notifications by destroying the instance. Furthermore, the instance
can only use one ifindex, that is determined at construct time too.
The state that NMLldpListener now represents is simpler. This simplifies
the usage from NMDevice, which now only call lldp_setup() to enable and
disable the listener.
There is also no need to restart the LLDP listener. The only exception
is, if the ifindex changes. In that case, we throw away the old instance
and create a new one. Otherwise, the LLDP listener is itself responsible
to keep running. There is no excuse for it to fail, and if it does, it needs
to autorecover as good as it can.
It's not clear why we would need to restart the instance. It
is supposed to work, and recover automatically.
The only thing that restarting should be necessary, is to change the
ifindex. But this is not the right place for handling changes of ifindex.
For certain options, kernel stores the numeric values in jiffies scale,
while the user space value is in USER_HZ (1/100th of a second) scale.
Jiffies scale depends on HZ setting (CONFIG_HZ), and depending on kernel
configuration its 100, 250, 300, or 1000.
That means, the round trip of clock_t_to_jiffies()/jiffies_to_clock_t()
has different rounding errors, depending on CONFIG_HZ and it maybe be
+/- 1 of the requested value.
Since the rounding error depends on CONFIG_HZ, we cannot find "good"
values for testing, that always behave the same. So we need to
workaround that.
Normalize the bridge values, if they look as if the value was mangled
due to rounding.
Related: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/665
NM_DHCP_STATE_DONE is for when the client reports that it is shutting
down. If we manually stop it, we should set the TERMINATED state, so
that NMDevice doesn't start a grace period waiting for a renewal.
This fixes the:
device (enp1s0): DHCPv4: trying to acquire a new lease within 90 seconds
message printed when NM is shutting down.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/802
build_message() is an internal helper function with a very specific
purpose.
Let's change it to take an NMStrBuf argument for generating
the string. The advantage is that we don't need to allocate and
free the buffers in between, but can just reuse it.
When using a GValue, we really should call g_value_unset(). Otherwise
it is a code smell, even if we technically only created GValue with
static strings and integers.
But changing that is not easy, because the AuditField structs are
allocated on the stack, and in different functions. So we cannot just
pass a GDestroyNotify to GPtrArray to cleanup all those fields, because
by then they will be out of scope.
The proper solution would be to heap allocate the AuditField struct, add
them to the GPtrArray, and free them with the free function. But that
seams really unnecessary overhead, for something that is correct in
practice. Let's accept the fact that by the time the fields array gets
destroyed, it contains dangling pointers.
If we already embrace the dangling pointers and that stuff is allocated
on the stack and that we don't need to free, also get rid of GValue
and use our plain NMValueType and NMValueTypUnion. GValue really doesn't
give us much here. And it only makes us wonder: is it OK to not call
g_value_unset()? With the plain tracking of the values, we know that
it is OK.
The "_lst" suffix already indicates that this is a list. We have
a list of ip-configs, so the prefix should be singular. It also matches
the "NMDnsConfigIPData.ip_config_lst" field.
The main reason for this renaming is that I want to search the file
for /ip_config_lst/ and find both the list head and the list elements.
