This way we can safely iterate over a %NULL instance with
nm_l3_config_data_iter_obj_for_each(). This avoids a NULL check,
which in this case seems more annoying than helpful.
This is the best default route that we commited the last time (if any).
It may not reflect what is currently configured (in NMPlatform) and it
may not reflect the latest changes since nm_l3cfg_add_config().
NML3Cfg already keeps track of the current NMPlatformLink object.
Allow accessing it directly from an NML3Cfg instance, which saves
a cache lookup from NMPlatform.
"active_slave" option is a deprecated alias for "primary". nmtui can configure
the "primary" option, so whenever it configures a profile the "active_slave"
option should be unset.
"active_slave" is by now deprecated and became an alias for "primary".
If a profile specifies both properties, only "primary" is honored, without
failing validation (to not break existing behavior).
Maybe we should introduce a normalization for such cases. But normalize
might not do the right thing, if a profile currently has "primary" set,
and the user modifies it to set "active_slave" to a different value,
normalize would not know which setting was set first and remove
"active_slave" again.
In the past, nm_setting_bond_add_option() performed some simple
normalization, but this was dropped, because (such incompatible) settings
can also be created via the GObject property. Our C accessor function
should not be less flexible than other ways of creating a profile.
In the end, whenever a user (or a tool) creates a profile, the tool must
be aware of the semantics. E.g. setting an IP route without a suitable
IP address is unlike to make sense, the tool must understand what it's
doing. The same is true for the bond options. When a tool (or user) sets
the "active_slave" property, then it must clear out the redundant
information from the "primary" setting. There is no alternative to this
problem than having tools smart enough to understand what they are
doing.
Setting "active_slave" fails unless the slave is currently present and
IFF_UP. That complicates the code, because we cannot set the property
at any time, but only under the right circumstances.
But really, "active_slave" option is something for debugging. It's not
an option which should be set by NetworkManager. The right option
instead is "primary", which will tell kernel to make the slave active,
when it is ready.
Deprecate the "active_slave" option and make it an alias for "primary".
https://bugzilla.redhat.com/show_bug.cgi?id=1856640
Code doesn't get simpler by having more functions -- if these functions
are only called once.
What actually is a problem is repeated, redundant code. Like the list of
bond options that can be reapplied. But the function didn't help to
avoid repeating the list.
Add a macro for the list of bond options we are going to set. By seeing
them side-by-side, it is hopefully simpler to see that all options are
specified correctly.
We see that:
- the *_SUBSET defines don't include the options that we are explicitly
setting, that is "mode", "active_slave" and "arp_ip_target".
- OPTIONS_REAPPLY_SUBSET contains 4 options less than OPTIONS_APPLY_SUBSET:
"ad_select", "ad_user_port_key", "lacp_rate" and "tlb_dynamic_lb".
These are the options that are marked as BOND_OPTFLAG_IFDOWN in
kernel.
I guess the idea was to only accept options that can be changed without
taking the interface !IFF_UP. "active_slave" is wrongly omitted from
that list.
Also, "active_slave" option doesn't really make sense for NetworkManager
to configure. Instead "primary" should be used. In the future, we should
re-map the properties and deprecate "active_slave" for "primary" ([1]).
Fixes: 746dc119a6 ('bond: let 'reapply()' reapply all supported options')
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1856640#c19https://bugzilla.redhat.com/show_bug.cgi?id=1876577
WireGuard's wg-quick primarily wants to avoid DNS leaks, and thus also
our import code should generate profiles that configure exclusive DNS
servers. This is done by setting "ipv[46].dns-priority" to a negative
value.
Note that if a profile leaves the DNS priority at zero (which in many
regard is the default), then the zero translates to 50 (for VPN
profiles) and 100 (for other profiles).
Instead of setting the DNS priority to -10, set it to -50. This gives
some more room so that the user can choose priorities that are worse
than the WireGuard's one, but still negative (exclusive). Also, since
the positive range defaults to 50 and 100, let's stretch the range a
bit.
Since this only affects import and creation of new profiles, such a
change in behavior seems acceptable.
more_asserts are our NetworkManager specific assertions, and the only
point of having them at all (beside g_assert(), assert() and g_return*()),
is that these assertions are disabled by default in production.
meson always enabled them by default. That is definitely wrong.
autotools enables more_asserts by default if we build a devel version
from master. I think that is bad too, because (again) having these assertions
disabled by default is the only point of having them. Anyway, mimic
the behavior of autotools, to at least disable them in release builds.
On Fedora rawhide (34), valgrind gives a lot of warnings like:
./src/platform/tests/test-cleanup-linux.valgrind-log:--48279-- WARNING: unhandled amd64-linux syscall: 439
./src/platform/tests/test-cleanup-linux.valgrind-log:--48279-- You may be able to write your own handler.
./src/platform/tests/test-cleanup-linux.valgrind-log:--48279-- Read the file README_MISSING_SYSCALL_OR_IOCTL.
./src/platform/tests/test-cleanup-linux.valgrind-log:--48279-- Nevertheless we consider this a bug. Please report
./src/platform/tests/test-cleanup-linux.valgrind-log:--48279-- it at http://valgrind.org/support/bug_reports.html.
Ignore them.
NMDBusObject is an abstract type which provides the glue code for exposing
a GObject on D-Bus. We almost never use that type directly, so as it was
before, we always had to use a C cast to convince the compiler that this
is right.
Being always required to cast is not very useful, nor more typesafe.
Just use a void pointer instead.
NML3Cfg manages one ifindex. In the future, we may want that multiple
NMDevice and/or NMVpnConnection instances independently contribute their
NML3ConfigData to the NML3Cfg instance.
That means, at any time somebody may want to call nm_l3cfg_platform_commit()
to apply the changes. Even ACD internally may do that, when configuration
changes (e.g. an IP address passes ACD check). We thus need to know
whether we are assuming, updating or reapplying the settings.
Add API so users can register their "commit" preference.
The current approach also tracks external configuration in an NMIP[46]Config, and
we need to special handle those. In the future, we only want to track what we actually
want to configure. So this flag won't be used with NML3Cfg/NML3ConfigData.
Currently, NMDevice does ACD. It intercepts certain NMIP4Config
instances, and tries to perform ACD on the addresses. I think this
functionality should be handled by NML3Cfg instead.
For one, NML3Cfg sees all configurations, and can perform ACD for all
(relevant) addresses. Also, it moves logic away from NMDevice and makes
the functionality available without an NMDevice. As such, it also will
allow that independent "controllers" contribute NML3ConfigData instances
and ACD will performed for all of them (as requested).
This will be our implementation for IPv4 ACD (https://tools.ietf.org/html/rfc5227)
based on nettools' n-acd library.
The code is not actually tested yes, because NMDevice did not yet switch
over to use NML3Cfg. Once that happens, surely issues with this patch
will be found that will need fixing.
