There are some Debian-supported architectures where `max_align_t` is
only aligned to 4-bytes. This is unfortunate and breaks our assumptions.
While glibc-malloc still guarantees 8 / 16 bytes alignment, this is not
necessarily guaranteed by the C standard (and alternative allocators
will deviate (see jemalloc, for instance)).
Fortunately, we only need 2 flags, so a 4-byte alignment is more than
enough.
Reported-by: Thomas Haller
Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>
https://github.com/c-util/c-rbtree/pull/4
The test spawns processes and tries to kill them, with timeouts and retry.
That is inherently racy, and it's hard to deterministically test the
interesting cases, without having unstable tests.
Try to adjust the timeout, to make it more stable:
14:02:27 /builds/NetworkManager/NetworkManager/tools/run-nm-test.sh --called-from-make /builds/NetworkManager/NetworkManager/build --launch-dbus=auto /builds/NetworkManager/NetworkManager/build/src/tests/test-core-with-expect
--- stdout ---
# random seed: R02S7748fae8fc946b7a755b72efb5815250
1..5
# Start of general tests
ok 1 /general/nm_utils_monotonic_timestamp_as_boottime
# NetworkManager-DEBUG: <debug> [1601992953.4091] kill child process 'test-s-1-3' (18615): sending SIGKILL...
# NetworkManager-DEBUG: <debug> [1601992953.4242] kill child process 'test-s-1-3' (18615): waiting for process to terminate after sending no signal (0) and SIGKILL...
# NetworkManager-DEBUG: <debug> [1601992953.4257] kill child process 'test-s-1-3' (18615): after sending no signal (0) and SIGKILL, process 18615 exited by signal 9 (20807 usec elapsed)
Bail out! GLib:ERROR:../src/tests/test-core-with-expect.c:154:test_nm_utils_kill_child_sync_do: Did not see expected message NetworkManager-DEBUG: *<debug> [*] kill child process 'test-s-1-3' (*): waiting up to 1 milliseconds for process to terminate normally after sending no signal (0)...
Bail out! test:ERROR:../src/tests/test-core-with-expect.c:457:test_nm_utils_kill_child: assertion failed (exit_status == 0): (6 == 0)
--- stderr ---
**
GLib:ERROR:../src/tests/test-core-with-expect.c:154:test_nm_utils_kill_child_sync_do: Did not see expected message NetworkManager-DEBUG: *<debug> [*] kill child process 'test-s-1-3' (*): waiting up to 1 milliseconds for process to terminate normally after sending no signal (0)...
**
test:ERROR:../src/tests/test-core-with-expect.c:457:test_nm_utils_kill_child: assertion failed (exit_status == 0): (6 == 0)
/builds/NetworkManager/NetworkManager/tools/run-nm-test.sh: line 279: 18325 Aborted "${NMTST_DBUS_RUN_SESSION[@]}" "${NMTST_LIBTOOL[@]}" "$NMTST_VALGRIND" --quiet --error-exitcode=$VALGRIND_ERROR --leak-check=full --gen-suppressions=all "${NMTST_SUPPRESSIONS[@]}" --num-callers=100 --log-file="$LOGFILE" "$TEST" "$@"
GDBusObjectManagerClient's interface-added and interface-removed signals
are not emitted when the new interfaces are added to a completely new
object or the removal results in the object disappearing. In other
words one interface is never reported both through interface-added and
object-added (or -removed) signals. This kind of makes sense but isn't
documented explicitly so interface-added seemed to correspond to DBus
InterfacesAdded signals which it doesn't.
We need to watch for both kinds of signals and although most things
work without us receiving the signals at all, it causes some race
conditions. For example on hotplug, devices wouldn't transition to
"disconnected" if a device was discovered by NMManager before it
appeared on IWD's dbus interface because that scenario relied on the
dbus signal.
The automatic scanning every 20 seconds while connected has been
annoying users because of the extra connection latency, drop it. The
UIs are supposed to be requesting scans whenever an AP list update is
needed (?).
Fix a crash on device unplugging caused by keeping our signal handlers
for GDBusProxies connected after a call to dispose(). Do this by
replacing most cleanup steps by a nm_device_iwd_set_dbus_object(self, NULL)
call which is more meticulous.
Despite `set -e`, the shell script does not fail if the command in the
here document fails. This can happen if binutils' "nm" fails.
NM=/bin/false "./tools/create-exports-NetworkManager.sh" --called-from-build "."
We already build a large variety of configurations in gitlab-ci,
we don't need yet another configuration to run tests on travis-ci.
Also, because the travis-ci setup is outdated and we don't look
at it. Let's focus on gitlab-ci instead.
Building against older libc/kernel headers can fail, because our glue
code for systemd has issues. Fix them by forward declaring "struct
statx" and by disabling parts of "socket-util.c".
The documentation of g_alloca()/alloca() isn't clear about what
happens when asking for zero bytes. Make it clear, by always returning
NULL.
Also, add a static assertion that @alloca_maxlen is a well-defined
positive integer.
As one of the arguments in unsigned, the calculation is performed as
unsigned integers. That can actually lead to the wrong result. Fix it by
casting to the right (signed) types.
Emitting signals is relatively expensive, because the arguments have to be packed
into a GValue. Avoid some overhad by only passing one signal argument: the notify-data
which also contains the type. Also with this we can use g_cclosure_marshal_VOID__POINTER.
Also, it's nice to have the type field part of the notify-data. Because clearly
the notify-data union is unusable without knowing the type. That means, if a user
passes the notify-data to a function, they anyway would also need to pass along
the type.
NML3Cfg tends to perform actions on an idle handler. That means, when
it configures something on platform, it tends to ignore the changes and
process them later.
That means the currently tracked NMPObject with the platform link may
not be the same as NMPlatform currently has cached.
Instead, track them both, and extend the API so that it's clear that
there is a difference. You now need to say whether you want the instance
from the platform cache (the "next") or the currently used instance. Of
course, after the idle handler runs, "next" and the current one
converge.
This is useful because we want to reason about the link state (also) by
looking a our NML3Cfg instance. Since it already is connected to
platform, it can expose the same NMPObject.
- add nm_l3cfg_platform_commit_on_idle_schedule() so that internal (and
external) code can schedule a commit on an idle handler. This already
existed, but is exposed now.
- rename nm_l3cfg_platform_commit() to simply nm_l3cfg_commit(). There
is no other form than "platform" commit, so the name was
unnecessarily long.
- also don't let nm_l3cfg_commit() return a boolean success. It's not
useful, because commits can be triggered internally (by NML3Cfg
itself) or by other users. Instead, there is the "post-commit" event,
and anybody who cares about such a failure would need to handle it
there.
Our NML3Cfg instance is the IP configuration manage of one ifindex.
Often users have an NML3Cfg instance at hand, but they still need to
react to platform signals. Instead of requiring those users to register
their own signal (which also gets notifications about uninteresting
interfaces), re-emit the signal from NML3Cfg.
We already had NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE which
does something similar, but collects multiple changes and emits them
on an idle handler.
This flag indicates that the NML3ConfigData should be ignored for most
purposes, except for doing ACD.
Note that as users can call nm_l3cfg_add_config() multiple times for
the same NML3ConfigData, a higher layer that enables ACD/IPv4LL can
then decide to actually use the configuration, while some layers
only have it hooked up to do ACD.
Now that NMPlatformIP[46]Route can contain a wildcard table/metric, we
can set the effectivey table/metric per NML3ConfigData that we merge.
Pass it to nm_l3cfg_add_config().
When we (for example) receive a DHCP lease, we track the routes that
should be configured via NMPlatformIP[46]Route instances. Thus, this
structure does not only track the routes that are configured (and
cached in NMPlatform), but it is also used to track the routes that
we want to configure.
This is also the case with the "rt_source" field, which represents the
NMIPConfigSource enum for routes that we want to configure, but
for routes in the cache it corresponds to rtm_protocol.
Note that NMDhcpClient creates NMIP4Config instances, which tracks the
routes as NMPlatformIP4Route instances. Previously, NMDhcpClient didn't
have any way to leave the table/metric undecided, but this information
isn't part of the DHCP lease tself. Instead, NMDevice knows the table/metric
to use. This has various problems:
- NMDhcpClient needs to know the table/metric, for no other purpose
than to set the value when creating the NMIP4Config instance for the
lease. We first pass the information down, only so that it can be
returned with the lease information.
- during reapply or when connectivity check changes, the effectively
used table/metric can change. Previously, we would have to
re-generate the NMIP4Config instances.
Improve that by allowing to leave the table/metric undecided. Higher
layers can decide the effective metric to use.
Kernel uses such typedefs (__le32) and systemd too (le32_t).
As we don't want to rely on systemd headers in our code
base, let's also define them.
They have of course very little effect beside making it clearer to
the reviewer that a certain variable is not supposed to be in native
endianness.
While we often use and prefer the glib typedefs (like guint32), there
are places where we want to use the fixed width integer types from C99.
In particular, next we will introduce typedefs like nm_le64_t for
integers in different endianness.
Also, here we are about "nm-std-aux", so the glib typedefs are not
available.
I feel a header like <stdint.h> is such a basic C requirement, that
is should just be available to us everywhere.
I would need these macros earlier in "nm-glib-aux/nm-shared-utils.h",
so it would be sufficient to just move them.
However, when I already move them, move them to "nm-std-aux/nm-std-aux.h"
because they don't need a glib dependency.
We use clang-format to format our code, and the exact format depends on
the clang version. Currently we use clang-11, as packaged in Fedora 33.
Add a script that runs a Fedora 33 container with podman and reformats
the current working directory.
Usage:
./contrib/scripts/nm-code-format-container.sh
Previously, both nm_setting_connection_add_permission() and the GObject
property setter would merely assert that the provided values are valid
(and otherwise don't do anything). That is bad for handling errors.
For example, we use the property setter to initialize the setting from
keyfile and GVariant (D-Bus). That means, if a user provides an invalid
permissions value, we would emit a g_critical() assertion failure, but
otherwise ignore the configuration. What we instead need to do is to
accept the value, and afterwards fail verification. That way, a proper error
message can be generated.
$ mcli connection add type ethernet autoconnect no ifname bogus con-name x connection.permissions 'bogus:'
(process:429514): libnm-CRITICAL **: 12:12:00.359: permission_new: assertion 'strchr (uname, ':') == NULL' failed
(process:429514): libnm-CRITICAL **: 12:12:00.359: nm_setting_connection_add_permission: assertion 'p != NULL' failed
Connection 'x' (2802d117-f84e-44d9-925b-bfe26fd85da1) successfully added.
$ $ nmcli -f connection.permissions connection show x
connection.permissions: --
While at it, also don't track the permissions in a GSList. Tracking one
permission in a GSList requires 3 allocations (one for the user string,
one for the Permission struct, and one for the GSList struct). Instead,
use a GArray. That is still not great, because GArray cannot be embedded
inside NMSettingConnectionPrivate, so tracking one permission also
requires 3 allocations (which is really a fault of GArray). So, GArray
is not better in the common case where there is only one permissions. But even
in the worst case (only one entry), GArray is no worse than GSList.
Also change the API of nm_setting_connection_add_permission().
Previously, the function would assert that the arguments are in
a certain form (strcmp (ptype, "user") == 0), but still document
the such behaviors like regular operation ("[returns] %FALSE if @ptype
or @pitem was invalid"). Don't assert against the function arguments.
Also, if you first set the user to "fo:o", then
nm_setting_connection_add_permission() would accept it -- only at
a later phase, the property setter would assert against such values.
Also, the function would return %FALSE both if the input value was
invalid (an error) and if the value already existed. I think the
function should not treat a duplicate entry like a badly formatted
input.
Now the function does much less asserting of the arguments, but will
return %FALSE only if the values are invalid. And it will silently ignore
duplicate entries.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/636
