We already have code that parses exactly this kinds of string:
nm_utils_parse_inaddr_prefix_bin(). Use it.
Also, it doesn't use g_strsplit_set() to separate a string at the first
'/'. Total overkill.
The caller should make a conscious decision which delimiters to use.
Unfortunately, there is a variety of different demiters in use. This
should be unitfied and the callers should use one of a few specific
set of delimiters.
This could be unified by (re)using a define as delimiters, like
strv = nm_utils_strsplit_set_full (value, MULTILIST_WITH_ESCAPE_CHARS, NM_UTILS_STRSPLIT_SET_FLAGS_ALLOW_ESCAPING);
where MULTILIST_WITH_ESCAPE_CHARS has a particular meaning that should
be reused for similar uses.
However, leaving the delimiter at NULL is not good because it's unclear who
wants that default behavior (and what the default should be). Don't allow that.
There are almost no callers that relied on this default anyway.
Fixes a crash on failed AddAndActivate:
$ ip link set eth0 down
$ nmcli d conn eth0
Error: Failed to add/activate new connection: Connection 'eth0' is not available on device eth0 because device has no carrier
<NetworkManager crashes>
#3 0x000055555558b6c5 in _nm_g_return_if_fail_warning
#4 0x00005555557008c7 in nm_settings_has_connection
#5 0x0000555555700e5f in pk_add_cb
#6 0x0000555555726e30 in pk_call_cb
#7 0x0000555555726e30 in pk_call_cb
#8 0x0000555555726e30 in pk_call_cb
#9 0x00005555555aaea8 in _call_id_invoke_callback
#10 0x00005555555ab2e8 in _call_on_idle
https://github.com/NetworkManager/NetworkManager/pull/325
initscripts support rule-* and rule6-* files for that.
Up until now, we ignored these files for the most part, except if
a user configured such files, the profile could not contain any static
routes (or specify a route-table setting). This also worked together
with the dispatcher script "examples/dispatcher/10-ifcfg-rh-routes.sh".
We cannot now start taking over that file format for rules. It might
break existing setups, because we can never fully understand all rules as
they are understood by iproute2. Also, if a user has a rule/rule6 file and
uses NetworkManager successfully today, then clearly there is a script
in place to make that work. We must not break that when adding rules
support.
Hence, store routing rules as numbered "ROUTING_RULE_#" and
"ROUTING_RULE6_#" keys.
Note that we use different keys for IPv4 and IPv6. The main reason is
that the string format is mostly compatible with iproute2. That means,
you can take the value and pass it to `ip rule add`.
However, `ip rule add` only accepts IPv4 rules. For IPv6 rules, the user
needs to call `ip -6 rule add`. If we would use the same key for IPv4
and IPv6, then it would be hard to write a script to do this.
Also, nm_ip_routing_rule_from_string() does take the address family as
hint in this case. This makes
ROUTING_RULE_1="pref 1"
ROUTING_RULE6_1="pref 1"
automatically determine that address families. Otherwise, such
abbreviated forms would be not valid.
It's usually not necessary, because _nm_utils_unescape_spaces()
gets called after nm_utils_strsplit_set(), which already removes
the non-escaped spaces.
Still, for completeness, this should be here. Also, because with
this the function is useful for individual options (not delimiter
separate list values), to support automatically dropping leading or
trailing whitespace, but also support escaping them.
The defaults for test timeouts in meson is 30 seconds. That is not long
enough when running
$ NMTST_USE_VALGRIND=1 ninja -C build test
Note that meson supports --timeout-multiplier, and automatically
increases the timeout when running under valgrind. However, meson
does not understand that we are running tests under valgrind via
NMTST_USE_VALGRIND=1 environment variable.
Timeouts are really not expected to be reached and are a mean of last
resort. Hence, increasing the timeout to a large value is likely to
have no effect or to fix test failures where the timeout was too rigid.
It's unlikely that the test indeed hangs and the increase of timeout
causes a unnecessary increase of waittime before aborting.
libnm exposes simplified variants of hexstr2bin in its public API. I
think that was a mistake, because libnm should provide NetworkManager
specific utils. It should not provide such string functions.
However, nmcli used to need this, so it was added to libnm.
The better approach is to add it to our internally shared static
library, so that all interested components can make use of it.
- use gs_free instead of explicit free().
- use nm_streq*() instead of strcmp().
- move deletion of existing file after we successfully wrote
the new file.
- add parameter existing_path_readonly, to avoid to overwrite or
delete the existing path (if it exists). This is still mostly unused,
but will be necessary when we have read-only directories.
Next, we will update g_steal_pointer() to cast the return type
to the type of the argument. Hence, this automatic conversion
from setting (sub) classes to NMSetting no longer works.
Add an explict cast.
- use cleanup attribute to free memory
- return floating reference from _nm_connection_for_each_secret().
It's more idiomatic that a function that constructs a variant and
returns it, returns a floating variant.
_nm_connection_for_each_secret() (formerly for_each_secret()) and
_nm_connection_find_secret() (formerly find_secret()) operate on a
GVariant of secrets. For that, they implement certain assumptions
of how to handle secrets. For example, it must special-case VPN settings,
because there is no generic abstraction to handle regular secret and VPN
secrets the same.
Such special casing should only be done in libnm-core, at one place.
Move the code to libnm-core as internal API.
nm_setting_get_secret_flags() looks whether we have a suitable "-flags"
data value, or whether we have a secret with that name.
In fact, we know this is a valid secret-name. Even if there are no secret-flags
and the secret (currently) does not exists. We shall not care about the
return value.
Note that nm_setting_get_secret_flags() also for non-secrets will set
the flags to "NONE", which is just what we need.
We should no longer use nm_connection_for_each_setting_value() and
nm_setting_for_each_value(). It's fundamentally broken as it does
not work with properties that are not backed by a GObject property
and it cannot be fixed because it is public API.
Add an internal function _nm_connection_aggregate() to replace it.
Compare the implementation of the aggregation functionality inside
libnm with the previous two checks for secret-flags that it replaces:
- previous approach broke abstraction and require detailed knowledge of
secret flags. Meaning, they must special case NMSettingVpn and
GObject-property based secrets.
If we implement a new way for implementing secrets (like we will need
for WireGuard), then this the new way should only affect libnm-core,
not require changes elsewhere.
- it's very inefficient to itereate over all settings. It involves
cloning and sorting the list of settings, and retrieve and clone all
GObject properties. Only to look at secret properties alone.
_nm_connection_aggregate() is supposed to be more flexible then just
the two new aggregate types that perform a "find-any" search. The
@arg argument and boolean return value can suffice to implement
different aggregation types in the future.
Also fixes the check of NMAgentManager for secret flags for VPNs
(NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS). A secret for VPNs
is a property that either has a secret or a secret-flag. The previous
implementation would only look at present secrets and
check their flags. It wouldn't check secret-flags that are
NM_SETTING_SECRET_FLAG_NONE, but have no secret.
While nm_utils_inet*_ntop() accepts a %NULL buffer to fallback
to a static buffer, don't do that.
I find the possibility of using a static buffer here error prone
and something that should be avoided. There is of course the downside,
that in some cases it requires an additional line of code to allocate
the buffer on the stack as auto-variable.
The 'number' property in GSM settings is a legacy thing that comes
from when ModemManager used user-provided numbers, if any, to connect
3GPP modems.
Since ModemManager 1.0, this property is completely unused for 3GPP
modems, and so it doesn't make sense to use it in the NetworkManager
settings. Ofono does not use it either.
For AT+PPP-based 3GPP modems, the 'number' to call to establish the
data connection is decided by ModemManager itself, e.g. for standard
GSM/UMTS/LTE modems it will connect a given predefined PDP context,
and for other modems like Iridium it will have the number to call
hardcoded in the plugin itself.
https://github.com/NetworkManager/NetworkManager/pull/261
When updating a connection passing agent-owned secret, they are lost
from @reread_connection after the settings-plugin persists the
connection. Therefore we need to cache and reapply them separately to
the connection so that they can be saved to secret agents later.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/82
The writer should write all properties of the sriov setting when the
setting exists without additional logic. Likewise, the reader should
instantiate a sriov setting when any sriov key is present and blindly
set properties from keys.
The old code did not always preserve the presence of a sriov setting
after a write/read cycle.
Fixes: c02d1c488f
Report an error when the user tries to add an unknown attribute
instead of silently accepting (and ignoring) it.
Note that this commit also changes the behavior of public API
nm_utils_sriov_vf_from_str() to return an error when an unknown
attribute is found. I think the previous behavior was buggy as wrong
attributes were simply ignored without any way for the user to know.
Fixes: a9b4532fa7
This code will be used later.
We want to remember which keyfiles are currently loaded (or hidden).
With the addition or multiple keyfile directories (soon), there are
two cases where this matters:
- if there are multiple keyfiles which reference the same UUID,
we can only load one of them. That is already a problem today
with only one keyfile directory, where multiple files can reference
the same UUID.
The implementation will pick the file based on priorities (like
the file modification date). However, the user may call explicitly
call `nmcli connection load`. In that case, we cannot reload
all files to find out whether the to be loaded file is hidden
according to the defined priorities. We cannot do that, because we
must not make decisions based on files on disk, which we are not told
to reload. So, during a `nmcli connection load` we must look at
unrelated files, to determine how to load the file.
Instead, we do allow the user to load any file, even if it would be
shadowed by other files. When we do that, we may want to persist which
file is currently loaded, so that a service restart and a `nmcli connection
reload` does not undo the load again. This can be later later be solved by
writing a symlink
"/var/run/NetworkManager/system-connections/.loaded-$UUID.nmkeyfile"
which targets the currently active file.
- if a profile was loaded from read-only persistant storage, the user
may still delete the profile. We also need to remember the deletion
of the file. That will be achieved by symlinking "/dev/null" as
"/etc/NetworkManager/system-connections/.loaded-$UUID.nmkeyfile".
Add helper functions to read and write these symlinks.
