nm_utils_lifetime_get() already has so many arguments.
Essentially, the function returned %TRUE if and only if the
lifetime was greater then zero.
Combine the return value and the output argument for the lifetime.
It also matches better the function name: to get the lifetime.
Add a function that allows to re-request all objects of a certain type.
Usually, the cache is supposed to keep itself in a consistent state and
this function is not useful.
It is however useful during testing and debugging to explicitly reload
an object type.
If you ever think to need this function in non-testing code, then
something else is probably wrong with the cache implementation.
Print the "tentative" flags as last. Most other flags, have more the character of
a user configured attribute, while "tentative" reflects the current state of the address.
Previously, we would log
secondary,tentative
and
tentative,mngtmpaddr,noprefixroute
Print the "tenative" flag last. This way, the flag that commonly
will flip by kernel's decision, is consistently printed last.
The @keep_link_local logic was wrong: when set to TRUE we must not
delete addresses and when set to FALSE we must delete addresses only
if they are unknown.
Also, ignore link-local addresses when comparing positions.
Fixes: 19d6d54b6f
nm_platform_ip6_address_sync() must take care not only of adding
missing addresses and removing unknown addresses, but also of the
order in which they are added. The order is important because it
determines which address is preferred by kernel.
Since we can only add addresses at the top of the list, in order to
change the position of an address we must first remove it and then
re-add it in the right position.
@kind might be NULL. There are 3 forms of the hash-update functions for
string: str(), str0(), and strarr().
- str0() is when the string might be NULL.
- str() does not allow the string to be NULL
- strarr() is like str(), except it adds a G_STATIC_ASSERT()
that the argument is a C array.
The reason why a difference between str() and str0() exists, is
because str0() hashes NULL different from a "" or any other string.
This has an overhead, because it effectively must hash another bit
of information that tells whether a string was passed or not.
The reason is, that hashing a tupple of two strings should always
yield a different hash value, even for "aa",""; "a","a"; "","aa",
where naive concatentation would yield identical hash values in all
three cases.
Fixes: e75fc8279b
It only makes sense to call delete() with NMPObjects that
we obtained from the platform cache. Otherwise, if we didn't
get it from the cache in the first place, we wouldn't know
what to delete.
Hence, the input argument is (almost) always an NMPObject
in the first place. That is different from add(), where
we might create a new specific NMPlatform* instance on the
stack. For add() it makes slightly more sense to have different
functions depending on the type. For delete(), it doesn't.
We also do this for libnm, where it causes visible changes
in behavior. But if somebody would rely on the hashing implementation
for hash tables, it would be seriously flawed.
GHashTable optimizes a NULL equality function to use direct pointer
comparison. That saves the overhead of calling g_direct_equal().
This is also documented behavior for g_hash_table_new().
While at it, also don't pass g_direct_hash() but use the default
of %NULL. The behavior is the same, but consistently don't use
g_direct_hash().
The "onlink" flag for IPv4 routes is part of the route ID.
Consider it in nm_platform_ip4_route_cmp().
Also, allow configuring the flag when adding a route.
Note that for IPv6, the onlink flag is still ignored.
Pretty much like kernel does.
and nm_utils_ip6_property_path(). The API with static buffers
looks a bit nicer. But I think they are dangerous, because
we tend to pass the buffer down several layers of the stack, and
it's not immediately clear, that we don't overwrite the static
buffer again (which we probably did not, but it's hard to verify
that there is no bug there).
Setting the MTU failes under regular conditions, for example when
setting the MTU of a master larger then the MTU of the slaves.
Logging a warning it too alarming.
We don't need this extra distinguisher. It makes no sense to ever
compare two routes with a different compare-type.
Also, the number of fields that is hashed already differs between each
compare type. If we have a good hashing algorithm, this already suffices
that the hash value looks largely different.
We often want to cascade hashing, meaning, to combine the
outcome of various hash functions in a larger hash.
Instead of having each hash function return a guint hash value,
accept a hash state argument. This saves the overhead of initializing
and completing the intermediate hash states.
It also avoids loosing entropy when we reduce the larger hash state
into the intermediate guint hash value.
By using a macro, we don't cast all the types to guint. Instead,
we use their native types directly. Hence, we don't need
nm_hash_update_uint64() nor nm_hash_update_ptr().
Also, for types smaller then guint like char, we save hashing
the all zero bytes.
siphash24() is wildly used by projects nowadays.
It's certainly slower then our djb hashing that we used before.
But quite likely it's fast enough for us, given how wildly it is
used. I think it would be hard to profile NetworkManager to show
that the performance of hash tables is the issue, be it with
djb or siphash24.
Certainly with siphash24() it's much harder to exploit the hashing
algorithm to cause worst case hash operations (provided that the
seed is kept private). Does this better resistance against a denial
of service matter for us? Probably not, but let's better be safe then
sorry.
Note that systemd's implementation uses a different seed for each hash
table (at least, after the hash table grows to a certain size).
We don't do that and use only one global seed.
The privious NM_HASH_* macros directly operated on a guint value
and were thus close to the actual implementation.
Replace them by adding a NMHashState struct and accessors to
update the hash state. This hides the implementation better
and would allow us to carry more state. For example, we could
switch to siphash24() transparently.
For now, we still do a form basically djb2 hashing, albeit with
differing start seed.
Also add nm_hash_str() and nm_str_hash():
- nm_hash_str() is our own string hashing implementation
- nm_str_hash() is our own string implementation, but with a
GHashFunc signature, suitable to pass it to g_hash_table_new().
Also, it has this name in order to remind you of g_str_hash(),
which it is replacing.
Introduce a NM_HASH_INIT() function. It makes the places
where we initialize a hash with a certain seed visually clear.
Also, move them from "shared/nm-utils/nm-shared-utils.h" to
"shared/nm-utils/nm-macros-internal.h". We might want to
have NM_HASH_INIT() non-inline (hence, define it in the
source file).
We added "ipv4.route-table-sync" and "ipv6.route-table-sync" to not change
behavior for users that configured policy routing outside of NetworkManager,
for example, via a dispatcher script. Users had to explicitly opt-in
for NetworkManager to fully manage all routing tables.
These settings were awkward. Replace them with new settings "ipv4.route-table"
and "ipv6.route-table". Note that this commit breaks API/ABI on the unstable
development branch by removing recently added API.
As before, a connection will have no route-table set by default. This
has the meaning that policy-routing is not enabled and only the main table
will be fully synced. Once the user sets a table, we recognize that and
NetworkManager manages all routing tables.
The new route-table setting has other important uses: analog to
"ipv4.route-metric", it is the default that applies to all routes.
Currently it only works for static routes, not DHCP, SLAAC,
default-route, etc. That will be implemented later.
For static routes, each route still can explicitly set a table, and
overwrite the per-connection setting in "ipv4.route-table" and
"ipv6.route-table".
gcc doesn't consider variables with cleanup attribute as unused.
clang does, and warns about them.
In one case, clang is right, in the other one the warning is bogus.
Fix both.
- use nm_utils_addr_family_to_char(). It asserts that the input argument
is either AF_INET or AF_INET6.
- rename variable @family to @addr_family for consistency.
- when logging addr_family for activation-stage, use v4 or v6 instead
of numeric AF_INET/AF_INET6.
Whenever we call a platform operation that reads or writes the netlink
socket, there is the possibility that the cache gets updated, as we
receive netlink events.
It is thus racy, if nm_platform_ip_route_sync() *first* adds routes, and
then obtains a list of routes to delete. The correct approach is to
determine which routes to delete first (and keep it in a list
@routes_prune), and pass that list down to nm_platform_ip_route_sync().
Arguably, this doesn't yet solve every race. For example, NMDevice
calls update_ext_ip_config() during ip4_config_merge_and_apply().
That is good, as it resyncs with platform. However, before calling
nm_ip4_config_commit() it calls other platform operations, like
_commit_mtu(). So, the race is still there.
Kernel does not allow to add a route with table 0 (RT_TABLE_UNSPEC). It
effectively is an alias for the main table. We must consider that when
comparing routes sementically.
No need for duplicate log lines
<debug> [1506146476.8462] platform: link: adding tap tap0 owner 107 group -1
<debug> [1506146476.8462] platform-linux: link: add tap tap0 owner 107 group -1
Merge them.
Also, for consistency change the logging output for adding generic
interfaces in nm_platform_link_add().
Before commit 6698bf58bb, we would rely on
kernel to add the device-route for manual IPv6 routes. We broke that and now
kernel would still add the device-route, however nm_platform_ip_route_sync()
would delete it immediately after.
That is because previously nm_platform_ip_route_sync() would ignore routes
with rtm_protocol RTPRO_KERNEL. Now, it will sync and delete those too.
Fix that by adding the device-route like we do it for IPv4. This also
fixes an actual issue where the automatically added route always had
route-metric 256. Instead, we now use the metric from ipv6.route-metric
setting.
Fixes: 6698bf58bb
Kernel does not allow to add IPv6 routes with "src", as long as the
corresponding address is still tentative (related bug rh#1457196).
The workaround for this is cumbersome. First, when we fail to add such a
route with "pref_src", we guess that it happend due to this issue. In
that case, nm_ip6_config_commit() returns the list of routes that could
not be added for the moment (but hopefully can be added later).
We track this list in NMDevice, and keep trying to merge the routes
back into ip6_config. In order to not try indefinitely, keep track of a
timestamp when we tried to add this route for the first time.
Another uglyness is that pending tentative routes don't explicitly block
activation. In practice they may do, because for these routes we also have
an IPv6 address that is still doing DAD, so the IP configuration is
still pending due to that.
https://bugzilla.redhat.com/show_bug.cgi?id=1452684
Let's not treat those routes special. I think this was originally done, because
we relied on kernel to add the IPv4 device route, so we would ignore RTPROT_KERNEL
routes and not delete them.
We want to track them for various reasons:
- for consistency, there is nothing special except that they might be
added by kernel.
- we expose the routes of NMIP4Config/NMIP6Config on D-Bus. That should
include also routes such as device routes. Note, this commit changes
that we now expose device routes on D-Bus too.
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.
Previously, we would first delete routes that are not to be added,
before adding the new ones.
This has the advantage, that even if delete removes the wrong route,
add would restore the expected state. This tries to workaround the fact
that RTM_DELROUTE allows for wild-card fields, and might delete the
wrong route.
However, for example when bumping the route metric after connectivty
check (removing the default-route with metric 20100 and adding the one
with metric 100), there is a short moment when there is no
default-route.
To avoid that, don't do delete-then-add, but add-then-delete.
