When merging IP addresses, we keep the best addr_source and the internally
configured timestamps. Since the check for the timestamp considers addr_source,
we must move the check before merging addr_source.
For kernel, route ID compare identical according to NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID.
Well, mostly. In practice, NM ignores several route properties that
kernel considers part of the ID too. This leaves the possibility that
kernel allows addition of two routes that compare idential for
NetworkManager.
Anyway, NMIP4Config/NMIP6Config should use the same equality as platform
cache. Otherwise, there is the odd situation that ip-config merges routes
that are treated as different by kernel.
For IP addresses the ID operator already corresponded to what kernel
does. There is no change for addresses.
Note that NMSettingIPConfig also uses a different algorithm for
comparing routes. But that doesn't really matter here, it it differed
before too.
Remove NMDefaultRouteManager. Instead, add the default-route to the
NMIP4Config/NMIP6Config instance.
This basically reverts commit e8824f6a52.
We added NMDefaultRouteManager because we used the corresponding to `ip
route replace` when configuring routes. That would replace default-routes
on other interfaces so we needed a central manager to coordinate routes.
Now, we use the corresponding of `ip route append` to configure routes,
and each interface can configure routes indepdentently.
In NMDevice, when creating the default-route, ignore @auto_method for
external devices. We shall not touch these devices.
Especially the code in NMPolicy regarding selection of the best-device
seems wrong. It probably needs further adjustments in the future.
Especially get_best_ip_config() should be replaced, because this
distinction VPN vs. devices seems wrong to me.
Thereby, remove the @ignore_never_default argument. It was added by
commit bb75026004, I don't think it's
needed anymore.
This brings another change. Now that we track default-routes in
NMIP4Config/NMIP6Config, they are also exposed on D-Bus like regular
routes. I think that makes sense, but it is a change in behavior, as
previously such routes were not exposed there.
Default-routes are for the most part like regular routes. Add support to
track them like regular routes in NMIP4Config/NMIP6Config.
One thing is, sometimes we need to figure out whether an ip-config
instance has a default-route. For that, keep track of the best
default-route (there might be multiple) and expose it. That is
the most complicated part of this patch, because there are so many
places where the list of routes gets modified (replace, intersect,
subtract, merge, add), and they all need to take care of updating
the best default-route.
In a next patch, NMDefaultRouteManager will be dropped and default-routes
will be tracked by NMIP4Config/NMIP6Config.
Avoid calling nm_dedup_multi_index_add() directly, like we do for all other places.
Instead, call the wrapper _nm_ip_config_add_obj() which does some pre-precessing.
In practice, the result is exactly the same (at the moment). But there should by
only one way to add the route.
We rely on clearing the dirty flag. For example in nm_ip4_config_replace(),
we first mark all entries dirty, then force-append the ones we keep,
and finally remove the ones that are still dirty.
Since _nm_ip_config_add_obj() short-cuts nm_dedup_multi_index_add_full(),
it must clear the dirty flag on its own.
Why would we do this? The route is there, so, add it.
This revises commit 4fba2260f3
which added this check for matching generated connections.
I don't think this is still necessary, and if it is, then
the matching should be relaxed instead. It's bad to hide
routes from NMIP4Config/NMIP6Config, because those routes are
also exported via D-Bus.
Previously, we would add exclusive routes via netlink message flags
NLM_F_CREATE | NLM_F_REPLACE for RTM_NEWROUTE. Similar to `ip route replace`.
Using that form of RTM_NEWROUTE message, we could only add a certain
route with a certain network/plen,metric triple once. That was already
hugely inconvenient, because
- when configuring routes, multiple (managed) interfaces may get
conflicting routes (multihoming). Only one of the routes can be actually
configured using `ip route replace`, so we need to track routes that are
currently shadowed.
- when configuring routes, we might replace externally configured
routes on unmanaged interfaces. We should not interfere with such
routes.
That was worked around by having NMRouteManager (and NMDefaultRouteManager).
NMRouteManager would keep a list of the routes which NetworkManager would like
to configure, even if momentarily being unable to do so due to conflicting routes.
This worked mostly well but was complicated. It involved bumping metrics to
avoid conflicts for device routes, as we might require them for gateway routes.
Drop that now. Instead, use the corresponding of `ip route append` to configure
routes. This allows NetworkManager to confiure (almost) all routes that we care.
Especially, it can configure all routes on a managed interface, without
replacing/interfering with routes on other interfaces. Hence, NMRouteManager
becomes obsolete.
It practice it is a bit more complicated because:
- when adding an IPv4 address, kernel will automatically create a device route
for the subnet. We should avoid that by using the IFA_F_NOPREFIXROUTE flag for
IPv4 addresses (still to-do). But as kernel may not support that flag for IPv4
addresses yet (and we don't require such a kernel yet), we still need functionality
similar to nm_route_manager_ip4_route_register_device_route_purge_list().
This functionality is now handled via nm_platform_ip4_dev_route_blacklist_set().
- trying to configure an IPv6 route with a source address will be rejected
by kernel as long as the address is tentative (see related bug rh#1457196).
Preferably, NMDevice would keep the list of routes which should be configured,
while kernel would have the list of what actually is configured. There is a
feed-back loop where both affect each other (for example, when externally deleting
a route, NMDevice must forget about it too). Previously, NMRouteManager would have
the task of remembering all routes which we currently want to configure, but cannot
due to conflicting routes.
We get rid of that, because now we configure non-exclusive routes. We however still
will need to remember IPv6 routes with a source address, that currently cannot be
configured yet. Hence, we will need to keep track of routes that
currently cannot be configured, but later may be.
That is still not done yet, as NMRouteManager didn't handle this
correctly either.
_nm_ip_config_add_obj() does some additional checking, like setting the ifindex.
We shall not bypass this also during bulk-update (replace).
Add options @merge and @append_force to make _nm_ip_config_add_obj() suitable
in those cases too, and use it.
For completeness of the API. remove_obj() is basically a shortcut
of nm_dedup_multi_index_lookup_obj() combined with
nm_dedup_multi_index_remove_entry(). As such, it is useful to return
the actually deleted object. Note that the lookup needle @obj is not
necessarily the same instance as the one that will be removed, it's
only an instance that compares equal according to the index's equality
operator.
nmp_lookup_init_route_visible() was originally named this way, to only return routes
that are nmp_object_is_visible(). However, all routes are visible (as long as they are
nmp_object_is_alive()). Hence, this is a historic misnomer.
Also, passing @only_default FALSE is identical to the
nmp_lookup_init_addrroute() lookup.
So, rename the function to indicate it is a lookup for default routes
only. Also, get rid of the unsupported ifindex argument for which there
is no index.
There are various notions of how to compare routes. Collect them all
in nm_platform_ip4_route_cmp(), nm_platform_ip4_route_hash(),
nm_platform_ip6_route_cmp(), and nm_platform_ip6_route_hash().
This way, we have them side-by-side, which makes the differences more
discoverable.
Routes are complicated.
`ip route add` and `ip route append` behaves differently with respect to
determine whether an existing route is idential or not.
Extend the cmp() and hash() functions to have a compare type, that
covers the different semantics.
Kernel requires that the host part of a route (based on network/plen)
is zero. Routes with non-zero host part don't really exist.
In settings (NMIPRoute), we don't enforce that. Hence we must ensure
that we don't let such invalid routes into NMIP4Config/NMIP6Config.
Also at other places where we obtain routes from untrusted sources,
we must sanitize them first.
Also add an assertion to catch such bugs.
Reasons:
- it adds an O(1) lookup index for accessing NMIPxConfig's addresses.
Hence, operations like merge/intersect have now runtime O(n) instead
of O(n^2).
Arguably, we expect low numbers of addresses in general. For low
numbers, the O(n^2) doesn't matter and quite likely in those cases
the previous implementation was just fine -- maybe even faster.
But the simple case works fine either way. It's important to scale
well in the exceptional case.
- the tracked objects can be shared between the various NMPI4Config,
NMIP6Config instances with NMPlatform and everybody else.
- the NMPObject can be treated generically, meaning it enables code to
handle both IPv4 and IPv6, or addresses and routes. See for example
_nm_ip_config_add_obj().
- I want core to evolve to somewhere where we don't keep copies of
NMPlatformIP4Address, et al. instances. Instead they shall all be
shared. I hope this will reduce memory consumption (although tracking a
reference consumes some memory too). Also, it shortcuts nmp_object_equal()
when comparing the same object. Calling nmp_object_equal() on the
identical objects would be a common case after the hash function
pre-evaluates equality.
Maintaining an index is expensive.Not so much in term of runtime, but
in term of memory.
Drop some indexes, and require the caller to use a more broad index (and
filter out unwanted elements).
Dropped:
- can no longer lookup visible default-routes by ifindex.
If you care about default-routes, lookup all and search for the
desired ifindex. The overall number of default-routes is expected
to be small.
We drop NMP_CACHE_ID_TYPE_ROUTES_VISIBLE_BY_IFINDEX_WITH_DEFAULT
entirely.
- no longer have a separate index for non-default routes. We
expect that the most routes are non-default routes. So, don't
have an index without default-routes, instead let the caller
just lookup all routes, and reject default-routes themself.
We keep NMP_CACHE_ID_TYPE_ROUTES_VISIBLE_BY_DEFAULT, but it
now no longer tracks non-default routes.
This drops 1 out of 6 route indexes, and modifes another one, so
that we expect that there are almost no entires tracked by it.
Implement the reference counting of NMPObject as part of
NMDedupMultiObj and get rid of NMDedupMultiBox.
With this change, the NMPObject is aware in which NMDedupMultiIndex
instance it is tracked.
- this saves an additional GSlice allocation for the NMDedupMultiBox.
- it is immediately known, whether an NMPObject is tracked by a
certain NMDedupMultiIndex or not. This saves an additional hash
lookup.
- previously, when all idx-types cease to reference an NMDedupMultiObj
instance, it was removed. Now, a tracked objects stays in the
NMDedupMultiIndex until it's last reference is deleted. This possibly
extends the lifetime of the object and we may reuse it better.
- it is no longer possible to add one object to more then one
NMDedupMultiIndex instance. As we anyway want to have only one
instance to deduplicate the objects, this is fine.
- the ref-counting implementation is now part of NMDedupMultiObj.
Previously, NMDedupMultiIndex could also track objects that were
not ref-counted. Hoever, the object anyway *must* implement the
NMDedupMultiObj API, so this flexibility is unneeded and was not
used.
- a downside is, that NMPObject grows by one pointer size, even if
it isn't tracked in the NMDedupMultiIndex. But we really want to
put all objects into the index for sharing and deduplication. So
this downside should be acceptable. Still, code like
nmp_object_stackinit*() needs to handle a larger object.
Eventually, every NMPlatformIP4Route, NMPlatformIP6Route,
NMPlatformIP4Address and NMPlatformIP6Address should be shared
an deduplicated via the global NMDedupMultiIndex instance.
As first proof of concept, refactor NMIP4Config to track
IPv4 routes via the shared multi_idx. There is later potential
for improvement, when we pass (deduplicated) NMPObject instances
around instead of plain NMPlatformIP4Route, which needs still
a lot of comparing and cloning.
NMIP4Config, NMIP6Config, and NMPlatform shall share one
NMDedupMultiIndex instance.
For that, pass an NMDedupMultiIndex instance to NMPlatform and NMNetns.
NMNetns than passes it on to NMDevice, NMDhcpClient, NMIP4Config and NMIP6Config.
So currently NMNetns is the access point to the shared NMDedupMultiIndex
instance, and it gets it from it's NMPlatform instance.
The NMDedupMultiIndex instance is really a singleton, we don't want
multiple instances of it. However, for testing, instead of adding a
singleton instance, pass the instance explicitly around.
When IPv4 addresses are synchronized to platform, the order of IPv4
addresses matters because the first address is considered the primary
one. Thus, nm_ip4_config_capture() should put the primary address as
first, otherwise during synchronization addresses will be removed and
added back with a different primary/secondary role.
https://bugzilla.redhat.com/show_bug.cgi?id=1459813
This also ensures that we own a reference to the
NMPlatform, NMRouteManager and NMDefaultRouteManager
instances. See bug rh#1440089 where we might access
the singleton getter after destroing the singleton
instance of NMRouteManager. This is prevented by
keeping a reference to those instances -- indirectly
via the netns instance.
Later, we may add support for multiple namespaces. Then it might
make sense to swap the NMNetns instance of a device when moving
the device between namespaces.
Also, drop the use of singelton instances.
https://bugzilla.redhat.com/show_bug.cgi?id=1440089
(cherry picked from commit c48a19b7c6)
It is wrong that nm_ip4_config_set_mtu() tries to ~merge~ the new MTU
with the existing. All callers of nm_ip4_config_set_mtu() want that the
new value prevails.
That is also already the case because the DHCP clients and PPP manager set
the MTU on a newly created NMIP4Config instance, thus their value is taken.
Similarly, the final merge with NM_IP_CONFIG_SOURCE_USER also prevails as the
source has the highest priority.
The setter should just set. The only place where we want the merge behavior
is in nm_ip4_config_merge(), where it is now implemented in-place.
For example, nm_ip4_config_replace() very much wants that the new value
wins, regardless of the previous setting. Using nm_ip4_config_set_mtu()
with the merge behavior was wrong because it means that the MTU of NMDevice's
composite can never be raised again (for example with a new DHCP event).
bool:1 bitfields allow for tighter packing and are guaranteed to be
strictly 0 or 1 (contrary to gboolean's typedef for int). Not that it
matters too much, but it's favorable.
Especially, because each device has several of these ip-config instances,
we might save a few bytes for no(?) downsides.
