Initially I thought I would use this somewhere else. Didn't do so far,
but this seems a useful function to have on its own because also
NMSettings is concerned about the relative priority of plugins.
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
The file got a wider scope to contain generic meta data about profiles.
Rename the internal API to reflect that (and be consistend with the
naming of the files).
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.
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.
The function only has one caller and it should be simple enough
to perform the necessary steps right in nms_ifcfg_rh_writer_write_connection().
More functions don't (always) simplify the code.
- most connections are not Wi-Fi connections and thus don't have a seen-bssids
list. Only create the seen_bssids hash when required. This avoids allocating the
hash in common cases and avoids checking the hash for the content (which is often
empty).
- nm_settings_connection_get_seen_bssids() should return a sorted list.
Leaving the sort order undefined is ugly.
- in try_fill_ssid_for_hidden_ap(), we need to check all
NMSettingsConnection instances whether they know this bssid.
Reorder the checks, to first call nm_settings_connection_has_seen_bssid(), which
is faster and in most cases returns a negative result (shortcutting
the rest).
The function determines the filename automatically, but we
need to blacklist certain names.
That is, because NetworkManager keeps a list of loaded files
in memory. When writing a new file, we really want to choose
a filename that is not yet taken. For that we must not only
consider files on disk, but also files that existed on the last
time of loading.
- avoid cloing the basename. Determining the basename can be done conveniently
with strrchr().
- use cleanup macro for temporary variable.
- while in practice it should not happen, check that the colon in the name
of alias file names is not followed by another '/'.
nm_connection_verify() returns success for fully valid (normalized)
connections and also connections that are NM_SETTING_VERIFY_NORMALIZABLE.
We really want to fully normalize the profiles during add-and-activate.
The settings plugins are created by NMSettings when the plugin
gets loaded. There is no need for these instances to be singletons
or to have a singleton getter.
Also, while in practice we create a settings plugin instance of
each type only once, there is nothing that would prevent creating
multiple instances. Hence, having a singleton getter is not right.
What is however useful, is to track them and block shutdown
via nm_shutdown_wait_obj_register*(). While the actual waiting
is not yet implemented, we should mark the plugin instances to
block shutdown (in the future).
Logging pointer values is useful to identify the object in the logging message.
But plain pointer values also can be used to defeat ASLR and should not be logged.
Instead, print NM_HASH_OBFUSCATE_PTR() value, which is a 64 bit number based on
the pointer value and some random seed. A minor problem is that there is still the
chance of duplicates, albeit small.
The functionality of the ibft settings plugin is now handled by
nm-initrd-generator. There is no need for it anymore, drop it.
Note that ibft called iscsiadm, which requires CAP_SYS_ADMIN to work
([1]). We really want to drop this capability, so the current solution
of a settings plugin (as it is implemented) is wrong. The solution
instead is nm-initrd-generator.
Also, on Fedora the ibft was disabled and probably on most other
distributions as well. This was only used on RHEL.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1371201#c7
The previous logic seems complicated to me. I even think it is wrong.
Rework it, I think this makes sense.
Also, previously the existing path was used if the file didn't exist.
I think that is wrong. If for force a rename, then the filename must
not be used even if the file currently does not exist.
Also add an "allow_filename_cb" argument, to reject filenames that
are blacklisted.
The keyfile plugin is special. For one, NetworkManager will always load
it.
In the future, only this plugin should handle in-memory connections.
In-memory connections are kinda special, and we don't need general
plugins to be concerned about them. They should be handled by keyfile
plugin.
But then NMSettings needs to have a reference to the keyfile plugin
instance at hand.
Note that now the empty list will be represented as %NULL instead of an
empty strv array.
That makes no difference in pratice. The main use of this property is as
glue for NMDBusManager to expose the property on D-Bus. Thereby it uses
g_dbus_gvalue_to_gvariant() which handles %NULL just fine.
NMManager and NMSettings both may have multiple authorization requests
ongoing. They need to keep track of them, at the very least to be able
to cancel them on shutdown.
Since NMAuthChain is not ref-countable, it always has only one clear
user/owner. It makes little sense otherwise. Since most callers already
want to track their NMAuthChain instances, let NMAuthChain help with that.
Embed a "parent" CList field inside NMAuthChain. This avoids requiring
an additional GSList allocation to track the element. Also, it allows to
link and append an element without iterating the list.
This ties the caller and the NMAuthChain a bit tighter together (making them
less indepdendent). Generally that is not desirable. But here it seems the
logic (of tracking the NMAuthChain) is still trivial and well separated.
It's just that NMAuthChain instances now can be linked in a CList.
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.
nm_device_check_connection_compatible() is potentially expensive.
Check first whether the connection candidate is of a relevant type,
hoping that this check is cheaper and thus shortcuts other checks
early.
NMSettings is complicated enough. We should try to move independent code out
of it, so that there is only logic that is essential there.
While at it, rework how we copy the GSList items. I don't like GSList as
a data structure, but there really is no need to allocate a new list.
Just unlink the list element and prepend it in the other list.
As nm_settings_plugin_initialize() could not fail (it returned no value indicating
failure), there is no reason to explicitly call this. Instead just
initialize the plugin when needed.
Also, we don't need the plugin to initialize early before nm_settings_plugin_get_connections().
Instead of
<info> [1558284380.2045] settings: Loaded settings plugin: SettingsPluginIfcfg ("/usr/lib64/NetworkManager/1.19.2/libnm-settings-plugin-ifcfg-rh.so")
log
<info> [1558284380.2045] settings: Loaded settings plugin: ifcfg-rh ("/usr/lib64/NetworkManager/1.19.2/libnm-settings-plugin-ifcfg-rh.so")
Note how `man NetworkManager.conf` documents "main.plugins" configuration
option where settings plugins have names like "keyfile" and "ifcfg-rh".
It's not helpful to log the GObject type name, which is an implementation
detail.
It was only kept to compare whether we loaded the same
plugin multiple times.
Note that load_plugins() already checks for duplicate plugin names,
so it actually could not happen that we tried to load the same file
more than once.
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.
This code is now unused.
Also, it does not seem state of the art to me
anymore.
Drop it, it could always be resurrected if need by, but maybe
GFileMonitor could be used instead.
It's deprecated and off by default for a long time.
It is bad to automatically reload connection profiles. For example, ifcfg
files may consist of multiple files, there is no guarantee that we
pick up the connection when it's fully written.
Just don't do this anymore.
Users should use D-Bus API or `nmcli connection reload` or `nmcli
connection load $FILENAME` to reload profiles from disk.
Before commit e3ac45c026 the reader set the private key in the
setting using the libnm function, which also set the key as client
certificate if it was in PKCS #12 format.
After the commit, existing connections with a PKCS #12 private key but
without a client certificate became invalid. Restore the old behavior.
Fixes: e3ac45c026 ('ifcfg-rh: don't use 802-1x certifcate setter functions')
Completely refactor the team/JSON handling in libnm's NMSettingTeam and
NMSettingTeamPort.
- team handling was added as rh#1398925. The goal is to have a more
convenient way to set properties than constructing JSON. This requires
libnm to implement the hard task of parsing JSON (and exposing well-understood
properties) and generating JSON (based on these "artificial" properties).
But not only libnm. In particular nmcli and the D-Bus API must make this
"simpler" API accessible.
- since NMSettingTeam and NMSettingTeamPort are conceptually the same,
add "libnm-core/nm-team-utils.h" and NMTeamSetting that tries to
handle the similar code side-by-sdie.
The setting classes now just delegate for everything to NMTeamSetting.
- Previously, there was a very fuzzy understanding of the provided
JSON config. Tighten that up, when setting a JSON config it
regenerates/parses all other properties and tries to make the
best of it. When modifying any abstraction property, the entire
JSON config gets regenerated. In particular, don't try to merge
existing JSON config with the new fields. If the user uses the
abstraction API, then the entire JSON gets replaced.
For example note that nm_setting_team_add_link_watcher() would not
be reflected in the JSON config (a bug). That only accidentally worked
because client would serializing the changed link watcher to
GVariant/D-Bus, then NetworkManager would set it via g_object_set(),
which would renerate the JSON, and finally persist it to disk. But
as far as libnm is concerned, nm_setting_team_add_link_watcher() would
bring the settings instance in an inconsistent state where JSON and
the link watcher property disagree. Setting any property must
immediately update both the JSON and the abstraction API.
- when constucting a team setting from D-Bus, we would previously parse
both "config" and abstraction properties. That is wrong. Since our
settings plugins only support JSON, all information must be present
in the JSON config anyway. So, when "config" is present, only the JSON
must be parsed. In the best case, the other information is redudant and
contributes nothing. In the worse case, they information differs
(which might happen if the client version differs from the server
version). As the settings plugin only supports JSON, it's wrong to
consider redundant, differing information from D-Bus.
- we now only convert string to JSON or back when needed. Previously,
setting a property resulted in parsing several JSON multiple times
(per property). All operations should now scale well and be reasonably
efficient.
- also the property-changed signals are now handled correctly. Since
NMTeamSetting knows the current state of all attributes, it can emit
the exact property changed signals for what changed.
- we no longer use libjansson to generate the JSON. JSON is supposed
to be a machine readable exchange format, hence a major goal is
to be easily handled by applications. While parsing JSON is not so
trivial, writing a well-known set of values to JSON is.
The advantage is that when you build libnm without libjansson support,
then we still can convert the artificial properties to JSON.
- Requiring libjansson in libnm is a burden, because most of the time
it is not needed (as most users don't create team configurations). With
this change we only require it to parse the team settings (no longer to
write them). It should be reasonably simple to use a more minimalistic
JSON parser that is sufficient for us, so that we can get rid of the
libjansson dependency (for libnm). This also avoids the pain that we have
due to the symbol collision of libjansson and libjson-glib.
https://bugzilla.redhat.com/show_bug.cgi?id=1691619
