fdo-mirrors/NetworkManager
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git synced 2026-05-04 23:28:08 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions

keep-alive: check GetNameOwner lazy and only when we rely on the information

Browse source 
Upside:

  - it may avoid a D-Bus call altogether.

  - currently there is no API to bind/unbind an existing NMActiveConnection,
    it can only be bound initially with AddAndActivate2.
    That makes sense when the calling applicatiion only wants to keep the
    profile active for as long as it lives. It never intends to extend the
    lifetime beyond that. In such a case, it is expected that eventually
    we need to be sure whether the D-Bus client is still alive, as we
    make a decision based on that. In that case, we eventually will call
    GetNameOwner and nothing is saved.
    A future feature could add D-Bus API to bind/unbind existing active
    connections after creation. That would allow an application to
    activate profiles and -- if anything goes wrong -- to be sure
    that the profile gets deactivted. Only in the success case, it
    would unbind the lifetime in a last step and terminate. Un such
    a case, binding to a D-Bus client is more of a fail-safe mechanism
    and commonly not expected to come into action.
    This is an interesting feature, because ActivateConnection D-Bus call
    may take a long time, while perfroming interactive polkit authentication.
    That means, `nmcli connection up $PROFILE` can fail with timeout before
    the active connection path is even created, but afterwards the profile may
    still succeed to activate. That seems wrong. nmcli could avoid that,
    by binding the active connection to itself, and when nmcli exits
    with failure, it would make sure that the active connection gets
    disconnected as well.

Downside:

  - the code is slightly more complicated(?).

  - if the D-Bus name is indeed gone (but the NMKeepAlive is still alive
    for other reasons), we will not unregister the D-Bus signal, because we
    never learn that it's gone. It's not a leak, but an unnecessary
    signal subscription.

  - during nm_keep_alive_set_dbus_client_watch() we don't notice right
    away that the D-Bus client is already gone. That is unavoidable as
    we confirm the state asynchronously.
    If the NMKeepAlive is kept alive for other reasons but only later
    depends on presence of the D-Bus client, then in the non-lazy approach
    we would have noticed already that the client is gone, and would disconnect
    right away. With the lazy approach, this takes another async D-Bus call to
    notice.
This commit is contained in:
Thomas Haller 2018-11-19 14:41:56 +01:00
parent 6f111b3d2e
commit a764061807
1 changed files with 38 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

53
src/nm-keep-alive.c
View file

@ -44,6 +44,7 @@ typedef struct {
bool floating:1;
bool forced:1;
bool alive:1;
bool dbus_client_confirmed:1;
} NMKeepAlivePrivate;
struct _NMKeepAlive {
@ -66,6 +67,7 @@ G_DEFINE_TYPE (NMKeepAlive, nm_keep_alive, G_TYPE_OBJECT)
/*****************************************************************************/
static gboolean _is_alive_dbus_client (NMKeepAlive *self);
static void cleanup_dbus_watch (NMKeepAlive *self);
/*****************************************************************************/
@ -84,7 +86,10 @@ _is_alive (NMKeepAlive *self)
NM_SETTINGS_CONNECTION_INT_FLAGS_VISIBLE))
return TRUE;
if (priv->dbus_client)
/* Perform this check as last. We want to confirm whether the dbus-client
* is alive lazyly, so if we already decided above that the keep-alive
* is good, we don't rely on the outcome of this check. */
if (_is_alive_dbus_client (self))
return TRUE;
return FALSE;
@ -213,6 +218,37 @@ get_name_owner_cb (GObject *source_object,
_notify_alive (self);
}
static gboolean
_is_alive_dbus_client (NMKeepAlive *self)
{
NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self);
if (!priv->dbus_client)
return FALSE;
if (!priv->dbus_client_confirmed) {
/* it's unconfirmed that the D-Bus client is really alive.
* It looks like it is, but as we are claiming that to be
* the case, issue an async GetNameOwner call to make sure. */
priv->dbus_client_confirmed = TRUE;
priv->dbus_client_confirm_cancellable = g_cancellable_new ();
g_dbus_connection_call (priv->dbus_connection,
"org.freedesktop.DBus",
"/org/freedesktop/DBus",
"org.freedesktop.DBus",
"GetNameOwner",
g_variant_new ("(s)", priv->dbus_client),
G_VARIANT_TYPE ("(s)"),
G_DBUS_CALL_FLAGS_NONE,
-1,
priv->dbus_client_confirm_cancellable,
get_name_owner_cb,
self);
}
return TRUE;
}
static void
cleanup_dbus_watch (NMKeepAlive *self)
{
@ -268,6 +304,7 @@ nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self,
_LOGD ("Registering dbus client watch for keep alive");
priv->dbus_client = g_strdup (client_address);
priv->dbus_client_confirmed = FALSE;
priv->dbus_connection = g_object_ref (connection);
priv->subscription_id = g_dbus_connection_signal_subscribe (connection,
"org.freedesktop.DBus",
@ -279,20 +316,6 @@ nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self,
name_owner_changed_cb,
self,
NULL);
priv->dbus_client_confirm_cancellable = g_cancellable_new ();
g_dbus_connection_call (priv->dbus_connection,
"org.freedesktop.DBus",
"/org/freedesktop/DBus",
"org.freedesktop.DBus",
"GetNameOwner",
g_variant_new ("(s)", priv->dbus_client),
G_VARIANT_TYPE ("(s)"),
G_DBUS_CALL_FLAGS_NONE,
-1,
priv->dbus_client_confirm_cancellable,
get_name_owner_cb,
self);
}
_notify_alive (self);