The devices tests' meson build files include only the build of a
single executable file and its execution as a test unit.
This has been moved to the devices' main meson build files so this
files can be removed.
In 878d4963e a new `nm-bt-test` helper program was added. However,
although `autotools` build steps were included, meson build steps
were not.
This add meson's build steps.
The test unit name string is used in different place so it has been
replaced by a variable.
The `nm-service-providers.c` source file is appended by using a
`files` generated object.
An extra variable is used for sources of
`libnm-settings-plugin-ifupdown` module. However, it only contains
one source file and using it directly avoiding the creation of the
extra variable doesn't hurt readibility.
The `ifcfg-rh` meson build file installs a new post install script
to create the `network-config` directory.
This has been moved to the main post install file so it's easier to
find because all post install steps are together and it avoids and
extra post install script execution.
The file has been fixed to be consistent with the rest of the files.
The data files to be installed have been grouped together. The
sourc files has been listed vertically and the link target in
`nm-settings-plugin-ifcfg-rh` does not use an array anymore.
The linker flags used when building `libnm_wwan` target uses an
array even when it only uses one value.
When using only one value the array is unnecessary so it has been
removed.
The set of c_flags used when building `ppp` targets has been grouped
together. Used dependencies have also been reviewed and removed
the unnecessary one.
Renamed the variable holding the compiler flags to be consistent
with different meson ports.
This naming pattern improves the use of different compiler flags
in environments with multiple languages.
The targets that involve the use of the `NetworkManager` library,
built in the `src` build file have been improved by applying a set
of changes:
- Indentation has been fixed.
- Set of objects used in targets have been grouped together.
- Aritificial dependencies used to group dependencies and custom
compiler flags have been removed and their use replaced with
proper dependencies and compiler flags to avoid any confussion.
The `libnm-core` build file has been improved by applying a set of
changes:
- Indentation has been fixed to be consistent.
- Library variable names have been changed to `lib{name}` pattern
following their filename pattern.
- `shared` prefix has been removed from all variables using it.
- Dependencies have been reviewed to store the necessary data.
- The use of the libraries and dependencies created in this file
has been reviewed through the entire source code. This has
required the addition or the removal of different libraries and
dependencies in different targets.
- Some files used directly with the `files` function have been moved
to their nearest path build file because meson stores their full
path seamessly and they can be used anywhere later.
The `nm-default.h` header is used widely in the code by many
targets. This header includes different headers and needs different
libraries depending the compilation flags.
A new set of `*nm_default_dep` dependencies have been created to
ease the inclusion of different directorires and libraries.
This allows cleaner build files and avoiding linking unnecessary
libraries so this has been applied allowing the removal of some
dependencies involving the linking of unnecessary libraries.
The `shared` build file has been improved by applying a set of
changes:
- Indentation has been fixed to be consistent.
- Unused libraries and dependencies have been removed.
- Dependencies have been reviewed to store the necessary data.
- Set of objects used in targets have been grouped together.
- Header files have been removed from sources lists as it's
unnecessary.
- Library variable names have been changed to `lib{name}` pattern
following their filename pattern.
- `shared` prefix has been removed from all variables using it.
- `version_header` its related configuration `version_conf`
variables have been renamed to `nm_version_macro*` following
its input and final file names.
Add support to the internal DHCP client for requesting a prefix and
distributing it to interfaces with 'shared' IPv6 mode.
The systemd-networkd API currently allows to request only a single
prefix and so there will be issues when the number of downstream
interfaces is greater than the number of /64 subnets available in the
returned prefix; but this is still an improvement over the previous
situation when no prefix was requested at all.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/247
Only reapply the IP configuration on link up if the IP state is CONF
or DONE. Previously we also reapplied it when the device was
disconnected (IP state NONE) and this could lead to a situation where
an incomplete config was applied; then we intersected the desired
configuration with the external - incomplete - one, causing the
removal of part of desired configuration (for example the default
route).
Fixes: d0b16b9283 ('device: unconditionally reapply IP configuration on link up')
https://bugzilla.redhat.com/show_bug.cgi?id=1754511https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/291
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
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.
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.
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.
