The memory layout of the NMPlatformIPAddress structure changed. The unit test
needs to be adjusted.
Fixes: 9ec9a92f17 ('platform: avoid bitfield at end of __NMPlatformIPAddress_COMMON macro')
This helper class is supposed to encapsulate most logic about
configuring IPv6 link local addresses and exposes a simpler API in order
to simplify NMDevice. Currently this logic is spread out in NMDevice.
Also, NML3IPv6LL directly uses NML3Cfg, thereby freeing NMDevice to care
about that too much.
For several reasons, NML3IPv6LL works different than NML3IPv4LL.
For one, with IPv6 we need to configure the address in kernel, which does
DAD for us. So, NML3IPv6LL will tell NML3Cfg to configure those
addresses that it wants to probe. For IPv4, it only tells NML3Cfg to do
ACD, without configuring anything yet. That is left to the caller.
NML3Cfg tracks state about all addresses/routes. It needs that (at
least) for the following reaons:
1) if a address/route gets added by NetworkManager and then gets
externally removed then it is presumed that the user did this. In this
case, we remember that ("externally-removed") to not re-add the
address/route, until we do a full reapply. This was previously
tracked as "externally_removed_objs_hash".
2) when NML3Cfg configures a address/route in kernel, and later the
address/route is no longer to be configured, then NML3Cfg needs to
delete it again. It thus needs to remember which addresses/routes
it configured earlier to remove them. This was previously tracked via
"last_addresses_x" and "last_routes_x".
3) kernel rejects configuring certain routes while a related IPv6
address is still tentative. That means, NML3Cfg needs to detect that,
remember it, and retry later. That is previously tracked as
"routes_temporary_not_available_hash".
4) during NM_L3_CFG_COMMIT_TYPE_ASSUME, we don't remove extraneous
and don't add missing addresses/routes. This commit mode is done
while assuming a device, that is, gracefully taking over after
a restart. However, sometimes while assuming a device we forcefully
want to configure an address/route. That happens for example if we
do IPv6 link local addressing. Then we really want to add that
address/route, even in assume mode. That is what the
NM_L3CFG_CONFIG_FLAGS_ASSUME_CONFIG_ONCE flag does, and to implement
that we need to track whether we already tried to add the
address/route previously. This is something new.
Consolidate these various states in a new "obj_state_hash" and
"ObjStateData" structure. This solves above points the following way:
1) to track externally removed objects, we have a flag in ObjStateData
that indicates whether the object was every configured and whether
it currently is configured. Based on that we make decisions to
configure (or not) an address. See "_obj_states_sync_filter()".
2) we now mark objects that NML3Cfg configured, which are still in platform
and which are no longer to be configured as "zombies".
3) this is now tracked via ObjStateData's "os_temporary_not_available_lst".
4) with the available ObjStateData we can make appropriate decisions
in "_obj_states_sync_filter()".
It's a bit tricky how this flag works. It's needed for IPv6
link local addresses, which commits changes in %NM_L3_CFG_COMMIT_TYPE_ASSUME
mode. See the code comments how it works.
This commit only adds the flags and let's the NMPlatformIP{Address,Route}
properly track it. What is still needed is to actually implement any
meaning to that during the sync.
This flag is only relevant for IPv4. That is, because the way we do
ACD/DAD is fundamentally different between IPv4 and IPv6. For IPv4, we
use libn-acd while IPv6 we configure the address in kernel and wait for
the tentative flag to go away.
The host-id gets read from /var/lib/NetworkManager/secret_key, and cached in
a global variable. Other parts of the code can get the host ID using a
singleton function.
For testing, we need to inject a different host-id. Add two push/pop
functions for that.
Unlike nm_utils_host_id_get(), these functions are not thread-safe (nor
is it possible to make them thread-safe in a reasonable manner).
The name prefix "nmtst_*" is reserved for test helpers and stub
function. Such functions should not be in the actual build artifacts,
like the NetworkManager binary.
Instead, nmtst_connection_assert_unchanging() is not a test helper. It
is a assertion function that is only enabled with NM_MORE_ASSERTS
builds. That's different.
Rename.
In other words,
$ nm src/core/NetworkManager src/libnm-client-impl/.libs/libnm.so | grep nmtst
should give no results.
The check whether the current setting are already as expected are wrong.
The reason is that nm_platform_ethtool_set_link_settings() also sets
the announced ethernet modes, but nm_platform_ethtool_get_link_settings()
does not give them.
That means, we cannot check whether the current link configuration is
the same, because the getter doesn't give that information.
Consequently, we must not skip the setting on the assumption that
there is nothing to change.
This bug has bad effects. If the device is currently activated with ethtool
option set, then re-activating the profile will result in wrongly
skipping the update.
There is no point in logging the current speed/duplex. OK, with
the "*", we could at least see whether the printed values are
to be set, or are currently configured on the interface.
But mixing these two outputs is confusing and meaningless.
Either log what we are about to do, or what the current configuration
is. Not a mix of both.
It's great to have functions that cannot fail, because it allows to
skip any error handling.
_set_stable_privacy() as it was could not fail, so the only reason why
nm_utils_ipv6_addr_set_stable_privacy() could fail is because the DAD
counter exhausted.
Also, it will be useful to have a function that does not do the counter
check, where the caller wants to handle that differently.
Rename some functions, and make the core nm_utils_ipv6_addr_set_stable_privacy()
not failable.
While NMUtilsIPv6IfaceId is only 8 bytes large, it seems unidiomatic to
pass the plain struct around.
With a "const NMUtilsIPv6IfaceId *" argument it is more clear what the
meaning of this is.
Change to use pointers.
The preference for IPv6 routes was added in kernel v4.1,
22 June 2015. It is even in latest RHEL7 kernels.
Drop trying to be compatible with such old kernels.
We use extended IFA_FLAGS for IFA_F_MANAGETEMPADDR (IPv6) and
IFA_F_NOPREFIXROUTE (IPv4 and IPv6).
These flags for IPv4 were added to kernel 3.14, 30 March, 2014.
The flag for IPv4 was added to kernel 4.4, 11 January 2016.
Even latest RHEL-7 kernels have backport for IFA_F_NOPREFIXROUTE
for IPv4 (rh#1221311).
Drop this. The backward compatibility code paths are likely broken
anyway, and add considerable complexity.
This is supported since kernel 3.17, dated 5 October, 2014. Drop the backward
compatibility for that.
It's very hard to sensibly support a mode where we set the interface up,
but prevent kernel from enabling IPv6. We would hack around that by disabling
IPv6 altogether.
But these code paths are not tested and likely make no sense. And it's hard
to implement a sensible behavior in this case anyway.
The term "user_ipv6ll" is confusing and not something somebody familiar
with kernel or `ip -d link` would understand.
Also, it maps a boolean to addr-gen-mode "none" or "eui64", although
there are 2 more address generation modes in kernel.
Don't abstract the underlying API, and name things as they are in
kernel.
NMDevice's act_stage3_ip_config_start() has an out parameter,
so that an NMIPConfig object can be returned. That is (luckily)
not used much, and it's fundamentally flawed. We want that
the start method becomes simpler and idempotent. That argument
is problematic there.
Instead, of the result is already ready, postpone the activation
and process the return on an idle handler.
Why not use nm_device_set_dev2_ip_config() to pass the configuration?
Good question, who knows? For now, just mimic the previous behavior.
Usually the IP configuration would be announced late, so we can just
do that artificially by scheduling an idle action.
The idea, that callers to nm_modem_stage3_ip4_config_start() pass
a NMDeviceClass, which then might invoke a virtual function on
the class is really bad.
nm_modem_stage3_ip4_config_start() should indicate what the caller
should do.
The main problem is, that act_stage3_ip_config_start() is not supposed
to be called by anybody except NMDevice. It's an internal hook, not for
NMModem's concern.
This approach does not seem to work with clang 3.4 (rhel-7). Instead,
make sure we actually use the symbol in NetworkManager so that it gets
preserved for the OVS device plugin.
This reverts commit 684f2acffe.
The two nm-sudo helper functions are only used by the OVS device plugin,
but they are part of NetworkManager core binary.
This is done commonly, where the NetworkManager binary has symbols that
are used by the plugins. But in this case, NetworkManager itself doesn't
use the symbols. That will case the linker to drop them.
A previous solution for that was commit 684f2acffe ('build: add way to
keep unused symbols when linking NetworkManager'), but that doesn't seem
to work with clang 3.4 (rhel-7).
Instead, actually use the symbol so that it cannot be dropped.
The formatting produced by clang-format depends on the version of the
tool. The version that we use is the one of the current Fedora release.
Fedora 34 recently updated clang (and clang-tools-extra) from version
12.0.0 to 12.0.1. This brings some changes.
Update the formatting.
This happens if there are duplicate BSSIDs for a profile in
"/var/lib/NetworkManager/seen-bssid" file.
#0 c_list_unlink_stale (what=0x555555bc8768) at ./src/c-list/src/c-list.h:160
#1 _seen_bssid_entry_free (data=0x555555bc8750) at src/core/settings/nm-settings-connection.c:98
#2 0x00007ffff77e834a in g_hash_table_insert_node
(hash_table=hash_table@entry=0x555555afa9e0 = {...}, node_index=node_index@entry=6, key_hash=key_hash@entry=967604099, new_key=new_key@entry=0x555555bc8750, new_value=new_value@entry=0x555555bc8750, keep_new_key=keep_new_key@entry=0, reusing_key=0) at ../glib/ghash.c:1352
#3 0x00007ffff77e88f0 in g_hash_table_insert_internal (keep_new_key=0, value=0x555555bc8750, key=0x555555bc8750, hash_table=0x555555afa9e0 = {...}) at ../glib/ghash.c:1600
#4 g_hash_table_insert (hash_table=0x555555afa9e0 = {...}, key=key@entry=0x555555bc8750, value=value@entry=0x555555bc8750) at ../glib/ghash.c:1629
#5 0x000055555586c5e1 in _nm_settings_connection_register_kf_dbs (self=self@entry=0x555555bbf5a0, kf_db_timestamps=<optimized out>, kf_db_seen_bssids=<optimized out>)
at src/core/settings/nm-settings-connection.c:2382
#6 0x00005555555b7e19 in _connection_changed_update
(self=self@entry=0x555555b1d0c0, sett_conn_entry=sett_conn_entry@entry=0x555555b60390, connection=0x555555b953f0, sett_flags=sett_flags@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, sett_mask=sett_mask@entry=_NM_SETTINGS_CONNECTION_INT_FLAGS_PERSISTENT_MASK, update_reason=update_reason@entry=(NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_AGENT_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET)) at src/core/settings/nm-settings.c:1080
#7 0x00005555555b8b5a in _connection_changed_process_one
(self=self@entry=0x555555b1d0c0, sett_conn_entry=0x555555b60390, allow_add_to_no_auto_default=allow_add_to_no_auto_default@entry=0, sett_flags=sett_flags@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, sett_mask=_NM_SETTINGS_CONNECTION_INT_FLAGS_PERSISTENT_MASK,
sett_mask@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, override_sett_flags=override_sett_flags@entry=1, update_reason=(NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_AGENT_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET)) at src/core/settings/nm-settings.c:1304
#8 0x00005555555b8c5e in _connection_changed_process_all_dirty
(self=self@entry=0x555555b1d0c0, allow_add_to_no_auto_default=allow_add_to_no_auto_default@entry=0, sett_flags=sett_flags@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, sett_mask=sett_mask@entry=NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, override_sett_flags=override_sett_flags@entry=1, update_reason=update_reason@entry=(NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_AGENT_SECRETS | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET)) at src/core/settings/nm-settings.c:1325
#9 0x00005555555b8d40 in _plugin_connections_reload (self=self@entry=0x555555b1d0c0) at src/core/settings/nm-settings.c:1448
#10 0x00005555555bddb5 in nm_settings_start (self=0x555555b1d0c0, error=error@entry=0x7fffffffe278) at src/core/settings/nm-settings.c:3892
#11 0x000055555560013d in nm_manager_start (self=self@entry=0x555555b19060, error=error@entry=0x7fffffffe278) at src/core/nm-manager.c:6961
#12 0x0000555555594b27 in main (argc=<optimized out>, argv=<optimized out>) at src/core/main.c:496
Fixes: 8278719840 ('settings: limit number of seen-bssids and preserve order')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/787
"test-ifcfg-rh.c" is huge, with lots of repeated, verbose code.
Refactor the code by using some helper macros so that the line noise
is smaller and we can easier see what is happening.
- use nmtst_connection_assert_setting() instead of
nm_connection_get_setting*(), followed by an assertion.
- use _nm_connection_new_setting() instead of multiple lines to
create and add the setting.
- drop all explicit unref/free and use cleanup macro.
- unify some variable names.
- drop some useless comments. In particular, comments were used as
visual separators because the code is verbose and hard to read. The
solution to verbose and hard to read is not more code/comments, the
solution is clearer, conciser code.
These type-specific getters are not very useful. _nm_connection_get_setting() is
better because the setting type is a parameter so they can be used more generically.
Have less code and use generic helpers.
- the writer/reader should be lossless. There is a difference
on whether a NMConnection has/hasn't a NMSettingBondPort instance.
If we thus have a NMSettingBondPort, we must always encode that
in the ifcfg file, by writing BOND_PORT_QUEUE_ID=0. Otherwise,
the reader will not create the setting.
- it's really not the task of the writer to validate what it writes.
All these write_bridge_port_setting() really should not fail. They
should serialize the setting as good as they can. And if they cannot,
it's probably a bug in the writer (by not being lossless).
write_bond_port_setting() did not ever fail. It should not ever fail.
So don't let the function return a potential failure, and don't
handle a failure that should never happen.
- use svGetValue() instead of svGetValueStr(). The difference is that
svGetValueStr() coerces "" to NULL. "" is not a valid value, but we
want to parse the value and print an warning message about it. Also,
the presence of the variable determines whether we add the bond-port
setting or not.
- don't use nm_clear_g_free(). @value_to_free is gs_free, it will be
cleared automatically.
- use g_object_set() instead of nm_g_object_set_property_uint(). The
latter is our own implementation that does error checking (e.g., that
the value is in range (0..2^16-1). But we already ensured that to
be the case. So just call g_object_set(), it cannot fail and if it
would, we want the assertion failure that it would cause.
- queue_id should be a "guint". It is always true on Linux/glib that
sizeof(guint) >= sizeof(guint32), the opposite theoretically might not
be true.
But later we use the variable in the variadic function g_object_set(),
where it should be guint.
- the errno from _nm_utils_ascii_str_to_uint64() isn't very useful for
logging. It's either ERANGE or EINVAL, and logging the numeric values
of these error codes isn't gonna help the user. We could stringify
with nm_strerror_native(errno), but that message is also not very
useful. Just say that the string is not a number.
Use sizeof(queue_id_str), so we don't rely on _MAX_QUEUE_ID_STR_LEN
being the correct size for the string.
Also, let's create an excessively large buffer. True, the previous size
should have always be enough, so in practice there is no difference.
But what if it were not? Should we try to handle an error? How? Just asserting
or report a failure? But we don't because the error cannot happen, can't
it?
Don't answer any of these questions, but by making the string buffer
larger, it's even less likely that these questions become relevant.
If for some reason nm_device_get_iface() gives a long string, then we
don't care and let kernel reject the invalid interface name.
While both functions are basically the same, the majority of the time
we use g_snprintf(). There is no strong reason to prefer one or the
other, but let's keep using one variant.
Add and call the new `nm_platform_link_get_permanent_address()` to
obtain `l_perm_address` via netlink or lookup via ethtool if kernel
does not expose the `IFLA_PERM_ADDRESS`.
And call the new `nm_platform_link_get_permanent_address()` in the unit
tests.
https://bugzilla.redhat.com/show_bug.cgi?id=1987286
Signed-off-by: Wen Liang <liangwen12year@gmail.com>
Replace the arguments "buf+length" of
`nm_platform_link_get_permanent_address()` with "NMPLinkAddress *out_addr"
Signed-off-by: Wen Liang <liangwen12year@gmail.com>
No actual caller should use the API without providing either a filename
or the directory name. I don't think this can actually happen, hence
fail and assert in that case.
