The _GET_PRIVATE() macros are all implemented based on
_NM_GET_PRIVATE(). That macro tries to be more type safe and uses
_Generic() to do the right thing. Explicitly casting is not only
unnecessary, it defeats these (static) type checks.
Don't do that.
Several macros are used to define function. They had a "_STATIC" variant,
to define the function as static.
I think those macros should not try to abstract entirely what they do.
They should not accept the function scope as argument (or have two
variants per scope). This also because it might make sense to add
additional __attribute__(()) to the function. That only works, if
the macro does not pretend to *not* define a plain function.
Instead, embrace what the function does and let the users place the
function scope as they see fit.
This also follows what is already done with
static NM_CACHED_QUARK_FCN ("autoconnect-root", autoconnect_root_quark)
In all the cases, we don't want to perform locale dependent comparison.
$ sed -i 's/\<strcasecmp\>/g_ascii_\0/g' $(git grep -w -l strcasecmp -- ':(exclude)shared/systemd/' )
Most callers would pass FALSE to nm_utils_error_is_cancelled(). That's
not very useful. Split the two functions and have nm_utils_error_is_cancelled()
and nm_utils_error_is_cancelled_is_disposing().
Add missing 'extern' keyword to fix the following error caused by GCC
10 defaulting to -fno-common:
src/settings/plugins/ifcfg-rh/.libs/libnms-ifcfg-rh-core.a(libnms_ifcfg_rh_core_la-shvar.o):/root/NetworkManager/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h:36: multiple definition of `nms_ifcfg_well_known_keys';
src/settings/plugins/ifcfg-rh/.libs/libnm_settings_plugin_ifcfg_rh_la-nms-ifcfg-rh-plugin.o:/root/NetworkManager/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h:36: first defined here
We should use the same "is-valid" function everywhere.
Since nm_utils_ipaddr_valid() is part of libnm, it does not qualify.
Use nm_utils_ipaddr_is_valid() instead.
and _nm_utils_inet6_ntop() instead of nm_utils_inet6_ntop().
nm_utils_inet4_ntop()/nm_utils_inet6_ntop() are public API of libnm.
For one, that means they are only available in code that links with
libnm/libnm-core. But such basic helpers should be available everywhere.
Also, they accept NULL as destination buffers. We keep that behavior
for potential libnm users, but internally we never want to use the
static buffers. This patch needs to take care that there are no callers
of _nm_utils_inet[46]_ntop() that pass NULL buffers.
Also, _nm_utils_inet[46]_ntop() are inline functions and the compiler
can get rid of them.
We should consistently use the same variant of the helper. The only
downside is that the "good" name is already taken. The leading
underscore is rather ugly and inconsistent.
Also, with our internal variants we can use "static array indices in
function parameter declarations" next. Thereby the compiler helps
to ensure that the provided buffers are of the right size.
Just read the content once. It's wasteful to read the file twice
while parsing.
But what is worse, the file content can change any time we read it.
We make decisions based on what we read, and hence we should only
read the file once in the parsing process to get one consistent result.
Split the parsing of the file content to a separate function.
Next we will load IPv4 files only once, and thus only need to parse
the content without reading it.
Note that the function temporarily modifies the input buffer, but
restores the original content before returning.
We will need both variants, so that the caller can read the file only
once.
Note that also utils_has_route_file_new_syntax_content() will
restore the original contents and not modify the input (from point
of view of the caller). In practice it will momentarily modify the
content.
Keyfile support was initially added under GPL-2.0+ license as part of
core. It was moved to "libnm-core" in commit 59eb5312a5 ('keyfile: merge
branch 'th/libnm-keyfile-bgo744699'').
"libnm-core" is statically linked with by core and "libnm". In
the former case under terms of GPL-2.0+ (good) and in the latter case
under terms of LGPL-2.1+ (bad).
In fact, to this day, "libnm" doesn't actually use the code. The linker
will probably remove all the GPL-2.0+ symbols when compiled with
gc-sections or LTO. Still, linking them together in the first place
makes "libnm" only available under GPL code (despite the code
not actually being used).
Instead, move the GPL code to a separate static library
"shared/nm-keyfile/libnm-keyfile.la" and only link it to the part
that actually uses the code (and which is GPL licensed too).
This fixes the license violation.
Eventually, it would be very useful to be able to expose keyfile
handling via "libnm". However that is not straight forward due to the
licensing conflict.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/381
We don't need a separate "GSList *chains" to track the NMAuthChain
requests for the agents. Every agent should only have one auth-chain in
fly at any time. We can attach that NMAuthChain to the secret-agent.
Also, fix a race where:
1) A secret agent registers. We would start an auth-chain check, but not
yet track the secret agent.
2) Then the secret agent unregisters. The unregistration request will fail,
because the secret agent is not yet in the list of fully registered agents.
The same happens if the secret agent disconnects at this point.
agent_disconnect_cb() would not find the secret agent to remove.
3) afterwards, authentication completes and we register the
secret-agent, although we should not.
There is also another race: if we get authority_changed_cb() we would
not restart the authentication for the secret-agent that is still
registering. Hence, we don't know whether the result once it completes
would already contain the latest state.
Don't access the singleton getter here. Pass the agent-manager argument
instead to maybe_remove_agent_on_error().
Also, don't lookup the agent by name. We already know, whether the agent
is still tracked or not. Look at agent->agent_lst.
nm_agent_manager_get_agent_by_user() would only return the first
matching secret agent for the user. This way, we might miss an agent
that has permissions.
Instead, add nm_agent_manager_has_agent_with_permission() and search
all agents.
There was literally only one place where we would make use of
O(1) lookup of secret-agents: during removal.
In all other cases (which are the common cases) we had to iterate the
known agents. CList is more efficient and more convenient to use when
the main mode of operation is iterating.
Also note that handling secret agents inevitably scales linear with
the number of agents. That is, because for every check we will have
to sort the list of agents and send requests to them. It would be
very complicated (and probably less efficient for reasonable numbers
of secret agents) to avoid O(n).
NMAgentManager and NMSecretAgent work closely together. In particular,
the NMAgentManager creates and tracks the NMSecretAgents and controls
it.
Move NMSecretAgent struct to the header, so that some fields may become
accessible to NMAgentManager. In particular, we will track secret agents
with a CList, and this CList element can be embedded in the
NMSecretAgent structure.
Move it to shared as it's useful for clients as well.
Move and rename nm_dbus_manager_new_auth_subject_from_context() and
nm_dbus_manager_new_auth_subject_from_message() in nm-dbus-manager.c
as they're needed there.
We call svFindFirstNumberedKey() to check whether we have any NETMASK
set. Since commit 9085c5c3a9 ('ifcfg-rh: rename
svFindFirstKeyWithPrefix() to svFindFirstNumberedKey() for finding
NETMASK') that function would no longer find the "NETMASK" without
number.
Fix that, by letting nms_ifcfg_rh_utils_is_numbered_tag() return TRUE
for the tag itself. This also makes more sense, because it matches our
common understanding what numbered tags are.
Adjust the other callers that don't want this behavior to explicitly
check.
Fixes: 9085c5c3a9 ('ifcfg-rh: rename svFindFirstKeyWithPrefix() to svFindFirstNumberedKey() for finding NETMASK')
Previously, setting or getting a variable required to scan all lines.
Note that frequently we would look up variables that didn't actually
exist, which we could only determine after searching the entire list.
Also, since we needed to handle having the same variable specified
multiple times (where the last occurrence wins), we always had to search
all keys and couldn't stop when finding the first key. Well, technically
we could have searched in reverse order for the getter, but that wasn't
done. For the setter we wanted to delete all but the last occurrences,
so to find them, we really had to search them all.
We want to support profiles with hundreds or thousands of addresses and routes.
This does not scale well.
Add an hash table to find the variables in constant time.
Test this commit and the parent commit:
$ git clean -fdx &&
CFLAGS=-O2 ./autogen.sh --with-more-asserts=0 &&
./tools/run-nm-test.sh -m src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh &&
perf stat -r 50 -B src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh 1>/dev/null
Before:
Performance counter stats for 'src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh' (50 runs):
330.94 msec task-clock:u # 0.961 CPUs utilized ( +- 0.33% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
1,081 page-faults:u # 0.003 M/sec ( +- 0.07% )
1,035,923,116 cycles:u # 3.130 GHz ( +- 0.29% )
1,800,084,022 instructions:u # 1.74 insn per cycle ( +- 0.01% )
362,313,301 branches:u # 1094.784 M/sec ( +- 0.02% )
6,259,421 branch-misses:u # 1.73% of all branches ( +- 0.13% )
0.34454 +- 0.00116 seconds time elapsed ( +- 0.34% )
Now:
Performance counter stats for 'src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh' (50 runs):
329.78 msec task-clock:u # 0.962 CPUs utilized ( +- 0.39% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
1,084 page-faults:u # 0.003 M/sec ( +- 0.05% )
1,036,130,698 cycles:u # 3.142 GHz ( +- 0.13% )
1,799,851,979 instructions:u # 1.74 insn per cycle ( +- 0.01% )
360,374,338 branches:u # 1092.756 M/sec ( +- 0.01% )
6,160,796 branch-misses:u # 1.71% of all branches ( +- 0.08% )
0.34287 +- 0.00133 seconds time elapsed ( +- 0.39% )
So, not much difference. But this is not surprising, because test-ifcfg-rh loads and
writes predominantly ifcfg files with few variables. The difference should be visible
when having large files.
svFindFirstKeyWithPrefix() only had one caller: to find whether there are
any NETMASK variables set. NETMASK is a numbered variable, so we should only
find variables that indeed follow the pattern. Since there was only
one caller, rename and repurpose the function.
When we write a connection profile to ifcfg-rh file, we first load the
possibly existing file and modify it. The purpose is to preserve
variables that we don't know about, keep comments and preserve the order
of the variables.
Note that the writer sets a bunch of variables according to the
profile's setting. At various places the writer would explicitly
clear variables with svUnsetValue(). However, that was problematic:
- we would not unset all variables that we care about. We really should
not leave previous variables if they make no sense anymore for the
profile. The only thing we want to preserve are entirely unknown keys
and comments. Note that when the writer omits to clear an unset variable,
it usually does so assuming that the reader would anyway ignore the
key, become some other key renders it irrelevant. Given the complexity
of the reader and writer, that is often not the case and hard to ensure.
We might have simply forgotten a svUnsetValue(), which was an easy
to make mistake and hard to find (because you'd have to test with
a pre-existing profile that happens to contain that key, which leaves
countless combinations for testing.
That means, a profile written by the writter might be interpreted
differently by the reader depending on which pre-existing keys were set.
- it was cumbersome to explicitly call svUnsetValue().
Note that for numbered tags in particular we would iterate the keys
trying to unset them. For example for addresses (like "IPADDR5") we
would iterate over the first 256 IPADDR keys, trying to unset them.
That is horrible. For one, it doesn't cover the case where there might
be more than 256 addresses. Also, it adds a significant overhead every
time.
While writing a ifcfg file currently is O(n^2) because setting one key
is O(l), with l being the number of keys/lines. So, if you set n keys
in a file with l lines, you get O(n*l). Which is basically O(n^2),
because the number of lines and the number of keys to set usually
corresponds.
So when setting 256 times IPADDR, the overall complexity was still
O(n^2 + 256 * n) and didn't change. However, the 256 factor here can
be very significant.
We should not explicitly unset variables, we should always unset all
known variables that we don't explicitly set.
The svUnsetValue() calls are still there. They will be dropped next.
Helper function to remove all variables that are still dirty (not
visited) and well-known.
Also add svWriteFileWithoutDirtyWellknown() to clear the lines
before persisting to disk.
This adds a lot of meta-data about how we handle ifcfg-rh.
We will use this to prune/delete all variables that are not explicitly
set (dirty) but also well-known.
We could now easily emit a warning when an ifcfg-rh file contains
unused key.
We also could add more meta-data for each key. For example, we write
different files (ifcfg- and keys- files). We could add flags to indicate
that variables are valid in certain files. Currently that's not done.
Also, for simple properties we could associate the key with the
NMSetting property, and treat does generically, like keyfile does.
Anyway, there are potentials. For now, we will use this to clear dirty
variables.
Previously, IS_NUMBERED_TAG() could only be called with a C literal.
Add is_numbered_tag() which can be called with any C string.
Also, IS_NUMBERED_TAG_PARSE() and IS_NUMBERED_TAG() didn't do exactly
the same. I think they should. The only difference was if the number
was larger than 2^63-1. Now IS_NUMBERED_TAG() starts ignoring such
keys, which is fine.
By default, all lines are now marked as dirty. Whenever we modify/set
a line, it becomes non-dirty. That will be used later to prune lines
that are dirty, that is, not yet visited.
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
- systemd-networkd and initscripts both support it.
- it seems suggested to configure routes with scope "link" on AWS.
- the scope is only supported for IPv4 routes. Kernel ignores the
attribute for IPv6 routes.
- we don't support the aliases like "link" or "global". Instead
only the numeric value is supported. This is different from
systemd-networkd, which accepts names like "global" and "link",
but no numerical values. I think restricting ourself only to
the aliases unnecessarily limits what is possible on netlink.
The alternative would be to allow aliases and numbers both,
but that causes multiple ways to define something and has
thus downsides. So, only numeric values.
- when setting rtm_scope to RT_SCOPE_NOWHERE (0, the default), kernel
will coerce that to RT_SCOPE_LINK. This ambiguity of nowhere vs. link
is a problem, but we don't do anything about it.
- The other problem is, that when deleting a route with scope RT_SCOPE_NOWHERE,
this acts as a wild care and removes the first route that matches (given the
other route attributes). That means, NetworkManager has no meaningful
way to delete a route with scope zero, there is always the danger that
we might delete the wrong route. But this is nothing new to this
patch. The problem existed already previously, except that
NetworkManager could only add routes with scope nowhere (i.e. link).
There is an "info" part and a part with the data that we parsed.
Don't track the static and mutable data in the same variable.
Also, this allows to mark the static part as "const static".
In the past, kernel (and NetworkManager) did not support the onlink
flags for IPv6 routes. That is no longer the case.
Fixes: f5e8bbc8e0 ('libnm,core: enable "onlink" flags also for IPv6 routes')
