From 3e3d53ce6960dc0195c29a32c46cecafee3a8a70 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Thu, 12 Apr 2018 11:32:18 +0200
Subject: [PATCH] manager: add is-vpn argument to _new_active_connection() and
 avoid searching existing activations

- pass is-vpn to _new_active_connection(). It is confusing that _new_active_connection()
  would re-determine whether a connection is a VPN type, although that was already
  established previously. The confusing part is: will they come to the
  same result? Why? What if not?
  Instead pass it as argument and assert that we got it right.

- the check for existing connections should look whether there is an existing
  active connection of type NMVpnConnection. Instead, what matters is,
  - do we have a connection of type VPN (otherwise, don't even bother
    to search for existing-ac)
  - is the connection already active?
  Checking whether the connection is already active, and ask backwards
  whether it's of type NMVpnConnection is odd, maybe even wrong in
  some cases.
---
 src/nm-manager.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/src/nm-manager.c b/src/nm-manager.c
index fff4fc412f..8dcfee238e 100644
--- a/src/nm-manager.c
+++ b/src/nm-manager.c
@@ -281,6 +281,7 @@ static void _emit_device_added_removed (NMManager *self,
                                         gboolean is_added);
 
 static NMActiveConnection *_new_active_connection (NMManager *self,
+                                                   gboolean is_vpn,
                                                    NMConnection *connection,
                                                    NMConnection *applied,
                                                    const char *specific_object,
@@ -2398,8 +2399,13 @@ recheck_assume_connection (NMManager *self,
 		GError *error = NULL;
 
 		subject = nm_auth_subject_new_internal ();
-		active = _new_active_connection (self, NM_CONNECTION (connection), NULL, NULL,
-		                                 device, subject,
+		active = _new_active_connection (self,
+		                                 FALSE,
+		                                 NM_CONNECTION (connection),
+		                                 NULL,
+		                                 NULL,
+		                                 device,
+		                                 subject,
 		                                 generated ? NM_ACTIVATION_TYPE_EXTERNAL : NM_ACTIVATION_TYPE_ASSUME,
 		                                 NM_ACTIVATION_REASON_AUTOCONNECT,
 		                                 &error);
@@ -4026,6 +4032,7 @@ _new_vpn_active_connection (NMManager *self,
 
 static NMActiveConnection *
 _new_active_connection (NMManager *self,
+                        gboolean is_vpn,
                         NMConnection *connection,
                         NMConnection *applied,
                         const char *specific_object,
@@ -4036,29 +4043,33 @@ _new_active_connection (NMManager *self,
                         GError **error)
 {
 	NMSettingsConnection *settings_connection = NULL;
-	NMActiveConnection *existing_ac;
-	gboolean is_vpn;
 
 	g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL);
 	g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL);
 
-	/* Can't create new AC for already-active connection */
-	existing_ac = active_connection_find_first_by_connection (self, connection);
-	if (NM_IS_VPN_CONNECTION (existing_ac)) {
-		g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_CONNECTION_ALREADY_ACTIVE,
-		             "Connection '%s' is already active",
-		             nm_connection_get_id (connection));
-		return NULL;
+	nm_assert (is_vpn == _connection_is_vpn (connection));
+
+	if (is_vpn) {
+		/* FIXME: for VPN connections, we don't allow re-activating an
+		 * already active connection. It's a bug, and should be fixed together
+		 * when reworking VPN handling. */
+		if (active_connection_find_first_by_connection (self, connection)) {
+			g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_CONNECTION_ALREADY_ACTIVE,
+			             "Connection '%s' is already active",
+			             nm_connection_get_id (connection));
+			return NULL;
+		}
 	}
 
 	specific_object = nm_utils_dbus_normalize_object_path (specific_object);
 
-	is_vpn = _connection_is_vpn (NM_CONNECTION (connection));
-
 	if (NM_IS_SETTINGS_CONNECTION (connection))
 		settings_connection = (NMSettingsConnection *) connection;
 
 	if (is_vpn) {
+		/* FIXME: apparently, activation here only works if @connection is
+		 * a settings-connection. Which is not the case during AddAndActivatate.
+		 * Probably, AddAndActivate is broken for VPN. */
 		if (activation_type != NM_ACTIVATION_TYPE_MANAGED)
 			g_return_val_if_reached (NULL);
 		return _new_vpn_active_connection (self,
@@ -4200,6 +4211,7 @@ nm_manager_activate_connection (NMManager *self,
 	}
 
 	active = _new_active_connection (self,
+	                                 FALSE,
 	                                 NM_CONNECTION (connection),
 	                                 applied,
 	                                 specific_object,
@@ -4439,6 +4451,7 @@ impl_manager_activate_connection (NMDBusObject *obj,
 		goto error;
 
 	active = _new_active_connection (self,
+	                                 is_vpn,
 	                                 NM_CONNECTION (connection),
 	                                 NULL,
 	                                 specific_object_path,
@@ -4658,6 +4671,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj,
 	}
 
 	active = _new_active_connection (self,
+	                                 is_vpn,
 	                                 connection,
 	                                 NULL,
 	                                 specific_object_path,