g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
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.
Add 'nm_setting_bond_get_option_normalized()', the purpose of this API
is to retrieve a bond option normalized value which is the option that
NetworkManager will actually apply to the bond when activating the
connection, this takes into account default values for some options that
NM assumes.
For example, if you create a connection:
$ nmcli c add type bond con-name nm-bond ifname bond0 bond.options mode=0
Calling 'nm_setting_bond_get_option_normalized(s_bond, "miimon")' would
return "100" as even if not specified NetworkManager enables miimon for
bond connections.
Another example:
$ nmcli c add type bond con-name nm-bond ifname bond0 bond.options mode=0,arp_interval=100
Calling 'nm_setting_bond_get_option_normalized(s_bond, "miimon")' would
return NULL in this case because NetworkManager disables miimon if
'arp_interval' is set explicitly but 'miimon' is not.
nm_setting_ip6_config_get_ra_timeout() was backported to nm-1-22 branch, and
will be released as 1.22.8. As such, on the stable branch the symbol will be
placed in a separate symbol version ("libnm_1_22_8").
To support the upgrade path from 1.22.8+ to 1.23+, we want this symbol
also present on master.
At that point, we don't need to duplicate the symbol. Just add the same linker
symbol version also to master.
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')
Note that the name "NMSecretAgentOld" comes from when libnm was forked
from libnm-glib. There was a plan to rework the secret agent API and
replace it by a better one. That didn't happen (yet), instead our one
and only agent implementation is still lacking. Don't add a new API, instead
try to improve the existing one, without breaking existing users. Just
get over the fact that the name "NMSecretAgentOld" is ugly.
Also note how nm-applet uses NMSecretAgentOld. It subtypes a class
AppletAgent. The constructor applet_agent_new() is calling the synchronous
g_initable_init() initialization with auto-register enabled. As it was,
g_initable_init() would call nm_secret_agent_old_register(), and if the
"Register" call failed, initialization failed for good. There are even
unit tests that test this behavior. This is bad behavior. It means, when
you start nm-applet without NetworkManager running, it will fail to create
the AppletAgent instance. It would hence be the responsibility of the applet
to recover from this situation (e.g. by retrying after timeout or watching
the D-Bus name owner). Of course, nm-applet doesn't do that and won't recover
from such a failure.
NMSecretAgentOld must try hard not to fail and recover automatically. The
user of the API is not interested in implementing the registration,
unregistration and retry handling. Instead, it should just work best
effort and transparently to the user of the API.
Differences:
- no longer use gdbus-codegen generate bindings. Use GDBusConnection
directly instead. These generated proxies complicate the code by
introducing an additional, stateful layer.
- properly handle GMainContext and synchronous initialization by using an
internal GMainContext.
With this NMSecretAgentOld can be used in a multi threaded context
with separate GMainContext. This does not mean that the object
itself became thread safe, but that the GMainContext gives the means
to coordinate multi-threaded access.
- there are no more blocking calls except g_initiable_init() which
iterates an internal GMainContext until initialization completes.
- obtaining the Unix user ID with "GetConnectionUnixUser" to authenticate
the server is now done asynchronously and only once per name-owner.
- NMSecretAgentOld will now register/export the Agent D-Bus object
already during initialization and stay registered as long as the
instance is alive. This is because usually registering a D-Bus
object would not fail, unless the D-Bus path is already taken.
Such an error would mean that another agent is registered for the same
GDBusConnection, that likely would be a bug in the caller. Hence,
such an issue is truly non-recoverable and should be reported early to
the user. There is a change in behavior compared to before, where previously
the D-Bus object would only be registered while the instance is enabled.
This makes a difference if the user intended to keep the NMSecretAgentOld
instance around in an unregistered state.
Note that nm_secret_agent_old_destroy() was added to really unregister
the D-Bus object. A destroyed instance can no longer be registered.
- the API no longer fully exposes the current registration state. The
user either enables or disables the agent. Then, in the background
NMSecretAgentOld will register, and serve requests as they come. It
will also always automatically re-register and it can de-facto no
longer fail. That is, there might be a failure to register, or the
NetworkManager peer might not be authenticated (non-root) or there
might be some other error, or NetworkManager might not be running.
But such errors are not exposed to the user. The instance is just not
able to provide the secrets in those cases, but it may recover if the
problem can be resolved.
- In particular, it makes no sense that nm_secret_agent_old_register*()
fails, returns an error, or waits until registration is complete. This
API is now only to enable/disable the agent. It is idempotent and
won't fail (there is a catch, see next point).
In particular, nm_secret_agent_old_unregister*() cannot fail anymore.
- However, with the previous point there is a problem/race. When you create
a NMSecretAgentOld instance and immediately afterwards activate a
profile, then you want to be sure that the registration is complete
first. Otherwise, NetworkManager might fail the activation because
no secret agent registered yet. A partial solution for this is
that g_initiable_init()/g_async_initable_init_async() will block
until registration is complete (or with or without success). That means,
if NetworkManager is running, initializing the NMSecretAgentOld will
wait until registration is complete (or failed). However, that does not
solve the race if NetworkManager was not running when creating the
instance.
To solve that race, the user may call nm_secret_agent_old_register_async()
and wait for the command to finish before starting activating. While
async registration no longer fails (in the sense of leaving the agent
permanently disconnected), it will try to ensure that we are
successfully registered and ready to serve requests. By using this
API correctly, a race can be avoided and the user can know that the
instance is now ready to serve request.
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.
