String properties in libnm's NMSetting really should have NULL as a
default value. The only property that didn't, was "dcb.app-fcoe-mode".
Change the default so that it is also NULL.
Changing a default value is an API change, but in this case probably no
issue. For one, DCB is little used. But also, it's not clear who would
care and notice the change. Also, because previously verify() would reject
a NULL value as invalid. That means, there are no existing, valid profiles
that have this value set to NULL. We just make NULL the default, and
define that it means the same as "fabric".
Note that when we convert integer properties to D-Bus/GVariant, we often
omit the default value. For string properties, they are serialized as
"s" variant type. As such, NULL cannot be expressed as "s" type, so we
represent NULL by omitting the property. That makes especially sense if
the default value is also NULL. Otherwise, it's rather odd. We change
that, and we will now always express non-NULL value on D-Bus and let
NULL be encoded by omitting the property.
The settings plugin is not supposed to normalize the profile. It should
read/write what is, and let NMConnection handle what is valid and what
needs normalization.
Give a consistent name.
A bit odd are now the names nm_g_bytes_hash() and nm_g_bytes_equal()
as they go together with nm_pg_bytes_hash()/nm_pg_bytes_equal().
But here the problem is more with the naming of "nm_p*_{equal,hash}()"
functions, which probably should be renamed to "nm_*_ptr_{equal,hash}()".
I don't think this warrants a warning. It's important to keep the number
of warnings and errors in the log low, and only print such messages if
there is really something that requires attention by the user. If you
run without /etc/network/interfaces, then this is pretty much expected
and the warning isn't going to tell you anything useful.
The name prefix "nmtst_*" is reserved for test helpers and stub
function. Such functions should not be in the actual build artifacts,
like the NetworkManager binary.
Instead, nmtst_connection_assert_unchanging() is not a test helper. It
is a assertion function that is only enabled with NM_MORE_ASSERTS
builds. That's different.
Rename.
In other words,
$ nm src/core/NetworkManager src/libnm-client-impl/.libs/libnm.so | grep nmtst
should give no results.
The formatting produced by clang-format depends on the version of the
tool. The version that we use is the one of the current Fedora release.
Fedora 34 recently updated clang (and clang-tools-extra) from version
12.0.0 to 12.0.1. This brings some changes.
Update the formatting.
This happens if there are duplicate BSSIDs for a profile in
"/var/lib/NetworkManager/seen-bssid" file.
#0 c_list_unlink_stale (what=0x555555bc8768) at ./src/c-list/src/c-list.h:160
#1 _seen_bssid_entry_free (data=0x555555bc8750) at src/core/settings/nm-settings-connection.c:98
#2 0x00007ffff77e834a in g_hash_table_insert_node
(hash_table=hash_table@entry=0x555555afa9e0 = {...}, node_index=node_index@entry=6, key_hash=key_hash@entry=967604099, new_key=new_key@entry=0x555555bc8750, new_value=new_value@entry=0x555555bc8750, keep_new_key=keep_new_key@entry=0, reusing_key=0) at ../glib/ghash.c:1352
#3 0x00007ffff77e88f0 in g_hash_table_insert_internal (keep_new_key=0, value=0x555555bc8750, key=0x555555bc8750, hash_table=0x555555afa9e0 = {...}) at ../glib/ghash.c:1600
#4 g_hash_table_insert (hash_table=0x555555afa9e0 = {...}, key=key@entry=0x555555bc8750, value=value@entry=0x555555bc8750) at ../glib/ghash.c:1629
#5 0x000055555586c5e1 in _nm_settings_connection_register_kf_dbs (self=self@entry=0x555555bbf5a0, kf_db_timestamps=<optimized out>, kf_db_seen_bssids=<optimized out>)
at src/core/settings/nm-settings-connection.c:2382
#6 0x00005555555b7e19 in _connection_changed_update
(self=self@entry=0x555555b1d0c0, sett_conn_entry=sett_conn_entry@entry=0x555555b60390, connection=0x555555b953f0, sett_flags=sett_flags@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, sett_mask=sett_mask@entry=_NM_SETTINGS_CONNECTION_INT_FLAGS_PERSISTENT_MASK, update_reason=update_reason@entry=(NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_AGENT_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET)) at src/core/settings/nm-settings.c:1080
#7 0x00005555555b8b5a in _connection_changed_process_one
(self=self@entry=0x555555b1d0c0, sett_conn_entry=0x555555b60390, allow_add_to_no_auto_default=allow_add_to_no_auto_default@entry=0, sett_flags=sett_flags@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, sett_mask=_NM_SETTINGS_CONNECTION_INT_FLAGS_PERSISTENT_MASK,
sett_mask@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, override_sett_flags=override_sett_flags@entry=1, update_reason=(NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_AGENT_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET)) at src/core/settings/nm-settings.c:1304
#8 0x00005555555b8c5e in _connection_changed_process_all_dirty
(self=self@entry=0x555555b1d0c0, allow_add_to_no_auto_default=allow_add_to_no_auto_default@entry=0, sett_flags=sett_flags@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, sett_mask=sett_mask@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, override_sett_flags=override_sett_flags@entry=1, update_reason=update_reason@entry=(NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_AGENT_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET)) at src/core/settings/nm-settings.c:1325
#9 0x00005555555b8d40 in _plugin_connections_reload (self=self@entry=0x555555b1d0c0) at src/core/settings/nm-settings.c:1448
#10 0x00005555555bddb5 in nm_settings_start (self=0x555555b1d0c0, error=error@entry=0x7fffffffe278) at src/core/settings/nm-settings.c:3892
#11 0x000055555560013d in nm_manager_start (self=self@entry=0x555555b19060, error=error@entry=0x7fffffffe278) at src/core/nm-manager.c:6961
#12 0x0000555555594b27 in main (argc=<optimized out>, argv=<optimized out>) at src/core/main.c:496
Fixes: 8278719840 ('settings: limit number of seen-bssids and preserve order')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/787
"test-ifcfg-rh.c" is huge, with lots of repeated, verbose code.
Refactor the code by using some helper macros so that the line noise
is smaller and we can easier see what is happening.
- use nmtst_connection_assert_setting() instead of
nm_connection_get_setting*(), followed by an assertion.
- use _nm_connection_new_setting() instead of multiple lines to
create and add the setting.
- drop all explicit unref/free and use cleanup macro.
- unify some variable names.
- drop some useless comments. In particular, comments were used as
visual separators because the code is verbose and hard to read. The
solution to verbose and hard to read is not more code/comments, the
solution is clearer, conciser code.
These type-specific getters are not very useful. _nm_connection_get_setting() is
better because the setting type is a parameter so they can be used more generically.
Have less code and use generic helpers.
- the writer/reader should be lossless. There is a difference
on whether a NMConnection has/hasn't a NMSettingBondPort instance.
If we thus have a NMSettingBondPort, we must always encode that
in the ifcfg file, by writing BOND_PORT_QUEUE_ID=0. Otherwise,
the reader will not create the setting.
- it's really not the task of the writer to validate what it writes.
All these write_bridge_port_setting() really should not fail. They
should serialize the setting as good as they can. And if they cannot,
it's probably a bug in the writer (by not being lossless).
write_bond_port_setting() did not ever fail. It should not ever fail.
So don't let the function return a potential failure, and don't
handle a failure that should never happen.
- use svGetValue() instead of svGetValueStr(). The difference is that
svGetValueStr() coerces "" to NULL. "" is not a valid value, but we
want to parse the value and print an warning message about it. Also,
the presence of the variable determines whether we add the bond-port
setting or not.
- don't use nm_clear_g_free(). @value_to_free is gs_free, it will be
cleared automatically.
- use g_object_set() instead of nm_g_object_set_property_uint(). The
latter is our own implementation that does error checking (e.g., that
the value is in range (0..2^16-1). But we already ensured that to
be the case. So just call g_object_set(), it cannot fail and if it
would, we want the assertion failure that it would cause.
- queue_id should be a "guint". It is always true on Linux/glib that
sizeof(guint) >= sizeof(guint32), the opposite theoretically might not
be true.
But later we use the variable in the variadic function g_object_set(),
where it should be guint.
- the errno from _nm_utils_ascii_str_to_uint64() isn't very useful for
logging. It's either ERANGE or EINVAL, and logging the numeric values
of these error codes isn't gonna help the user. We could stringify
with nm_strerror_native(errno), but that message is also not very
useful. Just say that the string is not a number.
No actual caller should use the API without providing either a filename
or the directory name. I don't think this can actually happen, hence
fail and assert in that case.
Have an ifcfg file loaded in NetworkManager, then move/remove the file and try
to modify it. That will fail with:
"failed to update connection: Could not read file '/etc/sysconfig/network-scripts/ifcfg-eth0': No such file or directory"
That is not right.
If the user didn't move/remove the file but merely modified it, NetworkManager
would silently overwrite it. There is no reason why move/remove should behave
differently and not just write a completely fresh file.
The reason why NetworkManager first loads the file before writing, is to
preserve comments and unrecognized shell variables. This is a certain effort
to play nice with users editing the file. It's not essential to load the file
first and a failure to do so should not result in a failure.
And of course, keyfile writer doesn't behave like this either.
This bug exists since 2009, but let's not add a "Fixes" comment for
commit 1974b257e0 ('ifcfg-rh: begin adding write support'), because
it seems not right to backport this patch to all the old releases.
Do "bother" to read/write settings.
For the umpteenth time, it's not up to the reader/writer to decide
what properties are valid for a profile or which makes sense.
Only nm_connection_verify() can decide that. For example nm_connection_verify()
has no problem with ipv6.method=disabled while also setting ipv6.addr-gen-mode.
We cannot just shortcut the parsing/writing.
The reader only ignores addresses, dns and dns-searches, so that we don't start
parsing invalid files, where the setting would have been ignore
previously.
In particular,
echo "DEVICE=eth0" > /etc/sysconfig/network-scripts/ifcfg-xxx
nmcli connection load /etc/sysconfig/network-scripts/ifcfg-xxx
nmcli -f ipv6.method,ipv6.addr-gen-mode connection show /etc/sysconfig/network-scripts/ifcfg-xxx
needs to show eui64 addr-gen-mode.
It's not the task of the ifcfg reader to pre-normalize profiles
to truncate the DNS server list. It's only nm_connection_verify()'s
task to indicate what is valid and what not.
Increase the number to something excessive. Note that the parsing
scales with O(n^2). So don't have it totally unbounded and have an
overall limit (of 10000 entries).
Now that NetworkManager on Fedora 33 and RHEL 9 no longer writes
ifcfg-rh files by default ([1]), ifup/ifdown became less useful.
Possibly users shouldn't use it and it would be fine that new-style profiles
(keyfile) no longer work with these commands. But this is deemed as too
disruptive for users.
Note that our previous ifup/ifdown compat scripts only honored the argument
to be part of the ifcfg filename. That was not what initscripts were doing,
which called `need_config()` function that searched also the contents of
the files. With this extension, ifup/ifdown gets smarter too, to better
guess what the user might have wanted.
Extend the script by making it smarter, and to work with connection profile
names.
With this extension we further solidify ifup/ifdown as part of NetworkManager
command line API. That is problematic, because these tools pollute the
$PATH, by not having a clear NM-specific name. Also, these scripts
should only exist on Fedora/RHEL, which makes their usage non-portable
to other distros.
Also, other distros already ship different tools with name ifup/ifdown.
Extending the use of these scripts is thus undesirable, as it furthers
distro-specific commands.
Still, these arguments seem to not hold and users need to be "helped".
As Fedora users cannot be expected to unlearn "ifup" today, there is no
reason to assume they could in a few years. This likely means we will
never get rid of these scripts.
Also, if we truly would make ifup/ifdown part of NetworkManager, then a better
implementation would be that nmcli honors being called with these names.
That is not done, because nmcli's implementation currently is not as
nice to make that extension trivial (as it should be). It also would
mean to embrace ifup/ifdown officially. A shell script works well enough
as a hack.
[1] https://fedoraproject.org/wiki/Changes/NetworkManager_keyfile_instead_of_ifcfg_rhhttps://bugzilla.redhat.com/show_bug.cgi?id=1954607https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/936
The "utils" part does not seem useful in the name.
Note that we also have NMStrBuf, which is named nm_str_buf_*().
There is an unfortunate similarity between the two, but it's still
distinct enough (in particular, because one takes an NMStrBuf and
the other not).
Naming is important, because the name of a thing should give you a good
idea what it does. Also, to find a thing, it needs a good name in the
first place. But naming is also hard.
Historically, some strv helper API was named as nm_utils_strv_*(),
and some API had a leading underscore (as it is internal API).
This was all inconsistent. Do some renaming and try to unify things.
We get rid of the leading underscore if this is just a regular
(internal) helper. But not for example from _nm_strv_find_first(),
because that is the implementation of nm_strv_find_first().
- _nm_utils_strv_cleanup() -> nm_strv_cleanup()
- _nm_utils_strv_cleanup_const() -> nm_strv_cleanup_const()
- _nm_utils_strv_cmp_n() -> _nm_strv_cmp_n()
- _nm_utils_strv_dup() -> _nm_strv_dup()
- _nm_utils_strv_dup_packed() -> _nm_strv_dup_packed()
- _nm_utils_strv_find_first() -> _nm_strv_find_first()
- _nm_utils_strv_sort() -> _nm_strv_sort()
- _nm_utils_strv_to_ptrarray() -> nm_strv_to_ptrarray()
- _nm_utils_strv_to_slist() -> nm_strv_to_gslist()
- nm_utils_strv_cmp_n() -> nm_strv_cmp_n()
- nm_utils_strv_dup() -> nm_strv_dup()
- nm_utils_strv_dup_packed() -> nm_strv_dup_packed()
- nm_utils_strv_dup_shallow_maybe_a() -> nm_strv_dup_shallow_maybe_a()
- nm_utils_strv_equal() -> nm_strv_equal()
- nm_utils_strv_find_binary_search() -> nm_strv_find_binary_search()
- nm_utils_strv_find_first() -> nm_strv_find_first()
- nm_utils_strv_make_deep_copied() -> nm_strv_make_deep_copied()
- nm_utils_strv_make_deep_copied_n() -> nm_strv_make_deep_copied_n()
- nm_utils_strv_make_deep_copied_nonnull() -> nm_strv_make_deep_copied_nonnull()
- nm_utils_strv_sort() -> nm_strv_sort()
Note that no names are swapped and none of the new names existed
previously. That means, all the new names are really new, which
simplifies to find errors due to this larger refactoring. E.g. if
you backport a patch from after this change to an old branch, you'll
get a compiler error and notice that something is missing.
Configuration can have [device*] and [connection*] settings and both
can include a 'match-device=' key, which is a list of device-specs.
Introduce a new 'allowed-connections' key for [device*] sections,
which specifies a list of connection-specs to indicate which
connections can be activated on the device.
With this, it becomes possible to have a device configuration like:
[device-enp1s0]
match-device=interface-name:enp1s0
allowed-connections=except:origin:nm-initrd-generator
so that NM in the real root ignores connections created by the
nm-initrd-generator, and starts activating a persistent
connection. This requires also setting 'keep-configuration=no' to not
generate an assumed connection.
This is a normalization employed by NMSettingIPConfig.gateway.
Also rework NMSettingIPConfig.set_property() to no longer assert against
valid input. We want to pass there untrusted strings from D-Bus,
asserting is a horrible idea. Instead, either normalize the string or
keep the invalid text that will be rejected by verify().
This is like using nm_ascii_is_ctrl_or_del() instead of
nm_ascii_is_ctrl() in the previous version of the patch.
We thus now always will switch to ANSIC escaping if we see
a ASCII DEL character. That is probable desirable, but either
way should not make a big difference (because we can parse
the DEL character both in regular quotation and in ANSIC quotation).
The patch is however larger, to also take the opportunity to only check
for nm_ascii_is_regular() in the "fast path". The behavior is the same
as changing nm_ascii_is_ctrl() to nm_ascii_is_ctrl_or_del().
Problems of this patch:
- the code does not differentiate between an ifcfg file and an alias
file. Different shell variables are honored however depending on the
context and the warning should reflect that.
- there are no warnings about /etc/sysconfig/network. The main problem
is that we read this file for every ifcfg file we parse, and we would
need to ratelimit the number of warnings. Another problem is that
the file likely contains keys that we intentionally don't support.
We would need a new way to omit warnings about those lines.
Example:
TYPE=Ethernet
PROXY_METHOD=none
BROWSER_ONLY=no
BOOTPROTO=dhcp
DEFROUTE=yes
STABLE_ID=$'xxx\xF4yy'
IPV4_FAILURE_FATAL=no
IPV6INIT=yes
XX=foo
XX1=foo'
'
IPV6_AUTOCONF=yes xxxx
IPV6_DEFROUTE=yesx
IPV6_DEFROUTE=yes
IPV6_FAILURE_FATAL=no
IPV6_ADDR_GEN_MODE=stable-privacy
NAME=xxx
UUID=9d8ed7ff-3cdd-4336-9e26-3e978dc87102
ONBOOT=no
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:6: key STABLE_ID does not contain valid UTF-8 and is treated as ""
<debug> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:9: key XX is unknown and ignored
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:10: key XX1 is badly quoted and is treated as ""
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:11: invalid line ignored
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:12: key IPV6_AUTOCONF is badly quoted and is treated as ""
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:13: key IPV6_DEFROUTE is duplicated and the early occurrence ignored
https://bugzilla.redhat.com/show_bug.cgi?id=1959656
ifcfg files are a text format. It makes no sense to ever accept
non-UTF-8 blobs. If binary data is to be encoded in a ifcfg file, then
the upper layers must escape/encode it in valid UTF-8.
Let svUnescape() silently reject any binary "text". This will lead to treat such
strings as empty strings "". This is no different than some invalid
quoting: the string is not parsable as (UTF-8) text and will be treated
as such.
This is potentially a breaking change. But the benefit is that all the
upper layers can rely on only getting valid UTF-8 strings. For example,
a non-UTF-8 string cannot be converted to a "s" GVariant (of course not,
it's not a string). But our nm_connection_verify() commonly does not
check that all strings are in fact valid UTF-8. So a user who edits
an ifcfg file could inject non-valid strings, and cause assertion
failures later on.
It's actually easy to provoke a crash (or at least an assertion failure)
by writing an ifcfg file with certain keys as binary.
Note that you can either reproduce the binary files by writing non-UTF-8
"strings" dirctly, or by using \x, \u, or \U escape sequences.
Note that also '\0' gets rejected and renders the string as invalid
(i.e. as empty). Before the returned string would have been simply
truncated and the rest ignored. Such NUL bytes can only be produced
using the escape sequences, because the ifcfg reader already (silently)
truncates the file on the first binary NUL.
Note that previously the check
if (s[slen] < ' ') {
...
return (*to_free = _escape_ansic(s));
}
would be TRUE for all UTF-8 characters if `char` is signed. That means,
depending on the compiler, we would always ANSI escape all UTF-8
characters. With this patch, we no longer do that!
Instead, valid unicode gets now preserved (albeit quoted).
On the other hand, always ANSIC escape invalid UTF-8 (regardless of the
compiler). ifcfg-rh is really a text based format. If a caller wants to store
binary data, they need to escape it first, for example with some own escaping
scheme, base64 or bin2hexstr.
A caller passing a non-text to svEscape() is likely a bug already and
they should have not done that.
Still, let svEscape() handle that by using ANSIC escaping. That works
as far as escaping is concerned, but likely later will be a problem
during unescaping, when the reader expects a valid UTF-8 string.
svEscape() is in no place to signal a sensible error, so proceed the
best it can, by escaping.
Add a new property to specify the minimum time interval in
milliseconds for which dynamic IP configuration should be tried before
the connection succeeds.
This property is useful for example if both IPv4 and IPv6 are enabled
and are allowed to fail. Normally the connection succeeds as soon as
one of the two address families completes; by setting a required
timeout for e.g. IPv4, one can ensure that even if IP6 succeeds
earlier than IPv4, NetworkManager waits some time for IPv4 before the
connection becomes active.
We have two GKeyfile files (timestamps and seen-bssids).
When a profile was deleted while NetworkManager was running, then
entries were removed from these keyfiles. But if a profile disappeared
while NetworkManger was stopped, then those UUIDs piled up.
This also happens if you have temporary connections in /run and reboot.
We need a way to garbage collect entries that are no longer relevant.
As the keyfile databases only get loaded once from disk, we will prune
all UUIDs for which we have no more connection loaded, on the first time
we write out the files again.
Note what this means: if you "temporarily" remove a connection profile
(without NetworkManager noticing) and restore it later, then the additional
information might have been pruned. There is no way how NetworkManager
could know that this UUID is coming back. The alternative is what we did
before: pile them up indefinitely. That seems more problematic.
Previously, there was no limit how many seen-bssids are tracked.
That seems problematic, also because there is no API how to get
rid of an excessive list of entries.
We should limit the number of entries. Add an (arbitrary) limit
of 30.
But this means that we drop the surplus of entries, and for that it
seems important to keep the newest, most recently seen entries.
Previously, entries were merely sorted ASCIIbetically. Now, honor
their order (with most recently seen first).
Also, normalize the BSSIDs. From internal code, we should only get
normalize strings, but when we load them from disk, they might be bogus.
As we might cut of the list, we don't want that invalid entries
cut of valid ones. And of course, invalid entries make no sense at
all.
ifcfg-rh plugin never stored the seen bssid list to file, and
keyfile no longer does, and it's no longer parsed from GVariant.
So there is actually no way how anything could be set here.
The seen-bssids should only be populate from
"/var/lib/NetworkManager/seen-bssids". Nowhere else.
This function is badly named, because it has no NMHostnameManager self
argument. It's just a simple function that entirely operates on a string
argument.
Move it away from "nm-hostname-manager.h" to "libnm-glib-aux/nm-shared-utils.h".
Hostname handling is complicated enough. Simple string validation
functions should not obscure the view on the complicated parts.
We somehow need to encode an NMSettingEthtool instance that has all
options unset. Previously, that would result in no "$ETHTOOL_OPTS"
variable and thus the reader would loose a previously existing setting.
Hack it by writing a bogus
ETHTOOL_OPTS="-A $IFACE"
line.
It just seems ugly to call g_getenv() repeatedly. Environment variables
must not change (in a multi-threaded program after other threads start),
so determine the mode once and cache it.
For the umpteenth time: it is not ifcfg-rh writers decision to decide
what are valid configurations and only persist settings based on
some other settings.
If s390-options would only be allowed together with subchannels, then
this is alone nm_connection_verify()'s task to ensure.
Reproduce with
$ nmcli connection add type ethernet autoconnect no con-name zz ethernet.s390-options bridge_role=primary
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1935842
Fixes: 16bccfd672 ('core: handle s390 options more cleanly')
We now have a cached list of NMSettingsConnection instances,
sorted by their autoconnect priority.
However, the sort order nm_settings_connection_cmp_autoconnect_priority()
depends on various properties of the connection:
- "connection.autoconnect" and "connection.autoconnect-priority"
- the timestamp
- "connection.uuid"
These properties almost never change, so it's a waste that every call
to nm_settings_get_connections_sorted_by_autoconnect_priority() needs
to check whether the sort order is still satisfied.
We can do better by tracking when the sort order might have been
destroyed and only check in those (much fewer) cases.
Note that we end up calling nm_settings_get_connections_sorted_by_autoconnect_priority()
a lot, so this makes a difference.
The GetSettings() call is not the only place where we convert a
NMConnection to D-Bus. However it is one of the most prominent ones
with a measurable performance overhead.
The connection seldom changes, so it makes sense to cache it.
Note that GetSettings() is the only caller that specifies an option,
thus it's the only caller that converts a NMConnection to variant
in this particular way. That means, other callers don't benefit from
this caching and we could not cache the variant in the NMConnection
instance itself, because those callers use different parameters.
Turns out, we call nm_settings_get_connection_clone() *a lot* with sort order
nm_settings_connection_cmp_autoconnect_priority_p_with_data().
As we cache the (differently sorted) list of connections, also cache
the presorted list. The only complication is that every time we still
need to check whether the list is still sorted, because it would be
more complicated to invalidate the list when an entry changes which
affects the sort order. Still, such a check is usually successful
and requires "only" N-1 comparisons.
