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.
We cannot know the key/value free functions, hence, our compat implementation
cannot free the values if they are not requested. The "solution" is to require
the caller to fetch all values, always.
nm_client_get_capabilities() was backported to 1.22.2. Add to to the
appropriate linker version.
Officially (and according to docs) nm_client_get_capabilities() still
appears first in libnm 1.24.0. However, as it got backported to 1.22.2,
it needs to be part of a different symbol version on 1.22. Instead
of adding the symbol twice (once for libnm_1_24_0 and libnm_1_22_2),
move it only to the libnm_1_22_2 symbol version, also on master.
Also the child fds are set to -1 so that nm_close() doesn't throw an assertion.
Fixes: df1d214b2 (clients: polkit-agent: implement polkit agent without using libpolkit)
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.
The method field is set from (only) via a GObject property setter,
which sets a value of type int. As we afterwards validate that the
value is in a valid range, we should use a suitable type to hold
the value to begin with. Of course, in almost all cases is the
underlying type of the enum already int.
G_PARAM_CONSTRUCT cause to explicitly initialize the property during
object construction. This is an unnecessary overhead that we can easily
avoid.
The overhead is because G_PARAM_CONSTRUCT parameters are always set with
g_object_set() before calling constructed(). Even if they are not specified
during g_object_new(), in which case it calls set with the property's default
value. This also requires g_object_new() to iterate all properties to
find and sort the construct properties.
NMSetting are supposed to be simple classes. They don't need to have
their properties initialized before object construction completes.
Especially if the default values are NULL or zero, in which case there
is nothing to do. If the default value is not NULL or zero, we need
to initialize the field instead in the nm_setting*_init() function.
If we know the address used previously, also tell the client to start
from the init-reboot phase, so that it will start with a DHCP request
instead of a discover.
Instead of terminating the program when the dispatch function returns
an invalid return code, log an error message and convert the error
code to a valid, generic one.
https://bugs.archlinux.org/task/64880
After t1, the client tries to renew the lease by contacting via the
udp socket the server specified in the server-id option. If this
fails, after t2 it tries to contact any server using broadcast. For
this to work, the packet socket must be used.
Currently the client always starts from the INIT state (i.e. sending a
discover message). If a requested-ip was specified by the caller, it
is added as an option in the discover.
It was reported that some DHCP servers don't respond to discover
messages with the requested-ip option set [1][2].
The RFC allows to skip the discover by entering the INIT-REBOOT state
and starting directly with a broadcast request message containing the
requested IP address. Implement that.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1781856
[2] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/310
The following used functions:
`compiler.has_link_argument`,
`link_whole arg in declare_dependency`,
`compiler.has_multi_link_argument`
are present only from meson 0.46.
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.
