The `ifcfg-rh` meson build file installs a new post install script
to create the `network-config` directory.
This has been moved to the main post install file so it's easier to
find because all post install steps are together and it avoids and
extra post install script execution.
The file has been fixed to be consistent with the rest of the files.
The data files to be installed have been grouped together. The
sourc files has been listed vertically and the link target in
`nm-settings-plugin-ifcfg-rh` does not use an array anymore.
The targets that involve the use of the `NetworkManager` library,
built in the `src` build file have been improved by applying a set
of changes:
- Indentation has been fixed.
- Set of objects used in targets have been grouped together.
- Aritificial dependencies used to group dependencies and custom
compiler flags have been removed and their use replaced with
proper dependencies and compiler flags to avoid any confussion.
The `libnm-core` build file has been improved by applying a set of
changes:
- Indentation has been fixed to be consistent.
- Library variable names have been changed to `lib{name}` pattern
following their filename pattern.
- `shared` prefix has been removed from all variables using it.
- Dependencies have been reviewed to store the necessary data.
- The use of the libraries and dependencies created in this file
has been reviewed through the entire source code. This has
required the addition or the removal of different libraries and
dependencies in different targets.
- Some files used directly with the `files` function have been moved
to their nearest path build file because meson stores their full
path seamessly and they can be used anywhere later.
The `nm-default.h` header is used widely in the code by many
targets. This header includes different headers and needs different
libraries depending the compilation flags.
A new set of `*nm_default_dep` dependencies have been created to
ease the inclusion of different directorires and libraries.
This allows cleaner build files and avoiding linking unnecessary
libraries so this has been applied allowing the removal of some
dependencies involving the linking of unnecessary libraries.
First of all, keyfile writer (and reader) are supposed to be able to store
every profile to disk and re-read a valid profile back. Note that the profile
might be modified in the process, for example, blob certificates are written
to a file. So, the result might no be exactly the same, but it must still be
valid (and should only diverge in expected ways from the original, like mangled
certificates).
Previously, we would re-read the profile after writing to disk. If that failed,
we would only fail an assertion but otherwise proceeed. It is a bug
after all. However, it's bad to check only after writing to file,
because it results in a unreadable profile on disk, and in the first
moment it appears that noting went wrong. Instead, we should fail early.
Note that nms_keyfile_reader_from_keyfile() must entirely operate on the in-memory
representation of the keyfile. It must not actually access any files on disk. Hence,
moving this check before writing the profile must work. Otherwise, that would be
a separate bug. Actually, keyfile reader and writer violate this. I
added FIXME comments for that. But it doesn't interfere with this
patch.
NM didn't support wpa-none for years because kernel drivers used to be
broken. Note that it wasn't even possible to *add* a connection with
wpa-none because it was rejected in nm_settings_add_connection_dbus().
Given that wpa-none is also deprecated in wpa_supplicant and is
considered insecure, drop altogether any reference to it.
This file causes a crash [1], add it to the tests.
Note that the test only check parsing the file and the
crash happens in the "upper" layers. So, it's not really
a test for the crash. But at least have such a file in
our repository.
[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/235
Up until now, a default-route (with prefix length zero) could not
be configured directly. The user could only set ipv4.gateway,
ipv4.never-default, ipv4.route-metric and ipv4.route-table to influence
the setting of the default-route (respectively for IPv6).
That is a problematic limitation. For one, whether a route has prefix
length zero or non-zero does not make a fundamental difference. Also,
it makes it impossible to configure all the routing attributes that one can
configure otherwise for static routes. For example, the default-route could
not be configured as "onlink", could not have a special MTU, nor could it be
placed in a dedicated routing table.
Fix that by lifting the restriction. Note that "ipv4.never-default" does
not apply to /0 manual routes. Likewise, the previous manners of
configuring default-routes ("ipv4.gateway") don't conflict with manual
default-routes.
Server-side this all the pieces are already in place to accept a default-route
as static routes. This was done by earlier commits like 5c299454b4
('core: rework tracking of gateway/default-route in ip-config').
A long time ago, NMIPRoute would assert that the prefix length is
positive. That was relaxed by commit a2e93f2de4 ('libnm: allow zero
prefix length for NMIPRoute'), already before 1.0.0. Using libnm from
before 1.0.0 would result in assertion failures.
Note that the default-route-metric-penalty based on connectivity
checking applies to all /0 routes, even these static routes. Be they
added due to DHCP, "ipv4.gateway", "ipv4.routes" or "wireguard.peer-routes".
I wonder whether doing that unconditionally is desirable, and maybe
there should be a way to opt-out/opt-in for the entire profile or even
per-routes.
https://bugzilla.redhat.com/show_bug.cgi?id=1714438
I encountered a failure in the log
<trace> [1564647990.7822] keyfile: commit: deleting nmmeta file "/etc/NetworkManager/system-connections/35370b0b-e53b-42ea-9fe3-f1b1d552343b.nmmeta" failed
<trace> [1564647990.7822] keyfile: commit: deleting nmmeta file "/etc/NetworkManager/system-connections/35370b0b-e53b-42ea-9fe3-f1b1d552343b.nmmeta" simulated
I think that was due to SELinux (rh #1738010).
Let nms_keyfile_nmmeta_write() return an errno code so we can log
more information about the failure.
... and nm_utils_fd_get_contents() and nm_utils_file_set_contents().
Don't mix negative errno return value with a GError output. Instead,
return a boolean result indicating success or failure.
Also, optionally
- output GError
- set out_errsv to the positive errno (or 0 on success)
Obviously, the return value and the output arguments (contents, length,
out_errsv, error) must all agree in their success/failure result.
That means, you may check any of the return value, out_errsv, error, and
contents to reliably detect failure or success.
Also note that out_errsv gives the positive(!) errno. But you probably
shouldn't care about the distinction and use nm_errno_native() either
way to normalize the value.
nm_utils_file_set_contents() is a re-implementation of g_file_set_contents(),
as such it returned merely a boolean success value.
It's sometimes interesting to get the native error code. Let the function
deviate from glib's original g_file_set_contents() and return the error code
(as negative value) instead.
This requires all callers to change. Also, it's potentially a dangerous
change, as this is easy to miss.
Note that nm_utils_file_get_contents() also returns an errno, and
already deviates from g_file_get_contents() in the same way. This patch
resolves at least the inconsistency with nm_utils_file_get_contents().
This triggers a coverity warning because we above already
check that not all relevant keys are NULL together.
Work around warning by modifying the code.
(cherry picked from commit 210d7eb528)
Note that we now support keyfiles from read-only storage in /usr/lib.
Note also that we do support modifying and deleting these profiles.
That works by placing a shadowing profile to /etc or /run.
Surely this is questionable. It means that once the user uses D-Bus
to modify/delete a profile in /usr/lib, that profile becomes forever
shadowed by a different file, and there is no D-Bus API to return
to the original file. The user would have to drop the shadowing storages
from the file system. That is a problem.
But on the other hand, disallowing changes to such read-only profiles
is not very useful either. If you no longer can use D-Bus to modify such
profiles, what's the value here? How are applications supposed to handle
such profiles if there is no D-Bus API to do something sensible to them?
So, whatever problems read-only profiles and this shadowing causes, I don't
think that the solution is to entirely disallow changes via D-Bus.
At that point, we can just as well allow changes to profiles from
ifupdown. Note that you still cannot modify the profile directly (as the
ifupdown plugin does not support that). But you can delete the profile
(either temporarily via a tombstone in /run or permanently via a
tombstone in /etc). You also can make the profile in-memory, and take
it from there. Note that if you try to later store the in-memory profile
to disk again, then it depends on the order of settings plugins whether
that succeeds. If you have "plugins=keyfile,ifupdown", then the profile
will be stored as keyfile to /etc. If you have "plugins=ifupdown,keyfile",
then the modification will only be done in /run and the "to-disk" command
silently ignored (there really is no better solution).
Before, the .nmmeta file could only contain one piece of information:
the loaded-path. This was persisted to disk by writing a "$UUID.nmmeta"
symlink that links to the loaded-path. Also, in practice this is used
for tombstones, so the only valid loaded-path is "/dev/null" (all other
paths are ignored).
Extend the .nmmeta file format to also be able to store additional data: the
shadowed-storage path. We will need that later but the idea is that if
we have a tombstone on disk, then this tombstone might explicitly shadow
another file. The use is when re-adding a profile with the same UUID, then
the existing storage is used (instead of creating a new file). This will
be necessary with Update2(NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED)
flag. This flag first allows to clone a profile from persistent storage
to a profile in /run. Later, when this profile gets deleted, the
original profile will be left on disk. If the same profile then gets
re-created with AddConnection(), then the original filename must be
taken over again. This is to avoid duplication of profiles on disk.
Note that this piece of information is relevent per-UUID, and as such
it's correct to store it in the .nmmeta file. That is related to the
"shadowed-storage" information that we store in the [.nmmeta] section
of keyfiles.
When we make runtime only changes, we may leave the profile in persistent
storage and store a overlay profile in /run. However, in various cases we need
to remember the original path.
Add code to store and retrieve that path from the keyfile.
Note that this data is written inside the keyfile in /run. That is because
this piece of information really depends on that particular keyfile, and not
on the profile/UUID. That is why we write it to the [.nmmeta] section of the
keyfile and not to the .nmmeta file (which is per-UUID).
This patch only adds the backend to write and load the setting from
disk. It's not yet used.
Currently, meta-data has a very narrow use: as tombstones.
Later, we will need to store additional per UUID meta-data. For example,
when a profile becomes unsaved, we may need to remember the original
filename.
Refactor the code for that. This is for the most part just renaming
and slightly different handling of the fields.
With plain "interface-name:$IFNAME" globbing is enabled. So this behaves
wrong if there are special characters like '*' or '?'.
Also, it behaves wrong if the first character of the interface name happens
to be '='.
Make an explicit match.
The define is better, because then we can grep for all the occurances
where they are used. The plain text like "mac:" is not at all unique in
our source-tree.
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.
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 '/'.
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 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.
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().
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.
