After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 730492 - [review] lr/conn-reactivation-bgo730492: NM should allow re-activation of already active connections
[review] lr/conn-reactivation-bgo730492: NM should allow re-activation of alr...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Lubomir Rintel
NetworkManager maintainer(s)
: 743187 (view as bug list)
Depends on:
Blocks: nm-review
 
 
Reported: 2014-05-21 07:19 UTC by Thomas Haller
Modified: 2015-04-03 17:06 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Thomas Haller 2014-05-21 07:19:33 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)
Comment 1 Lubomir Rintel 2015-02-03 11:29:02 UTC
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.
Comment 2 Thomas Haller 2015-02-03 16:04:48 UTC
maybe rename nm_manager_get_connection_best_device()? Something like nm_manager_get_best_connection_for_device()?



otherwise LGTM
Comment 3 Lubomir Rintel 2015-02-03 17:05:33 UTC
(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.
Comment 4 Lubomir Rintel 2015-02-03 17:07:26 UTC
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.
Comment 5 Lubomir Rintel 2015-02-07 16:43:43 UTC
*** Bug 743187 has been marked as a duplicate of this bug. ***
Comment 6 Thomas Haller 2015-02-24 10:27:36 UTC
>> 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.
Comment 7 Lubomir Rintel 2015-02-24 17:57:15 UTC
(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.
Comment 8 Dan Williams 2015-02-28 15:18:50 UTC
> 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.
Comment 9 Thomas Haller 2015-03-02 10:08:21 UTC
>> 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()
Comment 10 Lubomir Rintel 2015-03-20 18:22:33 UTC
(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
Comment 11 Thomas Haller 2015-03-20 20:15:19 UTC
>> 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)
Comment 12 Lubomir Rintel 2015-03-31 13:25:37 UTC
(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.
Comment 13 Thomas Haller 2015-04-01 09:41:15 UTC
>> 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
Comment 14 Lubomir Rintel 2015-04-03 17:06:39 UTC
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