- don't let no_auto_default_from_file() do any preprocessing of
the lines that it reads. It merely splits the lines at '\n'
and utf8safe-unescapes them.
This was previously duplicated also by NMConfigData's property
setter. We don't need to do it twice.
- sort the lines. This makes the entire handling O(n*ln(n)) instead
of O(n^2). Also, sorting effectively normalizes the content, and
it's desirable to have one true representation of what we write.
For devices that have no real MAC address (virtual devices) it makes no
sense to store the MAC address to "no-auto-default.state" file. Also,
because we later would not match the MAC address during
nm_match_spec_device().
Instead, extend the format and add a "interface-name:=$IFACE" match-spec.
Maybe we generally should prefer the interface-name over the MAC
address. Anyway, for now, just extend the previously non-working case.
Currently "no-auto-default.state" contains only MAC addresses in ASCII
representation.
Next, we will also want to write there interface names. Interface names
in Linux don't enforce any encoding, so they can contain almost all
characters (except NUL and '/'). In particular '\n', which
we use as line separator.
If we want to store there interface names, we need to properly escape
and unescape them. Use our nm_utils_str_utf8safe_*() API for that.
For one, nm_config_get_no_auto_default_for_device() uses
nm_device_spec_match_list(). This ignores fake MAC addresses.
Maybe it should not do that, but it's also not clear what it
would mean for the function to consider them.
As such, it makes not sense trying to persist such MAC addresses
to "/var/lib/NetworkManager/no-auto-default.state". For the moment,
just do nothing.
This still leaves the problem how we prevent the device from generating
a auto-default connection. But this patch is no change in behavior, because
it didn't work anyway.
nm_config_set_no_auto_default_for_device() is called by NMSettings,
so it makes sense that also NMSettings checks whether the device is
blocked.
Of course, there is little difference in practice.
The only downside is that most device types don't implement
new_default_connection(). So the previous form performed the
cheaper check first. On the other hand, we do expect to have
profiles for the devices anyway.
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.
Rejecting %NULL for a "is-a" function can be annoying. Of course, %NULL is
not a valid name. But it's sufficient that the function just returns
%FALSE in that case, and not assert against the input not being %NULL.
Asserting might be useful to catch bugs, but rejecting %NULL as input
is more cumbersome to the caller than helping with catching bugs.
Something similar was also recently done for nm_utils_is_uuid().
In particular when calling nm_utils_strv_sort() with a positive length
argument, then this is not a %NULL terminated strv arrary. That may mean
that it makes sense for the input array to contain %NULL strings.
Use a strcmp() function that accepts %NULL too.
While this is not used at the moment, I think nm_utils_strv_sort()
should accept %NULL strings beause otherwise it's a possibly unexpected
restriction of its API. The function should handle sensible input gracefully.
It is like strcmp(), but has a signature suitable for GCompareDataFunc.
This is necessary with nm_utils_ptrarray_find_binary_search()
to search a sorted strv array.
The fault is here really C, which doesn't allow inline static functions.
So, you need all kinds of slightly different flavors for the same
callbacks (with or without user-data).
Note that glib2 internally just casts strcmp() to GCompareDataFunc ([1]),
relying on the fact how arguments are passed to the function and
ignoring the additional user-data argument. But I find that really
ugly and probably not permissible in general C. Dunno whether POSIX
would guarantee for this to work. I'd rather not do such function
pointer casts.
[1] 0c0cf59858/glib/garray.c (L1792)
We usually want to combine the fields from "struct timespec" to
have one timestamp in either nanoseconds or milliseconds.
Use nm_utils_clock_gettime_*() util for that.
Using clock_gettime() directly is a bit inconvenient. We usually
want to combine the fields of struct timespec into one timestamp
(for example, in unit nanoseconds).
Add a helper function to do that.
Only NMDeviceEthernet implements new_default_connection(). Anyway, it
makes only sense to do this precheck by the caller first, and not by
each implementation.
Initially I thought I would use this somewhere else. Didn't do so far,
but this seems a useful function to have on its own because also
NMSettings is concerned about the relative priority of plugins.
Completely rework how settings plugin handle connections and how
NMSettings tracks the list of connections.
Previously, settings plugins would return objects of (a subtype of) type
NMSettingsConnection. The NMSettingsConnection was tightly coupled with
the settings plugin. That has a lot of downsides.
Change that. When changing this basic relation how settings connections
are tracked, everything falls appart. That's why this is a huge change.
Also, since I have to largely rewrite the settings plugins, I also
added support for multiple keyfile directories, handle in-memory
connections only by keyfile plugin and (partly) use copy-on-write NMConnection
instances. I don't want to spend effort rewriting large parts while
preserving the old way, that anyway should change. E.g. while rewriting ifcfg-rh,
I don't want to let it handle in-memory connections because that's not right
long-term.
--
If the settings plugins themself create subtypes of NMSettingsConnection
instances, then a lot of knowledge about tracking connections moves
to the plugins.
Just try to follow the code what happend during nm_settings_add_connection().
Note how the logic is spread out:
- nm_settings_add_connection() calls plugin's add_connection()
- add_connection() creates a NMSettingsConnection subtype
- the plugin has to know that it's called during add-connection and
not emit NM_SETTINGS_PLUGIN_CONNECTION_ADDED signal
- NMSettings calls claim_connection() which hocks up the new
NMSettingsConnection instance and configures the instance
(like calling nm_settings_connection_added()).
This summary does not sound like a lot, but try to follow that code. The logic
is all over the place.
Instead, settings plugins should have a very simple API for adding, modifying,
deleting, loading and reloading connections. All the plugin does is to return a
NMSettingsStorage handle. The storage instance is a handle to identify a profile
in storage (e.g. a particular file). The settings plugin is free to subtype
NMSettingsStorage, but it's not necessary.
There are no more events raised, and the settings plugin implements the small
API in a straightforward manner.
NMSettings now drives all of this. Even NMSettingsConnection has now
very little concern about how it's tracked and delegates only to NMSettings.
This should make settings plugins simpler. Currently settings plugins
are so cumbersome to implement, that we avoid having them. It should not be
like that and it should be easy, beneficial and lightweight to create a new
settings plugin.
Note also how the settings plugins no longer care about duplicate UUIDs.
Duplicated UUIDs are a fact of life and NMSettings must handle them. No
need to overly concern settings plugins with that.
--
NMSettingsConnection is exposed directly on D-Bus (being a subtype of
NMDBusObject) but it was also a GObject type provided by the settings
plugin. Hence, it was not possible to migrate a profile from one plugin to
another.
However that would be useful when one profile does not support a
connection type (like ifcfg-rh not supporting VPN). Currently such
migration is not implemented except for migrating them to/from keyfile's
run directory. The problem is that migrating profiles in general is
complicated but in some cases it is important to do.
For example checkpoint rollback should recreate the profile in the right
settings plugin, not just add it to persistent storage. This is not yet
properly implemented.
--
Previously, both keyfile and ifcfg-rh plugin implemented in-memory (unsaved)
profiles, while ifupdown plugin cannot handle them. That meant duplication of code
and a ifupdown profile could not be modified or made unsaved.
This is now unified and only keyfile plugin handles in-memory profiles (bgo #744711).
Also, NMSettings is aware of such profiles and treats them specially.
In particular, NMSettings drives the migration between persistent and non-persistent
storage.
Note that a settings plugins may create truly generated, in-memory profiles.
The settings plugin is free to generate and persist the profiles in any way it
wishes. But the concept of "unsaved" profiles is now something explicitly handled
by keyfile plugin. Also, these "unsaved" keyfile profiles are persisted to file system
too, to the /run directory. This is great for two reasons: first of all, all
profiles from keyfile storage in fact have a backing file -- even the
unsaved ones. It also means you can create "unsaved" profiles in /run
and load them with `nmcli connection load`, meaning there is a file
based API for creating unsaved profiles.
The other advantage is that these profiles now survive restarting
NetworkManager. It's paramount that restarting the daemon is as
non-disruptive as possible. Persisting unsaved files to /run improves
here significantly.
--
In the past, NMSettingsConnection also implemented NMConnection interface.
That was already changed a while ago and instead users call now
nm_settings_connection_get_connection() to delegate to a
NMSimpleConnection. What however still happened was that the NMConnection
instance gets never swapped but instead the instance was modified with
nm_connection_replace_settings_from_connection(), clear-secrets, etc.
Change that and treat the NMConnection instance immutable. Instead of modifying
it, reference/clone a new instance. This changes that previously when somebody
wanted to keep a reference to an NMConnection, then the profile would be cloned.
Now, it is supposed to be safe to reference the instance directly and everybody
must ensure not to modify the instance. nmtst_connection_assert_unchanging()
should help with that.
The point is that the settings plugins may keep references to the
NMConnection instance, and so does the NMSettingsConnection. We want
to avoid cloning the instances as long as they are the same.
Likewise, the device's applied connection can now also be referenced
instead of cloning it. This is not yet done, and possibly there are
further improvements possible.
--
Also implement multiple keyfile directores /usr/lib, /etc, /run (rh #1674545,
bgo #772414).
It was always the case that multiple files could provide the same UUID
(both in case of keyfile and ifcfg-rh). For keyfile plugin, if a profile in
read-only storage in /usr/lib gets modified, then it gets actually stored in
/etc (or /run, if the profile is unsaved).
--
While at it, make /etc/network/interfaces profiles for ifupdown plugin reloadable.
--
https://bugzilla.gnome.org/show_bug.cgi?id=772414https://bugzilla.gnome.org/show_bug.cgi?id=744711https://bugzilla.redhat.com/show_bug.cgi?id=1674545
The file got a wider scope to contain generic meta data about profiles.
Rename the internal API to reflect that (and be consistend with the
naming of the files).
We may want to store meta-data for a profile to disk. The immediate
need are "tombstones": markers that the particular UUID is shadowed
and the profile does not exist (despite being in read-only location).
Change the filename of these symlinks from
".loaded-${UUID}.nmconnection"
to
"${UUID}.nmmeta"
The leading dot is not desirable as tools tend to hide such files.
Use a different scheme for the filename that does not have the leading dot.
Note that nm_keyfile_utils_ignore_filename() would also ignore ".nmmeta"
as not a valid keyfile. This is just what we want, and influences the
choice of this file suffix.
Also, "nmmeta" is a better name, because this name alludes that there is
a wider use for the file: namely to have addtional per-profile metadata.
That is regardless that the upcoming first use will be only to store symlinks
to "/dev/null" to indicate the tombstones.
Note that per-profile metadata is not new. Currently we write the files
/var/lib/NetworkManager/{seen-bssids,timestamps}
that have a similar purpose. Maybe the content from these files could one
day be migrated to the ".nmmeta" file. The naming scheme would make it
suitable.
nm_strndup_a() uses strncpy() because we want the behavior of clearing out
the memory after the first NUL byte. But that can cause a compiler warning:
CC src/settings/plugins/keyfile/libNetworkManager_la-nms-keyfile-utils.lo
In file included from ../../shared/nm-default.h:279,
from ../../src/settings/plugins/keyfile/nms-keyfile-utils.c:20:
In function ‘_nm_strndup_a_step’,
inlined from ‘nms_keyfile_loaded_uuid_is_filename’ at ../../src/settings/plugins/keyfile/nms-keyfile-utils.c:65:9:
../../shared/nm-glib-aux/nm-macros-internal.h:1661:3: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
1661 | strncpy (s, str, len);
| ^~~~~~~~~~~~~~~~~~~~~
../../src/settings/plugins/keyfile/nms-keyfile-utils.c: In function ‘nms_keyfile_loaded_uuid_is_filename’:
../../src/settings/plugins/keyfile/nms-keyfile-utils.c:48:8: note: length computed here
48 | len = strlen (filename);
| ^~~~~~~~~~~~~~~~~
It's true that the len argument of _nm_strndup_a_step() depends on the
string length of the source string. But in this case it's safe, because
we checked that the destination buffer is exactly the right size too.
By that reasoning we should use memcpy() or strcpy(), but both are
unsuitable. That is because we want nm_strndup_a() to behave like
strndup(), which means we need to handle cases where the len argument
is larger than the string length of the source string. That is, we want
always to return a buffer of size len+1, but we want to copy only the
characters up to the first NUL byte, and clear out the rest. That's what
strncpy() does for us.
Silence the warning.
4 properties are not really relevant for an already activated connection
or it makes not sense to change them. These are connection.id, connection.uuid,
connection.autoconnect and connection.stable-id.
For convenience, we allow to reapply these. This way, one can take
a different setting (e.g. with a different connection.id or
connection.uuid) and reapply them, but such changes are silently
ignored.
However this was done wrongly. Instead of reverting the change to the new
applied connection, we would change the input connection.
This is bad, for example with
nmcli connection up uuid cb922f18-e99a-49c6-b200-1678b5070a82
nmcli connection modify cb922f18-e99a-49c6-b200-1678b5070a82 con-name "bogus"
nmcli device reapply eth0
the last re-apply would reset the settings-connection's connection ID to
what was before, while accepting the new name on the applied-connection
(while it should have been rejected).
Fixes: bf3b3d444c ('device: avoid changing immutable properties during reapply')
IP addresses, routes, TC and QDiscs are all tied to a certain interface.
So when NetworkManager manages an interface, it can be confident that
all related entires should be managed, deleted and modified by NetworkManager.
Routing policy rules are global. For that we have NMPRulesManager which
keeps track of whether NetworkManager owns a rule. This allows multiple
connection profiles to specify the same rule, and NMPRulesManager can
consolidate this information to know whether to add or remove the rule.
NMPRulesManager would also support to explicitly block a rule by
tracking it with negative priority. However that is still unused at
the moment. All that devices do is to add rules (track with positive
priority) and remove them (untrack) once the profile gets deactivated.
As rules are not exclusively owned by NetworkManager, NetworkManager
tries not to interfere with rules that it knows nothing about. That
means in particular, when NetworkManager starts it will "weakly track"
all rules that are present. "weakly track" is mostly interesting for two
cases:
- when NMPRulesManager had the same rule explicitly tracked (added) by a
device, then deactivating the device will leave the rule in place.
- when NMPRulesManager had the same rule explicitly blocked (tracked
with negative priority), then it would restore the rule when that
block gets removed (as said, currently nobody actually does this).
Note that when restarting NetworkManager, then the device may stay and
the rules kept. However after restart, NetworkManager no longer knows
that it previously added this route, so it would weakly track it and
never remove them again.
That is a problem. Avoid that, by whenever explicitly tracking a rule we
also make sure to no longer weakly track it. Most likely this rule was
indeed previously managed by NetworkManager. If this was really a rule
added by externally, then the user really should choose distinct
rule priorities to avoid such conflicts altogether.
WireGuard's wq-quick configures such rules to avoid routing loops.
While we currently don't have an automatic solution for this, at least
we should support it via explicit user configuration.
One problem is that suppress_prefixlength is relatively new and kernel
might not support this attribute. That can lead to odd results, because
the NetworkManager is valid but it cannot be configured on the current
kernel. But this is a general problem, and we would require a general
solution. The solution cannot be to only support rule attributes that
are supported by the oldest possible kernel. It's not clear how much of
a problem there really is, or which general solution is required (if
any).
The tables "main", "local", and "default" have well known names.
Accept them as aliases when parsing the string representation of
the rule.
Note that iproute2 also considers /etc/iproute2/rt_tables for table
names. In particular, that allows a user to re-map the well-known names
like "main" to a different table. We never honor that file, and "main"
always means table 254.
Note that this only affects how we parse the string representation for
rules. As the representation is neither unique nor enforced to be normalized,
being more graceful here is no problem.
The point is of course that the user possibly has existing iproute2
scripts that use such keyword. This makes it simpler to copy & paste
the rule.
