When you call
nm_uuid_generate_from_strings_strv(uuid_type, type_arg, v1, v2);
you'd probably expect that both values are honored in some way.
However, if v1 happened to be NULL, then previously v2 would be ignored.
Extend nm_uuid_generate_from_strings() to accept also NULL values and
pass on the length. Currently, there are no users of nm_uuid_generate_from_strings(),
so nobody is affected by this change.
Also extend nm_uuid_generate_from_strings_strv() to take a length
argument. It still accepts "-1" to take the input strv as a NULL
terminated array.
If a positive length is provided to nm_uuid_generate_from_strings_strv(),
it hashes the same UUID as the respective NULL terminated array. But of
course only, if there is no NULL inside the array. If there are any
NULLs, a distinct UUID gets generated.
For a long time we have a function like nm_uuid_generate_from_strings().
This was recently reworked and renamed, but it preserved behavior. Preserving
behavior is important for this function, because for the existing users,
we need to keep generating the same UUIDs.
Originally, this function was a variadic function with NULL sentinel.
That means, when you write
nm_uuid_generate_from_strings(uuid_type, type_arg, v1, v2, v3, NULL);
and v2 happens to be NULL, then v3 is ignored. That is most likely not
what the user intended. Maybe they had a bug and v2 should not be NULL.
But nm_uuid_generate_from_strings() should not require that all
arguments are non-NULL and it should not ignore arguments after the
first NULL.
For example, one user works around this via
uuid = nm_uuid_generate_from_strings_old("ibft",
s_hwaddr,
s_vlanid ? "V" : "v",
s_vlanid ? s_vlanid : "",
s_ipaddr ? "A" : "DHCP",
s_ipaddr ? s_ipaddr : "");
which is cumbersome and ugly.
That will be fixed next, by adding a function that doesn't suffer
from this problem. But "this problem" is part of the API of the
function, we cannot just change it. Instead, rename it and all
users, so they can keep doing the same.
New users of course should no longer use the "old" function.
The file "nm-meta-setting-base-impl.c" is shared by "libnm-core-impl" and
"libnmc-setting". For "libnm-core-impl" it uses a efficient lookup from the
gtype. For "libnmc-setting", that class information is not available, so
it did a linear search. Instead, do a binary search.
Tested:
diff --git a/src/libnm-core-impl/nm-meta-setting-base-impl.c b/src/libnm-core-impl/nm-meta-setting-base-impl.c
index 3434c858391f..62c366d2ca42 100644
--- a/src/libnm-core-impl/nm-meta-setting-base-impl.c
+++ b/src/libnm-core-impl/nm-meta-setting-base-impl.c
@@ -821,6 +821,11 @@ nm_meta_setting_infos_by_gtype(GType gtype)
{
const NMMetaSettingInfo *setting_info;
+#if _NM_META_SETTING_BASE_IMPL_LIBNM
+ return _infos_by_gtype_binary_search(gtype);
+#endif
+ nm_assert_not_reached();
+
#if _NM_META_SETTING_BASE_IMPL_LIBNM
setting_info = _infos_by_gtype_from_class(gtype);
#else
diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c
index 85d549eb766c..65fcafd076c9 100644
--- a/src/libnm-core-impl/tests/test-setting.c
+++ b/src/libnm-core-impl/tests/test-setting.c
@@ -5134,6 +5134,29 @@ main(int argc, char **argv)
{
nmtst_init(&argc, &argv, TRUE);
+ {
+ gs_unref_object NMConnection *con = NULL;
+ guint8 ctr = 0;
+ guint i;
+
+ con = nmtst_create_minimal_connection("test", NULL, NM_SETTING_WIRED_SETTING_NAME, NULL);
+
+ nm_connection_add_setting(con, nm_setting_wired_new());
+
+ nmtst_connection_normalize(con);
+ nmtst_assert_connection_verifies_without_normalization(con);
+
+ for (i = 0; i < 10000000; i++) {
+ ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_WIRED));
+ ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_CONNECTION));
+ ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_PROXY));
+ ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_WIREGUARD));
+ ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_IP4_CONFIG));
+ }
+
+ return !!ctr;
+ }
+
g_test_add_func("/libnm/test_connection_uuid", test_connection_uuid);
g_test_add_func("/libnm/settings/test_nm_meta_setting_types_by_priority",
Results of `make src/libnm-core-impl/tests/test-setting && libtool --mode=execute perf stat -r 5 -B src/libnm-core-impl/tests/test-setting`:
1) previous linear search: 3,182.81 msec
2) bsearch not inlined: 1,611.19 msec
3) bsearch open-coded: 1,214.45 msec
4) bsearch inlined: 1,167.70 msec
5) lookup from class: 1,147.64 msec
1) previous implementation
2) using nm_array_find_bsearch()
3) manually implementing binary search
4) using nm_array_find_bsearch_inline()
5) only available to libnm-core as it uses internal meta data
Note that for "libnm-core-impl" the implementation was already fast (5), and
it didn't change. It only changed to binary search for "libnmc-setting",
which arguably is a less strong use-case.
The file "nm-meta-setting-base-impl.c" is present twice in our source
tree and compiled twice. Once with internal headers of libnm-core and
once with only public headers.
Consequently, there are two implementations for
nm_meta_setting_infos_by_gtype(), one that can benefit from internal
access to the data structure, and the one for libnmc, which cannot.
Refactor the implementations, in the hope to have it clearer.
To implement binary search is not very hard. It's almost easy enough to
just open-code it, without using the existing nm_array_find_bsearch() function.
In particular, because nm_array_find_bsearch() won't be inlined,
and thus it is slower than implementing it by hand.
Add nm_array_find_bsearch_inline() as a variant that will be inlined.
This actually is as fast as reimplementing it by hand (I measured),
which takes away any reason to avoid the function.
However, our headers get huge. That may be a problem for complication
time. To counter that a bit, only define the function when the caller
requests it with a NM_WANT_NM_ARRAY_FIND_BSEARCH_INLINE define.
At startup, we remove from ovsdb any existing interface created by NM
and later an interface with the same name might be readded. This can
cause race conditions. Consider this series of events:
1. at startup NM removes the entry from ovsdb;
2. ovsdb reports success;
3. NM inserts an interface with the same name again;
4. ovs-vswitch monitors ovsdb changes, and gets events for removal and
insertion. Depending on how those events are split in different
batches, it might decide:
4a. to delete the link and add it back, or
4b. to keep the existing link because the delete and insertion
cancel out each other.
When NM sees the link staying in platform, it doesn't know if it's
because of 4b or because 4a will happen eventually.
To avoid this ambiguity, after ovsdb reports the successful deletion
NM should also wait that the link disappears from platform.
Unfortunately, this means that ovsdb gets a dependency to the platform
code.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1386
These checks don't seem very useful, to have them enabled
in production code.
What is actually the real danger of messing up with binary search,
is that the input array is not properly sorted. Asserting for that
would be way more useful, but also likely too expensive to be worth
it.
Checking that the input arguments are not NULL/zero, is not that useful,
because we "usually" won't make such mistakes.
While at it, declare each local variable on a separate line.
For connections with multi-connect property set to "multiple", the
autoconnect-retries should be tracked per device and not per connection.
That means, if autoconnect-retries is set to 2, each device using that
connection should retry to autoconnect 2 times.
The device autoconnect retries is -2 by default. This is a special
value, in NMPolicy context, if the connection used is multi-connect the
device value will be set to match the connection retries. Each time the
device picks a different connection, it will reset the device
autoconnect retries to -2 and if needed, sync. with the connection
retries.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1387https://bugzilla.redhat.com/show_bug.cgi?id=2039734
This partly reverts 1fe8166fc9 ('device: only deactivate when the master
we've enslaved to goes away').
If the controller fails while the port is not yet fully attached,
before this patch the following happened:
<info> [1664299566.1065] device (bond0): state change: ip-config -> failed (reason 'config-failed', sys-iface-state: 'managed')
...
<warn> [1664299566.1073] device (bond0): Activation: failed for connection 'bond0'
<trace> [1664299566.1073] device[6b76ac7314eb0b53] (bond0): master: release one slave a9f10ea824bb1725/eth1 (not enslaved) (configure)
<debug> [1664299566.1073] device[a9f10ea824bb1725] (eth1): unmanaged: flags set to [!sleeping,!by-type,!platform-init,!user-explicit,!user-settings,!user-conf=0x0/0x179/managed], forget [is-slave=0x800], reason removed)
...
<info> [1664299566.1080] device (eth1): state change: config -> ip-config (reason 'none', sys-iface-state: 'managed')
Note that now eth1 has no controller, but it lingers in "ip-config" state indefinitely.
If we look at a case where the port is already attached we see:
<info> [1664299540.9661] device (bond0): state change: secondaries -> failed (reason 'config-failed', sys-iface-state: 'managed')
...
<warn> [1664299540.9667] device (bond0): Activation: failed for connection 'bond0'
<trace> [1664299540.9667] device[6b76ac7314eb0b53] (bond0): master: release one slave a9f10ea824bb1725/eth1 (enslaved) (configure)
<debug> [1664299540.9667] platform: (eth1) link: releasing 10 from master 'bond0' (80)
...
<info> [1664299540.9740] device (bond0): detached bond port eth1
...
<debug> [1664299540.9749] device[a9f10ea824bb1725] (eth1): Activation: connection 'eth1' master failed
...
<warn> [1664299540.9749] device (eth1): queue-state[secondaries, reason:none, id:520]: replace previously queued state change
...
<debug> [1664299540.9750] device[a9f10ea824bb1725] (eth1): queue-state[deactivating, reason:dependency-failed, id:533]: queue state change
<debug> [1664299540.9751] device[a9f10ea824bb1725] (eth1): unmanaged: flags set to [!sleeping,!by-type,!platform-init,!user-explicit,!user-settings,!user-conf=0x0/0x179/managed], forget [is-slave=0x800], reason removed)
...
<debug> [1664299541.0201] device[a9f10ea824bb1725] (eth1): enslaved to unknown device 0 (??)
...
<debug> [1664299541.0227] device[a9f10ea824bb1725] (eth1): queue-state[deactivating, reason:dependency-failed, id:533]: change state
<info> [1664299541.0228] device (eth1): state change: ip-check -> deactivating (reason 'dependency-failed', sys-iface-state: 'managed')
Fix that by not ignoring the nm_device_slave_notify_release() call. Now we get:
<info> [1664391684.9757] device (bond0): state change: ip-config -> failed (reason 'config-failed', sys-iface-state: 'managed')
...
<debug> [1664391684.9759] active-connection[69c2b12d61f5b171]: set state deactivated (was activating)
<debug> [1664391684.9760] active-connection[142bb8240f6a696d]: check-master-ready: already signalled (state activating, master 0x56116f1480a0 is in state deactivated)
...
<debug> [1664391684.9762] manager: ActivatingConnection now (none)
...
<warn> [1664391684.9763] device (bond0): Activation: failed for connection 'bond0'
<trace> [1664391684.9763] device[142828814dec6e26] (bond0): master: release one slave 720791275fe8a68c/eth1 (not enslaved) (configure)
<debug> [1664391684.9763] device[720791275fe8a68c] (eth1): Activation: connection 'eth1' master failed
...
<debug> [1664391684.9764] device[720791275fe8a68c] (eth1): queue-state[deactivating, reason:dependency-failed, id:3047]: queue state change
<debug> [1664391684.9765] device[720791275fe8a68c] (eth1): unmanaged: flags set to [!sleeping,!by-type,!platform-init,!user-explicit,!user-settings,!user-conf=0x0/0x179/managed], forget [is-slave=0x800], reason removed)
...
<debug> [1664391684.9797] device[720791275fe8a68c] (eth1): queue-state[deactivating, reason:dependency-failed, id:3047]: change state
<info> [1664391684.9797] device (eth1): state change: config -> deactivating (reason 'dependency-failed', sys-iface-state: 'managed')
Commit 1fe8166fc9 ('device: only deactivate when the master we've
enslaved to goes away') added the "return", but it seems to also add it
in cases where we need to handle this. Restrict the return to cases if
we do "no-config".
https://bugzilla.redhat.com/show_bug.cgi?id=2130287
Fixes: 1fe8166fc9 ('device: only deactivate when the master we've enslaved to goes away')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1406
The G_TYPE_INSTANCE_GET_CLASS() macro is just one pointer dereference
(self)->g_class, plus additional assertions with debug builds.
As such, it is as fast as it gets. Embed the address family there, and
implement NM_SETTING_IP_CONFIG_GET_ADDR_FAMILY() that way.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1395
NMConnection is an interface, implemented by NMSimpleConnection and
NMRemoteConnection.
For the most part, an NMConnection is only the content of the profile
(the settings). The "path" of the connection refers to the D-Bus path,
and wouldn't really make sense of the NMConnection interface or the
NMSimpleConnection type.
As such, the daemon (which only uses NMConnection and
NMSimpleConnection) never sets the path. Only libnm does.
NMClient uses NMRefString extensively for the D-Bus interface and the
path is already internalized. Take advantage of that. It is very likely,
that we are able to share the path instance in libnm at which point it
makes sense to use NMRefString.
Also, during nm_simple_connection_new_clone(), we can just take another
reference instead of cloning the string.
"nm-setting-ip-config.c" is a base class for IPv4 and IPv6 settings. So
far, any tags there were ignored, which was not obvious. It can be
useful to document common properties there. Well, maybe every property
better has a IPv4/IPv6 specific text, but that should not be a technical
limitation of the tool. So also honor the base file for "ipv4" and
"ipv6" settings.
When doing that, the settings are no longer processed in the order as
they are provided on the command line. Because, one file would be parsed
twice, it wouldn't make much sense. Instead, sort the my setting name.
The advantage is that the generated XML is independent from the order
that make/meson passes to the tool.
If the information is missing, the entire attribute should not be there.
Don't set it to the empty word.
Also, don't alias the "variable" attribute to the "name". It's not clear
what the "variable" fields is supposed to mean, but if it's not
explicitly set, don't make up the information. If a user of that
information cares, the can always fallback to the "name".
Our docs can be long. It's important to be able to express paragraphs.
Honor a blank line to include a newline. For XML often whitespace is
ignored, but our tools can choose to honor the newline.
Also, don't strip the whitespace from the beginning and the end.
We keep whitespace for a certain indentation level, but additional
whitespace gets preserved. This is less important, because regular
spaces is indeed irrelevant. But when we write the annotations, we
should be in full control over spaces.
With the given input, this produces *exactly* the same XML as before.
- the parsing is now stricter (and thus the code more verbose). No
funny stuff, get the annotations correct.
- on parsing errors, we log now the affecting lines
- "nm-setting-ip-config.c" is a base class. Previously it was ignored
and for the moment we still do that. Next, we will allow to also
describe properties there.
- prepare the code to better preserve whitespace, indentation and line
wrappings. In particular, to honor a blank line to indicate a line break
and support paragraphs. This is not yet done to compare the output
to before, but will be turned on with a small patch next.
- the code will make it simple to promote the XML attributes to
nodes. Attributes aren't great, let's write XML nodes later.
We will only need to adjust the "keywords" dictionary for that,
but this change will require changes to the entire chain of tools.
Before:
$ nmcli device connect veth0; echo $?
Error: Connection activation failed: (5) IP configuration could not be reserved (no available address, timeout, etc.).
0
After
$ nmcli device connect veth0; echo $?
Error: Connection activation failed: (5) IP configuration could not be reserved (no available address, timeout, etc.).
4
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/902
For new uses of nm_uuid_generate_from_strings() we should generate version5
UUIDs and we should use unique namespace UUID arguments.
The namespace UUID was so far replaced by always passing a special prefix
as first string. It seems nicer to use a namespace instead.
Version3 UUIDs should not be used for new applications.
Hence, nm_uuid_generate_from_strings_v3() is no longer a desirable way to
generate UUIDs, so drop the wrapper.
Some snake oil, but this is a low level function and we don't know
whether the caller doesn't try to hash secret information. Just clear
the buffer after use.
