nmc_print() will be used for something else. Rename. Also,
nmc_print_table() is the better name anyway because the function does a
lot of formatting and not simple printf().
For debugging libnm, LIBNM_CLIENT_DEBUG can be very useful.
As the tests compare stdout/stderr from nmcli with expected output, just
enabling it will break the tests. However, in combination with
LIBNM_CLIENT_DEBUG_FILE this can be very useful.
Preserve and pass on the environment variables, if set.
With LIBNM_CLIENT_DEBUG we get debug logging for libnm, either to stdout
or to stderr.
"test-client.py" runs nmcli as a unit test. It thereby catches stdout
and stderr. That means, LIBNM_CLIENT_DEBUG would break the tests.
Now honor the LIBNM_CLIENT_DEBUG_FILE environment variable to specify a
file to which the debug logs get written. The pattern "%p" is replaced
by the process id.
As before, nm_utils_print(0, ...) also honors this environment variable
and uses the same logging destination.
Sort imports by name. Also avoid "from signal import SIGINT". I find
it ugly to import names in the current namespace. "SIGINT" should be
refered to by its full name, including the package/namespace.
This will allow to find some memory leaks and memory corruptions.
The bulk of the nmcli calls are still not hooked up with valgrind.
Since we call nmcli a thousand time, we could not just run valgrind with
all of them. We would have instead to enable it randomly. This is
more work.
The base class is not used, and it's not clear that it would be useful.
Sure, we could extend "test-client.py" will various non-nmcli tests. That
might make sense. And then it might make sense to have more unit test classes.
So far, we don't need that. Drop the unused base class NmTestBase.
Since glib 2.45, we are guaranteed that g_free() just calls free(), so
both can be used interchangeably. However, we still only depend on glib
2.40.
In any case, it's ugly to mix the two. Memory allocated by plain
malloc(), should be only freed with free(). The buffer in question comes
from readline, which allocates it using the system allocator.
Fixes: 995229181c ('cli: remove editor thread')
Such leaks show up in valgrind, and are simply bugs.
Also, various callers (not all of them, which is another bug!) like to
take ownership of the returned string and free it. That means, we leave
a dangling pointer in the global variable, which is very ugly and error
prone.
Also, the callers like to free the string with g_free(), which is not
appropriate for the "rl_string" memory which was allocated by readline.
It must be freed with free(). Avoid that, by cloning the string using
the glib allocator.
Fixes: 995229181c ('cli: remove editor thread')
The onlink flag is part of each next hop.
When NetworkManager configures ECMP routes, we won't support that. All
next hops of an ECMP route must share the same onlink flag. That is fine
and fixed by this commit.
What is not fine, is that we don't track the rtnh_flags flags in
NMPlatformIP4RtNextHop, and consequently our nmp_object_id_cmp() is
wrong.
Fixes: 5b5ce42682 ('nm-netns: track ECMP routes')
If the route with a next hop is already onlink, we don't need to add a
direct route to the gateway.
It also wouldn't work previously, because the onlink route to the
gateway that we would add, would have no gateway and the RTNH_F_ONLINK
set. Kernel would reject that with an error. We would have to clear the
RTNH_F_ONLINK flag, if there is no gateway.
For IPv6, kernel doesn't care. If the gateway is ::, you may or may
not set the onlink attribute. But for IPv4 routes, that gets rejected:
# ip route add 1.2.3.4/32 dev v onlink
Error: Invalid flags for nexthop - PERVASIVE and ONLINK can not be set.
Silently suppress setting the flag in that case and ignore the user
request. After all, the effect is probably the same (that is, the route
is onlink anyway).
The dns-type must be included in the hash because it contributes to
the generated composite configuration. Without this, when the type of
a configuration changes (e.g. from DEFAULT to BEST), the DNS manager
would determine that there was no change and it wouldn't call
update_dns().
https://bugzilla.redhat.com/show_bug.cgi?id=2161957
Fixes: 8995d44a0b ('core: compare the DNS configurations before updating DNS')
Kernel enforces that all nexthops must be reachable through a route.
L3Cfg is generating dependent onlink routes to solve this problem but
the IPv4 ECMP commit is happening before that.
To solve this we introduce two boolean fields "is_new" and "is_ready" to
know in which state is the L3Cfg affected. Initially, "is_new" is TRUE
and "is_ready" is FALSE. Here we schedule a commit on idle and we set
"is_new" to FALSE. When revisiting, we set "is_ready" to TRUE and then
we set the ECMP IPv4 routes.
When a reapply kicks in we reset the L3Cfg state by setting "is_new" to
TRUE.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1520
g_random_*() is based on GRand, which is not a CSPRNG. Instead, rely on
kernel to give us good random numbers, which is what nm_random_*() does.
Note that nm_random_*() calls getrandom() (or reads /dev/urandom), which
most likely is slower than GRand. It doesn't matter for our uses though.
It is cumbersome to review all uses of g_rand_*() whether their usage of
a non-cryptographically secure generator is appropriate. Instead, just
always use an appropriate function, thereby avoiding this question. Even
glib documentation refers to reading "/dev/urandom" as alternative. Which
is what nm_random_*() does. These days, it seems unnecessary to not use
the best random generator available, unless it's not fast enough or you
need a stable/seedable stream of random numbers.
In particular in nmcli, we used g_random_int_range() to generate
passwords. That is not appropriate. Sure, it's *only* for the hotspot,
but still.
Certain ip-tunnel modules automatically create network interfaces (for
example, "ip_gre" module creates "gre0" and others).
Btw, that's not the same as `modprobe bonding max_bonds=1`, where
loading the module merely automatically creates a "bond0" interface. In
case of ip tunnel modules, these generated interfaces seem essential to
how the tunnel works, for example they cannot be deleted. I don't
understand the purpose of those interfaces, but they seem not just
regular tunnel interfaces (unlike, "bond0" which is a regular bond
interface, albeit automatically created).
Btw, if at the time when loading the module, an interface with such name
already exists, it will bump the name (for example, adding a "gre1"
interfaces, and so on). That adds to the ugliness of the whole thing,
but for our unit tests, that is no problem. Our unit tests run in a
separate netns, and we don't create conflicting interfaces. That is, an
interface named "gre0" is always the special tunnel interface and we
can/do rely on that.
Note that when the kernel module gets loaded, it adds those interfaces
to all netns. Thus, even if "test-route-linux" does not do anything with
ip tunnels, such an interface can always appear in a netns, simply by
running "test-link-linux" (or any other tool that creates a tunnel) in
parallel or even in another container.
Theoretically, we could just ensure that we load all the conflicting
ip-tunnel modules (with nmtstp_ensure_module()). There there are two
problems. First, there might be other tunnel modules that interfere but
are not covered by nmtstp_ensure_module(). Second, when kernel creates
those interfaces, it does not send correct RTM_NEWLINK notifications (a
bug), so our platform cache will not be correct, and
nmtstp_assert_platform() will fail.
The only solution is to detect and ignore those interfaces. Also,
ignore all interfaces of link-type "unknown". Those might be from other
modules that we don't know about and that exhibit the same problem.
Ubuntu 18.04 comes with iproute2-4.15.0-2ubuntu1.3. The
"/etc/iproute2/rt_protos" file from that version does not yet support
the "bgp" entry. Also the "babel" entry is only from 2014. Just choose
other entries. The point is that NetworkManager would ignore those, and
that applies to "zebra" and "bird" alike.
This will make sure that the IP tunnel module is loaded. It does so by
creating (and deleting) a tunnel interface.
That is important, because those modules will create additional interfaces
that show up in `ip link` (like "gre0"), and those interfaces can interfere
with the tests.
Also add nmtstp_link_is_iptunnel_special() to detect whether an
interface is one of those special interfaces.
"debug" is implied when setting NMTST_DEBUG, but not specifying
"no-debug". This change has thus no effect, but it seems clearer to be
explicit.
The "debug" flag affects nmtst_is_debug(). Note that tests *must* not
result in different code paths based on debug, they may only
1) print more debug logging
2) do more assertion checks.
Having more assertion checks can result in different outcome of the
test, that is, that the additional assertion fails first. That is
acceptable, because failing earlier is possibly closer to the issue and
helps debugging. Also, when the additional failure is fixed and passes,
we still will fail at the assertion we are trying to debug.
In particular, an access to nmtst_get_rand*()/nmtst_rand*() must not
depend on nmtst_is_debug(), because then different randomized paths
are taken based on whether debugging is enabled.
Seems this test fails easily under gitlab-ci, if we set NMTST_SEED_RAND
to something else than "0". There is nothing particular special about
"0", except that a randomly different code paths are chosen.
A randomized test that doesn't pass on all systems with all random
paths, is broken. Disable for now. Needs to be fixed.
See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2165141
We must trust l3cfg when generating dependent onlink routes for all kind
of routes not default routes only. This was done by
"nm_platform_ip_route_sync()" so there is not change in behaviour at
all.
"nm_platform_ip_route_sync()" could be needed for other situation where
l3cfg cannot add the dependent onlink routes, so we are not removing
that logic.
This reverts commit 6b4123db1c.
We silently tolerate NetworkManager not responding at all (easily
reproduced with e.g. pkill -STOP NetworkManager):
$ LIBNM_CLIENT_DEBUG=trace nmcli c show dummy666
libnm-dbus[23540]: <debug> [3316.81989] nmclient[ddafb84b8deebe4a]: new NMClient instance
libnm-dbus[23540]: <debug> [3316.81998] nmclient[ddafb84b8deebe4a]: starting async initialization...
libnm-dbus[23540]: <debug> [3316.82461] nmclient[ddafb84b8deebe4a]: name owner changed: (null) -> ":1.2"
libnm-dbus[23540]: <debug> [3316.82464] nmclient[ddafb84b8deebe4a]: fetch all
libnm-dbus[23540]: <debug> [3341.85715] nmclient[ddafb84b8deebe4a]: GetManagedObjects() call failed: Timeout was reached
libnm-dbus[23540]: <debug> [3341.85740] nmclient[ddafb84b8deebe4a]: async init complete with success
Error: dummy666 - no such connection profile.
libnm-dbus[23540]: <debug> [3341.86723] nmclient[ddafb84b8deebe4a]: release all
libnm-dbus[23540]: <debug> [3341.86798] nmclient[ddafb84b8deebe4a]: disposed
$
As a comment in _dbus_get_managed_objects_cb() explains, this is sort of
intentional. NetworkManager might just be shutting down and the libnm
users will eventually see the objects once a new daemon starts up.
This may make some sense for long-running clients ("nmcli monitor",
various desktop environments), but not for one-shot invocations that
require the daemon running, such as those of "nmcli c ...".
Let's not consider the client running unless we actually got the manager
object. That way the error message will make more sense:
$ LIBNM_CLIENT_DEBUG=trace nmcli c show dummy666
libnm-dbus[24730]: <debug> [5360.95480] nmclient[8cb898d3c891e210]: new NMClient instance
libnm-dbus[24730]: <debug> [5360.95487] nmclient[8cb898d3c891e210]: starting async initialization...
libnm-dbus[24730]: <debug> [5360.95901] nmclient[8cb898d3c891e210]: name owner changed: (null) -> ":1.2"
libnm-dbus[24730]: <debug> [5360.95904] nmclient[8cb898d3c891e210]: fetch all
libnm-dbus[24730]: <debug> [5385.98487] nmclient[8cb898d3c891e210]: GetManagedObjects() call failed: Timeout was reached
libnm-dbus[24730]: <debug> [5385.98497] nmclient[8cb898d3c891e210]: async init complete with success
Error: NetworkManager is not running.
libnm-dbus[24730]: <debug> [5385.98571] nmclient[8cb898d3c891e210]: release all
libnm-dbus[24730]: <debug> [5385.98698] nmclient[8cb898d3c891e210]: disposed
$
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1502
Currently, the use of [global-dns] section for setting DNS options is
conditioned on presence of a nameserver in a [global-dns-domain-*] section.
Attempt to use the section for options alone results in an error:
[global-dns]
options=timeout:1
Or via D-Bus API:
# busctl set-property org.freedesktop.NetworkManager \
/org/freedesktop/NetworkManager org.freedesktop.NetworkManager \
GlobalDnsConfiguration 'a{sv}' 2 \
"options" as 1 "timeout:1" \
"domains" a{sv} 0
...
Nov 24 13:15:21 zmok.local NetworkManager[501184]: <debug> [1669292121.3904]
manager: set global DNS failed with error: Global
DNS configuration is missing the default domain
The insistence on existence of [global-dns-domain-*] would make sense if
other [global-dns-domain-...] sections were present.
However, the user might only want to set the options in resolv.conf and
still use connection-provide nameservers for the actual resolving.
Lift the limitation by allowing the [global-dns] to be used alone, while
still insist on [global-dns-domain-*] being there in presence of other
domain-specific options.
https://bugzilla.redhat.com/show_bug.cgi?id=2019306
This effectively makes [*global-dns-domain-*] sections in configuration be
ignored unless [*global-dns] is also present. This happens because
nm_config_keyfile_has_global_dns_config() mixes the group names up and
attempts to loop up [.intern.global-dns-domain-*] in user configuration and
[global-dns-domain-*] in the internal one.
Fixes: da0ded4927 ('config: drop global-dns.enable option in favor of .config.enable')
