While nmi_cmdline_reader_parse() only has one caller, which indeed has the
argv parameter at hand and doesn't care it to be modified, I think it
is ugly.
Arguments preferably are strictly either input or output arguments,
with input arguments not being modified by the call.
- nm_setting_wired_add_s390_option() asserts that a "value" argument
is given. Check that the string contains a '=' where we can split.
- pass the requested NM_SETTING_WIRED_SETTING_NAME type to get_conn().
Otherwise, @s_wired might be %NULL, resulting in an assertion.
I do wonder whether this always retrieves a connection of the
appropriate type for modification, or whether a profile could
be returned that was created for a different purpose. But that
isn't changed.
- avoid "g_strcmp0 (nettype, "ctc") != 0". I find it unexpected, that we add the
3rd subchannel component, if the nettype is "ctc" (intuitively, I'd expect it
to be the opposite). The reasons for this are not documented, but I
presume it is correct.
Anyway, using streq() makes this slightly more clear to me, as with
strcmp() I would wonder whether this was just a typo while with
streq() I'd be more confident that this is indeed intended.
- don't initialize local variables unnecessarily. The compiler would
warn if we would forget about this. Also, don'\''t use { } for a
one-line block.
syntax: rd.znet=<nettype>,<subchannels>,<options>
The s390 specific options used to create the network interface in the kernel
are currently not processed by nm-initrd-generator causing incomplete ifcfg file.
fixes https://bugzilla.redhat.com/show_bug.cgi?id=1753975
The client tests compare the test output with a .expected file that is
commit to git and that contains the expected output.
The expected output contains data like
size: 395
location: clients/tests/test-client.py:842:test_001()/1
cmd: $NMCLI
lang: C
returncode: 0
stdout: 277 bytes
>>>
...
Note that there is the line number (clients/tests/test-client.py:842) of
the source code where nmcli is called. This is to help correlate the output
with the test code.
However, Python 3.8 changes behavior and for function calls that span multiple
lines, frame.f_lineno will give now the starting line (previously, it gave the last
line) (see [1]).
No longer include the line number, as it is not stable accross Python versions.
If you really care, you can set NM_TEST_WITH_LINENO to get the line numbers back.
Of course, then the expected output won't match anymore, and you'd have to regenerate
it first. This is only useful if you debug tests, and want to have it easier to
correlate output with the tests while developing them.
[1] https://bugs.python.org/issue38283https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/292
It's unclear how to workaround this issue, so that the tests
work with older python versions and 3.8-beta.
Let's wait whether this will really be released as 3.8 and
for now just skip the test.
Note that the only DNS plugin that actually emits the FAILED signal was
NMDnsDnsmasq. Let's not handle restart, retry and rate-limiting by
NMDnsManager but by NMDnsDnsmasq itself.
There are three goals here:
(1) we want that when dnsmasq (infrequently) crashes, that we always keep
retrying. A random crash should be automatically resolved and
eventually dnsmasq should be working again.
Note that we anyway cannot fully detect whether something is wrong.
OK, we detect crashes, but if dnsmasq just gets catatonic, it's just
as broken. Point being: our ability to detect non-working dnsmasq is limited.
(2) when dnsmasq keeps crashing all the time, then rate limit the retry.
Of course, at this point there is already something seriously wrong,
but we shouldn't kill the system by respawning the process without rate
limiting.
(3) previously, when NMDnsManager noticed that the pluging was broken
(and rate-limiting kicked in), it would temporarily disable the plugin.
Basically, that meant to write the real name servers to /etc/resolv.conf
directly, instead of setting localhost. This partly conflicts with
(1), because we want to retry and recover automatically. So what good
is it to notice a problem, resort to plain /etc/resolv.conf for a
short time, and then run into the issues again? If something is really
broken, there is no way but to involve the user to investigate and
fix the issue. Hence, we don't need to concern NMDnsManager with this either.
The only thing that the manager notices is when the dnsmasq binary is not
available. In that case, update() fails right away, and the manager falls back
to configure the name servers in /etc/resolv.conf directly.
Also, change the backoff time from 5 minutes to 1 minute (twice the
burst interval). There is not particularly strong reason for either
choice, I think that if the ratelimit kicks in, then something is
already so wrong that it doesn't matter either way. Anyway, also 60
seconds is long enough to not kill the machine otherwise.
Several points.
- We spawn the dnsmasq process directly. That has several downsides:
- The lifetime of the process is tied to NetworkManager's. When
stopping NetworkManager, we usually also stop dnsmasq. Or we keep
the process running, but later the process is no longer a child process
of NetworkManager and we can only kill it using the pidfile.
- We don't do special sandboxing of the dnsmasq process.
- Note that we want to ensure that only one dnsmasq process is running
at any time. We should track that in a singletone. Note that NMDnsDnsmasq
is not a singleton. While there is only one instance active at any time,
the DNS plugin can be swapped (e.g. during SIGHUP). Hence, don't track the
process per-NMDnsDnsmasq instance, but in a global variable "gl_pid".
- Usually, when NetworkManager quits, it also stops the dnsmasq process.
Previously, we would always try to terminate the process based on the
pidfile. That is wrong. Most of the time, NetworkManager spawned the
process itself, as a child process. Hence, the PID is known and NetworkManager
will get a signal when dnsmasq exits. The only moment when NetworkManager should
use the pidfile, is the first time when checking to kill the previous instance.
That is: only once at the beginning, to kill instances that were
intentionally or unintentionally (crash) left running earlier.
This is now done by _gl_pid_kill_external().
- Previously, before starting a new dnsmasq instance we would kill a
possibly already running one, and block while waiting for the process to
disappear. We should never block. Especially, since we afterwards start
the process also in non-blocking way, there is no reason to kill the
existing process in a blocking way. For the most part, starting dnsmasq
is already asynchronous and so should be the killing of the dnsmasq
process.
- Drop GDBusProxy and only use GDBusConnection. It fully suffices.
- When we kill a dnsmasq instance, we actually don't have to wait at
all. That can happen fully in background. The only pecularity is that
when we restart a new instance before the previous instance is killed,
then we must wait for the previous process to terminate first. Also, if
we are about to exit while killing the dnsmasq instance, we must register
nm_shutdown_wait_obj_*() to wait until the process is fully gone.
We only have two real DNS plugins: "dnsmasq" and "systemd-resolved" (the "unbound"
plugin is very incomplete and should eventually be dropped).
Of these two, only "dnsmasq" spawns a child process. A lot of the logic
for that is in the parent class NMDnsPlugin, with the purpose for that
logic to be reusable.
However:
- We are unlikely to add more DNS plugins. Especially because
systemd-resolved seems the way forward.
- If we happen to add more plugins, then probably NetworkManager
should not spawn the process itself. That causes problems with
restarting the service. Rather, we should let the service manager
handle the lifetime of such "child" processes. Aside separating
the lifetime of the DNS plugin process from NetworkManager's,
this also would allow to sandbox NetworkManager and the DNS plugin
differently. Currently, NetworkManager itself may might need
capabilities only to pass them on to the DNS plugin, or (more likely)
NetworkManager would want to drop additional capabilities for the
DNS plugin (which we would rather not implement ourself, since that
seems job of the service management already).
- The current implementation is far from beautiful. For example,
it does synchronous (blocking) killing of the running process
from the PID file, and it uses PID fils. This is not something
we would want to reuse for other plugins. Also, note that
dnsmasq already spawns the service asynchronosly (of course).
Hence, we should also kill it asynchronously, but that is complicated
by having the logic separated in two different classes while
providing an abstract API between the two.
Move the code to NMDnsDnsmasq. This is the only place that cares about
this. Also, that makes it actually clearer what is happening, by seeing
the lifetime handling of the child proceess all in one place.
For logging, if the plugin fails with update, it should return a reason
that we can log.
Note that both dnsmasq and system-resolved plugins do the update asynchronously
(of course). Hence, usually they never fail right away, and there isn't really
possibility to handle the failure later. Still, we should print something sensible
for that we need information what went wrong.
The plugin name and whether a plugin is caching only depends on the type,
it does not require a virtual function where types would decided depending
on other reasons.
Convert the virtual functions into fields of the class.
The previous to wait-types (NM_SHUTDOWN_WAIT_TYPE_OBJECT and
NM_SHUTDOWN_WAIT_TYPE_CANCELLABLE) both required a GObject/GCancellable,
and the shutdown was automatically unblocked when the object got
destroyed.
Add another wait type NM_SHUTDOWN_WAIT_TYPE_HANDLE, which does not take
an object to wait. Instead, shutdown is indefinitely blocked, until the
user unregisters the handle again. While other wait-types allow to
ignore the handle, this wait-type only makes sense if the user keeps
track of the handle.
Having G_PID_FORMAT macro is useful, but it's only available in
recent glib versions. Add a compat implementation and a test that
our assumptions hold.
Fedora 32 drops "python" from the path. Hence "/usr/bin/env python" won't
work anymore. Of course, who needs a way to invoke the interpreter that works
accross different distributions! WTF.
In this case, easy to work around. We run it from meson, so we have access to
the Python 3 binary. Just call python explicitly, like we do with autotools.
REQUIRED_PACKAGES has two uses:
- to setup a system for developing NetworkManager. This installs
convenience packages like "cscope".
- to install the packages required for unit testing in gitlab-ci.
For gitlab-ci we should only install the packages that we actually
need.
Previously, dnf/yum used to ignore packages that didn't exist.
In Fedora 32, dnf starts to fail the entire command:
No match for argument: python-gobject-base
Error: Unable to find a match: python-gobject-base
Since this script is supposed to work with different RHEL/Fedora
versions, it's expected that not all packages are available everywhere.
Fix that, by installing packages that we know that they might be missing
one by one (and ignore the error).
Python 3 doesn't like this:
======================================================================
ERROR: test_001 (__main__.TestNmcli)
----------------------------------------------------------------------
Traceback (most recent call last):
File "./clients/tests/test-client.py", line 785, in f
self._nm_test_post()
File "./clients/tests/test-client.py", line 767, in _nm_test_post
content_new = ''.join([r['content'] for r in results])
TypeError: sequence item 0: expected str instance, bytes found
Recent Python versions warn about this:
./clients/tests/test-client.py:569: SyntaxWarning: "is" with a literal. Did you mean "=="?
elif lang is de:
./clients/tests/test-client.py:572: SyntaxWarning: "is" with a literal. Did you mean "=="?
elif lang is pl:
And rightly so: https://bugs.python.org/issue34850
property_to_dbus() returns NULL when called with
NM_CONNECTION_SERIALIZE_WITH_SECRETS_AGENT_OWNED and the property is
not an agent-owned secrets. The function doesn't handle VPN secrets
correctly, since they are all stored as a hash in the vpn.secrets
property and the flag for each of them is a matching '*-flags' key in
the vpn.data property. VPN secrets must be handled differently; do it
in the VPN setting to_dbus_fcn() function.
Fixes: 71928a3e5c ('settings: avoid cloning the connection to maintain agent-owned secrets')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/230https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/280
Makefile.am:3462: warning: $(src_devices_bluetooth_libnm_device_plugin_bluetooth_la_OBJECTS) was already defined in condition TRUE, which includes condition WITH_MODEM_MANAGER_1 ...
Makefile.am:960: ... '$(src_devices_bluetooth_libnm_device_plugin_bluetooth_la_OBJECTS)' previously defined here
Fixes: 878d4963ed ('bluetooth/tests: add "nm-bt-test helper" program for manual testing of bluetooth code')
nettools does not expose the original lease lifetime. It's a missing
API. Instead, it only exposes the timestamp when the lease will expire.
As a workaround, we calulate the timestamp by subtracting the current
timestamp from the expiration timestamp, assuming that the lease was
received just now. However, it was not received *exactly* now, but a
few milliseconds before. Hence, the calculated timestamp is not exact
here and likely a few milliseconds less then the actual (full integer)
value.
Account for that by rounding the value to the second.
The leading underscore has the notion that this would be a private function.
It really isn't, and it would be fine for the user to call it directly.
Just like we have g_slice_free() and g_slice_free1().
"NetworkManager-wifi" package requires either wpa_supplicant or iwd.
When installing the package without explicitly installing supplicant
or iwd (and not having it installed yet), then we want to drag in
wpa_supplicant by default. That is accomplished by suggesting wpa_supplicant
package.
Otherwise, the user installing NetworkManager-wifi might get iwd,
which is only functioning if the user explicitly enables the backend
in "NetworkManager.conf".
https://bugzilla.redhat.com/show_bug.cgi?id=1743585
have_connection_for_device() really should just call nm_device_check_connection_compatible().
Note that nm_device_check_connection_compatible() of course checks the
connection type already, so this is redundant.
This check is only useful for devices that implement new_default_connection.
We can shortcut the possibly expensive checks like have_connection_for_device(),
which need to iterate all profiles.
If a profile has only "ethernet.mac-address" set, but
"connection.interface-name" not, then the previous check
iface = nm_setting_connection_get_interface_name (s_con);
if (!nm_streq0 (iface, nm_device_get_iface (device)))
continue;
would wrongly consider the profile not matching for the device.
As a result, we would wrongly create a auto-default connection.
Fix that. We already call nm_device_check_connection_compatible()
above. That is fully suitable to compare the interface name and
the MAC address. We don't need to duplicate this check (wrongly).
See also commit 77d01c9094 ('settings: ignore incompatible connections
when looking for existing ones') for how this code changed.
https://bugzilla.redhat.com/show_bug.cgi?id=1727909
