core: merge branch 'th/device-availability'

https://bugzilla.redhat.com/show_bug.cgi?id=1639254

(cherry picked from commit 9cf64e2ee23fc1bebe13196460a78a4ee8651ade)

3 files changed, 99 insertions, 19 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 1845c8bccb..9ca585e6d2 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -13544,6 +13544,19 @@ nm_device_update_metered (NMDevice *self)
 	}
 }
 
+static NMDeviceCheckDevAvailableFlags
+_device_check_dev_available_flags_from_con (NMDeviceCheckConAvailableFlags con_flags)
+{
+	NMDeviceCheckDevAvailableFlags dev_flags;
+
+	dev_flags = NM_DEVICE_CHECK_DEV_AVAILABLE_NONE;
+
+	if (NM_FLAGS_HAS (con_flags, _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_WAITING_CARRIER))
+		dev_flags |= _NM_DEVICE_CHECK_DEV_AVAILABLE_IGNORE_CARRIER;
+
+	return dev_flags;
+}
+
 static gboolean
 _nm_device_check_connection_available (NMDevice *self,
                                        NMConnection *connection,
@@ -13585,34 +13598,30 @@ _nm_device_check_connection_available (NMDevice *self,
 		return FALSE;
 	}
 	if (state < NM_DEVICE_STATE_UNAVAILABLE) {
-		if (NM_FLAGS_ANY (flags, NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST)) {
-			if (!nm_device_get_managed (self, TRUE)) {
+		if (!nm_device_get_managed (self, TRUE)) {
+			if (!nm_device_get_managed (self, FALSE)) {
 				nm_utils_error_set_literal (error, NM_UTILS_ERROR_CONNECTION_AVAILABLE_UNMANAGED_DEVICE,
-				                            "device is unmanaged");
+				                            "device is strictly unmanaged");
 				return FALSE;
 			}
-		} else {
-			if (!nm_device_get_managed (self, FALSE)) {
+			if (!NM_FLAGS_HAS (flags, _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_OVERRULE_UNMANAGED)) {
 				nm_utils_error_set_literal (error, NM_UTILS_ERROR_CONNECTION_AVAILABLE_UNMANAGED_DEVICE,
-				                            "device is unmanaged for interal request");
+				                            "device is currently unmanaged");
 				return FALSE;
 			}
 		}
 	}
 	if (   state < NM_DEVICE_STATE_DISCONNECTED
 	    && !nm_device_is_software (self)) {
-		if (NM_FLAGS_ANY (flags, NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST)) {
-			if (!nm_device_is_available (self, NM_DEVICE_CHECK_DEV_AVAILABLE_FOR_USER_REQUEST)) {
+		if (!nm_device_is_available (self, _device_check_dev_available_flags_from_con (flags))) {
+			if (NM_FLAGS_HAS (flags, _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST)) {
 				nm_utils_error_set_literal (error, NM_UTILS_ERROR_CONNECTION_AVAILABLE_TEMPORARY,
 				                            "device is not available");
-				return FALSE;
-			}
-		} else {
-			if (!nm_device_is_available (self, NM_DEVICE_CHECK_DEV_AVAILABLE_NONE)) {
+			} else {
 				nm_utils_error_set_literal (error, NM_UTILS_ERROR_CONNECTION_AVAILABLE_TEMPORARY,
 				                            "device is not available for internal request");
-				return FALSE;
 			}
+			return FALSE;
 		}
 	}
 
diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h
index 6342a657b4..8f376b20b4 100644
--- a/src/devices/nm-device.h
+++ b/src/devices/nm-device.h
@@ -167,14 +167,34 @@ typedef enum NMActStageReturn NMActStageReturn;
 typedef enum { /*< skip >*/
 	NM_DEVICE_CHECK_CON_AVAILABLE_NONE                                  = 0,
 
+	/* since NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST is a collection of flags with more fine grained
+	 * parts, this flag in general indicates that this is a user-request. */
 	_NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST                     = (1L << 0),
+
+	/* we also consider devices which have no carrier but are still waiting for the driver
+	 * to detect carrier. Usually, such devices are not yet available, however for a user-request
+	 * they are. They might fail later if carrier doesn't come. */
 	_NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_WAITING_CARRIER     = (1L << 1),
+
+	/* usually, a profile is only available if the Wi-Fi AP is in range. For an
+	 * explicit user request, we also consider profiles for APs that are not (yet)
+	 * visible. */
 	_NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_IGNORE_AP           = (1L << 2),
+
+	/* a device can be marked as unmanaged for various reasons. Some of these reasons
+	 * are authorative, others not. Non-authoritative reasons can be overruled by
+	 * `nmcli device set $DEVICE managed yes`. Also, for an explicit user activation
+	 * request we may want to consider the device as managed. This flag makes devices
+	 * that are unmanaged appear available. */
+	_NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_OVERRULE_UNMANAGED  = (1L << 3),
+
+	/* a collection of flags, that are commonly set for an explict user-request. */
 	NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST                      = _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST
 	                                                                    | _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_WAITING_CARRIER
-	                                                                    | _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_IGNORE_AP,
+	                                                                    | _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_IGNORE_AP
+	                                                                    | _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_OVERRULE_UNMANAGED,
 
-	NM_DEVICE_CHECK_CON_AVAILABLE_ALL                                   = (1L << 3) - 1,
+	NM_DEVICE_CHECK_CON_AVAILABLE_ALL                                   = (1L << 4) - 1,
 } NMDeviceCheckConAvailableFlags;
 
 struct _NMDevicePrivate;
@@ -190,7 +210,12 @@ struct _NMDevice {
 typedef enum { /*< skip >*/
 	NM_DEVICE_CHECK_DEV_AVAILABLE_NONE                                  = 0,
 
+	/* the device is considered available, even if it has no carrier.
+	 *
+	 * For various device types (software devices) we ignore carrier based
+	 * on the type. So, for them, this flag has no effect anyway. */
 	_NM_DEVICE_CHECK_DEV_AVAILABLE_IGNORE_CARRIER                       = (1L << 0),
+
 	NM_DEVICE_CHECK_DEV_AVAILABLE_FOR_USER_REQUEST                      = _NM_DEVICE_CHECK_DEV_AVAILABLE_IGNORE_CARRIER,
 
 	NM_DEVICE_CHECK_DEV_AVAILABLE_ALL                                   = (1L << 1) - 1,
diff --git a/src/nm-manager.c b/src/nm-manager.c
index 47b7365c6d..52e490c81b 100644
--- a/src/nm-manager.c
+++ b/src/nm-manager.c
@@ -3385,6 +3385,7 @@ nm_manager_get_best_device_for_connection (NMManager *self,
 	NMDeviceCheckConAvailableFlags flags;
 	gs_unref_ptrarray GPtrArray *all_ac_arr = NULL;
 	gs_free_error GError *local_best = NULL;
+	NMConnectionMultiConnect multi_connect;
 
 	nm_assert (!sett_conn || NM_IS_SETTINGS_CONNECTION (sett_conn));
 	nm_assert (!connection || NM_IS_CONNECTION (connection));
@@ -3394,9 +3395,44 @@ nm_manager_get_best_device_for_connection (NMManager *self,
 	if (!connection)
 		connection = nm_settings_connection_get_connection (sett_conn);
 
-	flags = for_user_request ? NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST : NM_DEVICE_CHECK_CON_AVAILABLE_NONE;
+	multi_connect =  _nm_connection_get_multi_connect (connection);
 
-	if (   _nm_connection_get_multi_connect (nm_settings_connection_get_connection (sett_conn)) == NM_CONNECTION_MULTI_CONNECT_SINGLE
+	if (!for_user_request)
+		flags = NM_DEVICE_CHECK_CON_AVAILABLE_NONE;
+	else {
+		/* if the profile is multi-connect=single, we also consider devices which
+		 * are marked as unmanaged. And explicit user-request shows sufficent user
+		 * intent to make the device managed.
+		 * That is also, because we expect that such profile is suitably tied
+		 * to the intended device. So when an unmanaged device matches, the user's
+		 * intent is clear.
+		 *
+		 * For multi-connect != single devices that is different. The profile
+		 * is not restricted to a particular device.
+		 * For that reason, plain `nmcli connection up "$MULIT_PROFILE"` seems
+		 * less suitable for multi-connect profiles, because the target device is
+		 * left unspecified. Anyway, if a user issues
+		 *
+		 *   $ nmcli device set "$DEVICE" managed no
+		 *   $ nmcli connection up "$MULIT_PROFILE"
+		 *
+		 * then it is reasonable for multi-connect profiles to not consider
+		 * the device a suitable candidate.
+		 *
+		 * This may be seen inconsistent, but I think that it makes a lot of
+		 * sense. Also note that "connection.multi-connect" work quite differently
+		 * in aspects like activation. E.g. `nmcli connection up` of multi-connect
+		 * "single" profile, will deactivate the profile if it is active already.
+		 * That is different from multi-connect profiles, where it will aim to
+		 * activate the profile one more time on an hitherto disconnected device.
+		 */
+		if (multi_connect == NM_CONNECTION_MULTI_CONNECT_SINGLE)
+			flags = NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST;
+		else
+			flags = NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST & ~_NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_OVERRULE_UNMANAGED;
+	}
+
+	if (   multi_connect == NM_CONNECTION_MULTI_CONNECT_SINGLE
 	    && (ac = active_connection_find_by_connection (self, sett_conn, connection, NM_ACTIVE_CONNECTION_STATE_DEACTIVATING, &all_ac_arr))) {
 		/* if we have a profile which may activate on only one device (multi-connect single), then
 		 * we prefer the device on which the profile is already active. It means to reactivate
@@ -3483,13 +3519,23 @@ found_better:
 
 		/* determine the priority of this device. Currently this priority is independent
 		 * of the profile (connection) and the device's details (aside the state).
+		 *
 		 * Maybe nm_device_check_connection_available() should instead return a priority,
-		 * as it has more information available. For now, that is not needed nor implemented. */
+		 * as it has more information available.
+		 *
+		 * For example, if you have multiple Wi-Fi devices, currently a user-request would
+		 * also select the device if the AP is not visible. Optimally, if one of the two
+		 * devices sees the AP and the other one doesn't, the former would be preferred.
+		 * For that, the priority would need to be determined by nm_device_check_connection_available(). */
 		prio = _device_get_activation_prio (device);
 		if (   prio <= best.prio
 		    && best.device) {
 			/* we already have a matching device with a better priority. This candidate
-			 * cannot be better. Skip the check. */
+			 * cannot be better. Skip the check.
+			 *
+			 * Also note, that below we collect the best error message @local_best.
+			 * Since we already have best.device, the error message does not matter
+			 * either, and we can skip nm_device_check_connection_available() altogether. */
 			continue;
 		}