The main purpose is to simplify printf debugging and manual testing. We
can now trivially patch the code so that all output from nmcli gets
(additionally) written to a file. That is useful when debugging a unit
test in "test-client.py". Thereby we can duplicate all messages via
nm_utils_print(), which is in sync with the debug messages from libnm
and which honors LIBNM_CLIENT_DEBUG_FILE.
These will replace the direct calls to g_print()/g_printerr() in nmcli.
There are two purposes.
1) the new macros embody the concept of "printing something from nmcli".
It means, we can `git grep` for those functions, and find all the
relevant places, without hitting the irrelevant ones (e.g. tests that
also use g_print()).
2) by having one place, we can trivially change it. That is useful for
printf debugging. For example, "test-client.py" runs nmcli and
captures and compares the output. With libnm we can set
LIBNM_CLIENT_DEBUG and LIBNM_CLIENT_DEBUG_FILE to print libnm debug
messages to a file. But we cannot trivially synchronize the messages
from nmcli with that output (also because they are consumed by the test
and not immediately accessible). This would be easy, if we temporarily
could patch nmc_print*() to also log to nm_utils_print(). The new macros
will allow doing that at one place.
For example, patch the "#if 0" and run:
$ LIBNM_CLIENT_DEBUG=trace \
LIBNM_CLIENT_DEBUG_FILE='xxx.%p' \
NMTST_USE_VALGRIND=1 \
LIBTOOL="/bin/sh ./libtool"
./src/tests/client/test-client.sh -- -k monitor
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')
(cherry picked from commit 6ed966258c)
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.
(cherry picked from commit 93b46c8906)
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).
(cherry picked from commit 8b14849877)
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')
(cherry picked from commit 46ccc82a81)
On the main branch, we commonly rebase our WIP branches to latest HEAD,
before merging them with "--no-ff". The effect is to have a merge commit
that acts as a parentheses around the set of patches.
When backporting such a branch, we should preserve that structure and
take the merge commit too. We should must use `git cherry-pick -x` to
record the commit IDs of the original patch.
This script helps with that.
Also hook it up in "contrib/scripts/nm-setup-git.sh" to create an alias
for it. This alias has the advantage, of fetching the latest version of
the script from "main" or "origin/main", so it also works on older
branches.
The tests failed in certain cases on gitlab-ci and were temporarily
disabled.
These issues should be fixed now and the test pass. Reenable.
(cherry picked from commit 5c324adc7c)
