nmtstp_env1_add_test_func() prepares a certain environment (with dummy
interface) that is used by some tests. Extend it, to allow creating more
than one interface (currently up to two).
The major point of NMDedupMultiIndex is that it can de-duplicate
the objects. It thus makes sense the everybody is using the same
instance. Make the multi-idx instance of NMPlatform configurable.
This is not used outside of unit tests, because the daemon currently
always creates one platform instance and everybody then re-uses the
instance of the platform.
While this is (currently) only used by tests, and that the performance
optimization of de-duplicating is irrelevant for tests, this is still
useful. The test can then check whether two separate NMPlatform objects
shared the same instance and whether it was de-duplicated.
There really is no way around this. As we don't cache all the routes
(e.g. ignored based on rtm_protocol or rtm_type), we cannot know which
route was replaced, when we get a NLM_F_REPLACE message.
We need to request a new dump in that case, which can be expensive, if
there are a lot of routes or if replace happens frequently.
The only possible solutions would be:
1) NetworkManager caches all routes, but it also needs to make sure to
get *everything* right. In particular, to understand every relevant
route attribute (including those added in the future, which is
impossible).
2) kernel provides a reasonable API (rhbz#1337855, rhbz#1337860) that
allows to sufficiently understand what is going on based on the
netlink notifications.
When you issue
ip route replace broadcast 1.2.3.4/32 dev eth0
then this route may well replace a (unicast) route that we have in
the cache.
Previously, we would right away ignore such messages in
_new_from_nl_route(), which means we miss the fact that a route gets
replaced.
Instead, we need to parse the message at least so far, that we can
detect and handle the replace.
We don't cache certain routes, for example based on the protocol. This is
a performance optimization to ignore routes that we usually don't care
about.
Still, if the user does `ip route replace` with such a route, then we
need to pass it to nmp_cache_update_netlink_route(), so that we can
properly remove the replaced route.
Knowing which route was replaces might be impossible, as our cache does
not contain all routes. Likely all that nmp_cache_update_netlink_route()
can to is to set "resync_required" for NLM_F_REPLACE. But for that it
should see the object first.
This also means, if we ever write a BPF filter to filter out messages
that contain NLM_F_REPLACE, because that would lead to cache inconsistencies.
The route table is part of the weak-id. You can see that with:
ip route replace unicast 1.2.3.4/32 dev eth0 table 57
ip route replace unicast 1.2.3.4/32 dev eth0 table 58
afterwards, `ip route show table all` will list both routes. The replace
operation is only per-table. Note that NMP_CACHE_ID_TYPE_ROUTES_BY_WEAK_ID
already got this right.
Fixes: 10ac675299 ('platform: add support for routing tables to platform cache')
By setting "NMTST_DEBUG" to any non-empty string, "is_debug" is enabled.
So "NMTST_DEBUG='debug,...'" is mostly redundant. Note however you can
still disable debug mode explicitly, like "NMTST_DEBUG=no-debug,...",
which can make sense if you want to set other flags but not enabling
debug mode (like "NMTST_DEBUG=no-debug,quick").
You can also explicitly set the log level ("NMTST_DEBUG='log-level=TRACE,...'"
or enable trace debugging with "NMTST_DEBUG='d,...'", where "d" (or "D")
is shorthand for "NMTST_DEBUG=log-level=TRACE,no-expect-message,...".
Anyway. If you explicitly set the log level with "log-level=" or "d",
debug logging is enabled with the "debug" flag. But it only logged at
level "<debug>". That seems not best. Instead, enable "<trace>" level by
default in debug mode.
That's useful, because there is not a clear distinction between
"<debug>" and "<trace>" level. When debugging, you really want all the
information you got, you can also filter out later (`grep` is a thing).
There is g_ptr_array_copy() in glib, but only since 2.68 so we cannot use it.
We had a compat implementation nm_g_ptr_array_copy(), however that one always
requires an additional parameter, the free function of the new array.
g_ptr_array_copy() always does a deep clone, and uses the source array's
free function. We don't have access to the free function (seems quite a
limitation of GPtrArray API), so our nm_g_ptr_array_copy() cannot be
exactly the same.
Previously, nm_g_ptr_array_copy() aimed to be as similar as possible to
g_ptr_array_copy(), and it would require the caller that the free
function is the same as the array's. That seems an unnecessary
limitation, and our compat implementation still looks different and has
a different name. If we were able to fully re-implement it, we would
instead add it to "nm-glib.h".
Anyway. As our implementation already differs, there is no need for the
arbitrary limitation to only perform deep copies. Instead, also allow
shallow copies. Rename the function to nm_g_ptr_array_new_clone() to
make it clearly distinct from g_ptr_array_copy().
CURLOPT_PROTOCOLS [0] was deprecated in libcurl 7.85.0 with
CURLOPT_PROTOCOLS_STR [1] as a replacement.
Well, technically it was only deprecated in 7.87.0, and retroactively
marked as deprecated since 7.85.0 [2]. But CURLOPT_PROTOCOLS_STR exists
since 7.85.0, so that's what we want to use.
This causes compiler warnings and build errors:
../src/core/nm-connectivity.c: In function 'do_curl_request':
../src/core/nm-connectivity.c:770:5: error: 'CURLOPT_PROTOCOLS' is deprecated: since 7.85.0. Use CURLOPT_PROTOCOLS_STR [-Werror=deprecated-declarations]
770 | curl_easy_setopt(ehandle, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
| ^~~~~~~~~~~~~~~~
In file included from ../src/core/nm-connectivity.c:13:
/usr/include/curl/curl.h:1749:3: note: declared here
1749 | CURLOPTDEPRECATED(CURLOPT_PROTOCOLS, CURLOPTTYPE_LONG, 181,
| ^~~~~~~~~~~~~~~~~
This patch is largely taken from systemd patch [2].
Based-on-patch-by: Frantisek Sumsal <frantisek@sumsal.cz>
[0] https://curl.se/libcurl/c/CURLOPT_PROTOCOLS.html
[1] https://curl.se/libcurl/c/CURLOPT_PROTOCOLS_STR.html
[2] 6967571bf2
[3] e61a4c0b7c
Fixes: 7a1734926a ('connectivity,cloud-setup: restrict curl protocols to HTTP and HTTPS')
When only running a subset of the tests (with "-p"), then valgrind
indicates a leak. Avoid that.
$ ./tools/run-nm-test.sh -m src/core/platform/tests/test-route-linux -v
# no leak
$ ./tools/run-nm-test.sh -m src/core/platform/tests/test-route-linux -v -p /route/ip4
# many leaks:
==1662102== 107 (96 direct, 11 indirect) bytes in 1 blocks are definitely lost in loss record 388 of 448
==1662102== at 0x4848464: calloc (vg_replace_malloc.c:1340)
==1662102== by 0x4F615F0: g_malloc0 (gmem.c:163)
==1662102== by 0x1621A6: _nmtst_add_test_func_full (nm-test-utils.h:918)
==1662102== by 0x1623EB: _nmtstp_setup_tests (test-route.c:2179)
==1662102== by 0x16E53D: main (test-common.c:2693)
==1662102==
{
<insert_a_suppression_name_here>
Memcheck:Leak
match-leak-kinds: definite
fun:calloc
fun:g_malloc0
fun:_nmtst_add_test_func_full
fun:_nmtstp_setup_tests
fun:main
}
This allows to free resources (a pointer) at the end of the test.
The purpose is to avoid valgrind warnings about leaks. While a leak
in the test is not a severe issue by itself, it does interfere with
checking for actual leaks. Thus every leak must be avoided.
Only allocate one chunk of memory to contain all data of
NmtstTestData.
This isn't about performance (which doesn't matter for test code).
It's about packing all in one struct and being able to free all at
once with a simple g_free(). We no longer need _nmtst_test_data_free()
with this.
Note that NmtstTestData is never mutated, it just holds some data.
As such, the single place where such a structure gets initialized,
can become a bit more complicated, in exchange for having a trivial
free operation (and anyway there no functions that modify the data
or that would care about the data layout).
In kernel, the valid range for the weight is 1-256 (on netlink this is
expressed as u8 in rtnh_hops, ranging 0-255).
We need an additional value, to represent
- unset weight, for non-ECMP routes in kernel.
- in libnm API, to express routes that should not be merged as ECMP
routes (the default).
Extend the type in NMPlatformIP4Route.weight to u16, and fix the code
for the special handling of the numeric range.
Also the libnm API needs to change. Modify the type of the attribute on
D-Bus from "b" to "u", to use a 32 bit integer. We use 32 bit, because
we already have common code to handle 32 bit unsigned integers, despite
only requiring 257 values. It seems better to stick to a few data types
(u32) instead of introducing more, only because the range is limited.
Co-Authored-By: Fernando Fernandez Mancera <ffmancera@riseup.net>
Fixes: 1bbdecf5e1 ('platform: manage ECMP routes')
There are two callers of available_connections_add(). One from
cp_connection_added_or_updated() (which is when a connection
gets added/modified) and one from nm_device_recheck_available_connections().
They both call first nm_device_check_connection_available() to see
whether the profile is available on the device. They certainly
need to pass the same check flags, otherwise a profile might
be available in some cases, and not in others.
I didn't actually test this, but I think this could result
in a profile wrongly not being listed as an available-connection.
Moreover, that might mean, that `nmcli connection up $PROFILE`
might work to find the device/profile, but `nmcli device up $DEVICE`
couldn't find the suitable profile (because the latter calls
nm_device_get_best_connection(), which iterates the
available-connections). I didn't test this, because regardless of
that, it seems obvious that the conditions for when we call
available_connections_add() must be the same from both places.
So the only question is what is the right condition, and it would
seem that _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST is the right
flag.
Fixes: 02dbe670ca ('device: for available connections check whether they are available for user-request')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1496
Sometimes the buffer space of the netlink socket runs out and we lose
the response to our link change:
<info> [1670321010.2952] platform-linux: netlink[rtnl]: read: too many netlink events. Need to resynchronize platform cache
<warn> [1670321010.3467] platform-linux: do-change-link[2]: failure changing link: internal failure 3
With 3 above being WAIT_FOR_NL_RESPONSE_RESULT_FAILED_RESYNC.
Let's try harder.
https://bugzilla.redhat.com/show_bug.cgi?id=2154350
$ nmcli --offline connection add type wifi con-name hotspot ssid hotspot-ssid wifi.mode ap wifi-sec.key-mgmt none wifi-sec.wep-key-type 1 wifi-sec.wep-key0 1234567890
would previously always print a message
Info: WEP key is guessed to be of '1 (key)'
At least, when we explicitly set the key-type, this message is bogus.
Suppress it.
It's anyway questionable whether printing such warnings does anything good.
We would still get the warning with the arguments swapped, which seems wrong:
$ nmcli --offline connection add type wifi con-name hotspot ssid hotspot-ssid wifi.mode ap wifi-sec.key-mgmt none wifi-sec.wep-key0 1234567890 wifi-sec.wep-key-type 1
Info: WEP key is guessed to be of '1 (key)'
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1497
iptables takes a file lock at /run/xtables.lock. By default, if
the file is locked, iptables will fail with error. When that happens,
the iptables rules won't be configured, and the shared mode
(for which we use iptables) will not be setup properly.
Instead, pass "--wait 2", to block. Yes, it's ugly that we use
blocking program invocations, but that's how it is. Also, iptables
should be fast to not be a problem in practice.
This is not nice:
<warn> [1670321010.3467] platform-linux: do-change-link[2]: failure changing link: internal failure 3
Let's explain what "internal failure 3" is.
Next, support for "other_config" will be added. That is very similar
to "external_ids". Extend the existing code, to make that next update
simpler. The only purpose of this patch, is to reduce the diff of
when actually adding "other_config". Only in light of that, do some
of the changes here make sense.
We will add support for "other_config". This is in many aspects similar
to "external-ids". So first do a renaming, so that the code can be
sensibly reused. This is a separate patch, so that the followup commit
has less noise in the diff.
This function *only* renames (and reformats). No other changes.
"mutate" with operation "insert" does not update existing entries.
Delete them first.
Otherwise, a reapply that only change the value of an external-ids
entry does not work.
Note that https://www.rfc-editor.org/rfc/rfc7047 says about
"<mutations>":
If <mutator> is "insert", then each of the key-value pairs in
the map in <value> is added to <column> only if its key is not
already present. The required type of <value> is slightly
relaxed, in that it may have fewer than the minimum number of
elements specified by the column's type.
Fixes: 7055539c9f ('core/ovs: support setting OVS external-ids')
Doing an "update" is wrong, because that will replace all "other_config"
entries. We only want to reset the "hwaddr".
Note that https://www.rfc-editor.org/rfc/rfc7047 says about
"<mutations>":
If <mutator> is "insert", then each of the key-value pairs in
the map in <value> is added to <column> only if its key is not
already present. The required type of <value> is slightly
relaxed, in that it may have fewer than the minimum number of
elements specified by the column's type.
That means, we need to first delete, and then insert the key.
Fixes: 5d4c8521a3 ('ovs: set MAC address on the bridge for local interfaces')
