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.
nm_settings_add_connection_dbus() has two callers. One of them is NMManager
during AddAndActivate. In this case, the NMActiveConnection already created
an auth-subject. Re-use it.
Note how creating an auth-subject involves reading procfs to determine
whether the process still exists. This is not about the additional
overhead of that, but about the race where the process could drop
of in the meantime. The calling process might be gone now, and we would
fail creating the auth-subject. There is no need for that, because we
already evaluated all information we need. Quite likely, in the case
of this race, PolicyKit will also determine that the process is gone
and fail authorization too. But that's PolicyKit's decision to make,
not nm_settings_add_connection_dbus()'s.
For one, these flags are "internal" flags. Soon, we will gain
a new NMSettingsConnectionFlags type that is exported on D-Bus
and partly overlaps with these internal flags. However, then we
will need the "flags" properties to expose the public bits.
This property only exists because other parts are interested in
notification signals. Note that we encourage NMDbusObject types
to freeze/thaw property-changed notifications. As freezing the
notifications also delays the signals, this is not desired for
the purpose where internal users subscribe to the signal.
