The base image for the "check-tree" test got bumped to Fedora 39. This
brings a new python-black version (23.7.0 vs. 22.8.0) and requires
reformatting.
Maybe we should stick to 22.8.0, via `pip install`. But it seems better
to just follow the latest black version (the one from current Fedora).
So do the reformatting instead.
https://black.readthedocs.io/en/stable/change_log.html#id38
This ABI was backported all the way to 1.42.8 and 1.40.20 and to rhel-8.9.
Move the ABI to a separate symbol version, which we have in all those
versions.
The current implementation of libnm guarantees that "o" and "ao"
properties are cleared when the device object goes away, i.e. when all
its interfaces disappear from the bus.
The "manager:device-removed" signal is emitted just before the device
is unexported, and usually properties are not cleared at that
time. So, the assertions about empty available connections and active
connection during "device-removed" seem wrong; remove them.
Whether the test passes or not depends on a race condition in the way
the mock NM service is stopped: we first close the pipe to the process
to force a clean shutdown (where all objects are orderly unexported)
but just after that we send SIGTERM which causes the service to drop
from the bus.
If libnm sees the service dropping from the bus, it deletes all
objects (thus clearing properties) and then emits
"device-removed"; in this case the test passes.
However in case of a clean shutdown, NM first emits the
"device-removed" signal and then unexports devices, leading to a
failure.
Fixes: aaa9a9cd25 ('libnm/client: don't reset properties when interface goes away')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1486
This is the version shipped in Fedora 37. As Fedora 37 is now out, the
core developers switch to it. Our gitlab-ci will also use that as base
image for the check-{patch.tree} tests and to generate the pages. There
is a need that everybody agrees on which clang-format version to use,
and that version should be the one of the currently used Fedora release.
Also update the used Fedora image in "contrib/scripts/nm-code-format-container.sh"
script.
The gitlab-ci still needs update in the following commit. The change
in isolation will break the "check-tree" test.
In case the D-Bus interfaces start dropping off (typically all off them go
one by one when the object is being deleted), don't reset all the properties.
In particular, keep most properties around, only tear down "o" and "ao",
so that the object dependencies get torn down, but we still get enough
properties around to identify what the dead object was its heyday.
One example of where this is not good is when the device-removed signal
is emmitted, the device no longer has the ifname:
$ nmcli monitor
<quit NetworkManager>
(null): device removed
(null): device removed
...
Currently we assert that properties are reset on client teardown. That
is not the right thing to do and we're not going to do that in future.
However, what is important to test is that the properties are reset when
the daemon goes away. Test that.
The part where a device was created and its cleanup on client
description was only run randomly.
This is silly and gave me hard time. No reason not to be always running
it.
This verifies that what's in our public headers has version nodes, and
that they match Since: tags.
Not pretty (because python) but discovered a *lot* of issues.
Add a fire-and-forget function to wait for shutdown to be complete.
It's not entirely trivial to ensure all resources of NMClient are
cleaned up. That matters only if NMClient uses a temporary GMainContext
that the user wants to release while the application continues. For
example, to do some short-lived operations an a worker thread. It's
not trivial also because glib provides no convenient API to integrate
a GMainContext in another GMainContext. We have that code as
nm_utils_g_main_context_create_integrate_source(), so add a helper
function to allow the user to do this.
The function allows to omit the callback, in which case the caller
wouldn't know when shutdown is complete. That would still be useful
however, when integrating the client's context into the caller's
context, so that the client's context gets automatically iterated
until completion.
The following test script will run out of file descriptors,
when wait_shutdown() is not used:
#!/bin/python
import gi
gi.require_version("NM", "1.0")
from gi.repository import NM, GLib
for i in range(1200):
print(f">>>{i}")
ctx = GLib.MainContext()
ctx.push_thread_default()
nmc = NM.Client.new()
ctx.pop_thread_default()
def cb(unused, result, i):
try:
NM.Client.wait_shutdown_finish(result)
except Exception:
# cannot happen
assert False
else:
print(f">>>>> {i} complete")
nmc.wait_shutdown(True, None, cb, i)
while GLib.MainContext.default().iteration(False):
pass
We expect to read NUL terminated strings. Upon NUL, we should do
something. Treat it as a line break.
Fixes: 8ae9cf4698 ('Revert "libnm: buffer reads in nm_vpn_service_plugin_read_vpn_details()"')
gcc-12.0.1-0.8.fc36 is annoying with false positives.
It's related to g_error() and its `for(;;) ;`.
For example:
../src/libnm-glib-aux/nm-shared-utils.c: In function 'nm_utils_parse_inaddr_bin_full':
../src/libnm-glib-aux/nm-shared-utils.c:1145:26: error: dangling pointer to 'error' may be used [-Werror=dangling-pointer=]
1145 | error->message);
| ^~
/usr/include/glib-2.0/glib/gmessages.h:343:32: note: in definition of macro 'g_error'
343 | __VA_ARGS__); \
| ^~~~~~~~~~~
../src/libnm-glib-aux/nm-shared-utils.c:1133:31: note: 'error' declared here
1133 | gs_free_error GError *error = NULL;
| ^~~~~
/usr/include/glib-2.0/glib/gmessages.h:341:25: error: dangling pointer to 'addrbin' may be used [-Werror=dangling-pointer=]
341 | g_log (G_LOG_DOMAIN, \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
342 | G_LOG_LEVEL_ERROR, \
| ~~~~~~~~~~~~~~~~~~~~~~~
343 | __VA_ARGS__); \
| ~~~~~~~~~~~~
../src/libnm-glib-aux/nm-shared-utils.c:1141:13: note: in expansion of macro 'g_error'
1141 | g_error("unexpected assertion failure: could parse \"%s\" as %s, but not accepted by "
| ^~~~~~~
../src/libnm-glib-aux/nm-shared-utils.c:1112:14: note: 'addrbin' declared here
1112 | NMIPAddr addrbin;
| ^~~~~~~
I think the warning could potentially be useful and prevent real bugs.
So don't disable it altogether, but go through the effort to suppress it
at the places where it currently happens.
Note that NM_PRAGMA_WARNING_DISABLE_DANGLING_POINTER macro only expands
to suppressing the warning with __GNUC__ equal to 12. The purpose is to
only suppress the warning where we know we want to. Hopefully other gcc
versions don't have this problem.
I guess, we could also write a NM_COMPILER_WARNING() check in
"m4/compiler_options.m4", to disable the warning if we detect it. But
that seems too cumbersome.
This breaks test @nmcli_monitor. With this patch, `nmcli monitor` no
longer prints "There's no primary connection". Need to investigate why.
For now, revert the patch.
This reverts commit 2afecaf908.
While (and after) NMClient gets destroyed, nm_device_get_active_connection()
gives a dangling pointer. That can lead to a crash. This probably
affects all NMLDBusPropertyO type properties.
It's not clear how to fix that best. Usually, NMClient does updates in
two phases, first it processes the D-Bus events and tracks internal
data, then it emits all GObject signals and notifications.
When an object gets removed from the NMClient cache, then the second
phase is not fully processed, because the object is already removed
from the cache. Thus, the property was not properly cleared leaving
a dangling pointer.
A simple fix is to always clear the pointer during the first phase. Note that
effectively we do the same also for NMLDBusPropertyAO (by clearing the
"pr_ao->arr"), so at least this is consistent.
Somehow it seems that we should make sure that the "second" phase gets
full processed in this case too. But it's complicated, and it's not
clear how to do that. So this solution seems fine.
https://bugzilla.redhat.com/show_bug.cgi?id=2039331https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/896
When destroying NMClient, nm_device_get_active_connection() still
return dangling pointers. Add a unit test for that bug.
Obviously, the bug currently exists, so the relevant code is commented
out.
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
This patch is introducing a "ports" property to NMDevice. In addition it
is introducing nm_device_get_ports() and deprecating
nm_device_bond_get_slaves(), nm_device_bridge_get_slaves(),
nm_device_ovs_bridge_get_slaves(), nm_device_ovs_interface_get_slaves()
and nm_device_team_get_slaves().
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Currently a NML_DBUS_META_PROPERTY_INIT_FCN() property does not have
'extra' field available. In order to be able to call
'nml_dbus_property_ao_notify()' from the callback, the 'extra' field
must be available.
The patch is also dropping 'use_notify_update_prop' field as it only
existed to differentiate the union.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
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.
Previously, once in_parent was TRUE it was never reset, thus the
remainder of the string was cleared. That was most likely not intended.
If the intent really was to clear all the remainder, then the code could
have simply truncated the string at the first '('.
g_warning() and printing to stdout/stderr are not suitable actions
for a library. If there is something important, find a way to report the
condition to the caller. If it's not important, stay quiet.
Per convention, we shall no link our static libraries with other static
libraries of our own. The purpose is that we only link static libraries
at the end of each build product (that is, in executables, shared
libraries and shared modules).
Avoid dependencies but explicitly link the static library where it is
used.
This also fixes that we linked libnm-log-core into
libnm-settings-plugin-ifcfg-rh.so, which duplicated the symbols
while it should used them from NetworkManager.
"nm-compat.h" is not intended to be used by NetworkManager itself.
Instead, it's intended to be copied into the source tree of VPN plugins,
as adapter for different libnm versions.
Move it to "src/contrib/".
This file is not actually to be used by NetworkManager itself.
Instead, every (glib based) VPN plugin will want something like this,
hence we have a copy here.
Move it to a new directory "src/contrib/".
This helper code is already used by several of our unit tests.
Compile it as a separate library.
Previously, the source code lingered unmotivated under "shared/",
which is confusing.
