The return value for the delete methods checks whether the object
is actually deleted. That is questionable behavior, because if the netlink
request succeeds, there is little point in checking with the platform cache.
As it is, it is racy.
Anyway, the previous value was totally wrong.
But it also uncovers another platform bug, which currently breaks
route tests. Will be fixed next.
and refactor NMFakePlatform to also track links via NMPCache.
For one, now NMFakePlatform also tests NMPCache, increasing the
coverage of what we care about.
Also, all our NMPlatform implementations now use NMPObject and NMPCache.
That means, we can expose those as part of the public API. Which is
great, because callers can keep a reference to the NMPObject object
and make use of generic functions like nmp_object_to_string().
Routes and addresses don't implement cmd_obj_is_visible(),
hence they are always visible, and NMP_CACHE_ID_TYPE_OBJECT_TYPE_VISIBLE_ONLY
is identical to NMP_CACHE_ID_TYPE_OBJECT_TYPE.
Only link objects can be alive but invisible. Still, drop the index
for looking up visible links entirely. Let callers do the filtering,
if they care.
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.
Rework platform object cache to use NMDedupMultiIndex.
Already previously, NMPCache used NMMultiIndex and had thus
O(1) for most operations. What is new is:
- Contrary to NMMultiIndex, NMDedupMultiIndex preserves the order of
the cached items. That is crucial to handle routes properly as kernel
will replace the first matching route based on network/plen/metric
properties. See related bug rh#1337855.
Without tracking the order of routes as they are exposed
by kernel, we cannot properly maintain the route cache.
- All NMPObject instances are now treated immutable, refcounted
and get de-duplicated via NMDedupMultiIndex. This allows
to have a global NMDedupMultiIndex that can be shared with
NMIP4Config and NMRouteManager. It also allows to share the
objects themselves.
Immutable objects are so much nicer. We can get rid of the
update pre-hook callback, which was required previously because
we would mutate the object inplace. Now, we can just update
the cache, and compare obj_old and obj_new after the fact.
- NMMultiIndex was treated as an internal of NMPCache. On the other
hand, NMDedupMultiIndex exposes NMDedupMultiHeadEntry, which is
basically an object that allows to iterate over all related
objects. That means, we can now lookup objects in the cache
and give the NMDedupMultiHeadEntry instance to the caller,
which then can iterate the list on it's own -- without need
for copying anything.
Currently, at various places we still create copies of lookup
results. That can be improved later.
The ability to share NMPObject instances should enable us to
significantly improve performance and scale with large number
of routes.
Of course there is a memory overhead of having an index for each list
entry. Each NMPObject may also require an NMDedupMultiEntry,
NMDedupMultiHeadEntry, and NMDedupMultiBox item, which are tracked
in a GHashTable. Optimally, one NMDedupMultiHeadEntry is the head
for multiple objects, and NMDedupMultiBox is able to deduplicate several
NMPObjects, so that there is a net saving.
Also, each object type has several indexes of type NMPCacheIdType.
So, worst case an NMPlatformIP4Route in the platform cache is tracked
by 8 NMPCacheIdType indexes, for each we require a NMDedupMultiEntry,
plus the shared NMDedupMultiHeadEntry. The NMDedupMultiBox instance
is shared between the 8 indexes (and possibly other).
Platform has it's own, simple implementation of object types:
NMPObject. Extract a base type and move it to "shared/nm-utils/nm-obj.h"
so it can be reused.
The base type is trival, but it allows us to implement other objects
which are compatible with NMPObjects. Currently there is no API for generic
NMObjBaseInst type, so compatible in this case only means, that they
can be used in the same context (see example below).
The only thing that you can do with a NMObjBaseInst is check it's
NMObjBaseClass.
Incidentally, NMObjBaseInst is also made compatible to GTypeInstance.
It means, an NMObjBaseInst is not necessarily a valid GTypeInstance (like NMPObject
is not), but it could be implemented as such.
For example, you could do:
if (NMP_CLASS_IS_VALID ((NMPClass *) obj->klass)) {
/* is an NMPObject */
} else if (G_TYPE_CHECK_INSTANCE_TYPE (obj, NM_TYPE_SOMETHING)) {
/* it a NMSometing GType */
} else {
/* something else? */
}
The reason why NMPObject is not implemented as proper GTypeInstance is
because it would require us to register a GType (like
g_type_register_fundamental). However, then the NMPClass struct can
no longer be const and immutable memory. But we could.
NMObjBaseInst may or may not be a GTypeInstance. In a sense, it's
a base type of GTypeInstance and all our objects should be based
on it (optionally, they we may make them valid GTypes too).
- no need to call nm_platform_process_events() after
nmtstp_wait_for_signal(). The latter processes all events
that are pending.
- with addr_n number of addresses, we still only want to wait
a maximum time, not for each addresss individually. Basically,
the for-loop must be inside NMTST_WAIT(), not the other way around.
Having an unsigned "guint timeout_ms" argument is very inconvenient, because
nmtstp_wait_for_signal (NM_PLATFORM_GET (), end_time - now_time);
might easily be negative. In such a case, the correct behavior
is to wait not at all.
The previous behavior, of treating timeout_ms as *no timeout*, makes
no sense. At least not for unit tests. If you have a really long timeout,
then set it. "0" should really mean to schedule a zero timeout.
when adding a route with RTA_PREFSRC some kernel versions will reject
the request if the specified source address is still tentative: be sure
that the just added addresses are no more tentative before adding the
routes.
nmtstp_env1_add_test_func() allows to register test functions in a
particular test environment ("env1", for lack of a better name).
It will be reused for "test-route.c"
Commits 39d0559d9a ("platform: sort links by name instead of
ifindex") and 529a0a1a7f ("manager: sort slaves to be autoconnected
by device name") changed the order of activation of slaves. Introduce
a system-wide configuration property to preserve the old behavior.
https://bugzilla.redhat.com/show_bug.cgi?id=1452585
NMPlatform, NMRouteManager and NMDefaultRouteManager are singletons
instances. Users of those are for example NMDevice, which registers
to GObject signals of both NMPlatform and NMRouteManager.
Hence, as NMDevice:dispose() disconnects the signal handlers, it must
ensure that those singleton instances live longer then the NMDevice
instance. That is usually accomplished by having users of singleton
instances own a reference to those instances.
For NMDevice that effectively means that it shall own a reference to
several singletons.
NMPlatform, NMRouteManager, and NMDefaultRouteManager are all
per-namespace. In general it doesn't make sense to have more then
one instances of these per name space. Nnote that currently we don't
support multiple namespaces yet. If we will ever support multiple
namespaces, then a NMDevice would have a reference to all of these
manager instances. Hence, introduce a new class NMNetns which bundles
them together.
(cherry picked from commit 0af2f5c28b)
Wait that the address becomes non-tentative before adding the route
with RTA_PREFSRC. Dummy interfaces are NOARP and thus don't do DAD,
but the kernel sets the address as tentative for a small amount of
time, which prevents the immediate addition of the route with
preferred source.
Fixes: 36d9e252d2
We call nmp_utils_ethtool_get_driver_info() twice when receiving a
netlink message, but we don't need a clone of the string values.
Instead, expose a data structure that should be stack allocated
by the caller.
The ioctl APIs ethtool/mii require an interface ifname. That is inherrently
racy as interfaces can be renamed. This cannot be fixed, we can only
minimize the time between verifying the ifname and calling ioctl.
We already had problems with that when ethtool would access an interface
by name that didn't exists. See commit ab41c13b06 .
Checking for an existing interface only helps avoiding races when an interface
gets deleted. It does not help against renaming.
Go one step further, and instead of checking whether such an ifname
exists, try to get the ifname based on the ifindex immediately before
we need it.
This brings an additional overhead for each ethtool access.
Keep the include paths clean and separate. We use directories to group source
files together. That makes sense (I guess), but then we should use this
grouping also when including files. Thus require to #include files with their
path relative to "src/".
Also, we build various artifacts from the "src/" tree. Instead of having
individual CFLAGS for each artifact in Makefile.am, the CFLAGS should be
unified. Previously, the CFLAGS for each artifact differ and are inconsistent
in which paths they add to the search path. Fix the inconsistency by just
don't add the paths at all.
Avoid failure
NMPlatformSignalAssert: test-link.c:146, software_add(): failure to accept signal one time: link-changed-changed ifindex 15 (2 times received)
