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.
Have an ifcfg file loaded in NetworkManager, then move/remove the file and try
to modify it. That will fail with:
"failed to update connection: Could not read file '/etc/sysconfig/network-scripts/ifcfg-eth0': No such file or directory"
That is not right.
If the user didn't move/remove the file but merely modified it, NetworkManager
would silently overwrite it. There is no reason why move/remove should behave
differently and not just write a completely fresh file.
The reason why NetworkManager first loads the file before writing, is to
preserve comments and unrecognized shell variables. This is a certain effort
to play nice with users editing the file. It's not essential to load the file
first and a failure to do so should not result in a failure.
And of course, keyfile writer doesn't behave like this either.
This bug exists since 2009, but let's not add a "Fixes" comment for
commit 1974b257e0 ('ifcfg-rh: begin adding write support'), because
it seems not right to backport this patch to all the old releases.
Use `_nm_connection_ensure_setting()` to eliminate the
duplicated codes. This function will retrieve the specific setting from
connection, if not found, create new one and attach to the connection.
Signed-off-by: Gris Ge <fge@redhat.com>
Commit c631aa48f0 ('platform: capture NMIP[46]Config from platform
with correct (reversed) order of IP addresses') changed this for IPv6
and IPv4, but it's not correct for IPv4.
For IPv6, later `ip addr add` calls adds a new primary address, which
is also listed in `ip addr show` first. Hence, as NMIP6Config tracks
addresses in increasing priority, while NMPlatform tracks them as
exposed by kernel, the order when appending addresses form platform
to NMIP6Config must be reversed.
That is not the case for IPv4. For IPv4, later `ip addr add` calls
add a secondary IP address. Also, in `ip addr show` output they are
appended. Consequently, IPv4 addresses are tracked by NMPlatform with
decreasing priority (in the reverse order than for IPv6).
Fix constructing the NMIP4Config by fixing the address order. This is
important, because during restart devices get assumed and our code would
configure the order of addresses as it finds them.
Fixes: c631aa48f0 ('platform: capture NMIP[46]Config from platform with correct (reversed) order of IP addresses')
Do "bother" to read/write settings.
For the umpteenth time, it's not up to the reader/writer to decide
what properties are valid for a profile or which makes sense.
Only nm_connection_verify() can decide that. For example nm_connection_verify()
has no problem with ipv6.method=disabled while also setting ipv6.addr-gen-mode.
We cannot just shortcut the parsing/writing.
The reader only ignores addresses, dns and dns-searches, so that we don't start
parsing invalid files, where the setting would have been ignore
previously.
In particular,
echo "DEVICE=eth0" > /etc/sysconfig/network-scripts/ifcfg-xxx
nmcli connection load /etc/sysconfig/network-scripts/ifcfg-xxx
nmcli -f ipv6.method,ipv6.addr-gen-mode connection show /etc/sysconfig/network-scripts/ifcfg-xxx
needs to show eui64 addr-gen-mode.
It's not the task of the ifcfg reader to pre-normalize profiles
to truncate the DNS server list. It's only nm_connection_verify()'s
task to indicate what is valid and what not.
Increase the number to something excessive. Note that the parsing
scales with O(n^2). So don't have it totally unbounded and have an
overall limit (of 10000 entries).
Fix the order of IP addresses when assuming devices (service restart).
The order of IP addresses matters to kernel for selection of source IP
address.
If all other properties are equal ([1]), for IPv6, the address added *last*
will be preferred. That is the address you see *first*` in `ip -6 addr show`.
NMPlatform also preserves that order, so the address *first* is the most
important one.
On the other hand, in a connection profile, `ipv6.addresses` lists
addresses in increasing priority (the last address is the primary one).
That is for compatibility with initscripts, which iterates over the
list of addresses and calls `ip addr add` (meaning, the last address
will be added last and is thus preferred by kernel).
As the priority order in the profile is reversed, also the priority
order in NMIP[46]Config is reversed. Fix creating an NMIP[46]Config
instance from platform addresses to honor the priority.
This has real consequences. When restarting NetworkManager, the interface
stays up with the addresses configured in the right order. After
restart, the device gets assumed, which means that the NMIP[46]Config
instance from the connection is not yet set, only the config from the
platform gets synchronized. Previously the order was wrong, so during
restart the order of IP addresses was reverted.
[1] https://access.redhat.com/solutions/189153https://bugzilla.redhat.com/show_bug.cgi?id=1988751
The user of NMDhcpClient is supposed to call "nm_dhcp_client_stop()"
on the instance, also because it is a ref-counted GObject type. So we
wouldn't want to rely on the last unref to stop DHCP.
Still, in case that was not done, the code somehow made the assumption
it made any sense to possibly not stop the DHCP client. For the internal
client, there is of course nothing left after destroying the
NMDhcpClient instance, but what about dhclient? I don't think we should
ever leave dhclient running unsupervised. Even during restart of the
service, we need to stop it first, and restart it afterwards.
When we quit NetworkManager, we may want to leave the interface up and
configured. For that, we may need to take care that "nm_dhcp_client_stop()"
does not destory the IP configuration of the intrerface. But I don't
think that is a problem. What we still want to do, is kill the dhclient
instance.
NMDhcpClient is not supposed to be ever restarted. It starts when an
instance gets created, and it stops with "nm_dhcp_client_stop()". Hence,
the simple "is_stopped" flag is fine to prevent that multiple stop calls
are harmful.
NMDhcpManager was tracking DHCP clients. During start, it would check
whether an instance for the same ifindex is running, and stop it.
That seems unnecessary and wrong. Clearly, we cannot have multiple users
(like two `NMDevice`s) run DHCP on the same interface. But its up to
them to coordinate that. They also cannot configure IP addresses at the
same interface, and if they do, then there is a big problem already.
This comes from commit 1806235049 ('dhcp: convert dhcp backends to
classes'). Maybe back then there was also the idea that NetworkManager
could quit and leave dhclient running. That idea is also flawed. When
NetworkManager stops, it leaves the interface (possibly) up, so that
restart works without disruption. That does not mean that the DHCP
client needs to keep running. What works is to restart NetworkManager in
a timely manner, then NetworkManager will start a new DHCP client after
restart. What does not work is stop NetworkManager, do nothing (like
taking over the interface by running your own manager) and expect that
DHCP keeps working indefinitely. And of course, with the internal client
this cannot possibly work either. Don't stop NetworkManager for good, if
you expect NetworkManager to run DHCP on an interface.
A different things is that when NetworkManager crashes, that after
restart it kills the left over dhclient instances. That may require a
solution, for example systemd killing all processes or checking for
left-over PID files and kill the processes. But what was implemented in
NMDhcpManager was not a solution for that.
As such, it's not clear what conflicting instance we want to kill, or
why NMDhcpManager should even track NMDhcpClient instances.
NMDhcpClient communicates events via GObject signals. GObject signals in
principle could have several subscribers. In practice, a NMDhcpClient
instance has only one subscriber, because it was constructed with
certain parameters, so it's unlikely to be shared.
That one subscriber, always needs to subscribe to all signals
("state-changed" and "prefix-delegated"), Unless the subscriber only
creates a IPv4 client. In which case they won't subscribe to
"prefix-delegated", but that signal is also not invoked for IPv4
clients.
Combine the signals in one, and pass all parameters via a new
NMDhcpClientNotfiyData payload. I feel this is nicer, to pack all
parameters together. I find this more type-aware, where we can
switch (in the callback) based on a notify-type enum, instead
of subscribing multiple signal handlers.
With l3cfg work, DHCP handling will be refactored, where this model of
having one "generic" notify signal makes more sense than here. For the
moment, it is arguably pretty much the same. Also, because NMDhcpClient
subscribes two different handlers for IPv4 and IPv6. In the future,
there will be only one notify handler, and that can cover IPv4 and IPv6
and both "state-changed" and "prefix-delegated" (and other notification
types).
Now that NetworkManager on Fedora 33 and RHEL 9 no longer writes
ifcfg-rh files by default ([1]), ifup/ifdown became less useful.
Possibly users shouldn't use it and it would be fine that new-style profiles
(keyfile) no longer work with these commands. But this is deemed as too
disruptive for users.
Note that our previous ifup/ifdown compat scripts only honored the argument
to be part of the ifcfg filename. That was not what initscripts were doing,
which called `need_config()` function that searched also the contents of
the files. With this extension, ifup/ifdown gets smarter too, to better
guess what the user might have wanted.
Extend the script by making it smarter, and to work with connection profile
names.
With this extension we further solidify ifup/ifdown as part of NetworkManager
command line API. That is problematic, because these tools pollute the
$PATH, by not having a clear NM-specific name. Also, these scripts
should only exist on Fedora/RHEL, which makes their usage non-portable
to other distros.
Also, other distros already ship different tools with name ifup/ifdown.
Extending the use of these scripts is thus undesirable, as it furthers
distro-specific commands.
Still, these arguments seem to not hold and users need to be "helped".
As Fedora users cannot be expected to unlearn "ifup" today, there is no
reason to assume they could in a few years. This likely means we will
never get rid of these scripts.
Also, if we truly would make ifup/ifdown part of NetworkManager, then a better
implementation would be that nmcli honors being called with these names.
That is not done, because nmcli's implementation currently is not as
nice to make that extension trivial (as it should be). It also would
mean to embrace ifup/ifdown officially. A shell script works well enough
as a hack.
[1] https://fedoraproject.org/wiki/Changes/NetworkManager_keyfile_instead_of_ifcfg_rhhttps://bugzilla.redhat.com/show_bug.cgi?id=1954607https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/936
As we keep track of the current name owner, use its unique name
for the D-Bus requests.
We also track when the name owner changes, so at the point when we make
the D-Bus call, the current name owner was still running. We should talk
to it directly. If at the same time, firewalld restarts, we go through
our usual tracking of the name owner and will retry -- but always
talking to the unique name.
The "only_dirty" parameter to a remove-all() function is odd.
For one, the function is called remove-all, but depending on a parameter
it does not remove all.
Also, setting remove-all(only_dirty=TRUE) means it will remove not
everything, so passing TRUE will remove only parts. That logic seems
confusing.
Avoid that, by removing the parameter from nm_l3cfg_remove_config_all()
and add nm_l3cfg_remove_config_all_dirty().
add a NM_DISPATCHER_ACTION_DHCP_CHANGE_X() macro that can select the
right action based on a parameter.
Also rename the IPv4/IPv6 enum values, so that their naming scheme works
better with the NM_DISPATCHER_ACTION_DHCP_CHANGE_X() macro.
