ifcfg-rh: refactor update_connection()

Make update_connection() analogous to keyfiles implementation.
Effectively merge _internal_new_connection() and update_connection()
-- previously connection_new_or_changed().

https://bugzilla.redhat.com/show_bug.cgi?id=1171751

3 files changed, 231 insertions, 227 deletions

diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c
index eaa3aea2d9..06c3c9f0c7 100644
--- a/src/settings/plugins/ifcfg-rh/plugin.c
+++ b/src/settings/plugins/ifcfg-rh/plugin.c
@@ -58,6 +58,7 @@
 #include "reader.h"
 #include "writer.h"
 #include "utils.h"
+#include "gsystem-local-alloc.h"
 
 #define DBUS_SERVICE_NAME "com.redhat.ifcfgrh1"
 #define DBUS_OBJECT_PATH "/com/redhat/ifcfgrh1"
@@ -79,6 +80,8 @@
 #define _LOGW(...)      _LOG (LOGL_WARN,  _LOG_DEFAULT_DOMAIN, __VA_ARGS__)
 #define _LOGE(...)      _LOG (LOGL_ERR,   _LOG_DEFAULT_DOMAIN, __VA_ARGS__)
 
+#define ERR_GET_MSG(err) (((err) && (err)->message) ? (err)->message : "(unknown)")
+
 
 static gboolean impl_ifcfgrh_get_ifcfg_details (SCPluginIfcfg *plugin,
                                                 const char *in_ifcfg,
@@ -88,13 +91,13 @@ static gboolean impl_ifcfgrh_get_ifcfg_details (SCPluginIfcfg *plugin,
 
 #include "nm-ifcfg-rh-glue.h"
 
-static void update_connection (SCPluginIfcfg *plugin,
-                               NMConnection *source,
-                               const char *full_path,
-                               NMIfcfgConnection *connection,
-                               GHashTable *protected_connections,
-                               char **out_old_path,
-                               GError **error);
+static NMIfcfgConnection *update_connection (SCPluginIfcfg *plugin,
+                                             NMConnection *source,
+                                             const char *full_path,
+                                             NMIfcfgConnection *connection,
+                                             gboolean protect_existing_connection,
+                                             GHashTable *protected_connections,
+                                             GError **error);
 
 static void system_config_interface_init (NMSystemConfigInterface *system_config_interface_class);
 
@@ -131,7 +134,9 @@ connection_ifcfg_changed (NMIfcfgConnection *connection, gpointer user_data)
 	path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection));
 	g_return_if_fail (path != NULL);
 
-	update_connection (plugin, NULL, path, connection, NULL, NULL, NULL);
+	_LOGD ("connection_ifcfg_changed("NM_IFCFG_CONNECTION_LOG_FMTD")", NM_IFCFG_CONNECTION_LOG_ARGD (connection));
+
+	update_connection (plugin, NULL, path, connection, TRUE, NULL, NULL);
 }
 
 static void
@@ -141,69 +146,6 @@ connection_removed_cb (NMSettingsConnection *obj, gpointer user_data)
 	                     nm_connection_get_uuid (NM_CONNECTION (obj)));
 }
 
-static NMIfcfgConnection *
-_internal_new_connection (SCPluginIfcfg *self,
-                          const char *full_path,
-                          NMConnection *source,
-                          GError **error)
-{
-	SCPluginIfcfgPrivate *priv = SC_PLUGIN_IFCFG_GET_PRIVATE (self);
-	NMIfcfgConnection *connection;
-	const char *cid;
-	const char *uuid;
-
-	if (!source)
-		_LOGI ("parsing %s ... ", full_path);
-
-	connection = nm_ifcfg_connection_new (source, full_path, error);
-	if (!connection)
-		return NULL;
-
-	uuid = nm_connection_get_uuid (NM_CONNECTION (connection));
-
-	if (g_hash_table_contains (priv->connections, uuid)) {
-		g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED,
-		             "Failed to add duplicate connection for UUID %s", uuid);
-		g_object_unref (connection);
-		return NULL;
-	}
-
-	cid = nm_connection_get_id (NM_CONNECTION (connection));
-	g_assert (cid);
-
-	g_hash_table_insert (priv->connections,
-	                     g_strdup (uuid),
-	                     connection);
-	_LOGI ("    read connection '%s'", cid);
-	g_signal_connect (connection, NM_SETTINGS_CONNECTION_REMOVED,
-	                  G_CALLBACK (connection_removed_cb),
-	                  self);
-
-	if (nm_ifcfg_connection_get_unmanaged_spec (connection)) {
-		const char *spec;
-		const char *device_id;
-
-		spec = nm_ifcfg_connection_get_unmanaged_spec (connection);
-		device_id = strchr (spec, ':');
-		if (device_id)
-			device_id++;
-		else
-			device_id = spec;
-		_LOGW ("    Ignoring connection '%s' / device '%s' due to NM_CONTROLLED=no.",
-		             cid, device_id);
-	} else if (nm_ifcfg_connection_get_unrecognized_spec (connection)) {
-		_LOGW ("    Ignoring connection '%s' of unrecognized type.", cid);
-	}
-
-	/* watch changes of ifcfg hardlinks */
-	g_signal_connect (G_OBJECT (connection), "ifcfg-changed",
-	                  G_CALLBACK (connection_ifcfg_changed), self);
-
-	return connection;
-}
-
-/* Monitoring */
-
 static void
 remove_connection (SCPluginIfcfg *self, NMIfcfgConnection *connection)
 {
@@ -213,6 +155,8 @@ remove_connection (SCPluginIfcfg *self, NMIfcfgConnection *connection)
 	g_return_if_fail (self != NULL);
 	g_return_if_fail (connection != NULL);
 
+	_LOGI ("remove "NM_IFCFG_CONNECTION_LOG_FMT, NM_IFCFG_CONNECTION_LOG_ARG (connection));
+
 	unmanaged = !!nm_ifcfg_connection_get_unmanaged_spec (connection);
 	unrecognized = !!nm_ifcfg_connection_get_unrecognized_spec (connection);
 
@@ -246,157 +190,220 @@ find_by_path (SCPluginIfcfg *self, const char *path)
 }
 
 static NMIfcfgConnection *
-find_by_uuid_from_path (SCPluginIfcfg *self, const char *path)
-{
-	SCPluginIfcfgPrivate *priv = SC_PLUGIN_IFCFG_GET_PRIVATE (self);
-	char *uuid;
-
-	g_return_val_if_fail (path != NULL, NULL);
-
-	uuid = uuid_from_file (path);
-	if (uuid)
-		return g_hash_table_lookup (priv->connections, uuid);
-	else
-		return NULL;
-}
-
-static void
 update_connection (SCPluginIfcfg *self,
                    NMConnection *source,
                    const char *full_path,
                    NMIfcfgConnection *connection,
+                   gboolean protect_existing_connection,
                    GHashTable *protected_connections,
-                   char **out_old_path,
                    GError **error)
 {
 	SCPluginIfcfgPrivate *priv = SC_PLUGIN_IFCFG_GET_PRIVATE (self);
 	NMIfcfgConnection *connection_new;
+	NMIfcfgConnection *connection_by_uuid;
 	GError *local = NULL;
 	const char *new_unmanaged = NULL, *old_unmanaged = NULL;
 	const char *new_unrecognized = NULL, *old_unrecognized = NULL;
-	gboolean unmanaged_changed, unrecognized_changed;
+	gboolean unmanaged_changed = FALSE, unrecognized_changed = FALSE;
+	const char *uuid;
 
-	g_return_if_fail (self != NULL);
-	g_return_if_fail (full_path != NULL);
-
-	if (out_old_path)
-		*out_old_path = NULL;
-
-	if (!connection) {
-		/* See if it's a rename */
-		connection = find_by_uuid_from_path (self, full_path);
-		if (connection) {
-			const char *old_path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection));
-			_LOGI ("renaming %s -> %s", old_path, full_path);
-			if (out_old_path)
-				*out_old_path = g_strdup (old_path);
-			nm_settings_connection_set_filename (NM_SETTINGS_CONNECTION (connection), full_path);
-		}
+	g_return_val_if_fail (!source || NM_IS_CONNECTION (source), NULL);
+	g_return_val_if_fail (full_path || source, NULL);
+
+	if (full_path)
+		_LOGD ("loading from file \"%s\"...", full_path);
+
+	/* Create a NMIfcfgConnection instance, either by reading from @full_path or
+	 * based on @source. */
+	connection_new = nm_ifcfg_connection_new (source, full_path, error);
+	if (!connection_new) {
+		/* Unexpected failure. Probably the file is invalid? */
+		if (   connection
+		    && !protect_existing_connection
+		    && (!protected_connections || !g_hash_table_contains (protected_connections, connection)))
+			remove_connection (self, connection);
+		return NULL;
 	}
 
-	if (!connection) {
-		/* New connection */
-		connection_new = _internal_new_connection (self, full_path, NULL, NULL);
-		if (connection_new) {
-			if (nm_ifcfg_connection_get_unmanaged_spec (connection_new))
-				g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_UNMANAGED_SPECS_CHANGED);
-			else if (nm_ifcfg_connection_get_unrecognized_spec (connection_new))
-				g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_UNRECOGNIZED_SPECS_CHANGED);
+	uuid = nm_connection_get_uuid (NM_CONNECTION (connection_new));
+	connection_by_uuid = g_hash_table_lookup (priv->connections, uuid);
+
+	if (   connection
+	    && connection != connection_by_uuid) {
+
+		if (   (protect_existing_connection && connection_by_uuid != NULL)
+		    || (protected_connections && g_hash_table_contains (protected_connections, connection))) {
+			NMIfcfgConnection *conflicting = (protect_existing_connection && connection_by_uuid != NULL) ? connection_by_uuid : connection;
+
+			if (source)
+				_LOGW ("cannot update protected connection "NM_IFCFG_CONNECTION_LOG_FMT" due to conflicting UUID %s", NM_IFCFG_CONNECTION_LOG_ARG (conflicting), uuid);
 			else
-				g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection_new);
+				_LOGW ("cannot load %s due to conflicting UUID for "NM_IFCFG_CONNECTION_LOG_FMT, full_path, NM_IFCFG_CONNECTION_LOG_ARG (conflicting));
+			g_object_unref (connection_new);
+			g_set_error_literal (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED,
+			                     "Cannot update protected connection due to conflicting UUID");
+			return NULL;
 		}
-		return;
-	}
 
-	connection_new = (NMIfcfgConnection *) nm_ifcfg_connection_new (NULL, full_path, NULL);
-	if (!connection_new) {
-		/* errors reading connection; remove it */
-		_LOGI ("removed %s.", full_path);
+		/* The new connection has a different UUID then the original one that we
+		 * are about to update. Remove @connection. */
 		remove_connection (self, connection);
-		return;
 	}
 
-	/* Successfully read connection changes */
+	/* Check if the found connection with the same UUID is not protected from updating. */
+	if (   connection_by_uuid
+	    && (   (!connection && protect_existing_connection)
+	        || (protected_connections && g_hash_table_contains (protected_connections, connection_by_uuid)))) {
+		if (source)
+			_LOGW ("cannot update connection due to conflicting UUID for "NM_IFCFG_CONNECTION_LOG_FMT, NM_IFCFG_CONNECTION_LOG_ARG (connection_by_uuid));
+		else
+			_LOGW ("cannot load %s due to conflicting UUID for "NM_IFCFG_CONNECTION_LOG_FMT, full_path, NM_IFCFG_CONNECTION_LOG_ARG (connection_by_uuid));
+		g_object_unref (connection_new);
+		g_set_error_literal (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED,
+		                      "Skip updating protected connection during reload");
+		return NULL;
+	}
 
-	old_unmanaged = nm_ifcfg_connection_get_unmanaged_spec (NM_IFCFG_CONNECTION (connection));
-	new_unmanaged = nm_ifcfg_connection_get_unmanaged_spec (NM_IFCFG_CONNECTION (connection_new));
+	/* Evaluate unmanaged/unrecognized flags. */
+	if (connection_by_uuid)
+		old_unmanaged = nm_ifcfg_connection_get_unmanaged_spec (connection_by_uuid);
+	new_unmanaged = nm_ifcfg_connection_get_unmanaged_spec (connection_new);
 	unmanaged_changed = g_strcmp0 (old_unmanaged, new_unmanaged);
 
-	old_unrecognized = nm_ifcfg_connection_get_unrecognized_spec (NM_IFCFG_CONNECTION (connection));
-	new_unrecognized = nm_ifcfg_connection_get_unrecognized_spec (NM_IFCFG_CONNECTION (connection_new));
+	if (connection_by_uuid)
+		old_unrecognized = nm_ifcfg_connection_get_unrecognized_spec (connection_by_uuid);
+	new_unrecognized = nm_ifcfg_connection_get_unrecognized_spec (connection_new);
 	unrecognized_changed = g_strcmp0 (old_unrecognized, new_unrecognized);
 
-	if (   !unmanaged_changed
-	    && !unrecognized_changed
-	    && nm_connection_compare (NM_CONNECTION (connection),
-	                              NM_CONNECTION (connection_new),
-	                              NM_SETTING_COMPARE_FLAG_IGNORE_AGENT_OWNED_SECRETS |
-	                                  NM_SETTING_COMPARE_FLAG_IGNORE_NOT_SAVED_SECRETS)) {
-		g_object_unref (connection_new);
-		return;
-	}
-
-	if (g_strcmp0 (nm_connection_get_uuid (NM_CONNECTION (connection)), nm_connection_get_uuid (NM_CONNECTION (connection_new))) != 0) {
-		/* FIXME: UUID changes are not supported by nm_settings_connection_replace_settings().
-		 * This function should be merged with _internal_new_connection() to be like keyfiles
-		 * update_connection(). */
-		_LOGW ("UUID changes are not supported. Cannot update connection %s (%s)", nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection_new)), nm_connection_get_uuid (NM_CONNECTION (connection_new)));
-		g_object_unref (connection_new);
-		return;
-	}
+	if (connection_by_uuid) {
+		const char *old_path;
+
+		old_path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection_by_uuid));
+
+		if (   !unmanaged_changed
+		    && !unrecognized_changed
+		    && nm_connection_compare (NM_CONNECTION (connection_by_uuid),
+		                              NM_CONNECTION (connection_new),
+		                              NM_SETTING_COMPARE_FLAG_IGNORE_AGENT_OWNED_SECRETS |
+		                              NM_SETTING_COMPARE_FLAG_IGNORE_NOT_SAVED_SECRETS)) {
+			if (old_path && g_strcmp0 (old_path, full_path) != 0)
+				_LOGI ("rename \"%s\" to "NM_IFCFG_CONNECTION_LOG_FMT" without other changes", nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection_by_uuid)), NM_IFCFG_CONNECTION_LOG_ARG (connection_new));
+		} else {
+
+			/*******************************************************
+			 * UPDATE
+			 *******************************************************/
+
+			if (source)
+				_LOGI ("update "NM_IFCFG_CONNECTION_LOG_FMT" from %s", NM_IFCFG_CONNECTION_LOG_ARG (connection_new), NM_IFCFG_CONNECTION_LOG_PATH (old_path));
+			else if (!g_strcmp0 (old_path, nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection_new))))
+				_LOGI ("update "NM_IFCFG_CONNECTION_LOG_FMT, NM_IFCFG_CONNECTION_LOG_ARG (connection_new));
+			else if (old_path)
+				_LOGI ("rename \"%s\" to "NM_IFCFG_CONNECTION_LOG_FMT, old_path, NM_IFCFG_CONNECTION_LOG_ARG (connection_new));
+			else
+				_LOGI ("update and persist "NM_IFCFG_CONNECTION_LOG_FMT, NM_IFCFG_CONNECTION_LOG_ARG (connection_new));
+
+			g_object_set (connection_by_uuid,
+			              NM_IFCFG_CONNECTION_UNMANAGED_SPEC, new_unmanaged,
+			              NM_IFCFG_CONNECTION_UNRECOGNIZED_SPEC, new_unrecognized,
+			              NULL);
+
+			if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection_by_uuid),
+			                                              NM_CONNECTION (connection_new),
+			                                              FALSE,  /* don't set Unsaved */
+			                                              "ifcfg-update",
+			                                              &local)) {
+				/* Shouldn't ever get here as 'connection_new' was verified by the reader already
+				 * and the UUID did not change. */
+				g_assert_not_reached ();
+			}
+			g_assert_no_error (local);
+
+			if (new_unmanaged || new_unrecognized) {
+				if (!old_unmanaged && !old_unrecognized) {
+					g_object_ref (connection_by_uuid);
+					/* Unexport the connection by telling the settings service it's
+					 * been removed.
+					 */
+					nm_settings_connection_signal_remove (NM_SETTINGS_CONNECTION (connection_by_uuid));
+					/* Remove the path so that claim_connection() doesn't complain later when
+					 * interface gets managed and connection is re-added. */
+					nm_connection_set_path (NM_CONNECTION (connection_by_uuid), NULL);
+
+					/* signal_remove() will end up removing the connection from our hash,
+					 * so add it back now.
+					 */
+					g_hash_table_insert (priv->connections,
+					                     g_strdup (nm_connection_get_uuid (NM_CONNECTION (connection_by_uuid))),
+					                     connection_by_uuid);
+				}
+			} else {
+				if (old_unmanaged /* && !new_unmanaged */) {
+					_LOGI ("Managing connection "NM_IFCFG_CONNECTION_LOG_FMT" and its device because NM_CONTROLLED was true.",
+					       NM_IFCFG_CONNECTION_LOG_ARG (connection_new));
+					g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection_by_uuid);
+				} else if (old_unrecognized /* && !new_unrecognized */) {
+					_LOGI ("Managing connection "NM_IFCFG_CONNECTION_LOG_FMT" because it is now a recognized type.",
+					       NM_IFCFG_CONNECTION_LOG_ARG (connection_new));
+					g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection_by_uuid);
+				}
+			}
 
-	_LOGI ("updating %s", full_path);
-	g_object_set (connection,
-	              NM_IFCFG_CONNECTION_UNMANAGED_SPEC, new_unmanaged,
-	              NM_IFCFG_CONNECTION_UNRECOGNIZED_SPEC, new_unrecognized,
-	              NULL);
-
-	if (new_unmanaged || new_unrecognized) {
-		if (!old_unmanaged && !old_unrecognized) {
-			g_object_ref (connection);
-			/* Unexport the connection by telling the settings service it's
-			 * been removed.
-			 */
-			nm_settings_connection_signal_remove (NM_SETTINGS_CONNECTION (connection));
-			/* Remove the path so that claim_connection() doesn't complain later when
-			 * interface gets managed and connection is re-added. */
-			nm_connection_set_path (NM_CONNECTION (connection), NULL);
-
-			/* signal_remove() will end up removing the connection from our hash,
-			 * so add it back now.
-			 */
-			g_hash_table_insert (priv->connections,
-			                     g_strdup (nm_connection_get_uuid (NM_CONNECTION (connection))),
-			                     connection);
+			if (unmanaged_changed)
+				g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_UNMANAGED_SPECS_CHANGED);
+			if (unrecognized_changed)
+				g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_UNRECOGNIZED_SPECS_CHANGED);
 		}
+		nm_settings_connection_set_filename (NM_SETTINGS_CONNECTION (connection_by_uuid), full_path);
+		g_object_unref (connection_new);
+		return connection_by_uuid;
 	} else {
-		const char *cid = nm_connection_get_id (NM_CONNECTION (connection_new));
-
-		if (old_unmanaged /* && !new_unmanaged */) {
-			_LOGI ("Managing connection '%s' and its device because NM_CONTROLLED was true.", cid);
-			g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection);
-		} else if (old_unrecognized /* && !new_unrecognized */) {
-			_LOGI ("Managing connection '%s' because it is now a recognized type.", cid);
-			g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection);
-		}
 
-		if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection),
-		                                              NM_CONNECTION (connection_new),
-		                                              FALSE,  /* don't set Unsaved */
-		                                              "ifcfg-rh-update",
-		                                              &local)) {
-			/* Shouldn't ever get here as 'connection_new' was verified by the reader already
-			 * and the UUID did not change. */
-			g_assert_not_reached ();
+		/*******************************************************
+		 * ADD
+		 *******************************************************/
+
+		if (source)
+			_LOGI ("add connection "NM_IFCFG_CONNECTION_LOG_FMT, NM_IFCFG_CONNECTION_LOG_ARG (connection_new));
+		else
+			_LOGI ("new connection "NM_IFCFG_CONNECTION_LOG_FMT, NM_IFCFG_CONNECTION_LOG_ARG (connection_new));
+		g_hash_table_insert (priv->connections, g_strdup (uuid), connection_new);
+
+		g_signal_connect (connection_new, NM_SETTINGS_CONNECTION_REMOVED,
+		                  G_CALLBACK (connection_removed_cb),
+		                  self);
+
+		if (nm_ifcfg_connection_get_unmanaged_spec (connection_new)) {
+			const char *spec;
+			const char *device_id;
+
+			spec = nm_ifcfg_connection_get_unmanaged_spec (connection_new);
+			device_id = strchr (spec, ':');
+			if (device_id)
+				device_id++;
+			else
+				device_id = spec;
+			_LOGW ("Ignoring connection "NM_IFCFG_CONNECTION_LOG_FMT" / device '%s' due to NM_CONTROLLED=no.",
+			       NM_IFCFG_CONNECTION_LOG_ARG (connection_new), device_id);
+		} else if (nm_ifcfg_connection_get_unrecognized_spec (connection_new))
+			_LOGW ("Ignoring connection "NM_IFCFG_CONNECTION_LOG_FMT" of unrecognized type.", NM_IFCFG_CONNECTION_LOG_ARG (connection_new));
+
+		/* watch changes of ifcfg hardlinks */
+		g_signal_connect (G_OBJECT (connection_new), "ifcfg-changed",
+		                  G_CALLBACK (connection_ifcfg_changed), self);
+
+		if (!source) {
+			/* Only raise the signal if we were called without source, i.e. if we read the connection from file.
+			 * Otherwise, we were called by add_connection() which does not expect the signal. */
+			if (nm_ifcfg_connection_get_unmanaged_spec (connection_new))
+				g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_UNMANAGED_SPECS_CHANGED);
+			else if (nm_ifcfg_connection_get_unrecognized_spec (connection_new))
+				g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_UNRECOGNIZED_SPECS_CHANGED);
+			else
+				g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection_new);
 		}
-		g_assert_no_error (local);
+		return connection_new;
 	}
-	g_object_unref (connection_new);
-
-	if (unmanaged_changed)
-		g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_UNMANAGED_SPECS_CHANGED);
-	if (unrecognized_changed)
-		g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_UNRECOGNIZED_SPECS_CHANGED);
 }
 
 static void
@@ -416,6 +423,8 @@ ifcfg_dir_changed (GFileMonitor *monitor,
 		return;
 	}
 
+	_LOGD ("ifcfg_dir_changed(%s) = %d", path, event_type);
+
 	base = g_file_get_basename (file);
 	if (utils_is_ifcfg_alias_file (base, NULL)) {
 		/* Alias file changed. Get the base ifcfg file from it */
@@ -428,14 +437,13 @@ ifcfg_dir_changed (GFileMonitor *monitor,
 		connection = find_by_path (plugin, ifcfg_path);
 		switch (event_type) {
 		case G_FILE_MONITOR_EVENT_DELETED:
-			_LOGI ("removed %s.", ifcfg_path);
 			if (connection)
 				remove_connection (plugin, connection);
 			break;
 		case G_FILE_MONITOR_EVENT_CREATED:
 		case G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT:
 			/* Update or new */
-			update_connection (plugin, NULL, ifcfg_path, connection, NULL, NULL, NULL);
+			update_connection (plugin, NULL, ifcfg_path, connection, TRUE, NULL, NULL);
 			break;
 		default:
 			break;
@@ -508,10 +516,10 @@ read_connections (SCPluginIfcfg *plugin)
 	GDir *dir;
 	GError *err = NULL;
 	const char *item;
-	GHashTable *oldconns;
+	GHashTable *alive_connections;
 	GHashTableIter iter;
-	gpointer key, value;
 	NMIfcfgConnection *connection;
+	GPtrArray *dead_connections = NULL;
 	guint i;
 	GPtrArray *filenames;
 	GHashTable *paths;
@@ -523,13 +531,7 @@ read_connections (SCPluginIfcfg *plugin)
 		return;
 	}
 
-	oldconns = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
-	g_hash_table_iter_init (&iter, priv->connections);
-	while (g_hash_table_iter_next (&iter, NULL, &value)) {
-		const char *ifcfg_path = nm_settings_connection_get_filename (value);
-		if (ifcfg_path)
-			g_hash_table_insert (oldconns, g_strdup (ifcfg_path), value);
-	}
+	alive_connections = g_hash_table_new (NULL, NULL);
 
 	filenames = g_ptr_array_new_with_free_func (g_free);
 	while ((item = g_dir_read_name (dir))) {
@@ -559,29 +561,28 @@ read_connections (SCPluginIfcfg *plugin)
 	g_hash_table_destroy (paths);
 
 	for (i = 0; i < filenames->len; i++) {
-		const char *full_path;
-		char *old_path;
-
-		full_path = filenames->pdata[i];
-		connection = g_hash_table_lookup (oldconns, full_path);
-		g_hash_table_remove (oldconns, full_path);
-		update_connection (plugin, NULL, full_path, connection, NULL, &old_path, NULL);
-
-		if (old_path) {
-			g_hash_table_remove (oldconns, old_path);
-			g_free (old_path);
-		}
+		connection = update_connection (plugin, NULL, filenames->pdata[i], NULL, FALSE, alive_connections, NULL);
+		if (connection)
+			g_hash_table_add (alive_connections, connection);
 	}
 	g_ptr_array_free (filenames, TRUE);
 
-	g_hash_table_iter_init (&iter, oldconns);
-	while (g_hash_table_iter_next (&iter, &key, &value)) {
-		_LOGI ("removed %s.", (char *)key);
-		g_hash_table_iter_remove (&iter);
-		remove_connection (plugin, value);
+	g_hash_table_iter_init (&iter, priv->connections);
+	while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &connection)) {
+		if (   !g_hash_table_contains (alive_connections, connection)
+		    && nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection))) {
+			if (!dead_connections)
+				dead_connections = g_ptr_array_new ();
+			g_ptr_array_add (dead_connections, connection);
+		}
 	}
+	g_hash_table_destroy (alive_connections);
 
-	g_hash_table_destroy (oldconns);
+	if (dead_connections) {
+		for (i = 0; i < dead_connections->len; i++)
+			remove_connection (plugin, dead_connections->pdata[i]);
+		g_ptr_array_free (dead_connections, TRUE);
+	}
 }
 
 static GSList *
@@ -627,7 +628,7 @@ load_connection (NMSystemConfigInterface *config,
 		return FALSE;
 
 	connection = find_by_path (plugin, filename);
-	update_connection (plugin, NULL, filename, connection, NULL, NULL, NULL);
+	update_connection (plugin, NULL, filename, connection, TRUE, NULL, NULL);
 	if (!connection)
 		connection = find_by_path (plugin, filename);
 
@@ -692,8 +693,7 @@ add_connection (NMSystemConfigInterface *config,
                 GError **error)
 {
 	SCPluginIfcfg *self = SC_PLUGIN_IFCFG (config);
-	NMIfcfgConnection *added = NULL;
-	char *path = NULL;
+	gs_free char *path = NULL;
 
 	/* Ensure we reject attempts to add the connection long before we're
 	 * asked to write it to disk.
@@ -705,10 +705,7 @@ add_connection (NMSystemConfigInterface *config,
 		if (!writer_new_connection (connection, IFCFG_DIR, &path, error))
 			return NULL;
 	}
-
-	added = _internal_new_connection (self, path, connection, error);
-	g_free (path);
-	return (NMSettingsConnection *) added;
+	return NM_SETTINGS_CONNECTION (update_connection (self, connection, path, NULL, FALSE, NULL, error));
 }
 
 #define SC_NETWORK_FILE "/etc/sysconfig/network"
diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh-utils.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh-utils.c
index d82a327032..8494c63825 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh-utils.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh-utils.c
@@ -121,7 +121,7 @@ int main (int argc, char **argv)
 {
 	char *base;
 
-	nmtst_init (&argc, &argv, TRUE);
+	nmtst_init_assert_logging (&argc, &argv);
 
 	/* The tests */
 	test_get_ifcfg_name ("get-ifcfg-name-bad", "/foo/bar/adfasdfadf", FALSE, NULL);
diff --git a/src/settings/plugins/ifcfg-rh/utils.h b/src/settings/plugins/ifcfg-rh/utils.h
index 27c5e46104..445437c48b 100644
--- a/src/settings/plugins/ifcfg-rh/utils.h
+++ b/src/settings/plugins/ifcfg-rh/utils.h
@@ -25,6 +25,13 @@
 #include <nm-connection.h>
 #include "shvar.h"
 #include "common.h"
+#include "nm-logging.h"
+
+#define NM_IFCFG_CONNECTION_LOG_PATH(path)  str_if_set (path,"in-memory")
+#define NM_IFCFG_CONNECTION_LOG_FMT         "%s (%s,\"%s\")"
+#define NM_IFCFG_CONNECTION_LOG_ARG(con)    NM_IFCFG_CONNECTION_LOG_PATH (nm_settings_connection_get_filename ((NMSettingsConnection *) (con))), nm_connection_get_uuid ((NMConnection *) (con)), nm_connection_get_id ((NMConnection *) (con))
+#define NM_IFCFG_CONNECTION_LOG_FMTD        "%s (%s,\"%s\",%p)"
+#define NM_IFCFG_CONNECTION_LOG_ARGD(con)   NM_IFCFG_CONNECTION_LOG_PATH (nm_settings_connection_get_filename ((NMSettingsConnection *) (con))), nm_connection_get_uuid ((NMConnection *) (con)), nm_connection_get_id ((NMConnection *) (con)), (con)
 
 char *utils_single_quote_string (const char *str);