We use sysfs API for setting bond options. Note that the miimon and
arp_interval settings conflict each other, and whenever setting one
of these sysfs values, the other one gets reset. That means,
NetworkManager needs to mediate and handle a profile which has both
these options set.
Before 1.24, the libnm API nm_setting_bond_add_option() API would mangle
the content of the bond settings, to clear the respective other fields
when setting miimon/arp_interval. That also had the effect that the
settings plugins, weren't able to read such (conflicting) settings
back from disk (but they would write them to disk). If a keyfile
specified both miimon and arp_interval keys, then it would depend on
their order in the keyfile which wins.
It is wrong that a libnm property setter mangles the option in such a way,
especially, because you still could set the NM_SETTING_BOND_OPTIONS
property directly and bypass this. So, since 1.24, you can create
profiles that have conflicting options.
Also, we can now not start to reject such settings as invalid, because that
would be an API break. Instead, just make sure that when one of the
settings is set, that the other one consistently gets deactivated.
Also, before 1.24 already, NMDeviceBond would mediate whether to either set
miimon or arp_interval settings. Despite that the keyfile reader would
mangle the settings, it would also prefer miimon over arp_interval,
if both were set.
This mechanism was broken since we switch to _bond_get_option_normalized()
for that. As a consequence, NetworkManager would try to set both the
conflicting options. Fix that.
_bond_get_option_normalized() gets called with code paths that don't
assume a valid options hash. That means, the bond mode might be invalid
and we should fail an assertion.
- the arp_ip_target option in the settings might not have normalized
IP addresses or duplicates. If there would be duplicates, setting
them twice would fail with EINVAL. Hence, first normalize them
and make them unique.
- if what we want to set is identical to what is already set, don't
do anything.
We already have meta data for all bond options. For example,
"arp_ip_target" has type NM_BOND_OPTION_TYPE_IP.
Also, verify() already calls nm_setting_bond_validate_option() to validate
the option. Doing a second validation below is redundant (and done
inconsistently).
Validate the setting only once.
Also beef up the validation and use nm_utils_bond_option_arp_ip_targets_split()
to parse the IP addresses. This now strips extra whitespace and (as
before) removes empty entries.
When dispose() is called, there can't be any pending operation because
they keep a reference to the device. Instead, there can be a a queued
operation not yet executed. Destroy it.
When a 'ip=auto6' option is passed to kernel, the old dracut network
module only sets accept_ra in kernel and wait for the address to
appear. Instead, with a 'ip=dhcp6' option it starts 'dhclient -6',
leaving accept_ra to the initial value (that is already 1). So
'ip=dhcp6' in practice does kernel IPv6 autoconf and DHCPv6 at the
same time, without honoring the 'Managed' flag of the router
advertisement.
It seems that the only reason to have distinct 'auto6' and 'dhcp6'
options was that network module did not support starting DHCPv6 only
when necessary based on the M flag of the RA; so the user had to
specify if DHCPv6 was needed or not.
Given that 1) NM is smarter and can start DHCPv6 only when needed by
RA; 2) DHCPv6 alone only gets a /128 address without a prefix route
and so it's not useful; then it makes sense to generate a connection
with 'ipv6.method=auto' for both 'ip=auto6' and 'ip=dhcp6'.
https://bugzilla.redhat.com/show_bug.cgi?id=1854323https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/571
- assert that WITH_JANSSON and JANSSON_SONAME are defined consistently.
This check ensures that we can check at compile time that nm_json_vt()
will always fail (because JANSSON_SONAME) is undefined.
That is interesting, because this means you can do a compile time
for !WITH_JANSSON, and know if nm_json_vt() will *never* succeed.
With that, we could let the linker know when the code is unused
and remove all uses of nm_json_vt(), without using the traditional
conditional compilation with "#if WITH_JANSSON". But of course, we
currently don't do this micro optimization to remove defunct code.
- drop the "mode" helper variable and pass the flags directly to
dlopen().
jansson 2.7 was released October 2014. It's also in Ubuntu 16.06.
Other distros (like CentOS 7.5 and Debian Stretch/9) have both newer
versions.
Bump the requirement, simply because our CI does not use such old version
so it's not clear whether it works at all.
Our "nm-json-aux.h" redefines various things from <jansson.h> header.
Add a unit test that checks that what we redefine exactly matches what
libjansson would provide, so that they are compatible.
They serve a similar purpose.
Previously, nm-json-aux.h contained the virtual function table for accessing
the dynamically loaded libjansson. But there is no reason why our own
helper functions from nm-json.h cannot be there too.
We anyway load libjansson with dlopen(), and already before it could
happen that libjansson is not available. In that case, we would not
crash, but simply proceed without json validation.
Since libnm-core no longer uses libjansson directly, but only via
"nm-glib-aux/nm-json.h", we can just always compile with that, and use
it at runtime. That means, libjansson is not a build dependency for
libnm anymore, so we don't need a compile time check.
Note that if you build without libjansson, then JANSSON_SONAME is
undefined, and loading it will still fail at runtime. So, even if
we now always build with all our code enabled, it only works if you
actually build with libjansson. Still, it's simpler to drop the
conditional build, as the only benefit is a (minimally) smaller
build.
nm-json.[hc] uses libjansson, but only loads it at runtime with dlopen. There
is no more run compile time dependency. Move it to shared, so that it can be
(theoretically) used by other components.
Also, drop the conditional compilation. Granted, if you don't build with
libjansson enabled, then the JANSSON_SONAME define is unset and the code
will fail to load at runtime (which is fine). However, we can still build
against our JSON wrappers. The code savings of conditional build are minimal
so drop it.
It's error prone to include the header and trying not to use it.
Don't include <jansson.h>. Instead, redefine our nm variants of
everything.
Note that we only redefine stuff that is in public headers (like
"json_t" typedef). libjansson anyway must not change the struct layout
and the like, without breaking all applications. That is because the
non-opaque code from the header anyway is part of the applications that
include it. Later we will add additional unit test that checks that our
redefinition matches to what we had at compile time.
"shared/nm-glib-aux/nm-jansson.h" is a compat header for <jansson.h>. It
includes <jansson.h> and adds some compatibility workarounds and helper
functions.
We want that "libnm-core/nm-json.h" no longer includes <jansson.h>, so
that we don't accidentally use symbols from there.
Hence, "libnm-core/nm-json.h" must no longer include "nm-jansson.h".
In preparation of that, copy the content of "shared/nm-glib-aux/nm-jansson.h"
also to "libnm-core/nm-json.h". It will be reworked later.
Some symbols in jansson.h are macros, some are regular functions,
and some are inline functions.
Regular functions must not be used directly, only via dlsym().
Macros must be used directly, but it is non-obvious which symbols
are macros. Hence, for each json_* macro add an nm_json_* alias.
Inline functions are a bit odd. If they are inlined and don't use
any non-inlined symbols from libjansson, they could be used directly.
However, it's non obvious whether both of the conditions are met.
Hence, we reimplement them in nm-json.h. The only function of this kind
is json_decref().
The point is to not use any json_* symbols directly -- except structs
and typedefs.
Seemingly, with this change we don't use any jansson symbols directly.
However, that is not true, as macros like nm_json_object_foreach()
still are implemented based on what is included from <jansson.h>.
Hence, we cannot drop patching the included jansson.h header yet and
still need our wrapper functions.
Rework the code how we access libjansson.
libnm wants to use libjansson, but it doesn't directly link to it.
The reason is that (until recently), libjansson has conflicting symbols
with libjson-c and libjson-glib. That means, if libnm would directly
link against libjansson, then if the using application happens to drag
in one of the conflicting libraries, the application would crash. Instead,
we dlopen() the library (with flags RTLD_LOCAL|RTLD_DEEPBIND).
However, as it is currently done, it doesn't fully work, as unit test
failures of libnm show on Debian sid (where libmount links against
libcryptsetup which links against libjson-c). Theoretically, our current
approach should work. At least for libnm; not for the OVS and team
plugins which use libjansson directly in NetworkManager core.
What I dislike about the current approach is that we still include
<jansson.h>, but somehow try not to use any symbols from it (via #define
we remap the json functions). The previous approach is "smaller", but also highly
confusing, and error prone, as there is a subtle bug as the unit test failure
shows (which I don't understand).
Instead, add and load a virtual function table NMJsonVt. Later, we will
go further ad drop all direct uses of <jansson.h> header.
Pre-generate routes in the local table that are configured
by kernel when an ip-address is assigned to an interface.
This helps NM taking into account routes that are not to be deleted
when a connection is reapplied (or deactivated) on an interface instead of only
ignoring (when pruning) IPv6 routes having metric 0 and routes belonging
to the local table having 'kernel' as proto.
https://bugzilla.redhat.com/show_bug.cgi?id=1821787
@routes are the list of routes we want to configure. This contains
routes from DHCP and manual routes in the profile. It also contains
externally present routes, including the metric=0 routes in the local
table.
Trying to add an IPv6 route with metric zero adds instead a route with
metric 1024.
Usually, we wouldn't do that, because that route was present externally,
so it possibly is still present (in the platform cache) during sync and
we skip the addition. However, there is a race where the external route
might just disappear and we'd add a route with metric 1024.
Avoid that.
NM will now sync all tables when a connection has specified
at least 1 local route in 'ipv[4|6].routes' to correctly
reconcile local routes when reapplying connections on a device.
If the connection has no local routes only the main table will be
taken into account preserving the previous NM's behaviour.
https://bugzilla.redhat.com/show_bug.cgi?id=1821787
Pre-generate the device multicast route in the local table that are configured
by kernel when an ipv6-address is assigned to an interface.
This helps NM taking into account routes that are not to be deleted
when a connection is reapplied on an interface.
https://bugzilla.redhat.com/show_bug.cgi?id=1821787
NetworkManager can't control the name of the PPP interface name
created by pppd; so it has to wait for the interface to appear and
then rename it. This happens in nm_device_take_over_link() called by
nm-device-ppp.c:ppp_ifindex_set() when pppd tells NM the ifindex of
the interface that was created.
However, sometimes the initial interface name is already correct, for
example when the connection.interface-name is ppp0 and this is the
first PPP interface created.
When this happens, nm_device_update_from_platform_link() is called on
the NMDevicePPP and it sets the device ifindex. Later, when pppd
notifies NM, nm_device_take_over_link() fails because the ifindex is
already set:
nm_device_take_over_link: assertion 'priv->ifindex <= 0' failed
Make nm_device_take_over_link() more robust to cope with this
situation.
https://bugzilla.redhat.com/show_bug.cgi?id=1849386
NetworkManager can't control the name of the PPP interface name
created by pppd; so it has to wait for the interface to appear and
then rename it. This happens in nm_device_take_over_link() called by
nm-device-ppp.c:ppp_ifindex_set() when pppd tells NM the ifindex of
the interface that was created.
However, sometimes the initial interface name is already correct, for
example when the connection.interface-name is ppp0 and this is the
first PPP interface created.
When this happens, nm_device_update_from_platform_link() is called on
the NMDevicePPP and it sets the device ifindex. Later, when pppd
notifies NM, nm_device_take_over_link() fails because the ifindex is
already set:
nm_device_take_over_link: assertion 'priv->ifindex <= 0' failed
Make nm_device_take_over_link() more robust to cope with this
situation.
https://bugzilla.redhat.com/show_bug.cgi?id=1849386
The output of nm_utils_format_variant_attributes() must be accepted by
nm_utils_parse_variant_attributes(), producing the initial attributes.
The latter has a special handling of some attributes, depending on the
input NMVariantAttributeSpec list. For example, if the
NMVariantAttributeSpec is a boolean with the 'no_value' flag, the
parser doesn't look for a value.
Pass the NMVariantAttributeSpec list to the format function so that it
can behave in the same way as the parse one.
Support the creation of parameterless 'prio' qdiscs. The kernel needs
a TCA_OPTIONS attribute initialized with default values. We currently
don't support modifying the qdisc parameters.
There is the team of nm_auto*, nm_clear_pointer() and nm_steal_pointer().
These goes nicely together so that an autovariable owns a resource,
to free it (clear) and to transfer ownership (steal).
We have these also in glib, but we certainly also need it if we don't
have glib because that very much goes together with nm_auto*. Add it.
