The NMSecretAgentOld is build very much around a GDBusConnection, and GDBusConnection
is build around GMainContext. That means, a NMSecretAgentOld instance is
strongly related to these two. That is because NMSecretAgentOld will register
to signals on D-Bus, using GDBusConnection. Hence, that GDBusConnection instance
and the calling GMainContext becomes central to the NMSecretAgentOld instance.
Also, the GMainContext is the way to synchronize access to the
NMSecretAgentOld. Used properly, this allows using the API in multi
threaded context.
Expose these two in the public API. Since NMSecretAgentOld is part of
libnm and supposed to provide a flexible API, this is just useful to
have.
Also, allow to provide a GDBusConnection as construct-only property. This way,
the instance can be used independent of g_bus_get() and the user has full control.
There is no setter for the GMainContext, because it just takes the
g_main_context_get_thread_default() instance at the time of
construction.
This will also be useful for NMSecretAgentOld.
The mechanics how NMClient handles the GMainContext and the
context-busy-watcher apply really to every GObject that uses
GDBusConnection and registers to signals.
At least, as long as the API provides no shutdown/stop method,
because that means shutdown/stop happens when unreferencing the
instance, at which point pending operations get cancelled (but
they cannot complete right away due to the nature of GTask and
g_dbus_connection_call()). If there is a shutdown/stop API, then all
pending operations could keep the instance alive, and the instance
would sticks around (and keeps the GMainContext busy) until shutdown is
completed. Basically, then the instance could be the context-busy-watcher
itself.
But in existing API which does not require the user to explicitly shutdown,
that is not a feasible (backward compatible) addition. But the context-busy-watcher
object is.
The test only uses one GMainContext (the g_main_context_get_default()
singleton.
Between tests, ensure that we iterate the main context long enough,
so that no more sources from the previous test are queued. Otherwise,
there is an ugly dependency between tests and the order in which
they run.
Use nmtstc_context_object_new() to create the NMSecretAgentOld. This
randomly uses sync or async initialization, and checks whether the
main context gets iterated.
nmtst_main_context_iterate_until*() iterates until the condition is
satisfied. If that doesn't happen within timeout, it fails an assertion.
Rename the function to make that clearer.
The device instance might already be removed from the cache. At that
point, _nm_object_get_client(self) returns %NULL.
Use the correct NMClient instance.
For printf debugging (when you recompile the source) it can be useful
to have one switch to disable logging of NMClient.
For example, this is useful with
$ LIBNM_CLIENT_DEBUG=trace nmcli agent secret
We now move the deletion of the context-busy-watcher to and idle handler
on the D-Bus GMainContext.
Note that the idle source does not take an additional reference on the
context. Hence, in certain cases it might happen that the context will
be completely unrefed before the idle handler runs. In that case, we
would leak the object.
Avoid that, by taking an additional reference to the GMainContext.
Note that the alternative would be to unref the context-busy-watcher
via the GSource's GDestroyNotify. That is not done, because then the
busy watcher might be unrefed in a different thread. Instead, we want
that to happen for the right context. The only minor downside of this
is that the user now always pays the price and must iterate the context
to fully clean up. But note that the user anyway must be prepared to
iterate the context after NMClient is gone. And that depends on some
unpredictable events that the user cannot control. That means, either
the user handles this correctly already, or the problem anyway exists
(randomly).
Of course all of the discussed "problems" are very specific. In practice, the
users uses the g_main_context_default() instance and anyway will either
keep iterating it or quit the process after the NMClient instance goes
away.
The context-busy-watch has two purposes:
1) it allows the user to watch whether the NMClient still has pending
GSource'es attached to the GMainContext.
2) thereby, it also keeps the inner GMainContext integrated into the
caller's (in case of synchronous initialization of NMClient).
Especially for 2), we must not get this wrong. Otherwise, we might
un-integrate the inner GMainContext too early and it will be leaked
indefinitely (because the user has no means to access or iterate it).
To be extra careful, extend the lifetime of the context-busy-watcher
for one more idle invocation. Theoretically, this should not be necessary,
but it's not clear whether something else is still pending.
The downside of that extra safety is that it is probably unnecessary in
practice. And in case where it is necessary, it hides an actual
issue, making it harder to notice and fix it.
It seems to complicate things more than helping. Drop it. What we still have
is a wrapper around plain g_dbus_connection_signal_subscribe(). That one is
trivial and helpful. The previous wrapper seems to add more complexity.
When passing a destroy notify to g_dbus_connection_signal_subscribe(),
that callback gets invoked as an idle handler of the associated
GMainContext. That caused to have yet another source attached to the
context after the NMClient gets destroyed.
Especially with synchronous initialization of NMClient that is bad,
because we may destroy the context-busy-watcher too early. That results
in removing the integration of the inner GMainContext into the caller's
context, and thus we leak the inner context indefinitely.
Avoid that leak by not passing a cleanup function to
g_dbus_connection_signal_subscribe().
Fixes: ce0e898fb4 ('libnm: refactor caching of D-Bus objects in NMClient')
Groups currently are not exposed on D-Bus as separate objects.
Also, we might want to expose the property as "ao" instead of "as".
This API needs more thought.
There are likely no users that rely on this property. So, we will
drop it from server side, until it will be requested and newly designed.
Regardless, NMClient needs to gracefully ignore the property.
Despite we will remove it from 1.24 API, libnm should ignore the
property on previous versions. Mark it accordingly.
Add VRF support to the daemon. When the device we are activating is a
VRF or a VRF's slave, put routes in the table specified by the VRF
connection.
Also, introduce a VRF device type in libnm.
When iterating the GMainContext of the NMClient instance, D-Bus events
get processed. That means, every time you iterate the context (or "return to
the main loop"), the content of the cache might change completely.
It makes sense to keep a reference to an NMObject instance, do something,
and afterwards check whether the instance can still be found in the cache.
Add an API for that. nm_object_get_client() allows to know whether the
object is still cached.
Likewise, while NMClient abstracts D-Bus, it should still provide a way
to look up an NMObject by D-Bus path. Add nm_client_get_object_by_path()
for that.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/384
nm_client_get_capabilities() was backported to 1.22.2. Add to to the
appropriate linker version.
Officially (and according to docs) nm_client_get_capabilities() still
appears first in libnm 1.24.0. However, as it got backported to 1.22.2,
it needs to be part of a different symbol version on 1.22. Instead
of adding the symbol twice (once for libnm_1_24_0 and libnm_1_22_2),
move it only to the libnm_1_22_2 symbol version, also on master.
I hesitated to add this to libnm, because it's hardly used.
However, we already fetch the property during GetManagedObjects(),
we we should make it accessible, instead of requiring the user to
make another D-Bus call.
When NetworkManager starts, NMSecretAgentOld gets a name-owner changed
signal and registers right away.
Especially since commit ce0e898fb4 ('libnm: refactor caching of D-Bus
objects in NMClient') this hits a race where NetworkManager does not yet
export the org.freedesktop.NetworkManager.AgentManager interface and
the registration fails:
GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager
Previously, when NMClient recevied a name-owner changed, that would
block the main loop long enough to avoid the race. Note that NMClient
has nothing to do with NMSecretAgentOld, however in practice all
applications that use NMSecretAgentOld also use NMClient.
While we should fix the race server-side, we also need to work around it
in the client. Retry.
Also, make the async request actually cancellable and actually honor the passed
GCancellable.
Check output:
$ LIBNM_CLIENT_DEBUG=trace ./clients/cli/nmcli agent secret |& grep secret-agent
libnm-dbus: <trace> [21399.04862] secret-agent[2f2af4ee102d7570]: create new instance
libnm-dbus: <trace> [21399.04863] secret-agent[2f2af4ee102d7570]: init-sync
libnm-dbus: <trace> [21404.08147] secret-agent[2f2af4ee102d7570]: name owner changed: (null)
libnm-dbus: <trace> [21404.09085] secret-agent[2f2af4ee102d7570]: name owner changed: ":1.2504"
libnm-dbus: <trace> [21404.09085] secret-agent[2f2af4ee102d7570]: register: starting asynchronous registration...
libnm-dbus: <trace> [21404.09178] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 0 msec...
libnm-dbus: <trace> [21404.09178] secret-agent[2f2af4ee102d7570]: register: retry registration...
libnm-dbus: <trace> [21404.09195] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 4 msec...
libnm-dbus: <trace> [21404.09236] secret-agent[2f2af4ee102d7570]: register: retry registration...
[...]
libnm-dbus: <trace> [21405.01782] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 128 msec...
libnm-dbus: <trace> [21405.03063] secret-agent[2f2af4ee102d7570]: register: retry registration...
libnm-dbus: <trace> [21405.03068] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 128 msec...
libnm-dbus: <trace> [21405.04354] secret-agent[2f2af4ee102d7570]: register: retry registration...
libnm-dbus: <trace> [21406.01097] secret-agent[2f2af4ee102d7570]: register: registration succeeded
RegisterWithCapabilities() is supported since NetworkManager 0.9.9.1. Of course,
we don't support such old server anymore (also, because we require the standard
D-Bus interfaces like ObjectManager).
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
We have "shared/nm-libnm-core-aux", which is shared code that can be used
by anybody (including libnm-core, src, libnm and clients).
We have "clients/common", which are helper function for clients. But
that implies that the code is inside "clients". I think it would be
useful to have auxiliary code that extends libnm, but is not only
usable by code in "clients". In other words, "shared/nm-libnm-aux"
is a better place than "clients/common", and I think most of the
functionality form "clients/common" should move there.
Currently, NMClient by default always fetches the permissions
("GetPermissions()") and refreshes them on "CheckPermissions" signal.
Fetching permissions is relatively expensive, while they are not used
most of the time. Allow the user to opt out of this.
For that, have a NMClientInstanceFlags to enable/disable automatic
fetching. Also add a "permissions-state" property that allows the user
to understand whether the cached permissions are up to date or not.
This is a bit an awkward API for handling this. E.g. you cannot
explicitly request permissions, you can just enable/disable fetching
permissions. And then you can watch the permission-state to know whether
you are ready. It's done this way because it fits the previous model
and extends the API with a (relative) small amount of new functions and
properties.
Add a flags property to control behavior of NMClient.
Possible future use cases:
- currently it would always automatically fetch permissions. Often that
is not used and the user could opt out of it.
- currently, using sync init creates an internal GMainContext. This
has an overhead and may be undesirable. We could implement another
"sync" initialization that would merely iterate the callers mainloop
until the initialization completes. A flag would allow to opt in.
- currently, NMClient always fetches all connection settings
automatically. Via a flag the user could opt out of that.
Instead NMClient could provide an API so the user can request
settings as they are needed.
On D-Bus, the permission names are just the PolicyKit action names, like
"org.freedesktop.NetworkManager.wifi.scan". But NMClient already
ignores all strings that it doesn't know at compile time and only
keeps track of well known permission.
And neither does the API nm_client_get_permissions_result() allow to
expose permissions unknown to libnm.
Maybe the API of NMClient should be more generic and allow exposing
any permissions announced by NetworkManager. As it is however, it's
not necessary to track the permissions in a hash table. An array with
fixed indices is sufficient.
Not being able to compare two NMIPAddress instances is a major
limitation. Add nm_ip_address_cmp_full(). The choice here for adding
a "cmp()" function instead of a "equals()" function is that cmp is
more useful. We only want to add one of the two, so choose the
more powerful one. Yes, usually its also not the variant we want
or the variant that is convenient to use, such is life.
Compare this to:
- nm_ip_route_equal_full(), which is an equal() method and not
a cmp().
- nm_ip_route_equal_full() which has a guint flags argument,
instead of a typedef for an enum, with a proper generated
GType.
I have a coredump that seems to indicate that nm_device_get_active_connection()
did not return a valid object. Let's add an assertion, trying to identify the
issue earlier. Aside from that, this change isn't useful, but an nm_assert()
shouldn't hurt anyway.
The NMClient is associated with a certain context. Add a getter
function to give the context.
The context is really not internal API of NMClient, that is because
the user must iterate this context and be aware of it.
Usually, the nmobj never gets reused for one dbobj. That means,
we really don't expect a nml_dbus_property_o_notify() for a property
that was already cleared.
However, that is for example not the case with NMClient itself. As NetworkManager
gets restarted, the name owner gets lost, the property cleared but afterwards
it might get notified again.
That means, nml_dbus_property_o_notify() and nml_dbus_property_o_clear() must
work well together, otherwise a sequence of
nml_dbus_property_o_notify()
nml_dbus_property_o_clear()
nml_dbus_property_o_notify()
leads to an assertion failure "nm_assert (!pr_o->is_ready)".
Fixes: ce0e898fb4 ('libnm: refactor caching of D-Bus objects in NMClient')
NMClient makes asynchronous D-Bus calls via g_dbus_connection_call().
This references the current GMainContext to later invoke the
asynchronous callback. Even when we cancel the asynchronous call,
the callback will still be invoked (later) to complete the request.
In particular this means when we destroy (unref) an NMClient, there
are quite possibly pending requests in the GMainContext. Although they
are cancelled, they keep the GMainContext alive.
With synchronous initialization, we have an internal GMainContext.
When we destroy the NMClient, we cannot just unhook the integrated
source, instead, we need to keep it integrated in the caller's main
context, as long as there are pending requests.
Add a mechanism to track those pending requests and fix the leak for the
internal GMainContext. Also expose the same mechanism to the user via a new
API called nm_client_get_context_busy_watcher(). This allows the user
to know when it can stop iterating the main context and when all
resources are reclaimed.
For example the following will lead to a crash:
for i in range(1,2000):
nmc = NM.Client.new(None)
This creates a number of NMClient instances and destroys them again.
Note that here the GMainContext is never iterated, because
synchronous initialization does not iterate the caller's context. So,
while we correctly unref and dispose the created NMClient instances,
there are pending requests left in the inner GMainContext. These pile
up and soon the program will crash because it runs out of file descriptors.
We can have a similar problem with asynchronous initialization, when
we create a new GMainContext per client, and don't iterate it after
we are done with the client.
Note that this patch does not avoid the problem in general. The problem
cannot be avoided, the user must iterate the main contex at some point.
Otherwise resources (memory and file descriptors) will be leaked.
Fixes: ce0e898fb4 ('libnm: refactor caching of D-Bus objects in NMClient')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/347
