core: allow multiple devices with the same interface namedcbw/devices-for-all

But, of course, only one realized device can have the same
interface name at a time.

This commit effectively reverts most of:

1b37cd03409fa3c0fb26fb6e8093b54b2b7c5d8d
core: allow ActiveConnections to be created without a device

But it's not easy to do a separate revert of that code due to
interdependencies with nm-manager.c.

Creating devices when they are defined by a connection also makes
makes it possible to require the NMDevice to be present when
activating it, which means we can remove a bunch of code from
NMManager that had to handle software devices not existing yet at
the time of the activation request.

But it also means we must be more careful when finding master
interfaces during slave activation, since we cannot simply match
by interface name alone.  Instead we must find the master which
matches both the interface name and can control slaves of the type
which is being activated.

5 files changed, 191 insertions, 122 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 5435440e2d..6cbf29065b 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -2290,6 +2290,18 @@ nm_device_master_release_slaves (NMDevice *self)
 }
 
 /**
+ * nm_device_is_master:
+ * @self: the device
+ *
+ * Returns: %TRUE if the device can have slaves
+ */
+gboolean
+nm_device_is_master (NMDevice *self)
+{
+	return NM_DEVICE_GET_PRIVATE (self)->is_master;
+}
+
+/**
  * nm_device_get_master:
  * @self: the device
  *
@@ -2873,6 +2885,34 @@ nm_device_check_connection_compatible (NMDevice *self, NMConnection *connection)
 	return NM_DEVICE_GET_CLASS (self)->check_connection_compatible (self, connection);
 }
 
+gboolean
+nm_device_check_slave_connection_compatible (NMDevice *self, NMConnection *slave)
+{
+	NMDevicePrivate *priv;
+	NMSettingConnection *s_con;
+	const char *connection_type, *slave_type;
+
+	g_return_val_if_fail (NM_IS_DEVICE (self), FALSE);
+	g_return_val_if_fail (NM_IS_CONNECTION (slave), FALSE);
+
+	priv = NM_DEVICE_GET_PRIVATE (self);
+
+	if (!priv->is_master)
+		return FALSE;
+
+	/* All masters should have connection type set */
+	connection_type = NM_DEVICE_GET_CLASS (self)->connection_type;
+	g_return_val_if_fail (connection_type, FALSE);
+
+	s_con = nm_connection_get_setting_connection (slave);
+	g_assert (s_con);
+	slave_type = nm_setting_connection_get_slave_type (s_con);
+	if (!slave_type)
+		return FALSE;
+
+	return strcmp (connection_type, slave_type) == 0;
+}
+
 /**
  * nm_device_can_assume_connections:
  * @self: #NMDevice instance
diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h
index 55f05c4fab..a92719cd8a 100644
--- a/src/devices/nm-device.h
+++ b/src/devices/nm-device.h
@@ -388,6 +388,7 @@ void            nm_device_capture_initial_config (NMDevice *dev);
 
 /* Master */
 GSList *        nm_device_master_get_slaves (NMDevice *dev);
+gboolean        nm_device_is_master         (NMDevice *dev);
 
 /* Slave */
 NMDevice *      nm_device_get_master        (NMDevice *dev);
@@ -421,6 +422,7 @@ gboolean nm_device_complete_connection (NMDevice *device,
                                         GError **error);
 
 gboolean nm_device_check_connection_compatible (NMDevice *device, NMConnection *connection);
+gboolean nm_device_check_slave_connection_compatible (NMDevice *device, NMConnection *connection);
 
 gboolean nm_device_uses_assumed_connection (NMDevice *device);
 
diff --git a/src/nm-activation-request.c b/src/nm-activation-request.c
index e6645bdbc4..30e98fcacb 100644
--- a/src/nm-activation-request.c
+++ b/src/nm-activation-request.c
@@ -468,9 +468,7 @@ master_failed (NMActiveConnection *self)
  * @specific_object: the object path of the specific object (ie, WiFi access point,
  *    etc) that will be used to activate @connection and @device
  * @subject: the #NMAuthSubject representing the requestor of the activation
- * @device: the device/interface to configure according to @connection; or %NULL
- * if the connection describes a software device which will be created during
- * connection activation
+ * @device: the device/interface to configure according to @connection
  *
  * Creates a new device-based activation request.
  *
@@ -483,7 +481,7 @@ nm_act_request_new (NMSettingsConnection *settings_connection,
                     NMDevice *device)
 {
 	g_return_val_if_fail (!settings_connection || NM_IS_SETTINGS_CONNECTION (settings_connection), NULL);
-	g_return_val_if_fail (!device || NM_IS_DEVICE (device), NULL);
+	g_return_val_if_fail (NM_IS_DEVICE (device), NULL);
 	g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL);
 
 	return (NMActRequest *) g_object_new (NM_TYPE_ACT_REQUEST,
diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c
index 609e645e0a..604e6ec4ec 100644
--- a/src/nm-active-connection.c
+++ b/src/nm-active-connection.c
@@ -515,8 +515,13 @@ nm_active_connection_set_device (NMActiveConnection *self, NMDevice *device)
 			priv->pending_activation_id = g_strdup_printf ("activation::%p", (void *)self);
 			nm_device_add_pending_action (device, priv->pending_activation_id, TRUE);
 		}
-	} else
+	} else {
+		/* The ActiveConnection's device can only be cleared after the
+		 * connection is activated.
+		 */
+		g_warn_if_fail (priv->state > NM_ACTIVE_CONNECTION_STATE_UNKNOWN);
 		priv->device = NULL;
+	}
 	g_object_notify (G_OBJECT (self), NM_ACTIVE_CONNECTION_INT_DEVICE);
 
 	g_signal_emit (self, signals[DEVICE_CHANGED], 0, priv->device, old_device);
@@ -831,6 +836,7 @@ set_property (GObject *object, guint prop_id,
 		}
 		break;
 	case PROP_INT_DEVICE:
+		/* construct-only */
 		nm_active_connection_set_device (NM_ACTIVE_CONNECTION (object), g_value_get_object (value));
 		break;
 	case PROP_INT_SUBJECT:
@@ -1129,7 +1135,7 @@ nm_active_connection_class_init (NMActiveConnectionClass *ac_class)
 		(object_class, PROP_INT_DEVICE,
 		 g_param_spec_object (NM_ACTIVE_CONNECTION_INT_DEVICE, "", "",
 		                      NM_TYPE_DEVICE,
-		                      G_PARAM_READWRITE |
+		                      G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY |
 		                      G_PARAM_STATIC_STRINGS));
 
 	g_object_class_install_property
diff --git a/src/nm-manager.c b/src/nm-manager.c
index daaca90366..2c0d2418f3 100644
--- a/src/nm-manager.c
+++ b/src/nm-manager.c
@@ -499,16 +499,53 @@ find_device_by_ip_iface (NMManager *self, const gchar *iface)
 	return NULL;
 }
 
+/**
+ * find_device_by_iface:
+ * @self: the #NMManager
+ * @iface: the device interface to find
+ * @connection: a connection to ensure the returned device is compatible with
+ * @slave: a slave connection to ensure a master is compatible with
+ *
+ * Finds a device by interface name, preferring realized devices.  If @slave
+ * is given, this function will only return master devices and will ensure
+ * @slave, when activated, can be a slave of the returned master device.  If
+ * @connection is given, this function will only consider devices that are
+ * compatible with @connection.
+ *
+ * Returns: the matching #NMDevice
+ */
 static NMDevice *
-find_device_by_iface (NMManager *self, const gchar *iface)
+find_device_by_iface (NMManager *self,
+                      const char *iface,
+                      NMConnection *connection,
+                      NMConnection *slave)
 {
+	NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self);
+	NMDevice *fallback = NULL;
 	GSList *iter;
 
-	for (iter = NM_MANAGER_GET_PRIVATE (self)->devices; iter; iter = g_slist_next (iter)) {
-		if (g_strcmp0 (nm_device_get_iface (NM_DEVICE (iter->data)), iface) == 0)
-			return NM_DEVICE (iter->data);
+	g_return_val_if_fail (iface != NULL, NULL);
+
+	for (iter = priv->devices; iter; iter = iter->next) {
+		NMDevice *candidate = iter->data;
+
+		if (strcmp (nm_device_get_iface (candidate), iface))
+			continue;
+		if (connection && !nm_device_check_connection_compatible (candidate, connection))
+			continue;
+		if (slave) {
+			if (!nm_device_is_master (candidate))
+				continue;
+			if (!nm_device_check_slave_connection_compatible (candidate, slave))
+				continue;
+		}
+
+		if (nm_device_is_real (candidate))
+			return candidate;
+		else if (!fallback)
+			fallback = candidate;
 	}
-	return NULL;
+	return fallback;
 }
 
 static gboolean
@@ -860,8 +897,8 @@ find_parent_device_for_connection (NMManager *self, NMConnection *connection)
 	if (!parent_name)
 		return NULL;
 
-	/* Try as an interface name */
-	parent = find_device_by_iface (self, parent_name);
+	/* Try as an interface name of a parent device */
+	parent = find_device_by_iface (self, parent_name, NULL, NULL);
 	if (parent)
 		return parent;
 
@@ -988,18 +1025,16 @@ system_create_virtual_device (NMManager *self, NMConnection *connection, GError
 	if (!iface)
 		return NULL;
 
-	/* Make sure we didn't create a device for this connection already */
+	/* If some other device is already compatible with this connection,
+	 * don't create a new device for it.
+	 */
 	for (iter = priv->devices; iter; iter = g_slist_next (iter)) {
 		NMDevice *candidate = iter->data;
 
 		if (   g_strcmp0 (nm_device_get_iface (candidate), iface) == 0
-		    || nm_device_check_connection_compatible (candidate, connection)) {
+		    && nm_device_check_connection_compatible (candidate, connection)) {
 			nm_log_dbg (LOGD_DEVICE, "(%s) already created virtual interface name %s",
 			            nm_connection_get_id (connection), iface);
-			g_set_error (error,
-			             NM_MANAGER_ERROR,
-			             NM_MANAGER_ERROR_FAILED,
-			             "interface name '%s' already created", iface);
 			return NULL;
 		}
 	}
@@ -1665,7 +1700,8 @@ device_ip_iface_changed (NMDevice *device,
 		NMDevice *candidate = NM_DEVICE (iter->data);
 
 		if (   candidate != device
-		    && g_strcmp0 (nm_device_get_iface (candidate), ip_iface) == 0) {
+		    && g_strcmp0 (nm_device_get_iface (candidate), ip_iface) == 0
+		    && nm_device_is_real (candidate)) {
 			remove_device (self, candidate, FALSE, FALSE);
 			break;
 		}
@@ -1728,8 +1764,6 @@ add_device (NMManager *self, NMDevice *device)
 	ifindex = nm_device_get_ifindex (device);
 	if (ifindex > 0 && nm_manager_get_device_by_ifindex (self, ifindex))
 		return;
-	if (find_device_by_iface (self, nm_device_get_iface (device)))
-		return;
 
 	/* Remove existing devices owned by the new device; eg remove ethernet
 	 * ports that are owned by a WWAN modem, since udev may announce them
@@ -1739,9 +1773,11 @@ add_device (NMManager *self, NMDevice *device)
 	 * the child NMDevice entirely
 	 */
 	for (iter = priv->devices; iter; iter = iter->next) {
-		iface = nm_device_get_ip_iface (iter->data);
-		if (nm_device_owns_iface (device, iface))
-			remove = g_slist_prepend (remove, iter->data);
+		NMDevice *candidate = iter->data;
+
+		iface = nm_device_get_ip_iface (candidate);
+		if (nm_device_is_real (candidate) && nm_device_owns_iface (device, iface))
+			remove = g_slist_prepend (remove, candidate);
 	}
 	for (iter = remove; iter; iter = iter->next)
 		remove_device (self, NM_DEVICE (iter->data), FALSE, FALSE);
@@ -1881,37 +1917,37 @@ platform_link_added (NMManager *self,
 	NMDevice *device = NULL;
 	GError *error = NULL;
 	gboolean nm_plugin_missing = FALSE;
+	GSList *iter;
 
 	g_return_if_fail (ifindex > 0);
 
 	if (nm_manager_get_device_by_ifindex (self, ifindex))
 		return;
 
-	device = find_device_by_iface (self, plink->name);
-	if (device) {
-		gboolean compatible = FALSE;
+	/* Let unrealized devices try to realize themselves with the link */
+	for (iter = NM_MANAGER_GET_PRIVATE (self)->devices; iter; iter = iter->next) {
+		NMDevice *candidate = iter->data;
+		gboolean compatible = TRUE;
 
-		if (nm_device_is_real (device))
-			return;
+		if (strcmp (nm_device_get_iface (candidate), plink->name))
+			continue;
 
-		if (nm_device_realize (device, plink, &compatible, &error)) {
+		if (nm_device_is_real (candidate)) {
+			/* Ignore the link added event since there's already a realized
+			 * device with the link's name.
+			 */
+			return;
+		} else if (nm_device_realize (candidate, plink, &compatible, &error)) {
 			/* Success */
-			nm_device_setup_finish (device, plink);
+			nm_device_setup_finish (candidate, plink);
 			return;
 		}
 
-		nm_log_warn (LOGD_DEVICE, "(%s): %s", plink->name, error->message);
-		remove_device (self, device, FALSE, FALSE);
+		nm_log_dbg (LOGD_DEVICE, "(%s): failed to realize from plink: '%s'",
+		            plink->name, error->message);
 		g_clear_error (&error);
 
-		if (compatible) {
-			/* Device compatible with platform link, but some other fatal error
-			 * happened during realization.
-			 */
-			return;
-		}
-
-		/* Fall through and create new compatible device for the link */
+		/* Try next unrealized device */
 	}
 
 	/* Try registered device factories */
@@ -2245,7 +2281,7 @@ find_master (NMManager *self,
 		return TRUE;  /* success, but no master */
 
 	/* Try as an interface name first */
-	master_device = find_device_by_iface (self, master);
+	master_device = find_device_by_iface (self, master, NULL, connection);
 	if (master_device) {
 		if (master_device == device) {
 			g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_DEPENDENCY_FAILED,
@@ -2620,8 +2656,10 @@ _internal_activate_device (NMManager *self, NMActiveConnection *active, GError *
 	NMConnection *applied;
 	NMSettingsConnection *connection;
 	NMSettingsConnection *master_connection = NULL;
+	NMConnection *existing_connection = NULL;
 	NMActiveConnection *master_ac = NULL;
-	GError *local_err = NULL;
+	NMAuthSubject *subject;
+	char *error_desc = NULL;
 
 	g_return_val_if_fail (NM_IS_MANAGER (self), FALSE);
 	g_return_val_if_fail (NM_IS_ACTIVE_CONNECTION (active), FALSE);
@@ -2635,72 +2673,26 @@ _internal_activate_device (NMManager *self, NMActiveConnection *active, GError *
 	applied = nm_active_connection_get_applied_connection (active);
 
 	device = nm_active_connection_get_device (active);
-	if (!device) {
-		if (!nm_connection_is_virtual (applied)) {
-			NMSettingConnection *s_con = nm_connection_get_setting_connection (applied);
-
-			g_assert (s_con);
-			g_set_error (error,
-			             NM_MANAGER_ERROR,
-			             NM_MANAGER_ERROR_UNKNOWN_DEVICE,
-			             "Unsupported virtual interface type '%s'",
-			             nm_setting_connection_get_connection_type (s_con));
-			return FALSE;
-		}
-
-		device = system_create_virtual_device (self, applied, &local_err);
-		if (!device) {
-			g_set_error (error,
-			             NM_MANAGER_ERROR,
-			             NM_MANAGER_ERROR_UNKNOWN_DEVICE,
-			             "Failed to create virtual interface: %s",
-			             local_err ? local_err->message : "(unknown)");
-			g_clear_error (&local_err);
-			return FALSE;
-		}
+	g_return_val_if_fail (device != NULL, FALSE);
 
-		if (!nm_active_connection_set_device (active, device)) {
-			g_set_error_literal (error,
-			                     NM_MANAGER_ERROR,
-			                     NM_MANAGER_ERROR_UNKNOWN_DEVICE,
-			                     "The device could not be activated with this connection");
-			return FALSE;
-		}
-
-		/* A newly created device, if allowed to be managed by NM, will be
-		 * in the UNAVAILABLE state here.  To ensure it can be activated
-		 * immediately, we transition it to DISCONNECTED.
-		 */
-		if (   nm_device_is_available (device, NM_DEVICE_CHECK_DEV_AVAILABLE_NONE)
-			&& (nm_device_get_state (device) == NM_DEVICE_STATE_UNAVAILABLE)) {
-			nm_device_state_changed (device,
-			                         NM_DEVICE_STATE_DISCONNECTED,
-			                         NM_DEVICE_STATE_REASON_NONE);
-		}
-	} else {
-		NMConnection *existing_connection = NULL;
-		NMAuthSubject *subject;
-		char *error_desc = NULL;
-
-		/* If the device is active and its connection is not visible to the
-		 * user that's requesting this new activation, fail, since other users
-		 * should not be allowed to implicitly deactivate private connections
-		 * by activating a connection of their own.
-		 */
-		existing_connection = nm_device_get_applied_connection (device);
-		subject = nm_active_connection_get_subject (active);
-		if (existing_connection &&
-		    !nm_auth_is_subject_in_acl (existing_connection,
-			                            subject,
-			                            &error_desc)) {
-			g_set_error (error,
-			             NM_MANAGER_ERROR,
-			             NM_MANAGER_ERROR_PERMISSION_DENIED,
-			             "Private connection already active on the device: %s",
-			             error_desc);
-			g_free (error_desc);
-			return FALSE;
-		}
+	/* If the device is active and its connection is not visible to the
+	 * user that's requesting this new activation, fail, since other users
+	 * should not be allowed to implicitly deactivate private connections
+	 * by activating a connection of their own.
+	 */
+	existing_connection = nm_device_get_applied_connection (device);
+	subject = nm_active_connection_get_subject (active);
+	if (existing_connection &&
+	    !nm_auth_is_subject_in_acl (existing_connection,
+	                                subject,
+	                                &error_desc)) {
+		g_set_error (error,
+		             NM_MANAGER_ERROR,
+		             NM_MANAGER_ERROR_PERMISSION_DENIED,
+		             "Private connection already active on the device: %s",
+		             error_desc);
+		g_free (error_desc);
+		return FALSE;
 	}
 
 	/* Final connection must be available on device */
@@ -2777,9 +2769,10 @@ _internal_activate_device (NMManager *self, NMActiveConnection *active, GError *
 		}
 
 		nm_active_connection_set_master (active, master_ac);
-		nm_log_dbg (LOGD_CORE, "Activation of '%s' depends on active connection %p",
+		nm_log_dbg (LOGD_CORE, "Activation of '%s' depends on active connection %p %s",
 		            nm_settings_connection_get_id (connection),
-		            master_ac);
+		            master_ac,
+		            str_if_set (nm_exported_object_get_path (NM_EXPORTED_OBJECT  (master_ac)), ""));
 	}
 
 	/* Check slaves for master connection and possibly activate them */
@@ -2790,6 +2783,19 @@ _internal_activate_device (NMManager *self, NMActiveConnection *active, GError *
 	if (existing)
 		nm_device_steal_connection (existing, connection);
 
+	if (nm_device_get_state (device) == NM_DEVICE_STATE_UNMANAGED) {
+		nm_device_state_changed (device,
+		                         NM_DEVICE_STATE_UNAVAILABLE,
+		                         NM_DEVICE_STATE_REASON_USER_REQUESTED);
+	}
+
+	if (   nm_device_is_available (device, NM_DEVICE_CHECK_DEV_AVAILABLE_NONE)
+	    && (nm_device_get_state (device) == NM_DEVICE_STATE_UNAVAILABLE)) {
+		nm_device_state_changed (device,
+		                         NM_DEVICE_STATE_DISCONNECTED,
+		                         NM_DEVICE_STATE_REASON_USER_REQUESTED);
+	}
+
 	/* Export the new ActiveConnection to clients and start it on the device */
 	nm_exported_object_export (NM_EXPORTED_OBJECT (active));
 	g_object_notify (G_OBJECT (self), NM_MANAGER_ACTIVE_CONNECTIONS);
@@ -3042,6 +3048,23 @@ nm_manager_activate_connection (NMManager *self,
 	return active;
 }
 
+/**
+ * validate_activation_request:
+ * @self: the #NMManager
+ * @context: the D-Bus context of the requestor
+ * @connection: the partial or complete #NMConnection to be activated
+ * @device_path: the object path of the device to be activated, or "/"
+ * @out_device: on successful reutrn, the #NMDevice to be activated with @connection
+ * @out_vpn: on successful return, %TRUE if @connection is a VPN connection
+ * @error: location to store an error on failure
+ *
+ * Performs basic validation on an activation request, including ensuring that
+ * the requestor is a valid Unix process, is not disallowed in @connection
+ * permissions, and that a device exists that can activate @connection.
+ *
+ * Returns: on success, the #NMAuthSubject representing the requestor, or
+ *   %NULL on error
+ */
 static NMAuthSubject *
 validate_activation_request (NMManager *self,
                              GDBusMethodInvocation *context,
@@ -3116,11 +3139,11 @@ validate_activation_request (NMManager *self,
 	} else
 		device = nm_manager_get_best_device_for_connection (self, connection, TRUE);
 
-	if (!device) {
+	if (!device && !vpn) {
 		gboolean is_software = nm_connection_is_virtual (connection);
 
 		/* VPN and software-device connections don't need a device yet */
-		if (!vpn && !is_software) {
+		if (!is_software) {
 			g_set_error_literal (error,
 			                     NM_MANAGER_ERROR,
 			                     NM_MANAGER_ERROR_UNKNOWN_DEVICE,
@@ -3136,11 +3159,19 @@ validate_activation_request (NMManager *self,
 			if (!iface)
 				goto error;
 
-			device = find_device_by_iface (self, iface);
+			device = find_device_by_iface (self, iface, connection, NULL);
 			g_free (iface);
 		}
 	}
 
+	if ((!vpn || device_path) && !device) {
+		g_set_error_literal (error,
+		                     NM_MANAGER_ERROR,
+		                     NM_MANAGER_ERROR_UNKNOWN_DEVICE,
+		                     "Failed to find a compatible device for this connection");
+		goto error;
+	}
+
 	*out_device = device;
 	*out_vpn = vpn;
 	return subject;
@@ -3454,14 +3485,6 @@ impl_manager_add_and_activate_connection (NMManager *self,
 	if (!subject)
 		goto error;
 
-	/* AddAndActivate() requires a device to complete the connection with */
-	if (!device) {
-		error = g_error_new_literal (NM_MANAGER_ERROR,
-		                             NM_MANAGER_ERROR_UNKNOWN_DEVICE,
-		                             "This connection requires an existing device.");
-		goto error;
-	}
-
 	all_connections = nm_settings_get_connections (priv->settings);
 	if (vpn) {
 		/* Try to fill the VPN's connection setting and name at least */