It is bad style to rely on the last unref of an object for stopping
the operation. With a ref-counted object you should never rely on
anybody else still having (or not having) a reference. Hence, you
should not rely on stopping the ND during the last unref.
Add an explicit nm_ndisc_stop() function.
Previously, if we passed ra_timeout 0 to NMNDisc, then it would
calculate the effective timeout based on the router-solicitations
and the router-solicitation-interval.
The caller may want to know the used timeout, to also run its own timers
with the same timeout. Hence, it cannot leave this automatism internal
to NMNDisc.
nm_utils_inet4_ntop() is public API of libnm. Also, it accepts a
%NULL buffer to use a static buffer. That is error prone and we
should not use such convenience behavior for our own code.
Note that when NetworkManager tries to allocate more than 256 networks,
then previously the allocation would fail. We no longer fail, but log an
error and reuse the last address (10.42.255.1/24).
It's simpler to have code that cannot fail, because it's often hard to
handle failure properly. Also, if the user would configure two shared
profiles that explicitly use the same subnet, we also wouldn't fail. Why
not? Is that not a problem as well? If it is not, there is no need to
fail in this case. If it is a problem, then it would be much more
important to handle this case otherwise -- since it's more likely to
activate two profiles that accidentally use the same subnet than
activating 257+ shared profiles.
Add a better way of tracking the shared IP addresses that are in use.
This will replace NMDevice's usage of a global hash table. For one, the
API is more formalized with reserve() and release() functions.
Also, it ties the used IP addresses to the netns, which would be more
correct (in the future when we may support more netns).
It is solely computed from the lease information (the GHashTable).
No need to pass it along as separate argument in NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED,
especially since it only applies to IPv6.
It's important to find place in code where are field (state) gets
mutated. Make sys_iface_state field const, but add a mutable alias
via a union. You can now grep for places that change the field.
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.
