Via Update2() D-Bus API there are three ways how a profile can be stored
(or migrated) to in-memory:
- NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY
- NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED
- NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY
With the recent rework of settings I dropped NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY
and it had the same meaning as NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED.
However, the way NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED was implemented is
problematic. The problem is that it leaves the profile on disk but creates an
in-memory representation which shadows the persistent storage. Later,
when storing the profile to disk again, a new filename is chosen.
This allows via D-Bus API to toggle between NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED
and NM_SETTINGS_UPDATE2_FLAG_TO_DISK, and thereby pilling up profiles on disk.
Also, there is no D-Bus API to do anything sensible with these leaked, shadowed
profiles on disk.
Note that if we have a read-only profile in /usr/lib or in ifupdown
plugin, then the problem is not made any worse. That is, because via D-Bus
API such profiles can be made in-memory, and afterwards stored to /etc.
Thereby too the profile gets duplicate on disk, but this game only
works once. Afterwards, you cannot repeat it to create additional
profiles on disk. It means, you can only leak profiles once, and only
if they already exist in read-only storage to begin with.
This problem with NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED already existed
before the settings-delegate-storage rework, and is unrelated to whether in-memory
profiles now happen to be persisted to /run.
Note that NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY is simple and does not suffer
from this problem. When you move a profile to in-memory-only, it gets deleted from
persistent storage and no duplication happens.
The problem is that NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED used to
forget about the profile that it shadows, and that is wrong.
So, first re-add proper support for NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY. This
works by remembering the "shadowed-storage" path for in-memory profiles.
When later saving such a profile to disk again, the shadowed-storage
will be re-used. Likewise, when deleting such a profile, the shadowed
storage will be deleted.
Note that we keep NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED and it
also remembers the shadowed storage (but without "owning" it). That means,
when such a profile gets saved to disk again, the orginal storage is
reused too. As such, during future updates it behaves just like
NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY. The difference is when deleting
such a profile. In this case, the profile is left on storage and a
tombstone gets written. So, how is this better than before and why even
keep this complicated flag?
First, we keep this flag because we really want the ansible role to be
able to do in-memory changes only. That implies being able to delete a
profile from NetworkManager's view, but not from persistent storage. Without
this flag there is no way to do that. You can only modify an on-disk profile
by shadowing it, but you could not delete it form NetworkManager's view
while keeping it on disk.
The new form of NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED is safe and avoids
the duplication problem because also for tombstones it remembers the original
"shadowed-storage". That is, when the profile gets recreated later via
D-Bus API AddConnection, then the re-created profile will still reference
and reuse the shadowed storage that it had before deletion.
When we make runtime only changes, we may leave the profile in persistent
storage and store a overlay profile in /run. However, in various cases we need
to remember the original path.
Add code to store and retrieve that path from the keyfile.
Note that this data is written inside the keyfile in /run. That is because
this piece of information really depends on that particular keyfile, and not
on the profile/UUID. That is why we write it to the [.nmmeta] section of the
keyfile and not to the .nmmeta file (which is per-UUID).
This patch only adds the backend to write and load the setting from
disk. It's not yet used.
It should be possible to add a profile with autoconnect blocked form the
start. Update2() has a %NM_SETTINGS_UPDATE2_FLAG_BLOCK_AUTOCONNECT flag to
block autoconnect, and so we need something similar when adding a connection.
As the existing AddConnection() and AddConnectionUnsaved() API is not
extensible, add AddConnection2() that has flags and room for additional
arguments.
Then add and implement the new flag %NM_SETTINGS_ADD_CONNECTION2_FLAG_BLOCK_AUTOCONNECT
for AddConnection2().
Note that libnm's nm_client_add_connection2() API can completely replace
the existing nm_client_add_connection_async() call. In particular, it
will automatically prefer to call the D-Bus methods AddConnection() and
AddConnectionUnsaved(), in order to work with server versions older than
1.20. The purpose of this is that when upgrading the package, the
running NetworkManager might still be older than the installed libnm.
Anyway, so since nm_client_add_connection2_finish() also has a result
output, the caller needs to decide whether he cares about that result.
Hence it has an argument ignore_out_result, which allows to fallback to
the old API. One might argue that a caller who doesn't care about the
output results while still wanting to be backward compatible, should
itself choose to call nm_client_add_connection_async() or
nm_client_add_connection2(). But instead, it's more convenient if the
new function can fully replace the old one, so that the caller does not
need to switch which start/finish method to call.
https://bugzilla.redhat.com/show_bug.cgi?id=1677068
When modifying a connection profile that happens to be active on a
device, then most of the changes don't take effect immediately.
Only after a full re-activation or reapply (nmcli device reapply)
does the configuration of the active device change (the
"applied-connection").
With two execptions: "connection.zone" and "connection.metered" take
effect immediately. I think this is historic, but also to facilitate
firewall-cmd to modify a profile and change the zone right away.
Anyway, I think it should be possible to modify a profile without
changes to the runtime. Add a flag to prevent reapplying these
properties right away.
https://bugzilla.redhat.com/show_bug.cgi?id=1677070
Rejecting %NULL for a "is-a" function can be annoying. Of course, %NULL is
not a valid name. But it's sufficient that the function just returns
%FALSE in that case, and not assert against the input not being %NULL.
Asserting might be useful to catch bugs, but rejecting %NULL as input
is more cumbersome to the caller than helping with catching bugs.
Something similar was also recently done for nm_utils_is_uuid().
We usually want to combine the fields from "struct timespec" to
have one timestamp in either nanoseconds or milliseconds.
Use nm_utils_clock_gettime_*() util for that.
Completely rework how settings plugin handle connections and how
NMSettings tracks the list of connections.
Previously, settings plugins would return objects of (a subtype of) type
NMSettingsConnection. The NMSettingsConnection was tightly coupled with
the settings plugin. That has a lot of downsides.
Change that. When changing this basic relation how settings connections
are tracked, everything falls appart. That's why this is a huge change.
Also, since I have to largely rewrite the settings plugins, I also
added support for multiple keyfile directories, handle in-memory
connections only by keyfile plugin and (partly) use copy-on-write NMConnection
instances. I don't want to spend effort rewriting large parts while
preserving the old way, that anyway should change. E.g. while rewriting ifcfg-rh,
I don't want to let it handle in-memory connections because that's not right
long-term.
--
If the settings plugins themself create subtypes of NMSettingsConnection
instances, then a lot of knowledge about tracking connections moves
to the plugins.
Just try to follow the code what happend during nm_settings_add_connection().
Note how the logic is spread out:
- nm_settings_add_connection() calls plugin's add_connection()
- add_connection() creates a NMSettingsConnection subtype
- the plugin has to know that it's called during add-connection and
not emit NM_SETTINGS_PLUGIN_CONNECTION_ADDED signal
- NMSettings calls claim_connection() which hocks up the new
NMSettingsConnection instance and configures the instance
(like calling nm_settings_connection_added()).
This summary does not sound like a lot, but try to follow that code. The logic
is all over the place.
Instead, settings plugins should have a very simple API for adding, modifying,
deleting, loading and reloading connections. All the plugin does is to return a
NMSettingsStorage handle. The storage instance is a handle to identify a profile
in storage (e.g. a particular file). The settings plugin is free to subtype
NMSettingsStorage, but it's not necessary.
There are no more events raised, and the settings plugin implements the small
API in a straightforward manner.
NMSettings now drives all of this. Even NMSettingsConnection has now
very little concern about how it's tracked and delegates only to NMSettings.
This should make settings plugins simpler. Currently settings plugins
are so cumbersome to implement, that we avoid having them. It should not be
like that and it should be easy, beneficial and lightweight to create a new
settings plugin.
Note also how the settings plugins no longer care about duplicate UUIDs.
Duplicated UUIDs are a fact of life and NMSettings must handle them. No
need to overly concern settings plugins with that.
--
NMSettingsConnection is exposed directly on D-Bus (being a subtype of
NMDBusObject) but it was also a GObject type provided by the settings
plugin. Hence, it was not possible to migrate a profile from one plugin to
another.
However that would be useful when one profile does not support a
connection type (like ifcfg-rh not supporting VPN). Currently such
migration is not implemented except for migrating them to/from keyfile's
run directory. The problem is that migrating profiles in general is
complicated but in some cases it is important to do.
For example checkpoint rollback should recreate the profile in the right
settings plugin, not just add it to persistent storage. This is not yet
properly implemented.
--
Previously, both keyfile and ifcfg-rh plugin implemented in-memory (unsaved)
profiles, while ifupdown plugin cannot handle them. That meant duplication of code
and a ifupdown profile could not be modified or made unsaved.
This is now unified and only keyfile plugin handles in-memory profiles (bgo #744711).
Also, NMSettings is aware of such profiles and treats them specially.
In particular, NMSettings drives the migration between persistent and non-persistent
storage.
Note that a settings plugins may create truly generated, in-memory profiles.
The settings plugin is free to generate and persist the profiles in any way it
wishes. But the concept of "unsaved" profiles is now something explicitly handled
by keyfile plugin. Also, these "unsaved" keyfile profiles are persisted to file system
too, to the /run directory. This is great for two reasons: first of all, all
profiles from keyfile storage in fact have a backing file -- even the
unsaved ones. It also means you can create "unsaved" profiles in /run
and load them with `nmcli connection load`, meaning there is a file
based API for creating unsaved profiles.
The other advantage is that these profiles now survive restarting
NetworkManager. It's paramount that restarting the daemon is as
non-disruptive as possible. Persisting unsaved files to /run improves
here significantly.
--
In the past, NMSettingsConnection also implemented NMConnection interface.
That was already changed a while ago and instead users call now
nm_settings_connection_get_connection() to delegate to a
NMSimpleConnection. What however still happened was that the NMConnection
instance gets never swapped but instead the instance was modified with
nm_connection_replace_settings_from_connection(), clear-secrets, etc.
Change that and treat the NMConnection instance immutable. Instead of modifying
it, reference/clone a new instance. This changes that previously when somebody
wanted to keep a reference to an NMConnection, then the profile would be cloned.
Now, it is supposed to be safe to reference the instance directly and everybody
must ensure not to modify the instance. nmtst_connection_assert_unchanging()
should help with that.
The point is that the settings plugins may keep references to the
NMConnection instance, and so does the NMSettingsConnection. We want
to avoid cloning the instances as long as they are the same.
Likewise, the device's applied connection can now also be referenced
instead of cloning it. This is not yet done, and possibly there are
further improvements possible.
--
Also implement multiple keyfile directores /usr/lib, /etc, /run (rh #1674545,
bgo #772414).
It was always the case that multiple files could provide the same UUID
(both in case of keyfile and ifcfg-rh). For keyfile plugin, if a profile in
read-only storage in /usr/lib gets modified, then it gets actually stored in
/etc (or /run, if the profile is unsaved).
--
While at it, make /etc/network/interfaces profiles for ifupdown plugin reloadable.
--
https://bugzilla.gnome.org/show_bug.cgi?id=772414https://bugzilla.gnome.org/show_bug.cgi?id=744711https://bugzilla.redhat.com/show_bug.cgi?id=1674545
We may want to store meta-data for a profile to disk. The immediate
need are "tombstones": markers that the particular UUID is shadowed
and the profile does not exist (despite being in read-only location).
Change the filename of these symlinks from
".loaded-${UUID}.nmconnection"
to
"${UUID}.nmmeta"
The leading dot is not desirable as tools tend to hide such files.
Use a different scheme for the filename that does not have the leading dot.
Note that nm_keyfile_utils_ignore_filename() would also ignore ".nmmeta"
as not a valid keyfile. This is just what we want, and influences the
choice of this file suffix.
Also, "nmmeta" is a better name, because this name alludes that there is
a wider use for the file: namely to have addtional per-profile metadata.
That is regardless that the upcoming first use will be only to store symlinks
to "/dev/null" to indicate the tombstones.
Note that per-profile metadata is not new. Currently we write the files
/var/lib/NetworkManager/{seen-bssids,timestamps}
that have a similar purpose. Maybe the content from these files could one
day be migrated to the ".nmmeta" file. The naming scheme would make it
suitable.
WireGuard's wq-quick configures such rules to avoid routing loops.
While we currently don't have an automatic solution for this, at least
we should support it via explicit user configuration.
One problem is that suppress_prefixlength is relatively new and kernel
might not support this attribute. That can lead to odd results, because
the NetworkManager is valid but it cannot be configured on the current
kernel. But this is a general problem, and we would require a general
solution. The solution cannot be to only support rule attributes that
are supported by the oldest possible kernel. It's not clear how much of
a problem there really is, or which general solution is required (if
any).
The tables "main", "local", and "default" have well known names.
Accept them as aliases when parsing the string representation of
the rule.
Note that iproute2 also considers /etc/iproute2/rt_tables for table
names. In particular, that allows a user to re-map the well-known names
like "main" to a different table. We never honor that file, and "main"
always means table 254.
Note that this only affects how we parse the string representation for
rules. As the representation is neither unique nor enforced to be normalized,
being more graceful here is no problem.
The point is of course that the user possibly has existing iproute2
scripts that use such keyword. This makes it simpler to copy & paste
the rule.
Some tools that NM can interact with (eg. openconnect) have added
automated support to handle TPM2-wrapped PEM keys as drop-in
replacements for ordinary key files. Make sure that NM doesn't reject
these keys upfront. We cannot reliably assume NM to be able to unwrap
and validate the key. Therefore, accept any key as long as the PEM
header and trailer look ok.
Initscripts already honor the DEVTIMEOUT variable (rh #1171917).
Don't make this a property only supported by initscripts. Every
useful property should also be supported by keyfile and it should
be accessible via D-Bus.
Also, I will soon drop NMSIfcfgConnection, so handling this would
require extra code. It's easier when DEVTIMEOUT is a regular property of
the connection profile.
The property is not yet implemented. ifcfg-rh still uses the old
implementation, and keyfile is not yet adjusted. Since both keyfile
and ifcfg-rh will both be rewritten soon, this property will be
implemented then.
GPtrArray allows direct lookup by index. Since the NMSettingWireless
API is based on lookup by index, this is a common operation.
Note that nm_setting_wireless_add_seen_bssid() is still O(n), meaning to
add n elements, it takes O(n^2). That's not great but no worse than
before.
The cases where GSList is the best choice for a data type are few.
nmtst_connection_assert_unchanging() registers to the changed signals
and asserts that they are not invoked. The purpose is that sometimes
we want to keep a reference to an NMConnection and be sure that it does
not get modified. This allows everybody to keep a reference to the very
same connection instance without cloning it -- provided they too promise
not to change it. This assert is to ensure that.
Note that NMSimpleConnection.dispose() clears the secrets and thus upon
destruction the assertion fails. At that point, the assertion is no longer
relevant, because the purpose was to ensure that no alive instances gets
modified. While destroying the instance, it's fine to modify it (nobody should
have a reference to it anymore).
This avoids the assertion failure when destroying a NMSimpleConnection with secrets
that is set with nmtst_connection_assert_unchanging().
At various places we only want to serialize agent-owned secrets. Without this
flag, we need to clone the setting first, then drop the secrets, then serialize
to D-Bus. Add a serialization flag to avoid that.
The name ("with") and the meaning of the flag is chosen in a way, that
there could be multiple such flags (NM_CONNECTION_SERIALIZE_WITH_SECRETS_NOT_REQUIRED),
and specifying at least one of them, would have the meaning to whitelist
flags of this kind. Specifying non of these "with" flags would have the
meaning of specifying *all*. Currently there is only one kind, so the name
and meaning is slightly counter intuitive.
- in _nm_connection_ensure_normalized() allow also to only check that
the UUID is as expected, without really resetting it.
- split the normalization part out of nm_connection_normalize() and
reuse it in _nm_connection_ensure_normalized(). As we already verified
the connnection, we know that normalization is due and don't need to
verify again.
VPN settings (for openconnect) can only be handled by the keyfile settings
plugin.
In any case, such special casing belongs to the settings plugin and not
"nm-settings.c". The reason is that the settings plugin already has an
intimate understanding of the content of connections, it knows which fields
exist, their meaning, etc. It makes sense special handling of
openconnect is done there.
See also commit 304d0b869b ('core: openconnect migration hack').
Unfortunately it's not clear to me why/whether this is still the
right thing to do.
For a "is" check, it's inconvenient to assert against the parameter
being %NULL. We should accept %NULL and just say that it's not a valid
uuid.
This relaxes previous API.
If the mode is one of '802.3ad', 'tlb' or 'alb' and the connection has
both 'arp_interval' and 'arp_ip_target' options, during normalization
we remove 'arp_interval' because unsupported in the current mode. The
connection then becomes invalid because 'arp_ip_target' requires
'arp_interval'.
Since 'arp_interval' and 'arp_ip_target' are mutually dependent, the
latter should also be unsupported for those bonding modes.
https://bugzilla.redhat.com/show_bug.cgi?id=1718173
Also, plan right away to backport this symbol all the way back to
1.14.8. As such, we only need to add it once, with the right linker
version "libnm_1_14_8".
But still, the symbols first appears on a major release 1.20.0.
It's rather limiting if we have no API to ask NMSettingEthtool which
options are set.
Note that currently NMSettingEthtool only supports offload features.
In the future, it should also support other options like coalesce
or ring options. Hence, this returns all option names, not only
features.
If a caller needs to know whether the name is an option name, he/she
should call nm_ethtool_optname_is_feature().
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
nmtst_get_rand_int() was originally named that way, because it
calls g_rand_int(). But I think if a function returns an uint32, it
should also be named that way.
Rename.
When we set the same JSON config twice in a row, the second time has
indeed no effect and we can just return right away (indicating that
no attributes changed).
However, that is not true, if we are in strict validating mode and
the JSON string just happens to be the same. In this case, we still
want to switch from strict validating mode to relaxed mode. Hence,
we should not return early but continue setting the property.
When parsing a NMSettingTeam/NMSettingTeamPort from GVariant, the JSON
"config" is always preferred. If that field is present, all other
attributes are ignored (aside from validating that the individual fields
are as expected).
The idea is that the JSON config anyway must contain everything that artificial
properties could. In this mode, also no validation is performed and the setting is
always accepted (regardless whether libnm thinks the setting verifies).
When a libnm client created the setting via the artificial properties,
only send them via D-Bus and exclude the JSON config. This turns on
strict validation server side.
Now we actually get validation of the settings:
$ nmcli connection add type team team.runner-tx-hash l3
Error: Failed to add 'team' connection: team.runner: runner-tx-hash is only allowed for runners loadbalance,lacp
this is obviously a change in behavior as previously all settings
would have been accepted, regardless whether they made sense.
For each artifical team property we need to track whether it was
explicitly set (i.e., present in JSON/GVariant or set by the user
via NMSettingTeam/NMSettingTeamPort API).
--
As a plus, libnm is now no longer concerned with the underling default values
that teamd uses. For example, the effective default value for "notify_peers.count"
depends on the selected runner. But libnm does not need to care, it only cares
wheher the property is set in JSON or not. This also means that the default (e.g. as
interesting to `nmcli -o con show $PROFILE`) is independent from other properties
(like the runner).
Also change the default value for the GObject properties of
NMSettingTeam and NMSettingTeamPort to indicate the "unset" value.
For most properties, the default value is a special value that is
not a valid configuration itself.
For some properties the default value is itself a valid value, namely,
"runner.active", "runner.fast_rate", "port.sticky" and "port.prio".
As far as NMTeamSetting is concerned, it distinguishes between unset
value and set value (including the default value). That means,
when it parses a JSON or GVariant, it will remember whether the property
was present or not.
When using API of NMSettingTeam/NMSettingTeamPort to set a property to the
default value, it marks the property as unset. For example, setting
NM_SETTING_TEAM_RUNNER_ACTIVE to TRUE (the default), means that the
value will not be serialized to JSON/GVariant. For the above 4
properties (where the default value is itself a valid value) this is a
limitation of libnm API, as it does not allow to explicitly set
'"runner": { "active": true }'. See SET_FIELD_MODE_SET_UNLESS_DEFAULT,
Note that changing the default value for properties of NMSetting is problematic,
because it changes behavior for how settings are parsed from keyfile/GVariant.
For team settings that's not the case, because if a JSON "config" is
present, all other properties are ignore. Also, we serialize properties
to JSON/GVariant depending on whether it's marked as present, and not
whether the value is set to the default (_nm_team_settings_property_to_dbus()).
--
While at it, sticter validate the settings. Note that if a setting is
initialized from JSON, the strict validation is not not performed. That
means, such a setting will always validate, regardless whether the values
in JSON are invalid according to libnm. Only when using the extended
properties, strict validation is turned on.
Note that libnm serializes the properties to GVariant both as JSON "config"
and extended properties. Since when parsing a setting from GVariant will
prefer the "config" (if present), in most cases also validation is
performed.
Likewise, settings plugins (keyfile, ifcfg-rh) only persist the JSON
config to disk. When loading a setting from file, strict validation is
also not performed.
The stricter validation only happens if as last operation one of the
artificial properties was set, or if the setting was created from a
GVariant that has no "config" field.
--
This is a (another) change in behavior.
The order of the fields in the JSON object does not really matter.
Note that with the recent rework the order changed. Before it was
arbitrarily, now it still is arbitrary.
Reorder again, to follow the same order as `man teamd.conf`.
