"libnm-core/" is rather complicated. It provides a static library that
is linked into libnm.so and NetworkManager. It also contains public
headers (like "nm-setting.h") which are part of public libnm API.
Then we have helper libraries ("libnm-core/nm-libnm-core-*/") which
only rely on public API of libnm-core, but are themself static
libraries that can be used by anybody who uses libnm-core. And
"libnm-core/nm-libnm-core-intern" is used by libnm-core itself.
Move "libnm-core/" to "src/". But also split it in different
directories so that they have a clearer purpose.
The goal is to have a flat directory hierarchy. The "src/libnm-core*/"
directories correspond to the different modules (static libraries and set
of headers that we have). We have different kinds of such modules because
of how we combine various code together. The directory layout now reflects
this.
GObject's dispose() functions may be called multiple times
to break reference cycles.
As dispose() calls _init_release_all(), the object might
already be partially destroyed.
Fixes: ce0e898fb4 ('libnm: refactor caching of D-Bus objects in NMClient')
We will need more levels of priority. Change the naming
to make room for that.
sed 's/NML_DBUS_META_INTERFACE_PRIO_INSTANTIATE_LOW/NML_DBUS_META_INTERFACE_PRIO_INSTANTIATE_10/g' `git grep -l NML_DBUS_META_INTERFACE_PRIO_INSTANTIATE_ ` -i
sed 's/NML_DBUS_META_INTERFACE_PRIO_INSTANTIATE_LOW/NML_DBUS_META_INTERFACE_PRIO_INSTANTIATE_10/g' `git grep -l NML_DBUS_META_INTERFACE_PRIO_INSTANTIATE_ ` -i
./contrib/scripts/nm-code-format-container.sh
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
[thaller@redhat.com: split original patch]
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
With 1.22, various synchronous functions for invoking D-Bus methods were
deprecated. The reason was that D-Bus is fundamentally asynchronous, and
providing synchronous API in NMClient is inherently wrong. That is
because NMClient essentially is a cache of the D-Bus API, and invoking
g_dbus_connection_call_sync() messes up the order of events from D-Bus.
In particular, when the synchronous function completes, the content of
the cache does not yet reflect the change.
Since they got deprecated, the question is with what to replace them.
Instead of adding a (e.g.) nm_client_networking_set_enabled_async()
for nm_client_networking_set_enabled(), just expect the user to call
D-Bus directly.
D-Bus itself defines a reasonable API, and with GDBusConnection it
is fine (and convenient) to just call D-Bus operations directly.
Often libraries try to abstract D-Bus by providing convenience
wrappers around D-Bus API. I think that often is wrong and unnecessary.
Note that libnm's NMClient does a lot more than just wrapping simple
D-Bus calls. It provides a complete client-side cache of the D-Bus
interface. As such, what libnm's NMClient does is more than simple
wrappers around D-Bus. NMClient is a reasonable thing to do.
However, it is unnecessary to add API like nm_client_networking_set_enabled_async()
that only calls g_dbus_connection_call(). Don't pretend that we would need such
trivial wrappers in libnm.
Instead, recommend to use g_dbus_connection_call(). Or alternatively,
the convenience wrappers nm_client_dbus_call() and
nm_client_dbus_set_property().
Similar to nm_client_dbus_call(), but useful for setting a D-Bus
property on NetworkManager's D-Bus interface.
Note that we currently have various synchronous API for setting D-Bus
properties (like nm_client_networking_set_enabled()). Synchronous
API does not play well with the content of NMClient's cache, and was
thus deprecated. However, until now no async variant exists.
Instead of adding multiple async operations, I think it should be
sufficient to only add one nm_client_dbus_set_property() property.
It's still reasonably convenient to use for setting a property.
Add an API for calling D-Bus methods arbitrary objects of
NetworkManager's API.
Of course, this is basically just a call to g_dbus_connection_call(),
using the current name owner, nm_client_get_dbus_connection() and
nm_client_get_main_context().
All of this could also be achieved without this new API. However,
nm_client_dbus_call() also gracefully handles if the current name
owner is %NULL.
It's a valid concern whether such API is useful, as the users already
have all pieces to do it themself. I think it is.
Several macros are used to define function. They had a "_STATIC" variant,
to define the function as static.
I think those macros should not try to abstract entirely what they do.
They should not accept the function scope as argument (or have two
variants per scope). This also because it might make sense to add
additional __attribute__(()) to the function. That only works, if
the macro does not pretend to *not* define a plain function.
Instead, embrace what the function does and let the users place the
function scope as they see fit.
This also follows what is already done with
static NM_CACHED_QUARK_FCN ("autoconnect-root", autoconnect_root_quark)
Most callers would pass FALSE to nm_utils_error_is_cancelled(). That's
not very useful. Split the two functions and have nm_utils_error_is_cancelled()
and nm_utils_error_is_cancelled_is_disposing().
Generally, libnm's NMClient cache only wants to expose NMObjects that
are fully initalized. Most objects don't require anything special,
except NMRemoteConnection which waits for the GetSettings() call to complete.
NMObjects reference each other. For example, NMActiveConnection
references NMDevice and NMRemoteConnection. There is a desire that an
object is only ready, if the objects that it references are ready too.
In practice that is not done, because usually every objects references
other objects, that means all objects would be declared as non-ready
as long as any of them is still initializing. That does not seem
desirable. Instead, most objects (except NMRemoteConnection and now
NMActiveConnection) are considered ready and visible, once their first
notification completes. In case the objects reference any object that is
not yet ready, the references is NULL (but the source object is visible
already). This is also done this way, to cope with cycles where
objects reference each other. In practice, such cycles should not be
exposed by NetworkManager. However, libnm should be robust against that.
This has the undesired effect that when you call AddAndActivate(), then
the NMActiveConnection might already be visible while its
NMRemoteConnection isn't. That means, ac.get_connection() will
initially return NULL, until the remote connection becomes ready.
Also add a special handling that NMActiveConnection waits for their
NMRemoteConnection to be ready, before being ready itself.
Fixes: ce0e898fb4 ('libnm: refactor caching of D-Bus objects in NMClient')
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.
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')
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
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.
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.
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.
