all: merge branch 'th/various-settings-cleanup-4'

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/192

shared/nm-glib-aux/nm-c-list.h

@@ -67,6 +67,18 @@ nm_c_list_elem_free (NMCListElem *elem)
 	return nm_c_list_elem_free_full (elem, NULL);
 }
 
+static inline void *
+nm_c_list_elem_free_steal (NMCListElem *elem)
+{
+	gpointer data;
+
+	if (!elem)
+		return NULL;
+	data = elem->data;
+	nm_c_list_elem_free_full (elem, NULL);
+	return data;
+}
+
 static inline void
 nm_c_list_elem_free_all (CList *head, GDestroyNotify free_fcn)
 {

src/devices/nm-device-bond.c

@@ -413,6 +413,12 @@ release_slave (NMDevice *device,
 	NMDeviceBond *self = NM_DEVICE_BOND (device);
 	gboolean success;
 	gs_free char *address = NULL;
+	int ifindex_slave;
+
+	ifindex_slave = nm_device_get_ip_ifindex (slave);
+
+	if (ifindex_slave <= 0)
+		_LOGD (LOGD_TEAM, "bond slave %s is already released", nm_device_get_ip_iface (slave));
 
 	if (configure) {
 		/* When the last slave is released the bond MAC will be set to a random
@@ -420,9 +426,10 @@ release_slave (NMDevice *device,
 		 */
 		address = g_strdup (nm_device_get_hw_address (device));
 
+		if (ifindex_slave > 0) {
 			success = nm_platform_link_release (nm_device_get_platform (device),
 			                                    nm_device_get_ip_ifindex (device),
-		                                    nm_device_get_ip_ifindex (slave));
+			                                    ifindex_slave);
 
 			if (success) {
 				_LOGI (LOGD_BOND, "released bond slave %s",
@@ -431,6 +438,7 @@ release_slave (NMDevice *device,
 				_LOGW (LOGD_BOND, "failed to release bond slave %s",
 				       nm_device_get_ip_iface (slave));
 			}
+		}
 
 		nm_platform_process_events (nm_device_get_platform (device));
 		if (nm_device_update_hw_address (device))
@@ -440,13 +448,17 @@ release_slave (NMDevice *device,
 		 * IFF_UP), so we must bring it back up here to ensure carrier changes and
 		 * other state is noticed by the now-released slave.
 		 */
+		if (ifindex_slave > 0) {
 			if (!nm_device_bring_up (slave, TRUE, NULL))
 				_LOGW (LOGD_BOND, "released bond slave could not be brought up.");
+		}
 	} else {
+		if (ifindex_slave > 0) {
 			_LOGI (LOGD_BOND, "bond slave %s was released",
 			       nm_device_get_ip_iface (slave));
 		}
 	}
+}
 
 static gboolean
 create_and_realize (NMDevice *device,

src/devices/nm-device-bridge.c

@@ -625,11 +625,19 @@ release_slave (NMDevice *device,
 {
 	NMDeviceBridge *self = NM_DEVICE_BRIDGE (device);
 	gboolean success;
+	int ifindex_slave;
+
+	ifindex_slave = nm_device_get_ip_ifindex (slave);
+
+	if (ifindex_slave <= 0) {
+		_LOGD (LOGD_TEAM, "bond slave %s is already released", nm_device_get_ip_iface (slave));
+		return;
+	}
 
 	if (configure) {
 		success = nm_platform_link_release (nm_device_get_platform (device),
 		                                    nm_device_get_ip_ifindex (device),
-		                                    nm_device_get_ip_ifindex (slave));
+		                                    ifindex_slave);
 
 		if (success) {
 			_LOGI (LOGD_BRIDGE, "detached bridge port %s",

src/devices/team/nm-device-team.c

@@ -774,11 +774,19 @@ release_slave (NMDevice *device,
 	NMDeviceTeam *self = NM_DEVICE_TEAM (device);
 	NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (self);
 	gboolean success;
+	int ifindex_slave;
+
+	ifindex_slave = nm_device_get_ip_ifindex (slave);
+
+	if (ifindex_slave <= 0) {
+		_LOGD (LOGD_TEAM, "team port %s is already released", nm_device_get_ip_iface (slave));
+		return;
+	}
 
 	if (configure) {
 		success = nm_platform_link_release (nm_device_get_platform (device),
 		                                    nm_device_get_ip_ifindex (device),
-		                                    nm_device_get_ip_ifindex (slave));
+		                                    ifindex_slave);
 
 		if (success)
 			_LOGI (LOGD_TEAM, "released team port %s", nm_device_get_ip_iface (slave));
@@ -789,9 +797,10 @@ release_slave (NMDevice *device,
 		 * IFF_UP), so we must bring it back up here to ensure carrier changes and
 		 * other state is noticed by the now-released port.
 		 */
-		if (!nm_device_bring_up (slave, TRUE, NULL))
+		if (!nm_device_bring_up (slave, TRUE, NULL)) {
 			_LOGW (LOGD_TEAM, "released team port %s could not be brought up",
 			       nm_device_get_ip_iface (slave));
+		}
 
 		nm_clear_g_source (&priv->teamd_read_timeout);
 		priv->teamd_read_timeout = g_timeout_add_seconds (5,

src/nm-manager.c

@@ -176,6 +176,9 @@ typedef struct {
 
 	NMSettings *settings;
 
+	CList connection_changed_on_idle_lst;
+	guint connection_changed_on_idle_id;
+
 	RadioState radio_states[RFKILL_TYPE_MAX];
 	NMVpnManager *vpn_manager;
 
@@ -2064,6 +2067,7 @@ static void
 connection_changed (NMManager *self,
                     NMSettingsConnection *sett_conn)
 {
+	NMManagerPrivate *priv;
 	NMConnection *connection;
 	NMDevice *device;
 
@@ -2071,6 +2075,11 @@ connection_changed (NMManager *self,
 	                  NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE))
 		return;
 
+	priv = NM_MANAGER_GET_PRIVATE (self);
+
+	if (!nm_settings_has_connection (priv->settings, sett_conn))
+		return;
+
 	connection = nm_settings_connection_get_connection (sett_conn);
 
 	if (!nm_connection_is_virtual (connection))
@@ -2086,12 +2095,46 @@ connection_changed (NMManager *self,
 	retry_connections_for_parent_device (self, device);
 }
 
+static gboolean
+connection_changed_on_idle_cb (gpointer user_data)
+{
+	NMManager *self = user_data;
+	NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self);
+	NMCListElem *elem;
+
+	priv->connection_changed_on_idle_id = 0;
+
+	while ((elem = c_list_first_entry (&priv->connection_changed_on_idle_lst, NMCListElem, lst))) {
+		gs_unref_object NMSettingsConnection *sett_conn = NULL;
+
+		sett_conn = nm_c_list_elem_free_steal (elem);
+		connection_changed (self, sett_conn);
+	}
+
+	return G_SOURCE_REMOVE;
+}
+
+static void
+connection_changed_on_idle (NMManager *self,
+                            NMSettingsConnection *sett_conn)
+{
+	NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self);
+
+	if (priv->connection_changed_on_idle_id == 0)
+		priv->connection_changed_on_idle_id = g_idle_add (connection_changed_on_idle_cb, self);
+
+	if (!nm_c_list_elem_find_first (&priv->connection_changed_on_idle_lst, sett_conn)) {
+		c_list_link_tail (&priv->connection_changed_on_idle_lst,
+		                  &nm_c_list_elem_new_stale (g_object_ref (sett_conn))->lst);
+	}
+}
+
 static void
 connection_added_cb (NMSettings *settings,
                      NMSettingsConnection *sett_conn,
                      NMManager *self)
 {
-	connection_changed (self, sett_conn);
+	connection_changed_on_idle (self, sett_conn);
 }
 
 static void
@@ -2100,7 +2143,7 @@ connection_updated_cb (NMSettings *settings,
                        gboolean by_user,
                        NMManager *self)
 {
-	connection_changed (self, sett_conn);
+	connection_changed_on_idle (self, sett_conn);
 }
 
 /*****************************************************************************/
@@ -7357,6 +7400,7 @@ nm_manager_init (NMManager *self)
 	c_list_init (&priv->active_connections_lst_head);
 	c_list_init (&priv->async_op_lst_head);
 	c_list_init (&priv->delete_volatile_connection_lst_head);
+	c_list_init (&priv->connection_changed_on_idle_lst);
 
 	priv->platform = g_object_ref (NM_PLATFORM_GET);
 
@@ -7658,6 +7702,9 @@ dispose (GObject *object)
 		g_clear_object (&priv->policy);
 	}
 
+	nm_clear_g_source (&priv->connection_changed_on_idle_id);
+	nm_c_list_elem_free_all (&priv->connection_changed_on_idle_lst, g_object_unref);
+
 	if (priv->settings) {
 		g_signal_handlers_disconnect_by_func (priv->settings, settings_startup_complete_changed, self);
 		g_signal_handlers_disconnect_by_func (priv->settings, system_unmanaged_devices_changed_cb, self);

src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c

@@ -3332,50 +3332,6 @@ do_write_to_disk (NMConnection *connection,
 	return TRUE;
 }
 
-static gboolean
-do_write_reread (NMConnection *connection,
-                 const char *ifcfg_name,
-                 NMConnection **out_reread,
-                 gboolean *out_reread_same,
-                 GError **error)
-{
-	gs_unref_object NMConnection *reread = NULL;
-	gs_free_error GError *local = NULL;
-	gs_free char *unhandled = NULL;
-	gboolean reread_same = FALSE;
-
-	nm_assert (!out_reread || !*out_reread);
-
-	reread = connection_from_file (ifcfg_name, &unhandled, &local, NULL);
-
-	if (!reread) {
-		g_propagate_error (error, local);
-		local = NULL;
-		return FALSE;
-	}
-	if (unhandled) {
-		g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED,
-		             "connection is unhandled");
-		return FALSE;
-	}
-	if (out_reread_same) {
-		if (nm_connection_compare (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT))
-			reread_same = TRUE;
-
-		nm_assert (reread_same == nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT));
-		nm_assert (reread_same == ({
-		                                gs_unref_hashtable GHashTable *_settings = NULL;
-
-		                                (   nm_connection_diff (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT, &_settings)
-		                                 && !_settings);
-		                           }));
-	}
-
-	NM_SET_OUT (out_reread, g_steal_pointer (&reread));
-	NM_SET_OUT (out_reread_same, reread_same);
-	return TRUE;
-}
-
 gboolean
 nms_ifcfg_rh_writer_write_connection (NMConnection *connection,
                                       const char *ifcfg_dir,
@@ -3394,7 +3350,6 @@ nms_ifcfg_rh_writer_write_connection (NMConnection *connection,
 	nm_auto_free_gstring GString *route6_content = NULL;
 	gs_unref_hashtable GHashTable *secrets = NULL;
 	gs_unref_hashtable GHashTable *blobs = NULL;
-	GError *local = NULL;
 
 	nm_assert (!out_reread || !*out_reread);
 
@@ -3431,22 +3386,36 @@ nms_ifcfg_rh_writer_write_connection (NMConnection *connection,
 
 	/* Note that we just wrote the connection to disk, and re-read it from there.
 	 * That is racy if somebody else modifies the connection.
+	 * That race is why we must not tread a failure to re-read the profile
+	 * as an error.
 	 *
-	 * A better solution might be, to re-read the connection only based on the
+	 * FIXME: a much better solution might be, to re-read the connection only based
-	 * in-memory representation of what we collected above. But the reader
+	 * on the in-memory representation of what we collected above. But the reader
 	 * does not yet allow to inject the configuration. */
-	if (out_reread || out_reread_same) {
+	if (   out_reread
-		if (!do_write_reread (connection,
+	    || out_reread_same) {
-		                      svFileGetName (ifcfg),
+		gs_unref_object NMConnection *reread = NULL;
-		                      out_reread,
+		gboolean reread_same = FALSE;
-		                      out_reread_same,
+		gs_free_error GError *local = NULL;
-		                      &local)) {
+		gs_free char *unhandled = NULL;
+
+		reread = connection_from_file (svFileGetName (ifcfg),
+		                               &unhandled,
+		                               &local,
+		                               NULL);
+		nm_assert ((NM_IS_CONNECTION (reread) && !local) || (!reread && local));
+
+		if (!reread) {
 			_LOGW ("write: failure to re-read connection \"%s\": %s",
 			       svFileGetName (ifcfg), local->message);
-			g_clear_error (&local);
+		} else if (unhandled) {
+			g_clear_object (&reread);
+			_LOGW ("write: failure to re-read connection \"%s\": %s",
+			       svFileGetName (ifcfg), "connection is unhandled");
 		} else {
-			if (   out_reread_same
+			if (out_reread_same) {
-			    && !*out_reread_same) {
+				reread_same = nm_connection_compare (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT);
+				if (!reread_same) {
 					_LOGD ("write: connection %s (%s) was modified by persisting it to \"%s\" ",
 					       nm_connection_get_id (connection),
 					       nm_connection_get_uuid (connection),
@@ -3455,6 +3424,10 @@ nms_ifcfg_rh_writer_write_connection (NMConnection *connection,
 			}
 		}
 
+		NM_SET_OUT (out_reread, g_steal_pointer (&reread));
+		NM_SET_OUT (out_reread_same, reread_same);
+	}
+
 	/* Only return the filename if this was a newly written ifcfg */
 	if (out_filename && !filename)
 		*out_filename = g_strdup (svFileGetName (ifcfg));