Re: [PATCH] manager: fix assumption of child connections with autoconnect=no



On Tue, 2016-03-08 at 12:02 +0100, Beniamino Galvani wrote:
During startup, when a link is detected (enp0s25 in the example
below)
we try to create also virtual devices (ipip1) on it through
system_create_virtual_device(), however this realizes only devices
for
connections which can autoactivate.

To support the assumption of child devices with autoconnect=no, we
should take in consideration in retry_connections_for_parent_device()
only connections for which the link does not exist, and let existing
links be handled by platform_link_added(), which also realizes them.

Reproducer:
 $ nmcli c add type ip-tunnel ifname ipip1 con-name ipip1+
autoconnect no \
               mode ipip remote 172.25.16.1 dev enp0s25 ip4
1.2.3.4/31
 $ nmcli c up ipip1+
 $ systemctl restart NetworkManager

Result:
 * before: ipip1+ is not assumed, ipip1 is not present in 'nmcli d'
output
 * after:  ipip1+ is assumed, ipip1 detected
---
 src/nm-manager.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/nm-manager.c b/src/nm-manager.c
index e7eb7c6..625a235 100644
--- a/src/nm-manager.c
+++ b/src/nm-manager.c
@@ -1127,23 +1127,31 @@ system_create_virtual_device (NMManager
*self, NMConnection *connection)
 
 static void
 retry_connections_for_parent_device (NMManager *self, NMDevice
*device)
 {
      NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self);
      GSList *connections, *iter;
+     gs_free_error GError *error = NULL;
+     gs_free char *ifname = NULL;

should be declared inside the loop, otherwise as you iterate they get
reset and leak (or fail assertion g_return_if_fail (!error ||
!*error)).

 
      g_return_if_fail (device);
 
      connections = nm_settings_get_connections (priv->settings);
      for (iter = connections; iter; iter = g_slist_next (iter)) {
              NMConnection *candidate = iter->data;
              NMDevice *parent;
 
              parent = find_parent_device_for_connection (self,
candidate, NULL);
-             if (parent == device)
-                     connection_changed (priv->settings,
candidate, self);
+             if (parent == device) {
+                     /* Only try to activate devices that don't
already exist */
+                     ifname = nm_manager_get_connection_iface
(self, candidate, &parent, &error);
+                     if (ifname) {
+                             if (!nm_platform_link_get_by_ifname
(NM_PLATFORM_GET, ifname))
+                                     connection_changed (priv-
settings, candidate, self);
+                     }
+             }
      }
 
      g_slist_free (connections);
 }
 
 static void


otherwise, lgtm.

Thomas

Attachment: signature.asc
Description: This is a digitally signed message part



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]