If an address is removed during pruning and it had the TENTATIVE flag
before, the most likely cause of the removal is that it failed DAD. It
could also be that the user removed it at the same time we needed to
resync the platform cache, but that seems more unlikely.
This is useful for manual testing ("manual", in the sense that you can
write a script that tests the behavior of the platform cache, without
humanly reading the logfile).
Usage:
To write the content of the platform cache once:
./src/core/platform/tests/monitor -P -S './statefile'
To keep monitor running, and update the state file:
./src/core/platform/tests/monitor -S './statefile'
The variable is passed to nmtstp_run_command_check_external(), which accepts
-1 to mean choose randomly. Change the function signature to reflect that.
Handle IP Configuration requests from IWD so that, when IWD's main.conf
setting [General].NetworkConfigurationEnabled is true, we don't try to
run DHCP or static addressing in parallel with IWD's internal DHCP or
static addressing.
Since part of the IWD secret agent and the new NetConfig agent
registration code is common, the agent object's path is changed.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1337
On the surface, writing a file seams simple enough. But there are many
pitfalls:
- we should retry on EINTR.
- we should check for incomplete writes and loop.
- we possibly should check errors from close.
- we possibly should write to a temporary file and do atomic rename.
Use nm_utils_file_set_contents() to get this right.
Previously, nm_close() accepted EBADF, as long as fd was negative.
Getting EBADF for a non-negative file descriptor is a serious bug,
because in multi threaded applications there is a race to close
an unrelated file descriptor. Also, it indicates that somebody
messed up the tracking of the resource, which indicates a bug.
Getting EBADF on a negative FD is less of a problem.
But it still indicates that the caller does something they likely
don't intend to.
Assert against any EBADF.
Note that nm_clear_fd() and nm_auto_close take already care to not
close negative "file descriptors". So, if you use those, you are
not affected.
If you want a close function that doesn't care about whether fd is set,
then maybe use `nm_clear_fd(&fd)` instead.
Cleanup the handling of close().
First of all, closing an invalid (non-negative) file descriptor (EBADF) is
always a serious bug. We want to catch that. Hence, we should use nm_close()
(or nm_close_with_error()) which asserts against such bugs. Don't ever use
close() directly, to get that additional assertion.
Also, our nm_close() handles EINTR internally and correctly. Recent
POSIX defines that on EINTR the close should be retried. On Linux,
that is never correct. After close() returns, the file descriptor is
always closed (or invalid). nm_close() gets this right, and pretends
that EINTR is a success (without retrying).
The majority of our file descriptors are sockets, etc. That means,
often an error from close isn't something that we want to handle. Adjust
nm_close() to return no error and preserve the caller's errno. That is
the appropriate reaction to error (ignoring it) in most of our cases.
And error from close may mean that there was an IO error (except EINTR
and EBADF). In a few cases, we may want to handle that. For those
cases we have nm_close_with_error().
TL;DR: use almost always nm_close(). Unless you want to handle the error
code, then use nm_close_with_error(). Never use close() directly.
There is much reading on the internet about handling errors of close and
in particular EINTR. See the following links:
https://lwn.net/Articles/576478/https://askcodes.net/coding/what-to-do-if-a-posix-close-call-fails-https://www.austingroupbugs.net/view.php?id=529https://sourceware.org/bugzilla/show_bug.cgi?id=14627https://news.ycombinator.com/item?id=3363819https://peps.python.org/pep-0475/
For g_assert() and g_return*() we already do the same, in
"src/libnm-glib-aux/nm-gassert-patch.h"
Also patch __assert_fail() so that it omits the condition text and the
function name in production builds.
Note that this is a bit ugly, for two reasons:
- again, we make assumptions that __assert_fail() exists. In practice,
this is the case for glibc and musl.
- <assert.h> can be included multiple times, while also forward
declaring __assert_fail(). That means, we cannot add a macro
#define __assert_fail(...)
because that would break the forward declaration. Instead,
just `#define __assert_fail _nm_assert_fail_internal`
Of course, this only affects direct calls to assert(), which we have
few. nm_assert() is not affected, because that anyway doesn't do
anything, unless NM_MORE_ASSERTS is enabled.
1) ensure the compiler always sees the condition (even if
it's unreachable). That is important, to avoid warnings
about unused variables and to ensure the condition compiles.
Previously, if NM_MORE_ASSERTS was enabled and NDEBUG (or
G_DISABLE_ASSERT) was defined, the condition was not seen by
the compiler.
2) to achieve point 1, we evaluate the expression now always in
nm_assert() macro directly. This also has the benefit that we
control exactly what is done there, and the implementation is
guaranteed to not use any code that is not async-signal-safe
(unless the assertion fails, of course).
3) add NM_MORE_ASSERTS_EFFECTIVE.
When using no glib (libnm-std-aux), the assert is implemented
by C89 assert(), while libnm-glib-aux redirects that to g_assert().
Note that these assertions are only in effect, if both NM_MORE_ASSERTS
and NDEBUG/G_DISABLE_ASSERT say so. NM_MORE_ASSERTS_EFFECTIVE
is thus the effectively used level for nm_assert().
4) use the proper __assert_fail() and g_assertion_message_expr()
calls. __assert_fail() is not standard, but it is there for glibc
and musl. So relying on this is probably fine. Otherwise, we will
get a compilation error and notice it.
While we usually don't do that, we also want to build with NDEBUG.
But in that case, we don't want that the assertions from our unit
tests are disabled.
Solve that by undefining NDEBUG and re-including <assert.h>.
It's not needed outside the source file, and lgtm.com complains
that global variables should have a long name.
Poor global variable name 'gl'. Prefer longer, descriptive names for
globals (eg. kMyGlobalConstant, not foo).
We currently use the systemd LLDP client, which we consume by forking
systemd code. That is a maintenance burden, because it's not a
self-contained, stable library that we use. Hence there is a need for an
individual library or properly integrating the fork in our tree.
Optimally, we would create a new nettools project with an LLDP library.
That was not done because:
- nettools may want to be dual licensed with LGPL-2.1+ and Apache.
Systemd code is LGPL-2.1+ so it is fine for NetworkManager but
possibly not for nettools.
- nettools provides independent librares, as such they don't have an
event loop, instead they expose an epoll file descriptor and the user
needs to integrate it. Systemd and NetworkManager on the other hand
have their established event loop (sd_event and GMainContext,
respectively). It's simpler to implement the library on those terms,
in particular porting the systemd library from sd_event to
GMainContext.
- NetworkManager uses glib and has various helper utils. While it's
possible to do without them, it's more work.
The main reason to not write a new NetworkManager-agnostic library from
scratch, is that it's much simpler to fork the systemd library and make
it part of NetworkManager, than making it a nettools library.
Do it.
The boottime argument might come from the system, and we should not
assert that it's reasonably small. It might be infinity. In that
case, keep it at infinity.
It belongs there, beside NMEtherAddr. Maybe NMEtherAddr should be moved to a
separate header, but it here for now.
The only oddity is that nm_ether_addr_zero actually aliases nm_ip_addr_zero,
which is in "libnm-glib-aux/nm-inet-utils.h". We can workaround that.
Taken from systemd's "Prioq".
Differences from Prioq:
- It is glib-ized, so certain operations cannot fail since g_malloc()
never fails.
- Unlike Prioq, this structure is stack allocated. I think that makes
sense, because we basically always want to embed the data structure
in another object. There is never a need for passing this around as a
pointer. And if you really want, you can box it yourself.
- The queue either accepts a GCompareFunc or a GComareDataFunc. This
is for convenience. The prioq_ensure_allocated() and
prioq_ensure_put() consequently are dropped, as they would be
cumbersome with this pattern and don't seem useful.
