There is a mix of new /usr/lib/systemd/libsystemd-shared-239.so
(systemd-libs rpm) and old /usr/bin/udevadm (systemd-udev rpm) on
the system at the point NetworkManager's post scriptlet is run,
what causes warning messages when updating NetworkManager's version.
This commit fixes this.
https://bugzilla.redhat.com/show_bug.cgi?id=2012123
Make checkpatch.pl identify subtree merges in "git am"-formatted
patches and reconstruct the full path names based in the subtree root.
This fixes some spurious warnings for parts of the tree that use
different coding style from what we usually do.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/989
It doesn't actually work inside the root-less container...
Well, it works as far as starting to activate, before it
fails. That is still somewhat useful. So have it there...
The majority of times when I call this script, I want it to do the reformatting,
not the check-only mode. This is also because we use git, so I start with a
clean working directory and run the reformatting code. In the best case, there
is nothing to reformat, and all is good. I seldom want to only check.
Change the default of the script.
"nm-code-format.sh" is going to change the default behavior from "-n" to
"-i", that is, from check-only to reformat. Explicitly pass "-n" where
we want it.
There was always the idea that you could pass paths and filenames
to "nm-code-format.sh" to format only a subset. However, the script
also needs to honor files that should be excluded and don't need
formatting.
Previously, what was implemented via `git ls-files -- ':(exclude)...'`
command, but git-ls-files has a bug ([1]) and might not list all files.
Refactor and do the filtering ourselves.
[1] https://www.spinics.net/lists/git/msg397982.html
The only reason is consistency. The majority of times we
do use g_snprintf(). As there are no strong reasons to
prefer one over the other, prefer the one that use use
most of the time.
NetworkManager runs as root and has lots of capabilities.
We want to reduce the attach surface by dropping capabilities,
but there is a genuine need to do certain things.
For example, we currently require dac_override capability, to open
the unix socket of ovsdb. Most users wouldn't use OVS, so we should
find a way to not require that dac_override capability. The solution
is to have a separate, D-Bus activate service (nm-sudo), which
has the capability to open and provide the file descriptor.
For authentication, we only rely on D-Bus. We watch the name owner
of NetworkManager, and only accept requests from that service. We trust
D-Bus to get it right a request from that name owner is really coming
from NetworkManager. If we couldn't trust that, how could PolicyKit
or any authentication via D-Bus work? For testing, the user can set
NM_SUDO_NO_AUTH_FOR_TESTING=1.
https://bugzilla.redhat.com/show_bug.cgi?id=1921826
g_idle_add() is discouraged, and the checkpatch.pl script warns
about it.
Sometimes there is a legitimate use of it, when you want to always
schedule an idle action (without intent to cancel or track it). That
makes more sense for g_idle_add() than it does for g_timeout_add(),
because a timeout really should be tracked and cancelled if necessary.
Add a wrapper to rename the legitimate uses. This way, we can avoid the
checkpatch.pl warnings, and can grep for the remaining illegitimate uses.
The numeric source IDs exist from a time before 2000, when there
was only one "GMainContext" singleton instance. Nowadays, the source
ID is only relative to one GMainContext, and you'd have to track
that association yourself. Als, g_source_remove() requires an additional
hash lookup, when you could simply track the GSource instance from the
start.
This API should not be used anymore. Operate on GSouce instances
direclty and use API like
nm_clear_g_source_inst()
nm_g_idle_add_source()
nm_g_idle_souce_new()
nm_g_source_attach()
g_source_attach
g_source_destroy
g_source_unref
etc.
Note that if you don't care about to ever remove a source again, like
scheduling an idle action that should not be cancelled, then
g_idle_add(callback, user_data);
is fine. It is only problematic to do something with those numeric IDs.
checkpatch.pl would also flag those uses, but these are just warnings
and in the few cases where such a warning is emitted wrongly, it's find
to ignore them.
It's an example for how to use libnm and asynchronous API.
But it's also a script I will use to test activating many
profiles in parallel.
Also add a test script that creates many veth interfaces and connection
profiles. So now you can do:
sudo NUM_DEVS=100 contrib/scripts/test-create-many-device-setup.sh setup
./examples/python/gi/nm-up-many.py c-a{1..100}
and cleanup with
nmcli connection down c-a{1..100}
sudo contrib/scripts/test-create-many-device-setup.sh cleanup
Of course, be careful to do this on your production machine.
Changing "NetworkManager.conf" is problematic, because the package management
system will detect if the user modified the file and leave .rpmnew files (or
similar).
Still, we only recently modified the file already to mention Libera.Chat.
So now is the time for more rewording.
Ups, we actually still require libuuid. Actually, we only need to
to build the example script `examples/C/glib/add-connection-gdbus.c`.
The proper solution would be to make this an optional dependency.
So far this was not yet done. Also, libuuid is really an ubiquitous
dependency on Linux, so it's not really a problem to have this build
dependency, even if it's just to build the examples.
This reverts commit c0a3947ff9.
These subpackages existed before commit 886366d0fd ('contrib/rpm:
update spec file after renaming NM plugins') (2014, before 0.9.9.95).
rpm warns about unversioned obsoletes like:
It's not recommended to have unversioned Obsoletes: Obsoletes: NetworkManager-atm
It's not recommended to have unversioned Obsoletes: Obsoletes: NetworkManager-bt
These packages are so long gone by now, let's just drop the Obsoletes.
"dhcdbd" is gone since 2007. Drop it. Also, rpm doesn't really like
unversioned obsoletes and warns:
It's not recommended to have unversioned Obsoletes: Obsoletes: dhcdbd
We really only require "iptables" as build dependency to autodetect the
path where iptables is installed. On Fedora/RHEL, this is always /usr/sbin,
so we can just as well hard code this.
Alternatively, if the autodetection is really necessary, we would also require
a build dependency on /usr/sbin/nft. That seems a waste.
"/etc/NetworkManager/VPN" was historically the place for .name files for
VPN plugins. In the meantime, those should be under "/usr/lib/NetworkManager/VPN".
Still, NetworkManager honors (and possibly watches) the directory in
/etc. Mark the directory as %ghost.
The exact effect of this is not clear to me. It seems however right to
do, and works for my testing.
Since commit a447942fc0 ('contrib/rpm: rename package
"NetworkManager-config-routing-rules" to
"NetworkManager-dispatcher-routing-rules"'), the config-routing-rules
subpackage is gone.
This way to specify the version number with a variable parameter, causes
repeated messages in rpmdiff:
INFO NetworkManager-dispatcher-routing-rules changed from Obsoletes: NetworkManager-config-routing-rules < 1:1.32.0-0.2.el8 to Obsoletes: NetworkManager-config-routing-rules < 1:1.32.0-0.3.el8 on noarch
Avoid this by hard coding the obsoleted version.
This "Conflicts" is since commit b85b8ed6fa ('contrib/rpm: let
NetworkManager-libnm and NetworkManager-glib of differing version
conflict'). This was probably fine back then, but NetworkManager-glib is
long gone.
Also, not hard coding the version number leads to rpmdiff messages like:
NEEDS INSPECTION NetworkManager-libnm changed from Conflicts: NetworkManager-glib < 1:1.32.0-0.2.el8 to Conflicts: NetworkManager-glib < 1:1.32.0-0.3.el8 on all architectures
As NetworkManager-glib is long gone, hard code the version with which
we conflict.
On Fedora 33, we get it automatically because "clang" package
has an indirect (weak) dependency for clang-tools-extra. On
Fedora 34, that is no loger the case.
We need to explicitly install it.
When supported by the D-Bus daemon, it's better to have service files
in /usr rather than in /etc. Change the path for RHEL 8.
See also commit ef8c292881 ('contrib/rpm: install D-Bus service
files to /usr if we can').
We should write our CONTRIBUTING files in markdown syntax, because
it's nice to read a plain text and gets nicely rendered.
However, if the file doesn't have a ".md" extension, gitlab's
web interface shows it as plain text file.
Rename the file.
This possibly breaks links like [1], but referring to a branch name
(and not a commit ID or a tag) is anyway fragile. Hence, I don't try
to fix that by adding a symlink or similar, because I think that just
makes it more confusing.
[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/master/CONTRIBUTING
"--with test" does two things:
(1) it enables "-Werror" compiler option. We always enable all
compiler warnings we care about, but this option makes all
warnings fatal.
Compiler warnings depend on compiler version and build options.
It's hard to build without any compiler warnings, in particular
for *future* compiler versions which we don't know yet. It
is desirable that a SRPM from yesterday can also be build
tomorrow.
(2) it fails build if any unit tests fail. We always run all
unit tests, but "--with test" makes it fatal. Again, we
have many unit tests that interact with the system (that is,
make system calls, like creating IP addresses or write files).
It is surprisingly hard to get them pass 100% on all the systems
we care. For example, on copr a test setup randomly fails during
ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
nm_utils_ifname_cpy(ifr.ifr_name, TEST_IFNAME);
r = ioctl(fd, TUNSETIFF, &ifr);
It's not clear why, nor is it at all clear that there is a bug
in NetworkManager. Making tests fatal basically means that a build
on copr infrastructure fails with a probability from a few percent.
Enough to be seriously annoying.
Note that on copr we actually build "--with test", because we want to catch these
issues. Likewise for our CI builds we explicitly specify "--with test".
In general, we build with various build configurations (compiler warnings)
and run unit tests on a source package many times. Starting on the
developer machine (`make check`), gitlab-ci, copr builds,
NetworkManager-ci. If you build an SRPM with such sources, a failure
of the unit tests is much more likely a glitch than an actual issue.
This is about changing the default if you build a Fedora/RHEL package.
That is with the Fedora/RHEL packages that are build in koji/brew.
Well, at least usually. In practice, we don't build frequently on non
x64_86 archs, so what I said there is less true. But the package build
is not there to replace CI/testing. The package build is there to get
a (mostly) working binary.
Note that RHEL packages anyway go through rpmdiff too, and rpmdiff
parses the build log and complain if `make check` fails.
This reverts commit e68e5c0a4c.
Currently "src/" mostly contains the source code of the daemon.
I say mostly, because that is not true, there are also the device,
settings, wwan, ppp plugins, the initrd generator, the pppd and dhcp
helper, and probably more.
Also we have source code under libnm-core/, libnm/, clients/, and
shared/ directories. That is all confusing.
We should have one "src" directory, that contains subdirectories. Those
subdirectories should contain individual parts (libraries or
applications), that possibly have dependencies on other subdirectories.
There should be a flat hierarchy of directories under src/, which
contains individual modules.
As the name "src/" is already taken, that prevents any sensible
restructuring of the code.
As a first step, move "src/" to "src/core/". This gives space to
reorganize the code better by moving individual components into "src/".
For inspiration, look at systemd's "src/" directory.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/743
nm-cloud-setup is provided by sub-package "NetworkManager-cloud-setup",
which also has the manual page. The main package "NetworkManager" should
not also contain the manual page.
- accept directory names in the command line. In that case,
still honor the excluded files. That is a major improvement
for me, because I usually only want to reformat a directory
that I know has changed and it is fast to only process some
directories.
- pass all files at once to clang-format. For me that gives
a significant speed improvement (about 3 times faster), although
clang-format is only single threaded. Possibly clang-format could
even be faster by checking files in parallel.
In case of a style error, the script still falls back to
iterate over all files to find the first bad file and print
the full diff. But that is considered an unusual case.
- make it correctly work from calling it from a subdirectory.
In that case, we only check files inside that directory --
but still correctly honor the excluded files.
"shared/nm-std-aux/unaligned.h" is taken from systemd and frequently
re-imported via the "systemd" branch.
It is not our code, and should not be formatted with our clang-format.
Alpine is especially interesting because it uses musl as libc.
The build does not yet succeed. There are several issues that
need to be fixed.
However, it will be simpler to fix things, if we have tests
in place -- even if at the moment they are known to be broken.
See-also: https://git.alpinelinux.org/aports/tree/community/networkmanager?h=master
With Fedora 33+ and RHEL 9+, the default plugins are
"plugins=keyfile,ifcfg-rh", instead of "plugins=ifcfg-rh,keyfile".
Update our "NetworkManager.conf" file to reflect that.
"build_clean.sh" (and "build.sh") scripts can both create a source
tarball (via `make dist`/`make distcheck`), an SRPM (and a spec file),
or build RPMs from the SRPM.
Note that the generated spec file has various options, like
%bcond_without nmtui
%bcond_without debug
%bcond_without test
When building an RPM from the SRPM, you can specify the "--with" or
"--without" option for rpmbuild. This is also what the "-w" / "-W" options
for "build_clean.sh" do.
However, the SRPM still has the intrinsic defaults, and if you later
build an RPM from it, you would have to pass "--with" / "--without"
to rpmbuild.
Often that is not conveniently possible, for example, when you build the
SRPM in koji.
Extend the scripts so that also the defaults for "-w debug" and "-w
test" can be specified when generating the SRPM. You can do that with
the new options "--default-for-{debug,test}" to "build_clean.sh".
Alternatively, it suffices to specify the previously supported
"-w" / "-W" options. That way, we will pass those options to rpmbuild,
but also set them as defaults in the generate spec file. The new
options "--default-for-{debug,test}" are only needed if you want
the default in the spec file to be different then what you use
when creating the SRPM.
By default, "build_clean.sh" script likes to automatically add "-w test"
-- unless the user specified "-w test" or "-W test" on the command line.
That is mostly fine. However, the spec file has an internal default for the
"test" option. So if you want to use the default that gets determined
by the spec file, then we should suppress that automatism.
We always run the unit tests during package build and also enable all compiler
warnings. However, by default we used to ignore failures. That is, because
rebuilding a package on another, future distro led to frequent, annoying build
failures. Especially compiler warnings appear easily when using a
different compiler version.
The default mostly matters here when you want to build the package in
brew/koji, where you don't have a possibility to explicitly select the
build option.
Note that rpmdiff detects failures in the build log, and thus we usually
would not miss failures for builds we add to errata. Also, all our CI
tests build packages with a manner where they would not allow a failure
of the unit tests. So, we run these unit tests frequently and in a
manner where we notice a failure.
For rhel-9 builds, change the default here and let test failures and
compiler warnings be fatal to the build.
"find-backports" searches commit messages of upstream branches for
"Fixes:" comments. Those will then be highlighted to be backported,
if the script determines that to be necessary.
"find-backports" also honors the "cherry picked from" comments, to detect when
a patch was already backported. That is thus a way to suppress reporting a
commit to be backported.
Add another way to flag commits so they don't need backporting. Via
"Ignore-Backport:" tag.
As "find-backports" also honors "refs/notes/bugs" notes, this can be used
like:
git notes \
--ref refs/notes/bugs \
append \
-m "Ignore-Backport: e""29f00fa0c69 ('NEWS: fix entry that is targeted for 1.30 instead of 1.28')" \
2''3364aa8f3bd6b11e2ac9e30117eaabfe1f3a9f2
The checkpatch test tests the patches on the merg-request, as they
branch off from master (or one of the stable branches).
It thus need the full git history, but the git repository might be a
shallow clone. Fix it.
"debian/REQUIRED_PACKAGES" is used by gitlab-ci to prepare the image. We require
"udev" package, if only to install "/usr/share/pkgconfig/udev.pc" to get the
udev directory.
Otherwise build fails with:
Run-time dependency udev found: NO (tried pkgconfig)
meson.build:371:2: ERROR: Dependency "udev" not found, tried pkgconfig
These packages are also used during CI tests (in the checkpatch
stage). They are also used for formatting C/python code, and thus
useful for developing.
Install them as part of REQUIRED_PACKAGES script.
We already build a large variety of configurations in gitlab-ci,
we don't need yet another configuration to run tests on travis-ci.
Also, because the travis-ci setup is outdated and we don't look
at it. Let's focus on gitlab-ci instead.
We use clang-format to format our code, and the exact format depends on
the clang version. Currently we use clang-11, as packaged in Fedora 33.
Add a script that runs a Fedora 33 container with podman and reformats
the current working directory.
Usage:
./contrib/scripts/nm-code-format-container.sh
Our new format gets enforced by clang-format, and we now only use
four space indentation, instead of tabs.
Adjust the checkpatch script to account for that.
Also, now there are probably no cases left where we want to see any
tabs in our sources. Complain about any tabs we find.
Add a new `main.rc-manager=auto` setting, that favours to use
systemd-resolved (and not touch "/etc/resolv.conf" but configure
it via D-Bus), or falls back to `resolvconf`/`netconfig` binaries
if they are installed and enabled at compile time.
As final fallback use "symlink", like before.
Note that on Fedora there is no "openresolv" package ([1]). Instead, "systemd"
package provides "/usr/sbin/resolvconf" as a wrapper for systemd-resolved's
"resolvectl". On such a system the fallback to resolvconf is always
wrong, because NetworkManager should either talk to systemd-resolved
directly or not but never call "/usr/sbin/resolvconf". So, the special handling
for resolvconf and netconfig is only done if NetworkManager was build with these
applications explicitly enabled.
Note that SUSE builds NetworkManager with
--with-netconfig=yes
--with-config-dns-rc-manager-default=netconfig
and the new option won't be used there either. But of course, netconfig
already does all the right things on SUSE.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=668153
Suggested-by: Jason A. Donenfeld <Jason@zx2c4.com>
With Fedora 33, LTO will be enabled by default via CFLAGS in
redhat-rpm-config ([1]).
That basically sets "CFLAGS=-flto -ffat-lto-objects".
Note that we have our own configure/meson option to enable LTO.
With "--with-lto" we set CFLAGS="-flto -flto-partition=none". This
is necessary due ([2], [3]).
So, disable Fedora's automatism, but turn on the suitable configure
option to get working LTO.
[1] 5baaf4a99c?branch=master
[2] e6cf4213a7 ('build: fix building with LTO')
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48200#c28
This defaults to $DO_RELEASE. In that case, the script will also GPG sign
the source tarball.
The purpose is that when we do a release we want to ensure that the
published tarball is really the one that we generated. In that case,
the SHA sum would suffice, however that requires you to manually note
it down and compare the result. With the gpg signature, that
verification can be better automated.
Currently only "minor" and "devel" releases are implement. It's also not yet
tested with --no-dry-run, because that would actually create a release.
Test it when using it the next time.
On CentOS 8, many devel packages are not available. Even after
# dnf config-manager --set-enabled PowerTools
certain devel packages are missing. Some of these (libndp-devel,
mobile-broadband-provider-info-devel, teamd-devel) we build in copr
([1]), but libpsl-devel and qt-devel are still missing.
Only install them optionally and allow failure for them not being
present.
[1] https://copr.fedorainfracloud.org/coprs/nmstate/nm-build-deps/repo/epel-8/nmstate-nm-build-deps-epel-8.repo
When doing a release, we should care about the checksum of the tarball.
Log all of them... also, because fedpkg uses sha512, ftpadmin@gnome uses
sha256, etc.
error: bare words are no longer supported, please use "...": no != "yes"
error: ^
error: /builds/NetworkManager/NetworkManager/contrib/fedora/rpm/NetworkManager.20200418-170120.dp5cp5/SPECS/NetworkManager.spec:596: bad %if condition: no != "yes"
error: bare words are no longer supported, please use "...": no != yes
error: /builds/NetworkManager/NetworkManager/contrib/fedora/rpm/NetworkManager.20200418-163008.VM582H/SPECS/NetworkManager.spec:596: bad %if condition: no != yes
error: bare words are no longer supported, please use "...": "x" != x
error: ^
error: /root/nm-build/NetworkManager/contrib/fedora/rpm/NetworkManager.20200402-030113.Hk7EGs/SPECS/NetworkManager.spec:32: bad %if condition: "x" != x
ERROR: rpmbuild FAILED
We always build PolicyKit support, because it merely depends on some
D-Bus calls. However, there are two things to configure:
- the default value for main.auth-polkit in NetworkManager.conf. This
is now called "-Dconfig_auth_polkit_default=$VAL".
- whether to install the policy file. This is called "-Dpolkit=$VAL".
These settings are mostly independent, so add "config_auth_polkit_default" to
make the default explicitly configurable.
(cherry picked from commit c21c6bc0be)
The bluetooth plugin (with BlueZ5/NAP support) always gets
build, but DUN support requires a library.
When enabling build of the bluetooth subpackage, then always
enable DUN support. And enable it explicitly, especially meson
would not autodetect support and disable it by default.
(cherry picked from commit 30f6a5dd21)
The "ibft" plugin is no more. The default on RHEL/Fedora is now "ifcfg-rh[,keyfile]".
Adjust the configuration, because a wrong comment is confusing here.
Modifying configuration snippets is potentially annoying, because the user might
have edited the file, so on upgrade a "NetworkManager.conf.rpmnew" file
will be created. Still do it.
warning: extra tokens at the end of %endif directive in line 717: %endif # end autotools
warning: extra tokens at the end of %endif directive in line 775: %endif # end autotools
When opening a merge request from a fork of NetworkManager, then the
pipeline runs with the a checkout of the fork. That means, checkpatch
would compare the branch against "master" (or "nm-x-y" stable branches)
of the fork, instead of upstream.
That doesn't seem too useful. Instead, also add upstream NetworkManager
as git remote, fetch the branches, and use the branches from there as
base for checkpatch.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/255
The preferred logo and logotype are the ones in the main "logo" folder.
There are also few variants available in the "alternate" folder that are
allowed.
Main color for the logo is blue #32557dff.
The alternatei logo red is #cc0000ff.
The font is Montserrat.
Thanks to Máirín Duffy for all the help and support!
REQUIRED_PACKAGES has two uses:
- to setup a system for developing NetworkManager. This installs
convenience packages like "cscope".
- to install the packages required for unit testing in gitlab-ci.
For gitlab-ci we should only install the packages that we actually
need.
Previously, dnf/yum used to ignore packages that didn't exist.
In Fedora 32, dnf starts to fail the entire command:
No match for argument: python-gobject-base
Error: Unable to find a match: python-gobject-base
Since this script is supposed to work with different RHEL/Fedora
versions, it's expected that not all packages are available everywhere.
Fix that, by installing packages that we know that they might be missing
one by one (and ignore the error).
"NetworkManager-wifi" package requires either wpa_supplicant or iwd.
When installing the package without explicitly installing supplicant
or iwd (and not having it installed yet), then we want to drag in
wpa_supplicant by default. That is accomplished by suggesting wpa_supplicant
package.
Otherwise, the user installing NetworkManager-wifi might get iwd,
which is only functioning if the user explicitly enables the backend
in "NetworkManager.conf".
https://bugzilla.redhat.com/show_bug.cgi?id=1743585
This is a one-off hacky tool that we'll use to convert the long license
boilerplates to SPDX headers that are more friendly to automated tools.
Then we can drop it and forget it existed.
Nowadays, we should prefer "/run" over "/var/run". When not specifying
during ./configure, autotools however still defaults to "/var/run".
This default is also visible in the pre-generated documenation, for
example `man NetworkManager.conf` says
Unless the symlink points to the internal file /run/NetworkManager/resolv.conf,
in which case the ...
Let's enable the option to use IWD as an alternative to wpa_supplicant
for Wi-Fi support. People have been asking for this, it works, and is well
maintained.
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