From 45913c11df17ced27996eb369414c8a827b3ca23 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 17 Oct 2018 12:21:33 +0200 Subject: [PATCH 1/6] core: fix checking multi-connect flag in nm_manager_get_best_device_for_connection() We should not check @sett_conn, but @connection. Fixes: 09719bc479b63c8e5fef3950e980b263aca7eff5 --- src/nm-manager.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 9ea83ef8db..6f7b456c77 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -3394,6 +3394,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)); @@ -3405,7 +3406,9 @@ nm_manager_get_best_device_for_connection (NMManager *self, flags = for_user_request ? NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST : NM_DEVICE_CHECK_CON_AVAILABLE_NONE; - if ( _nm_connection_get_multi_connect (nm_settings_connection_get_connection (sett_conn)) == NM_CONNECTION_MULTI_CONNECT_SINGLE + multi_connect = _nm_connection_get_multi_connect (connection); + + 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 From c37b028abad9105525a5f12dd833fe6592cd0b95 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 17 Oct 2018 12:21:49 +0200 Subject: [PATCH 2/6] core/trivial: add code comment for nm_manager_get_best_device_for_connection() --- src/nm-manager.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 6f7b456c77..2714690b6b 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -3495,13 +3495,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; } From e6523fbbbcbf348a803cb202e433f9b7eb1ffe91 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 17 Oct 2018 12:13:49 +0200 Subject: [PATCH 3/6] core/trivial: add code comment for NMDeviceCheckDevAvailableFlags and NMDeviceCheckConAvailableFlags --- src/devices/nm-device.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index 199ef51b0b..bd7af87fd6 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -168,9 +168,21 @@ 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 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, @@ -191,7 +203,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, From 5412fd389b051bc3598c08b9d0eb63db4f2af45d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 17 Oct 2018 12:02:51 +0200 Subject: [PATCH 4/6] device: cleanup checking device avilability for ignoring carrier The flags NMDeviceCheckConAvailableFlags and NMDeviceCheckDevAvailableFlags both control whether a device appears available (either, available in general, or related to a particular profile). Also, both flag types strictly increase availability. Meaning: more flags, more available. There is some overlap between the flags, however they still have their own distinct parts. Improve the mapping from NMDeviceCheckConAvailableFlags to NMDeviceCheckDevAvailableFlags, by picking exactly the flags that are relevant. --- src/devices/nm-device.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index ce21803bfa..5057f7967e 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -13615,6 +13615,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, @@ -13672,18 +13685,15 @@ _nm_device_check_connection_available (NMDevice *self, } 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; } } From 920346a5b98c875161116990f1d5d4178df31ba5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 17 Oct 2018 12:26:35 +0200 Subject: [PATCH 5/6] device: add and use overrule-unmanaged flag for nm_device_check_connection_available() This flag is more granular in whether to consider the connection available or not. We probably should never check for the combined flag NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST directly, but always explicitly for the relevant parts. Also, improve the error message, to indicate whether the device is strictly unmanaged or whether it could be overruled. --- src/devices/nm-device.c | 15 +++++++-------- src/devices/nm-device.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 5057f7967e..1a7b90cb57 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -13669,16 +13669,15 @@ _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)) { - nm_utils_error_set_literal (error, NM_UTILS_ERROR_CONNECTION_AVAILABLE_UNMANAGED_DEVICE, - "device is unmanaged"); - return FALSE; - } - } else { + 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 for internal request"); + "device is strictly unmanaged"); + return 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 currently unmanaged"); return FALSE; } } diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index bd7af87fd6..bf867f4852 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -182,12 +182,20 @@ typedef enum { /*< skip >*/ * 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; From 0cb8bed23c22b01cce38e03d14ce29e5f73c28d2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 17 Oct 2018 11:33:02 +0200 Subject: [PATCH 6/6] core: ignore unmanaged devices for explicit activation request depending on multi-connect When a device is unmanaged, an explicit activation request can still activate it. In particular, that is the case for $ nmcli connection up "$PROFILE" ifname "$DEVICE" It is also the case, for plain $ nmcli connection up "$PROFILE" where NetworkManager searches for a suitable device -- depending on multi-connect setting of the profile. The idea is, that a profile with "multi-connect=single" is expected to sufficently and uniquely match a device, based on matching properties like "connection.interface-name". In that case, an explicit activation request from the user shows the intent to manage the device. Note that it's hard to understand whether the profile really uniquely selects a particular device. For example, if the profile doesn't specify "connection.interface-name", it might still uniquely identify an ethernet device, if you only have one such device. On the other hand, with "connection.multi-connect" other than "single", it is very much expected that the profile does not strictly match one device. Change the behavior here for multi-connect profiles. This allows the user to block individual devices from activation via $ nmcli device set "$DEVICE" managed not A subsequent $ nmcli connection up "$MULTI_PROFILE" will not consider "$DEVICE" as suitable candidate for activation. Likewise, in the future we may want to add a $ nmcli connection up --all "$MULTI_PROFILE" command, to activate the profile on all suitable device. In that case again, unmanaged devices probably also should be skipped for multi-connect profiles. https://bugzilla.redhat.com/show_bug.cgi?id=1639254 --- src/nm-manager.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 2714690b6b..d2fad95519 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -3404,10 +3404,43 @@ 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 (!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