For NMPlatform instances we had an error reporting mechanism
which stores the last error reason in a private field. Later we
would check it via nm_platform_get_error().
Remove this. It was not used much, and it is not a great way
to report errors.
One problem is that at the point where the error happens, you don't
know whether anybody cares about an error code. So, you add code to set
the error reason because somebody *might* need it (but in realitiy, almost
no caller cares).
Also, we tested this functionality which is hardly used in non-testing code.
While this was a burden to maintain in the tests, it was likely still buggy
because there were no real use-cases, beside the tests.
Then, sometimes platform functions call each other which might overwrite the
error reason. So, every function must be cautious to preserve/set
the error reason according to it's own meaning. This can involve storing
the error code, calling another function, and restoring it afterwards.
This is harder to get right compared to a "return-error-code" pattern, where
every function manages its error code independently.
It is better to return the error reason whenever due. For that we already
have our common glib patterns
(1) gboolean fcn (...);
(2) gboolean fcn (..., GError **error);
In few cases, we need more details then a #gboolean, but don't want
to bother constructing a #GError. Then we should do instead:
(3) NMPlatformError fcn (...);
Later remove nm_platform_get_error() and signal errors via return
error codes.
Also, fix nm_platform_infiniband_partition_add() and
nm_platform_vlan_add() to check the type of the existing link
and fail with WRONG_TYPE otherwise.
- rename "NONE" to "SUCCESS", what it really is.
- change the to-string result not to contain spaces
and being closer the name of the enum value.
- add new error reasons "UNSPECIFIED" and "BUG".
- remove the code comments around the enum definition.
They add no further description about why this error
happens and only paraphrase the name of the enum.
- reserve negative integers for 'errno'. This is neat
because if we get a system error we can pass on the
underlying errno as cause.
The @udi field is not a static string, so any user of a NMPlatformLink
instance must make sure not to use the field beyond the lifetime of the
NMPlatformLink instance.
As we pass on the platform link instance during platform changed events,
this is hard to ensure for the subscriber of the signal -- because a
call back into platform could invalidate/modify the object.
Just not expose this field as part of the link instance. The few callers
who actually needed it should instead call nm_platform_get_uid(). With
that, the lifetime of the returned 'const char *' pointer is clearly
defined.
Use the event socket to request object via NLM_F_DUMP.
No longer use 'priv->nlh' socket to fetch objects.
Instead fetch them via the priv->nlh_event socket that also
provides asynchronous events when objects change.
That way, the events are in sync with our explicit requests
and we can directly use the events. Previously, the events were
only used to indicate that a refetch must happen, so that every
event triggered a complete dump of all addresses/routes.
We still use 'priv->nlh' to make synchronous requests such as
adding/changing/deleting objects. That means, after we send a
request, we must make sure that the result manifested itself
at 'nlh_event' socket and the platform cache.
That's why we sometimes still must force a dump to sync changes.
That could be improved by using only one netlink socket so that
we would wait for the ACK of our request.
While not yet perfect, this already significantly reduces the number of
fetches. Additionally, before, whenever requesting a dump of addresses
or routes (which we did much more often, search for "get_kernel_object for type"
log lines), we always dumped IPv4 and IPv6 together. Now only request
the addr-family in question.
https://bugzilla.gnome.org/show_bug.cgi?id=747985https://bugzilla.redhat.com/show_bug.cgi?id=1211133
Switch platform caching implementation. Instead of caching libnl
objects, cache our own types.
Don't remove yet the now obsolete functions.
Advantage:
* Performance
- as we now cache our native NMPlatformObject instances, we no longer
have to convert libnl objects every time we access the platform
cache.
- for most cases, access is now O(1) because we can lookup the object
in a hash table. Note that ip4_address_get_all() still has to
create a copy of the result (O(n)), but as the caller is about to
use those elements, he cannot do better then O(n) anyway.
* We cache our own native types and have full control over them. We
cannot extend the libnl objects, which has many short-commings:
- _rtnl_addr_hack_lifetimes_rel_to_abs() to convert the timestamps
to absolute values (and back).
- hack_empty_master_iff_lower_up() would modify the internal flag,
but it looses the original value. That means, we can only hack
the state before putting a link into the cache, but we cannot revert
that change, when a slave in the cache changes state.
That was previously solved by always refetching the master when
a slave changed. Now we can re-evaluate the connected state
(DELAYED_ACTION_TYPE_MASTER_CONNECTED).
- we implement functions like equality, to-string as most suitable
for us. Before we needed hacks like nm_nl_object_diff(),
nm_nl_cache_search(), route_search_cache().
- we can extend our objects with exactly those properties we care,
and possibly additional properties that are not representable in
the libnl objects.
- we no longer cache RTM_F_CLONED routes and they get rejected early
on as we receive them.
- In the future, maybe it'd be interesting the make platform objects
immutable (and ref-counted) and expose them directly.
* Previous implementation did not order the refresh of objects but
called check_cache_items(). Now, those actions are delayed and
combined in an attempt to reduce the overall number of reloads.
Realize how expensive a check_cache_items() for addresses and routes
was: it would iterate all addresses/routes and call refresh_object().
The latter obtains a full dump of *all* objects again, and ignores
all but the needle.
Note that we probably still schedule some delayed actions that
are not needed.
Later we can optimize that further (related bug bgo #747985).
While some of these points could also have been implemented with
caching of libnl objects, that would have become hard to maintain.
https://bugzilla.gnome.org/show_bug.cgi?id=747981
NMPObject is a simple "object" implemenation around NMPlatformObject.
They are ref-counted and have a class-pointer. Several basic functions
like equality, hash, to-string are implemented.
NMPCache is can be used to store the NMPObject. Objects are indexed
via their primary id, but there is also multi-lookup via NMCacheId
and NMMultiIndex.
Part of the implementation is inside "nm-linux-platform.c",
because it depends on utility functions from there.
Cache the scope as part of the NMPlatformIP4Route and
no longer read it from libnl object when needed. Later
there will be no more libnl objects around, and we need
to scope when deleting an IPv4 route.
warning: function declaration isn’t a prototype [-Wstrict-prototypes]
In C function() and function(void) are two different prototypes (as opposed to
C++).
function() accepts an arbitrary number of arguments
function(void) accepts zero arguments
There is no general purpose file for platform utilities.
We only have nm-platform.h, which contains (mostly) functions
that operate on a NMPlatform instance (and that can be mocked
using NMFakePlatform).
Add a new file for independent utility functions. nm-platform-utils.c
should not call into functions having a NMPlatform instance, to
have them independent from platform caching and the platform
singleton.
Support accepting more then one signal at a time.
It is to be expected, that one change in platform raises
several signals. Extend the assertion helpers to express
that.
==21573== Syscall param mount(type) points to unaddressable byte(s)
==21573== at 0x854B9BA: mount (syscall-template.S:81)
==21573== by 0x158922: main (test-common.c:295)
==21573== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==21573==
{
<insert_a_suppression_name_here>
Memcheck:Param
mount(type)
fun:mount
fun:main
}
Fixes: d6aef9c188
Mount a private sysfs instance. Otherwise gudev sees the devices from the
parent netns as opposed to our netns.
We do, however need a writable /sys/devices subtree for testing the bridge
code. There doesn't seem to be any other way to get a writable subtree of a
read-only filesystem than remounting it with no parameters after the initial
mount. We use this to get a writable sysfs instance and then bindmount it so
that it fits properly in the sysfs hierarchy.
Co-Authored-By: Thomas Haller <thaller@redhat.com>
The link test was disabled in commit 67ad3fcb5b.
The previous issues are not fixed, but apparently disabling the test doesn't
help to get it fixed.
Re-enable it and if it fails we have a better reason to fix it.
Or maybe it works now (?). Didn't fail for me...
link_extract_type() would return the NMLinkType and a
@type_name string. If the type was unknown, this string
was rtnl_link_get_type() (IFLA_INFO_KIND).
Split up this behavior and treat those values independently.
link_extract_type() now only detects the NMLinkType. Most users
don't care about unknown types and can just use nm_link_type_to_string()
to get a string represenation.
Only nm_platform_link_get_type_name() (and NMDeviceGeneric:type_description)
cared about a more descriptive type. For that, modify link_get_type_name()
to return nm_link_type_to_string() if NMLinkType could be detected.
As fallback, return rtnl_link_get_type().
Also, rename the field NMPlatformLink:link_type to "kind". For now this
field is mostly unused. It will be used later when refactoring platform
caching.
Instead of having a nm_platform_free() function, use NM_DEFINE_SINGLETON_WEAK_REF()
and register a weak reference. That way, users who want to free the platform
instance can just unref it.
Most nm_platform_*() functions operate on the platform
singleton nm_platform_get(). That made sense because the
NMPlatform instance was mainly to hook fake platform for
testing.
While the implicit argument saved some typing, I think explicit is
better. Especially, because NMPlatform could become a more usable
object then just a hook for testing.
With this change, NMPlatform instances can be used individually, not
only as a singleton instance.
Before this change, the constructor of NMLinuxPlatform could not
call any nm_platform_*() functions because the singleton was not
yet initialized. We could only instantiate an incomplete instance,
register it via nm_platform_setup(), and then complete initialization
via singleton->setup().
With this change, we can create and fully initialize NMPlatform instances
before/without setting them up them as singleton.
Also, currently there is no clear distinction between functions
that operate on the NMPlatform instance, and functions that can
be used stand-alone (e.g. nm_platform_ip4_address_to_string()).
The latter can not be mocked for testing. With this change, the
distinction becomes obvious. That is also useful because it becomes
clearer which functions make use of the platform cache and which not.
Inside nm-linux-platform.c, continue the pattern that the
self instance is named @platform. That makes sense because
its type is NMPlatform, and not NMLinuxPlatform what we
would expect from a paramter named @self.
This is a major diff that causes some pain when rebasing. Try
to rebase to the parent commit of this commit as a first step.
Then rebase on top of this commit using merge-strategy "ours".
Tests that use g_assert_expect_message() must initialize with
nmtst_init_assert_logging().
Otherwise, the caller can change the logging level via
NMTST_DEBUG=log-level=DEBUG,log-domains=DEFAULT
which breaks the assertions.
nmtst_init_assert_logging() allows the caller to turn of
checking of assertions via
NMTST_DEBUG=log-level=DEBUG,log-domains=DEFAULT,no-expect-message
Also, don't use g_message() in platform tests otherwise the test fail
because nmtst now sets g_log_set_always_fatal().
There's no udev running in containers, it only starts if /sys is writable. If a
hardware device is added to the container's namespace NM would not announce it.
This also removes the software link special case -- the software links will now
wait for udev initialization (in case udev is there) as well. There's no reason
to treat them differently anymore. This makes it possible to use udev properties
of the software links.
https://bugzilla.gnome.org/show_bug.cgi?id=740526
refresh_object() raised a spurious change event for the route we
are about to delete. Suppress that by adding an internal reason flag.
Fixes: 41e6c4fac1
Handling a route with metric 0 effectively means
a metric of 1024 (user default). Adjust the add(),
delete() and exist() functions to consider routes
with metric 0 as 1024.
If we have NM running, adding a route with metric 20 might conflict
and cause NM to remove the route.
Choose a different (higher) metric that is less likely to cause a
conflict.
config.h should be included from every .c file, and it should be
included before any other include. Fix that.
(As a side effect of how I did this, this also changes us to
consistently use "config.h" rather than <config.h>. To the extent that
it matters [which is not much], quotes are more correct anyway, since
we're talking about a file in our own build tree, not a system
include.)
Add a new enum NMPlatformGetRouteMode. This extends the existing
functions nm_platform_ip4_route_get_all() and nm_platform_ip6_route_get_all()
to return default routes only.
Signed-off-by: Thomas Haller <thaller@redhat.com>
