GNOME Bugzilla – Bug 730492
[review] lr/conn-reactivation-bgo730492: NM should allow re-activation of already active connections
Last modified: 2015-04-03 17:06:39 UTC
When activating (`nmcli con up`) an already activated connection, the behavior is inconsistent. Scenario 1: Have two disconnected (ethernet) interfaces 'eth1' and 'eth2', with no other autoconnect connections. $ nmcli connection add type ethernet con-name test ifname '*' This (arbitrarily) auto-connects 'test' on 'eth1' Then: $ nmcli connection up id test Error: Connection activation failed: Connection 'test' is already active on eth1 $ nmcli connection up id test ifname eth2 Error: Connection activation failed: Connection 'test' is already active on eth1 $ nmcli connection up id test ifname eth1 Success Scenario 2: if you only have eth1, then both $ nmcli connection up id test $ nmcli connection up id test ifname eth1 succeed The above behavior is inconsitent. Following things should be fixed: 1) currently, NMManager needs a device when activating a connection (line src/nm-manager.c:2908). NM should allow activation without providing a device and NM should find the best device itself. If the connection is already active on a device, that device should be preferred, so that we re-activate the connection on the very same device. 2) currently, if you don't specify the 'ifname' in nmcli, nmcli will guess a device (cli/src/connections.c:1886). With 1), nmcli should leave the device unspecified and leave the decision to NM. 3) currently NM allows re-activation of a connection only on the same device and fails with "Error: Connection activation failed: Connection 'test' is already active on eth1" Instead NM should always down the connection before activating it again (on a potentially different device). (above line number relative to commit 0d09472e4585bd097b4551b3c086aee7bf329279)
http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/conn-reactivation-bgo730492 First two points addressed, third not yet. Still, adding the nm-review blocker, so that at least they can be reviewed now.
maybe rename nm_manager_get_connection_best_device()? Something like nm_manager_get_best_connection_for_device()? otherwise LGTM
(In reply to comment #2) > maybe rename nm_manager_get_connection_best_device()? Something like > nm_manager_get_best_connection_for_device()? Done. > otherwise LGTM I've also added the last bits and reorganized it a bit: > 0adc80f manager: remove a connection from device if we're activating it on another device Split off the part that looks for a device the connection is active on so that it can be reused for the third patch below. > 4bd7a8d manager: pick an available device if none was specified upon connection activation Remaining half; did the rename that was suggested. > 0a6637b manager: reuse a device conneciton is active on if none was given upon activation Added. > 176db49 cli: don't look up a device for activation request unless we have to Stays the same.
Sorry, messed up the commits and comment; please disregard the above. Another attempt: (In reply to comment #2) > maybe rename nm_manager_get_connection_best_device()? Something like > nm_manager_get_best_connection_for_device()? Done. > otherwise LGTM I've also added the last bits and reorganized it a bit: > 0a6637b manager: reuse a device conneciton is active on if none was given upon activation Split off the part that looks for a device the connection is active on so that it can be reused for the third patch below. > 4bd7a8d manager: pick an available device if none was specified upon connection activation Remaining half; did the rename that was suggested. > 0adc80f manager: remove a connection from device if we're activating it on another device Added. > 176db49 cli: don't look up a device for activation request unless we have to Stays the same.
*** Bug 743187 has been marked as a duplicate of this bug. ***
>> manager: remove a connection from device if we're activating it on another device + existing = nm_manager_get_connection_device (self, connection); + if (existing) { + /* Deactivate existing activation request first */ + nm_log_dbg (LOGD_CORE, "disconnecting for new activation request."); + nm_device_state_changed (existing, + NM_DEVICE_STATE_DEACTIVATING, + NM_DEVICE_STATE_REASON_NONE); + } I think this is not entirely correct. The connection could not be the currently active connection, but priv->queued_act_request. In that case, you just want to unqueue.
(In reply to Thomas Haller from comment #6) > >> manager: remove a connection from device if we're activating it on another device > > > + existing = nm_manager_get_connection_device (self, connection); > + if (existing) { > + /* Deactivate existing activation request first */ > + nm_log_dbg (LOGD_CORE, "disconnecting for new activation > request."); > + nm_device_state_changed (existing, > + NM_DEVICE_STATE_DEACTIVATING, > + NM_DEVICE_STATE_REASON_NONE); > + } > > I think this is not entirely correct. The connection could not be the > currently active connection, but priv->queued_act_request. In that case, you > just want to unqueue. You're right. Pushed an updated branch.
> manager: reuse a device conneciton is active on if none was given upon activation "conneciton" -> "the connection" > manager: pick an available device if none was specified upon connection activation Some whitespace issues here. More later.
>> manager: remove a connection from device if we're activating it on another device + /* Disconnect the connection if connected or queued on another device */ + existing = nm_manager_get_connection_device (self, connection); + if (existing) + nm_device_deactivate (existing); if @connection is only queued on @existing for activation, it should not deactivate the (unrelated) active connection. And if there is another (unrelated) connection queued to be activated, that one should not be deactivated. nm_device_deactivate (NMDevice *self, NMConnection *connection) { if (priv->queued_act_request && connection == nm_active_connection_get_connection (priv->queued_act_request)) _clear_queued_act_request (priv); if (priv->act_request && connection == nm_active_connection_get_connection (priv->act_request)) { _LOGI (LOGD_DEVICE, "disconnecting for new activation request."); nm_device_state_changed (self, NM_DEVICE_STATE_DEACTIVATING, NM_DEVICE_STATE_REASON_USER_REQUESTED); } } maybe also something like: if (priv->state < NM_DEVICE_STATE_DEACTIVATING) before nm_device_state_changed()
(In reply to Thomas Haller from comment #9) > >> manager: remove a connection from device if we're activating it on another device > > + /* Disconnect the connection if connected or queued on another device */ > + existing = nm_manager_get_connection_device (self, connection); > + if (existing) > + nm_device_deactivate (existing); > > > if @connection is only queued on @existing for activation, it should not > deactivate the (unrelated) active connection. And if there is another > (unrelated) connection queued to be activated, that one should not be > deactivated. ... > maybe also something like: > if (priv->state < NM_DEVICE_STATE_DEACTIVATING) > before nm_device_state_changed() Yes, both make sense to me. Integrated here: lr/conn-reactivation-bgo730492
>> manager: pick an available device if none was specified upon connection + if (nm_device_check_connection_available (device, connection, NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST, NULL)); how about passing flags: (NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST & ~_NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_WAITING_CARRIER) on an explicit user request (FOR_USER_REQUEST), a device is also available if it is still waiting for carrier. But here, when the device is not explicitly given, we should skip devices without carrier (if the connection requires carrier). Otherwise, it looks right to me. (but I didn't test it)
(In reply to Thomas Haller from comment #11) > >> manager: pick an available device if none was specified upon connection > > + if (nm_device_check_connection_available (device, connection, > NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST, NULL)); > > how about passing flags: > > (NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST & > ~_NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_WAITING_CARRIER) > > > on an explicit user request (FOR_USER_REQUEST), a device is also available > if it is still waiting for carrier. But here, when the device is not > explicitly given, we should skip devices without carrier (if the connection > requires carrier). > > > Otherwise, it looks right to me. (but I didn't test it) It seems a bit weird to first NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST which or-s _NM_DEVICE_CHECK_CON_AVAILABLE_FOR_USER_REQUEST_WAITING_CARRIER and then remove it. However it in fact seems that a NM_DEVICE_CHECK_CON_AVAILABLE_NONE is a better pick anyway -- we don't want to guess devices for hidden networks anyway. Pushed an updated branch.
>> dbus-manager: disconnect thre private server before destroying it ^^^^ about >> manager: remove a connection from device if we're activating it on another I cannot confidently say that it is correct (but I don't see anything wrong with it). otherwise LGTM
Merged. 3535702 device,cli: allow re-activation of already active connections (bgo #730492) 0e8a14c cli: don't look up a device for activation request unless we have to 4cb97cf manager: remove a connection from device if we're activating it on another device 6fc3736 manager: pick an available device if none was specified upon connection activation 6e94f30 manager: reuse a device connection is active on if none was given upon activation