Otherwise, this file would need to be included in POTFILES.in.
This is unnecessary.
Fixes: 06cf1f5e2d ('platform/tests: extend monitor tool to dump the state of NMPlatform')
Unfortunately, for this we require SetLinkDNSEx() API from v246.
That adds extra complexity.
If the configuration contains no server name, we continue using
SetLinkDNS(). Otherwise, at first we try using SetLinkDNSEx().
We will notice if that method is unsupported, reconfigure with
SetLinkDNS(), and set a flag to not try that again.
- rename the "has_" variables to have the same name as the API that they
check.
- do an if-else-if for checking the operation when detecting support.
This just feels nicer. No strong reasons.
The DNS name can now also contain the DoT server name. It's not longer a
binary IP address only.
Extend NML3ConfigData to account for that. To track the additional
data, use the string representation. The alternative to have a separate
type that contains the parsed information would be cumbersome too.
Now, nm_setting_ip_config_add_dns() no longer asserts that
the name is a valid DNS nameserver. Instead, that is handled
by nm_connection_verify().
Also, the DNS property is going to be extended to support
specifying the SNI server name for DNS over TLS. The validation
would need to be extended.
Instead, drop the validation from nmcli. nmcli often needs to understand
what is happening. But in this case, it doesn't need to know (or
validate) the exact text. Don't duplicate the validation and let
libnm (or the daemon) reject invalid settings.
- nm_setting_ip_config_add_dns() and nm_setting_ip_config_remove_dns_by_value()
used to assert that the provided input is valid. That is not
documented and highly problematic.
Our parsing code for keyfile, ifcfg-rh and GVariant rightly just call
add. Likewise, nmcli. We cannot reasonably expect them to pre-validate
the input. Why would we anyway?
This is wrong in particular because we usually want the user to be
able to construct invalid settings. That is often necessary, because
whether a value is valid depends on other values. So in general, we
can only validate when all properties are set. We have
nm_connection_verify() for that, and asserting/validating during add
is very wrong. Note that "add" still filters out duplicates, which
may be an inconsistency, but well.
Also, the user could set any bogus value via the NM_SETTING_IP_CONFIG_DNS
property. Those should be allowed to be removed, and the same values
should be allowed to be added via the add method.
- add() does a normalization, presumably so that the values look nice.
Do the same normalization also when using the NM_SETTING_IP_CONFIG_DNS
property setter.
- previously, the setter could also set unnormalized values. As
nm_setting_ip_config_remove_dns_by_value() looked for the normalized
value, you couldn't remove such values anymore. That is fixed now,
by letting the property setter do the same normalization.
- don't allocate a GPtrArray unless we need it. No need for the extra
allocation.
- in the property setter, first set the new value before destroying the
previous GPtrArray. It might not be possible, but it's not clear to me
whether the strv argument from the GValue is always deep-copied or
whether it could contain strings to the DNS property itself.
It is almost always wrong, to split IPv4 and IPv6 behaviors at a high level.
Most of the code does something very similar. Combine the two functions.
and let them handle the difference closer to where it is.
On D-Bus, the properties "ipv[46].dns" are of type "au" and "aay",
respectively.
Btw, in particular "au" is bad, because we put there a big-endian
number. There is no D-Bus type to represent big endian numbers, so "u"
is bad because it can cause endianess problem when trying to remote
the D-Bus communication to another host (without explicitly
understanding which "u" properties need to swap for endinness).
Anyway. The plain addresses are not enough. We soon will also support
the DNS-over-TLS server name, or maybe a DoT port number. The previous
property was not extensible, so deprecate it and replace it by
"dns-data".
This one is just a list of strings. That is unlike "address-data" or
"route-data", which do a similar thing but are "a{sv}" dictionaries.
Here a string is supposed to be sufficient also for the future. Also,
because in nmcli and keyfile will will simply have a string format for
representing the extra data, not a structure (unlike for routes or
addresses).
The previous could would first check whether the new property is not
set. In almost all cases, the new property is actually set.
We can get away with fewer lookups, by checking for the expected things
first.
We have 4 legacy properties ("ipv[46].addresses", "ipv[46].routes") that
got replaced by newer variants ("ipv[46].address-data", "ipv[46].route-data").
When the client side of libnm (_nm_utils_is_manager_process) serializes
those properties, it must only serialize the newer version. That is so
that the forward/backward compatibility works as intended.
Previously, there was the NM_SETTING_PARAM_LEGACY GObject property flag.
That was fine, but not very clear.
For one, the legacy part of those properties is only about D-Bus. In
particular, they are not deprecated in libnm, keyfile, or nmcli. Thus
the name wasn't very clear.
Also, in the meantime we have more elaborate property meta data, that
goes beyond the meta data of the GObject property.
Move NM_SETTING_PARAM_LEGACY to NMSettInfoProperty.to_dbus_only_in_manager_process.
I think, this is a better name. It's also right at
```
_nm_properties_override_gobj(
properties_override,
g_object_class_find_property(G_OBJECT_CLASS(setting_class), NM_SETTING_IP_CONFIG_ROUTES),
NM_SETT_INFO_PROPERT_TYPE_DBUS(NM_G_VARIANT_TYPE("a(ayuayu)"),
.to_dbus_fcn = ip6_routes_to_dbus,
.compare_fcn = _nm_setting_ip_config_compare_fcn_routes,
.from_dbus_fcn = ip6_routes_from_dbus, ),
.to_dbus_only_in_manager_process = TRUE,
.dbus_deprecated = TRUE, );
```
that is, directly at the place where we describe how the D-Bus property behaves.
Instead of assuming any address that disappeared was because of a DAD
failure, check explicitly that either:
- the address is still present with DADFAILED flag (in case it was a
permanent address), or
- the address was removed and platform recorded that it had the
DADFAILED flag.
A DAD failure is in most cases a symptom of a network
misconfiguration; as such it must be logged in the default
configuration (info level).
While at it, fix other log messages.
Since we evaluate platform changes in a idle handler, there can be
multiple DAD failure at the same time that must generate a single
ndisc.configuration-change signal.
The function is unused at the moment.
All the callers pass either AF_INET or AF_INET6, drop support for
AF_UNSPEC; this simplifies the function for the next commit that adds
a @conflicts argument.
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.
