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
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).
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 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.
Out of the 33 callers of nm_auth_chain_add_call(), the permission
argument is:
- 29 times a C string literal like NM_AUTH_PERMISSION_NETWORK_CONTROL.
- 3 times assign a string that is in fact a static string (it's just
not a string literal)
- only NMManager's device_auth_request_cb() passes a permission of
(possibly) non static origin. But it already duplicates the string
for it's own purposes and attaches it as user-data to the
NMAuthChain.
There really is no need to duplicate the string.
Replace nm_auth_chain_add_call() by a macro that ensures that the
permission string is a C literal.
Rename nm_auth_chain_add_call() to nm_auth_chain_add_call_unsafe() to
indicate that the lifetime of the permission argument is now the
responsibility of the caller.
NMAuthChain usually requests several permissions at once. Hence, an error
argument in the overall callback does not make sense, because you
wouldn't know which request failed.
If at all, it could only mean that the overall request failed (like an
D-Bus failure communicating to D-Bus *for all permisssions*),
but we don't need to handle that specially. In fact, we don't really care
why permission was not granted, whether it's due to an error or legitimate
reasons.
The error in the callback was always set to %NULL. Remove it.
NMAuthChain is not ref-counted.
You may call nm_auth_chain_destroy() once before the callback
gets invoked. This destroys the auth-chain instance right away.
You may call nm_auth_chain_destroy() once from inside the callback.
This basically has no effect but is allowed for convenince.
All this does is remembering that destroy was called and asserts that
destroy gets called at most once.
After the callback returns, the auth-chain will always be destroyed.
That means, generally there is no need to call nm_auth_chain_destroy()
from inside the callback.
Remove that code, and refactor some code to return early (where it makes
sense).
The boolean value is intended to indicate success. It would indicated
failure due to a bug.
Fixes: 297d4985ab ('core/dbus: rework D-Bus implementation to use lower layer GDBusConnection API'):
Only read the keyfile databases once and cache them for the remainder of
the program.
- this avoids the overhead of opening the file over and over again.
- it also avoids the data changing without us expecting it. The state
files are internal and we don't support changing it outside of
NetworkManager. So in the base case we read the same data over
and over. In the worst case, we read different data but are not
interested in handling the changes.
- only write the file when the content changes or before exiting
(normally).
- better log what is happening.
- our state files tend to grow as we don't garbage collect old entries.
Keeping this all in memory might be problematic. However, the right
solution for this is that we come up with some form of garbage
collection so that the state files are reaonsably small to begin with.
Fix the following assertion failure:
g_object_ref: assertion 'G_IS_OBJECT (object)' failed.
nm_settings_add_connection() can return a NULL connection.
Fixes: f034f17ff6 ('settings: keep the added connection alive for a bit longer')
"libnm-core" implements common functionality for "NetworkManager" and
"libnm".
Note that clients like "nmcli" cannot access the internal API provided
by "libnm-core". So, if nmcli wants to do something that is also done by
"libnm-core", , "libnm", or "NetworkManager", the code would have to be
duplicated.
Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
Note that:
0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
On the other hand, "libnm-libnm-core-aux.la" is not used by
libnm-core, but provides utilities on top of it.
1) they both extend "libnm-core" with utlities that are not public
API of libnm itself. Maybe part of the code should one day become
public API of libnm. On the other hand, this is code for which
we may not want to commit to a stable interface or which we
don't want to provide as part of the API.
2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
and thus directly available to "libnm" and "NetworkManager".
On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
and "NetworkManager".
Both libraries may be statically linked by libnm clients (like
nmcli).
3) it must only use glib, libnm-glib-aux.la, and the public API
of libnm-core.
This is important: it must not use "libnm-core/nm-core-internal.h"
nor "libnm-core/nm-utils-private.h" so the static library is usable
by nmcli which couldn't access these.
Note that "shared/nm-meta-setting.c" is an entirely different case,
because it behaves differently depending on whether linking against
"libnm-core" or the client programs. As such, this file must be compiled
twice.
(cherry picked from commit af07ed01c0)
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
Fixes a crash on failed AddAndActivate:
$ ip link set eth0 down
$ nmcli d conn eth0
Error: Failed to add/activate new connection: Connection 'eth0' is not available on device eth0 because device has no carrier
<NetworkManager crashes>
#3 0x000055555558b6c5 in _nm_g_return_if_fail_warning
#4 0x00005555557008c7 in nm_settings_has_connection
#5 0x0000555555700e5f in pk_add_cb
#6 0x0000555555726e30 in pk_call_cb
#7 0x0000555555726e30 in pk_call_cb
#8 0x0000555555726e30 in pk_call_cb
#9 0x00005555555aaea8 in _call_id_invoke_callback
#10 0x00005555555ab2e8 in _call_on_idle
https://github.com/NetworkManager/NetworkManager/pull/325
Before:
"manager: check_if_startup_complete returns FALSE because of eth0"
Now:
"manager: startup complete is waiting for device 'eth0' (autoactivate)"
Also, the logging line is now more a human readable sentence, but still
follows the same pattern as later
"manager: startup complete"
Meaning: grepping for "startup complete" becomes more helpful because
one first finds the reasons why startup-complete is not yet reached,
followed by the moment when it is reached.
The virtual function init() naturally leads to calling the wrapper
function nm_settings_plugin_init(). However, such ${TYPE}_init() functions
are generated by G_DEFINE_TYPE().
Rename to avoid the naming conflict, which will matter next, when the
interface will be converted to a regular GObject class.
Note that while these are settings plugin, there is no public
or stable API which we need to preserve.
NMConnection is an interface, which is implemented by the types
NMSimpleConnection (libnm-core), NMSettingsConnection (src) and
NMRemoteConnection (libnm).
NMSettingsConnection does a lot of things already:
1) it "is-a" NMDBusObject and exports the API of a connection profile
on D-Bus
2) it interacts with NMSettings and contains functionality
for tracking the profiles.
3) it is the base-class of types like NMSKeyfileConnection and
NMIfcfgConnection. These handle how the profile is persisted
on disk.
4) it implements NMConnection interface, to itself track the
settings of the profile.
3) and 4) would be better implemented via delegation than inheritance.
Address 4) and don't let NMSettingsConnection implemente the NMConnection
interface. Instead, a settings-connection references now a NMSimpleConnection
instance, to which it delegates for keeping the actual profiles.
Advantages:
- by delegating, there is a clearer separation of what
NMSettingsConnection does. For example, in C we often required
casts from NMSettingsConnection to NMConnection. NMConnection
is a very trivial object with very little logic. When we have
a NMConnection instance at hand, it's good to know that it is
*only* that simple instead of also being an entire
NMSettingsConnection instance.
The main purpose of this patch is to simplify the code by separating
the NMConnection from the NMSettingsConnection. We should generally
be aware whether we handle a NMSettingsConnection or a trivial
NMConnection instance. Now, because NMSettingsConnection no longer
"is-a" NMConnection, this distinction is apparent.
- NMConnection is implemented as an interface and we create
NMSimpleConnection instances whenever we need a real instance.
In GLib, interfaces have a performance overhead, that we needlessly
pay all the time. With this change, we no longer require
NMConnection to be an interface. Thus, in the future we could compile
a version of libnm-core for the daemon, where NMConnection is not an
interface but a GObject implementation akin to NMSimpleConnection.
- In the previous implementation, we cannot treat NMConnection immutable
and copy-on-write.
For example, when NMDevice needs a snapshot of the activated
profile as applied-connection, all it can do is clone the entire
NMSettingsConnection as a NMSimpleConnection.
Likewise, when we get a NMConnection instance and want to keep
a reference to it, we cannot do that, because we never know
who also references and modifies the instance.
By separating NMSettingsConnection we could in the future have
NMConnection immutable and copy-on-write, to avoid all unnecessary
clones.
Note the special error codes NM_UTILS_ERROR_CONNECTION_AVAILABLE_*.
This will be used to determine, whether the profile is fundamentally
incompatible with the device, or whether just some other properties
mismatch. That information will be importand during a plain `nmcli
connection up`, where NetworkManager searches all devices for a device
to activate. If no device is found (and multiple errors happened),
we want to show the error that is most likely relevant for the user.
Also note, how NMDevice's check_connection_compatible() uses the new
class field "device_class->connection_type_check_compatible" to simplify
checks for compatible profiles.
The error reason is still unused.
The NMSettings instance can't be disposed while there is any exported
connection. Ideally we should unexport all connections on NMSettings'
disposal, but for now leak @self on termination when there are
connections alive.
This fixes the following bug on shutdown:
assertion failed: (c_list_is_empty (&priv->connections_lst_head))
#0 raise () from target:/lib64/libc.so.6
#1 abort () from target:/lib64/libc.so.6
#2 g_assertion_message (domain=0x66cab2 "NetworkManager", file=0x6a5e48 "src/settings/nm-settings.c", line=1929)
#3 g_assertion_message_expr () at gtestutils.c:2555
#4 finalize (object=0x1dab170) at src/settings/nm-settings.c:1929
#5 g_object_unref (_object=0x1dab170) at gobject.c:3340
#6 dispose (object=0x1de50b0) at src/nm-manager.c:7139
#7 g_object_unref (_object=0x1de50b0) at gobject.c:3303
#8 _nm_singleton_instance_destroy () at src/nm-core-utils.c:138
#9 _dl_fini () from target:/lib64/ld-linux-x86-64.so.2
#10 __run_exit_handlers () from target:/lib64/libc.so.6
#11 exit () from target:/lib64/libc.so.6
#12 main (argc=<optimized out>, argv=<optimized out>) at src/main.c:460
https://bugzilla.redhat.com/show_bug.cgi?id=1579858
Use the path instead. This drop an useless use of the "name" property,
which is, coincidentally also wrong. (We use "ibft" in the plugin path
whereas the property is set to "iBFT".)
It's actually annoying, useless and wraps over even on wide displays.
Let's make it consistent with the log line we use for device plugins.
Also, this drops the last use of the "info" property and one useless use
of the "name" property.
There is no need to perform a lookup by path. NMSettings is a singleton,
it has the connection exactly iff the connection is linked.
Also add an assertion to double-check that the results agree with
the previous implementation.
