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

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.

1 files changed, 38 insertions, 15 deletions

diff --git a/src/nm-keep-alive.c b/src/nm-keep-alive.c
index bb48dc43b7..6949682571 100644
--- a/src/nm-keep-alive.c
+++ b/src/nm-keep-alive.c
@@ -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);