From fdacbe8dddaba9a529f8fd5b7d83a3986a63e44b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 10 Dec 2014 13:56:45 +0100 Subject: [PATCH 01/19] core: log ignored property notification with level TRACE Avoids for example: notify(): ignoring notification for prop visible on type NMKeyfileConnection (cherry picked from commit 45094c71d841f559c8f006c513624b2c4f5daed2) --- src/nm-properties-changed-signal.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nm-properties-changed-signal.c b/src/nm-properties-changed-signal.c index 127da794e7..c6dbb3d532 100644 --- a/src/nm-properties-changed-signal.c +++ b/src/nm-properties-changed-signal.c @@ -148,8 +148,8 @@ notify (GObject *object, GParamSpec *pspec) break; } if (!dbus_property_name) { - nm_log_dbg (LOGD_DBUS_PROPS, "ignoring notification for prop %s on type %s", - pspec->name, G_OBJECT_TYPE_NAME (object)); + nm_log_trace (LOGD_DBUS_PROPS, "ignoring notification for prop %s on type %s", + pspec->name, G_OBJECT_TYPE_NAME (object)); return; } From 46e2e11a69bba3e1f86b1290a4c7ac19ed9db103 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 9 Dec 2014 11:48:48 +0100 Subject: [PATCH 02/19] core: log object type in nm_utils_log_connection_diff() (cherry picked from commit 73703c4d1982f253e51325d1ed497eb12df1c313) --- src/NetworkManagerUtils.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index b2049cf886..68c0dac303 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -2091,9 +2091,9 @@ nm_utils_log_connection_diff (NMConnection *connection, NMConnection *diff_base, connection_diff_are_same = nm_connection_diff (connection, diff_base, NM_SETTING_COMPARE_FLAG_EXACT | NM_SETTING_COMPARE_FLAG_DIFF_RESULT_NO_DEFAULT, &connection_diff); if (connection_diff_are_same) { if (diff_base) - nm_log (level, domain, "%sconnection '%s' (%p and %p): no difference", prefix, name, connection, diff_base); + nm_log (level, domain, "%sconnection '%s' (%p/%s and %p/%s): no difference", prefix, name, connection, G_OBJECT_TYPE_NAME (connection), diff_base, G_OBJECT_TYPE_NAME (diff_base)); else - nm_log (level, domain, "%sconnection '%s' (%p): no properties set", prefix, name, connection); + nm_log (level, domain, "%sconnection '%s' (%p/%s): no properties set", prefix, name, connection, G_OBJECT_TYPE_NAME (connection)); g_assert (!connection_diff); return; } @@ -2128,9 +2128,9 @@ nm_utils_log_connection_diff (NMConnection *connection, NMConnection *diff_base, GError *err_verify = NULL; if (diff_base) - nm_log (level, domain, "%sconnection '%s' (%p < %p):", prefix, name, connection, diff_base); + nm_log (level, domain, "%sconnection '%s' (%p/%s < %p/%s):", prefix, name, connection, G_OBJECT_TYPE_NAME (connection), diff_base, G_OBJECT_TYPE_NAME (diff_base)); else - nm_log (level, domain, "%sconnection '%s' (%p):", prefix, name, connection); + nm_log (level, domain, "%sconnection '%s' (%p/%s):", prefix, name, connection, G_OBJECT_TYPE_NAME (connection)); print_header = FALSE; if (!nm_connection_verify (connection, &err_verify)) { From d0283bc9e61c9ba91a2f1ac940a848f9703e1b1f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Dec 2014 19:49:46 +0100 Subject: [PATCH 03/19] settings: fix wrong assertions for calling nm_settings_connection_replace_settings() (cherry picked from commit c2dc5d3b0f11dd9d9076bbc7e8500860ea00e339) --- src/settings/plugins/ifcfg-rh/plugin.c | 3 ++- src/settings/plugins/ifnet/plugin.c | 3 ++- src/settings/plugins/keyfile/plugin.c | 6 ++++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index 3594399fb7..458e62898d 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -342,8 +342,9 @@ connection_new_or_changed (SCPluginIfcfg *self, FALSE, /* don't set Unsaved */ &error)) { /* Shouldn't ever get here as 'new' was verified by the reader already */ - g_assert_no_error (error); + g_assert_not_reached (); } + g_assert_no_error (error); } g_object_unref (new); diff --git a/src/settings/plugins/ifnet/plugin.c b/src/settings/plugins/ifnet/plugin.c index 6896a4da1a..37e2907777 100644 --- a/src/settings/plugins/ifnet/plugin.c +++ b/src/settings/plugins/ifnet/plugin.c @@ -315,8 +315,9 @@ reload_connections (NMSystemConfigInterface *config) FALSE, /* don't set Unsaved */ &error)) { /* Shouldn't ever get here as 'new' was verified by the reader already */ - g_assert_no_error (error); + g_assert_not_reached (); } + g_assert_no_error (error); nm_log_info (LOGD_SETTINGS, "Connection %s updated", nm_connection_get_id (NM_CONNECTION (new))); } diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index ead074362e..f7c9211c7e 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -128,8 +128,9 @@ update_connection (SCPluginKeyfile *self, FALSE, /* don't set Unsaved */ &error)) { /* Shouldn't ever get here as 'new' was verified by the reader already */ - g_assert_no_error (error); + g_assert_not_reached (); } + g_assert_no_error (error); } g_object_unref (tmp); } @@ -183,8 +184,9 @@ new_connection (SCPluginKeyfile *self, FALSE, /* don't set Unsaved */ &error)) { /* Shouldn't ever get here as 'tmp' was verified by the reader already */ - g_assert_no_error (error); + g_assert_not_reached (); } + g_assert_no_error (error); g_object_unref (tmp); if (out_old_path) *out_old_path = g_strdup (nm_settings_connection_get_filename (connection)); From 857b5bc345431d12e2c0b6699ad5c0894b54dc59 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 9 Dec 2014 11:37:33 +0100 Subject: [PATCH 04/19] settings: no need to check nm_connection_compare() before nm_settings_connection_replace_settings() nm_settings_connection_replace_settings() does nothing, if there are no changes. (cherry picked from commit 750f01dfcbf0ea91d5e69a117528b44f04247b9b) --- src/settings/nm-settings-connection.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index f62000a63d..9618ccd9ee 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -535,6 +535,7 @@ replace_and_commit (NMSettingsConnection *self, if (nm_settings_connection_replace_settings (self, new_connection, TRUE, &error)) { nm_settings_connection_commit_changes (self, callback, user_data); } else { + g_assert (error); if (callback) callback (self, error, user_data); g_clear_error (&error); @@ -1364,11 +1365,8 @@ update_auth_cb (NMSettingsConnection *self, con_update_cb, info); } else { - /* Do nothing if there's nothing to update */ - if (!nm_connection_compare (NM_CONNECTION (self), info->new_settings, NM_SETTING_COMPARE_FLAG_EXACT)) { - if (!nm_settings_connection_replace_settings (self, info->new_settings, TRUE, &local)) - g_assert (local); - } + if (!nm_settings_connection_replace_settings (self, info->new_settings, TRUE, &local)) + g_assert (local); con_update_cb (self, local, info); g_clear_error (&local); } From 2e0293cc8193df1f9267d2c5d09e2bcb05c3d372 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 9 Dec 2014 11:45:32 +0100 Subject: [PATCH 05/19] settings: log connection diffs in replace_settings only where appropriate Only log connection diffs when we update a connection that we actually care about. Note that most plugin specific connections use nm_settings_connection_replace_settings() in their constructor to initialize themselves. These occurrences are not interesting and spam the logfile. (cherry picked from commit e14ea6818a7edab2438b8fc4664a4499cc981748) --- src/settings/nm-settings-connection.c | 10 ++++++---- src/settings/nm-settings-connection.h | 1 + src/settings/plugins/example/nm-example-connection.c | 1 + src/settings/plugins/ibft/nm-ibft-connection.c | 1 + src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c | 1 + src/settings/plugins/ifcfg-rh/plugin.c | 1 + src/settings/plugins/ifnet/nm-ifnet-connection.c | 1 + src/settings/plugins/ifnet/plugin.c | 1 + src/settings/plugins/keyfile/nm-keyfile-connection.c | 1 + src/settings/plugins/keyfile/plugin.c | 2 ++ 10 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 9618ccd9ee..b5e1e56812 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -447,6 +447,7 @@ gboolean nm_settings_connection_replace_settings (NMSettingsConnection *self, NMConnection *new_connection, gboolean update_unsaved, + const char *log_diff_name, GError **error) { NMSettingsConnectionPrivate *priv; @@ -472,7 +473,8 @@ nm_settings_connection_replace_settings (NMSettingsConnection *self, */ g_signal_handlers_block_by_func (self, G_CALLBACK (changed_cb), GUINT_TO_POINTER (TRUE)); - nm_utils_log_connection_diff (new_connection, NM_CONNECTION (self), LOGL_DEBUG, LOGD_CORE, "update connection", "++ "); + if (log_diff_name) + nm_utils_log_connection_diff (new_connection, NM_CONNECTION (self), LOGL_DEBUG, LOGD_CORE, log_diff_name, "++ "); nm_connection_replace_settings_from_connection (NM_CONNECTION (self), new_connection); nm_settings_connection_set_flags (self, @@ -532,9 +534,9 @@ replace_and_commit (NMSettingsConnection *self, { GError *error = NULL; - if (nm_settings_connection_replace_settings (self, new_connection, TRUE, &error)) { + if (nm_settings_connection_replace_settings (self, new_connection, TRUE, "replace-and-commit-disk", &error)) nm_settings_connection_commit_changes (self, callback, user_data); - } else { + else { g_assert (error); if (callback) callback (self, error, user_data); @@ -1365,7 +1367,7 @@ update_auth_cb (NMSettingsConnection *self, con_update_cb, info); } else { - if (!nm_settings_connection_replace_settings (self, info->new_settings, TRUE, &local)) + if (!nm_settings_connection_replace_settings (self, info->new_settings, TRUE, "replace-and-commit-memory", &local)) g_assert (local); con_update_cb (self, local, info); g_clear_error (&local); diff --git a/src/settings/nm-settings-connection.h b/src/settings/nm-settings-connection.h index 8875097215..49661f38c6 100644 --- a/src/settings/nm-settings-connection.h +++ b/src/settings/nm-settings-connection.h @@ -123,6 +123,7 @@ void nm_settings_connection_commit_changes (NMSettingsConnection *connection, gboolean nm_settings_connection_replace_settings (NMSettingsConnection *self, NMConnection *new_connection, gboolean update_unsaved, + const char *log_diff_name, GError **error); void nm_settings_connection_replace_and_commit (NMSettingsConnection *self, diff --git a/src/settings/plugins/example/nm-example-connection.c b/src/settings/plugins/example/nm-example-connection.c index 2f0b20a08b..e125961130 100644 --- a/src/settings/plugins/example/nm-example-connection.c +++ b/src/settings/plugins/example/nm-example-connection.c @@ -82,6 +82,7 @@ nm_example_connection_new (const char *full_path, if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (object), tmp, TRUE, + NULL, error)) { g_object_unref (object); object = NULL; diff --git a/src/settings/plugins/ibft/nm-ibft-connection.c b/src/settings/plugins/ibft/nm-ibft-connection.c index 5459a70504..c6a9054e47 100644 --- a/src/settings/plugins/ibft/nm-ibft-connection.c +++ b/src/settings/plugins/ibft/nm-ibft-connection.c @@ -46,6 +46,7 @@ nm_ibft_connection_new (const GPtrArray *block, GError **error) if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (object), source, FALSE, + NULL, error)) g_clear_object (&object); diff --git a/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c b/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c index 014f69e44a..9005a375f0 100644 --- a/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c +++ b/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c @@ -235,6 +235,7 @@ nm_ifcfg_connection_new (NMConnection *source, if (nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (object), tmp, update_unsaved, + NULL, error)) nm_ifcfg_connection_check_devtimeout (NM_IFCFG_CONNECTION (object)); else diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index 458e62898d..e608a29f88 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -340,6 +340,7 @@ connection_new_or_changed (SCPluginIfcfg *self, if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (existing), NM_CONNECTION (new), FALSE, /* don't set Unsaved */ + "ifcfg-rh-update", &error)) { /* Shouldn't ever get here as 'new' was verified by the reader already */ g_assert_not_reached (); diff --git a/src/settings/plugins/ifnet/nm-ifnet-connection.c b/src/settings/plugins/ifnet/nm-ifnet-connection.c index c84ad5ed95..693e653288 100644 --- a/src/settings/plugins/ifnet/nm-ifnet-connection.c +++ b/src/settings/plugins/ifnet/nm-ifnet-connection.c @@ -82,6 +82,7 @@ nm_ifnet_connection_new (NMConnection *source, const char *conn_name) nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (object), tmp, update_unsaved, + NULL, NULL); g_object_unref (tmp); diff --git a/src/settings/plugins/ifnet/plugin.c b/src/settings/plugins/ifnet/plugin.c index 37e2907777..218e44aa08 100644 --- a/src/settings/plugins/ifnet/plugin.c +++ b/src/settings/plugins/ifnet/plugin.c @@ -313,6 +313,7 @@ reload_connections (NMSystemConfigInterface *config) if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (old), NM_CONNECTION (new), FALSE, /* don't set Unsaved */ + "ifnet-update", &error)) { /* Shouldn't ever get here as 'new' was verified by the reader already */ g_assert_not_reached (); diff --git a/src/settings/plugins/keyfile/nm-keyfile-connection.c b/src/settings/plugins/keyfile/nm-keyfile-connection.c index 9ff65a20aa..8fb94302d7 100644 --- a/src/settings/plugins/keyfile/nm-keyfile-connection.c +++ b/src/settings/plugins/keyfile/nm-keyfile-connection.c @@ -76,6 +76,7 @@ nm_keyfile_connection_new (NMConnection *source, if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (object), tmp, update_unsaved, + NULL, error)) { g_object_unref (object); object = NULL; diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index f7c9211c7e..e79ad551b8 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -126,6 +126,7 @@ update_connection (SCPluginKeyfile *self, if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection), NM_CONNECTION (tmp), FALSE, /* don't set Unsaved */ + "keyfile-update", &error)) { /* Shouldn't ever get here as 'new' was verified by the reader already */ g_assert_not_reached (); @@ -182,6 +183,7 @@ new_connection (SCPluginKeyfile *self, if (!nm_settings_connection_replace_settings (connection, NM_CONNECTION (tmp), FALSE, /* don't set Unsaved */ + "keyfile-update-new", &error)) { /* Shouldn't ever get here as 'tmp' was verified by the reader already */ g_assert_not_reached (); From 6f5c18f8fa4db03f3c1395aefb8e08ec881f7ae9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 10 Dec 2014 11:24:07 +0100 Subject: [PATCH 06/19] keyfile: simplify g_return() checks for _internal_write_connection() (cherry picked from commit fbd30c7dd2b41fbf3b37b6e408119dab9b98185b) --- src/settings/plugins/keyfile/writer.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/settings/plugins/keyfile/writer.c b/src/settings/plugins/keyfile/writer.c index 5b82ad0903..7aa5bfb018 100644 --- a/src/settings/plugins/keyfile/writer.c +++ b/src/settings/plugins/keyfile/writer.c @@ -816,13 +816,10 @@ _internal_write_connection (NMConnection *connection, WriteInfo info; GError *local_err = NULL; - if (out_path) - g_return_val_if_fail (*out_path == NULL, FALSE); + g_return_val_if_fail (!out_path || !*out_path, FALSE); - if (!nm_connection_verify (connection, error)) { + if (!nm_connection_verify (connection, error)) g_return_val_if_reached (FALSE); - return FALSE; - } id = nm_connection_get_id (connection); g_assert (id && *id); From 09e88d1d452bac61badc719a2fc377f439f82c36 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Dec 2014 21:11:52 +0100 Subject: [PATCH 07/19] keyfile/trival: move code (cherry picked from commit f41586f00c0662d593328caa735ed9b26ac71c48) --- src/settings/plugins/keyfile/plugin.c | 34 +++++++++++++-------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index e79ad551b8..f27aea9db3 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -100,6 +100,23 @@ remove_connection (SCPluginKeyfile *self, NMKeyfileConnection *connection) g_return_if_fail (removed); } +static NMKeyfileConnection * +find_by_path (SCPluginKeyfile *self, const char *path) +{ + SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (self); + GHashTableIter iter; + NMSettingsConnection *candidate = NULL; + + g_return_val_if_fail (path != NULL, NULL); + + g_hash_table_iter_init (&iter, priv->connections); + while (g_hash_table_iter_next (&iter, NULL, (gpointer) &candidate)) { + if (g_strcmp0 (path, nm_settings_connection_get_filename (candidate)) == 0) + return NM_KEYFILE_CONNECTION (candidate); + } + return NULL; +} + static void update_connection (SCPluginKeyfile *self, NMKeyfileConnection *connection, @@ -136,23 +153,6 @@ update_connection (SCPluginKeyfile *self, g_object_unref (tmp); } -static NMKeyfileConnection * -find_by_path (SCPluginKeyfile *self, const char *path) -{ - SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (self); - GHashTableIter iter; - NMSettingsConnection *candidate = NULL; - - g_return_val_if_fail (path != NULL, NULL); - - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, NULL, (gpointer) &candidate)) { - if (g_strcmp0 (path, nm_settings_connection_get_filename (candidate)) == 0) - return NM_KEYFILE_CONNECTION (candidate); - } - return NULL; -} - static void new_connection (SCPluginKeyfile *self, const char *name, From b21d0b68fed00ed2e29df190eb4f6e5670915878 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Dec 2014 21:53:24 +0100 Subject: [PATCH 08/19] keyfile: merge update_connection() and new_connection() new_connection() and update_connection() are very similar as both must anticipate collisions of UUIDs. When reloading a connection (update_connection(), previously), the loaded connection for a certain path might actually replace another existing connection. In this case, the old connection must be removed, and the existing one updated instead. If reloading a connection changes the UUID to a new value, the old connection must be removed likewise and a new connection added. Merge both functions into update_connection(). (cherry picked from commit c2fcb680f85911fcec511ba105241cae3b2a0764) --- src/settings/plugins/keyfile/plugin.c | 198 ++++++++++++-------------- 1 file changed, 93 insertions(+), 105 deletions(-) diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index f27aea9db3..26fc8608d9 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -117,90 +117,91 @@ find_by_path (SCPluginKeyfile *self, const char *path) return NULL; } -static void +/* update_connection: + * @self: the plugin instance + * @full_path: the filename of the keyfile to be loaded + * @connection: an existing connection that might be updated. + * If given, @connection must be an existing connection that is currently + * owned by the plugin. + * + * Loads a connection from file @full_path. This can both be used to + * load a connection initially or to update an existing connection. + * + * If you pass in an existing connection and the reloaded file happens + * to have a different UUID, the connection is deleted. + * Beware, that means that after the function, you have a dangling pointer + * if the returned connection is different from @connection. + * + * Returns: the updated connection. + * */ +static NMKeyfileConnection * update_connection (SCPluginKeyfile *self, - NMKeyfileConnection *connection, - const char *name) -{ - NMKeyfileConnection *tmp; - GError *error = NULL; - - tmp = nm_keyfile_connection_new (NULL, name, &error); - if (!tmp) { - /* Error; remove the connection */ - nm_log_warn (LOGD_SETTINGS, " error in connection %s: %s", name, - (error && error->message) ? error->message : "(unknown)"); - g_clear_error (&error); - remove_connection (self, connection); - return; - } - - if (!nm_connection_compare (NM_CONNECTION (connection), - NM_CONNECTION (tmp), - NM_SETTING_COMPARE_FLAG_IGNORE_AGENT_OWNED_SECRETS | - NM_SETTING_COMPARE_FLAG_IGNORE_NOT_SAVED_SECRETS)) { - nm_log_info (LOGD_SETTINGS, "updating %s", name); - if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection), - NM_CONNECTION (tmp), - FALSE, /* don't set Unsaved */ - "keyfile-update", - &error)) { - /* Shouldn't ever get here as 'new' was verified by the reader already */ - g_assert_not_reached (); - } - g_assert_no_error (error); - } - g_object_unref (tmp); -} - -static void -new_connection (SCPluginKeyfile *self, - const char *name, - char **out_old_path) + const char *full_path, + NMKeyfileConnection *connection) { SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (self); - NMKeyfileConnection *tmp; - NMSettingsConnection *connection; + NMKeyfileConnection *connection_new; + NMKeyfileConnection *connection_by_uuid; GError *error = NULL; const char *uuid; - if (out_old_path) - *out_old_path = NULL; - - tmp = nm_keyfile_connection_new (NULL, name, &error); - if (!tmp) { - nm_log_warn (LOGD_SETTINGS, " error in connection %s: %s", name, + connection_new = nm_keyfile_connection_new (NULL, full_path, &error); + if (!connection_new) { + /* Error; remove the connection */ + nm_log_warn (LOGD_SETTINGS, " error in connection %s: %s", full_path, (error && error->message) ? error->message : "(unknown)"); g_clear_error (&error); - return; + if (connection) + remove_connection (self, connection); + return NULL; } - /* Connection renames will show as different paths but same UUID */ - uuid = nm_connection_get_uuid (NM_CONNECTION (tmp)); - connection = g_hash_table_lookup (priv->connections, uuid); - if (connection) { - nm_log_info (LOGD_SETTINGS, "rename %s -> %s", nm_settings_connection_get_filename (connection), name); - if (!nm_settings_connection_replace_settings (connection, - NM_CONNECTION (tmp), + 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) { + /* The new connection has a different UUID then the original one. + * Remove @connection. */ + remove_connection (self, connection); + } + + if ( connection_by_uuid + && 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)) { + /* Nothing to do... except updating the path. */ + nm_settings_connection_set_filename (NM_SETTINGS_CONNECTION (connection_by_uuid), full_path); + g_object_unref (connection_new); + return connection_by_uuid; + } + + if (connection_by_uuid) { + /* An existing connection changed. */ + nm_log_info (LOGD_SETTINGS, "updating %s", full_path); + if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection_by_uuid), + NM_CONNECTION (connection_new), FALSE, /* don't set Unsaved */ - "keyfile-update-new", + "keyfile-update", &error)) { - /* Shouldn't ever get here as 'tmp' was verified by the reader already */ + /* Shouldn't ever get here as 'connection_new' was verified by the reader already */ g_assert_not_reached (); } g_assert_no_error (error); - g_object_unref (tmp); - if (out_old_path) - *out_old_path = g_strdup (nm_settings_connection_get_filename (connection)); - nm_settings_connection_set_filename (connection, name); + nm_settings_connection_set_filename (NM_SETTINGS_CONNECTION (connection_by_uuid), full_path); + g_object_unref (connection_new); + return connection_by_uuid; } else { - nm_log_info (LOGD_SETTINGS, "new connection %s", name); - g_hash_table_insert (priv->connections, g_strdup (uuid), tmp); - g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, tmp); + nm_log_info (LOGD_SETTINGS, "new connection %s", full_path); + g_hash_table_insert (priv->connections, g_strdup (uuid), connection_new); - g_signal_connect (tmp, NM_SETTINGS_CONNECTION_REMOVED, + g_signal_connect (connection_new, NM_SETTINGS_CONNECTION_REMOVED, G_CALLBACK (connection_removed_cb), self); + + g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection_new); + return connection_new; } } @@ -231,10 +232,7 @@ dir_changed (GFileMonitor *monitor, break; case G_FILE_MONITOR_EVENT_CREATED: case G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT: - if (connection) - update_connection (SC_PLUGIN_KEYFILE (config), connection, full_path); - else - new_connection (SC_PLUGIN_KEYFILE (config), full_path, NULL); + update_connection (SC_PLUGIN_KEYFILE (config), full_path, connection); break; default: break; @@ -318,9 +316,11 @@ read_connections (NMSystemConfigInterface *config) GDir *dir; GError *error = NULL; const char *item; - GHashTable *oldconns; + GHashTable *alive_connections; GHashTableIter iter; - gpointer data; + NMKeyfileConnection *connection; + GPtrArray *dead_connections = NULL; + guint i; dir = g_dir_open (KEYFILE_DIR, 0, &error); if (!dir) { @@ -332,45 +332,39 @@ read_connections (NMSystemConfigInterface *config) 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, &data)) { - const char *con_path = nm_settings_connection_get_filename (data); - if (con_path) - g_hash_table_insert (oldconns, g_strdup (con_path), data); - } + alive_connections = g_hash_table_new (NULL, NULL); while ((item = g_dir_read_name (dir))) { - NMKeyfileConnection *connection; - char *full_path, *old_path; + char *full_path; if (nm_keyfile_plugin_utils_should_ignore_file (item)) continue; full_path = g_build_filename (KEYFILE_DIR, item, NULL); - - connection = g_hash_table_lookup (oldconns, full_path); - if (connection) { - g_hash_table_remove (oldconns, full_path); - update_connection (self, connection, full_path); - } else { - new_connection (self, full_path, &old_path); - if (old_path) { - g_hash_table_remove (oldconns, old_path); - g_free (old_path); - } - } - + connection = update_connection (self, full_path, NULL); g_free (full_path); + + if (connection) + g_hash_table_add (alive_connections, connection); } g_dir_close (dir); - g_hash_table_iter_init (&iter, oldconns); - while (g_hash_table_iter_next (&iter, NULL, &data)) { - g_hash_table_iter_remove (&iter); - remove_connection (self, data); + 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); + + if (dead_connections) { + for (i = 0; i < dead_connections->len; i++) + remove_connection (self, dead_connections->pdata[i]); + g_ptr_array_free (dead_connections, TRUE); } - g_hash_table_destroy (oldconns); } /* Plugin */ @@ -404,13 +398,7 @@ load_connection (NMSystemConfigInterface *config, if (nm_keyfile_plugin_utils_should_ignore_file (filename + dir_len + 1)) return FALSE; - connection = find_by_path (self, filename); - if (connection) - update_connection (self, connection, filename); - else { - new_connection (self, filename, NULL); - connection = find_by_path (self, filename); - } + connection = update_connection (self, filename, find_by_path (self, filename)); return (connection != NULL); } From 1b0fd9fae08b5ef1524001753f39f9e37d06e44f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 9 Dec 2014 13:50:05 +0100 Subject: [PATCH 09/19] keyfile: read_connections() must skip duplicate connections If there are keyfiles with duplicate UUIDs, read_connections() would iterate over the files, loading them as they appear and overwriting duplicate connections that were just loaded. For example, have keyfiles 'A' and 'B' with the same UUID. On start, NM might first load 'A', then 'B'. 'B' would replace the content of 'A' which was just loaded. On reload, NM would first overwrite 'B' with 'A', and then again overwriting 'A' with 'B'. Fix that by accept the first found connection and don't overwrite it during the same read_connections() run. Also sort the files by file modification timestamp so that we get a reproducible and sensible behavior. (cherry picked from commit 8a4e64c6aa5068ecb80d7d0d597a77a37048436a) --- src/settings/plugins/keyfile/plugin.c | 80 ++++++++++++++++++++++++--- 1 file changed, 71 insertions(+), 9 deletions(-) diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index 26fc8608d9..67f49d6d85 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -123,6 +123,8 @@ find_by_path (SCPluginKeyfile *self, const char *path) * @connection: an existing connection that might be updated. * If given, @connection must be an existing connection that is currently * owned by the plugin. + * @protected_connections: (allow-none): if given, we only update an + * existing connection if it is not contained in this hash. * * Loads a connection from file @full_path. This can both be used to * load a connection initially or to update an existing connection. @@ -137,7 +139,8 @@ find_by_path (SCPluginKeyfile *self, const char *path) static NMKeyfileConnection * update_connection (SCPluginKeyfile *self, const char *full_path, - NMKeyfileConnection *connection) + NMKeyfileConnection *connection, + GHashTable *protected_connections) { SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (self); NMKeyfileConnection *connection_new; @@ -166,6 +169,14 @@ update_connection (SCPluginKeyfile *self, remove_connection (self, connection); } + if ( connection_by_uuid + && protected_connections + && g_hash_table_contains (protected_connections, connection_by_uuid)) { + nm_log_warn (LOGD_SETTINGS, "keyfile: cannot load %s due to conflicting UUID for %s", full_path, nm_connection_get_uuid (NM_CONNECTION (connection_by_uuid))); + g_object_unref (connection_new); + return NULL; + } + if ( connection_by_uuid && nm_connection_compare (NM_CONNECTION (connection_by_uuid), NM_CONNECTION (connection_new), @@ -179,7 +190,9 @@ update_connection (SCPluginKeyfile *self, if (connection_by_uuid) { /* An existing connection changed. */ + nm_log_info (LOGD_SETTINGS, "updating %s", full_path); + if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection_by_uuid), NM_CONNECTION (connection_new), FALSE, /* don't set Unsaved */ @@ -232,7 +245,7 @@ dir_changed (GFileMonitor *monitor, break; case G_FILE_MONITOR_EVENT_CREATED: case G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT: - update_connection (SC_PLUGIN_KEYFILE (config), full_path, connection); + update_connection (SC_PLUGIN_KEYFILE (config), full_path, connection, NULL); break; default: break; @@ -308,6 +321,43 @@ setup_monitoring (NMSystemConfigInterface *config) } } +static GHashTable * +_paths_from_connections (GHashTable *connections) +{ + GHashTableIter iter; + NMKeyfileConnection *connection; + GHashTable *paths = g_hash_table_new (g_str_hash, g_str_equal); + + g_hash_table_iter_init (&iter, connections); + while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &connection)) { + const char *path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection)); + + if (path) + g_hash_table_add (paths, (void *) path); + } + return paths; +} + +static int +_sort_paths (const char **f1, const char **f2, GHashTable *paths) +{ + struct stat st; + gboolean c1, c2; + gint64 m1, m2; + + c1 = !!g_hash_table_contains (paths, *f1); + c2 = !!g_hash_table_contains (paths, *f2); + if (c1 != c2) + return c1 ? -1 : 1; + + m1 = stat (*f1, &st) == 0 ? (gint64) st.st_mtime : G_MININT64; + m2 = stat (*f2, &st) == 0 ? (gint64) st.st_mtime : G_MININT64; + if (m1 != m2) + return m1 > m2 ? -1 : 1; + + return strcmp (*f1, *f2); +} + static void read_connections (NMSystemConfigInterface *config) { @@ -321,6 +371,8 @@ read_connections (NMSystemConfigInterface *config) NMKeyfileConnection *connection; GPtrArray *dead_connections = NULL; guint i; + GPtrArray *filenames; + GHashTable *paths; dir = g_dir_open (KEYFILE_DIR, 0, &error); if (!dir) { @@ -334,20 +386,30 @@ read_connections (NMSystemConfigInterface *config) 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))) { - char *full_path; - if (nm_keyfile_plugin_utils_should_ignore_file (item)) continue; + g_ptr_array_add (filenames, g_build_filename (KEYFILE_DIR, item, NULL)); + } + g_dir_close (dir); - full_path = g_build_filename (KEYFILE_DIR, item, NULL); - connection = update_connection (self, full_path, NULL); - g_free (full_path); + /* While reloading, we don't replace connections that we already loaded while + * iterating over the files. + * + * To have sensible, reproducible behavior, sort the paths by last modification + * time prefering older files. + */ + paths = _paths_from_connections (priv->connections); + g_ptr_array_sort_with_data (filenames, (GCompareDataFunc) _sort_paths, paths); + g_hash_table_destroy (paths); + for (i = 0; i < filenames->len; i++) { + connection = update_connection (self, filenames->pdata[i], NULL, alive_connections); if (connection) g_hash_table_add (alive_connections, connection); } - g_dir_close (dir); + g_ptr_array_free (filenames, TRUE); g_hash_table_iter_init (&iter, priv->connections); while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &connection)) { @@ -398,7 +460,7 @@ load_connection (NMSystemConfigInterface *config, if (nm_keyfile_plugin_utils_should_ignore_file (filename + dir_len + 1)) return FALSE; - connection = update_connection (self, filename, find_by_path (self, filename)); + connection = update_connection (self, filename, find_by_path (self, filename), NULL); return (connection != NULL); } From 84353078c102835abf61eb70ed196646d0bb8b9f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 9 Dec 2014 12:15:18 +0100 Subject: [PATCH 10/19] keyfile: cleanup logging Log lines with a "keyfile:" prefix and show more information about the loaded connection. Especially printing the UUID is interesting. (cherry picked from commit 5c2fa920995f2170098ebc6cd3acdb6a978d1a6e) --- src/settings/plugins/keyfile/plugin.c | 30 +++++++++++++++++---------- src/settings/plugins/keyfile/utils.h | 7 +++++++ 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index 67f49d6d85..25a4630c01 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -87,7 +87,7 @@ remove_connection (SCPluginKeyfile *self, NMKeyfileConnection *connection) g_return_if_fail (connection != NULL); - nm_log_info (LOGD_SETTINGS, "removed %s.", nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection))); + nm_log_info (LOGD_SETTINGS, "keyfile: removed " NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection)); /* Removing from the hash table should drop the last reference */ g_object_ref (connection); @@ -151,8 +151,7 @@ update_connection (SCPluginKeyfile *self, connection_new = nm_keyfile_connection_new (NULL, full_path, &error); if (!connection_new) { /* Error; remove the connection */ - nm_log_warn (LOGD_SETTINGS, " error in connection %s: %s", full_path, - (error && error->message) ? error->message : "(unknown)"); + nm_log_warn (LOGD_SETTINGS, "keyfile: error loading connection from file %s: %s", full_path, error->message); g_clear_error (&error); if (connection) remove_connection (self, connection); @@ -172,7 +171,7 @@ update_connection (SCPluginKeyfile *self, if ( connection_by_uuid && protected_connections && g_hash_table_contains (protected_connections, connection_by_uuid)) { - nm_log_warn (LOGD_SETTINGS, "keyfile: cannot load %s due to conflicting UUID for %s", full_path, nm_connection_get_uuid (NM_CONNECTION (connection_by_uuid))); + nm_log_warn (LOGD_SETTINGS, "keyfile: cannot load %s due to conflicting UUID for "NM_KEYFILE_CONNECTION_LOG_FMT, full_path, NM_KEYFILE_CONNECTION_LOG_ARG (connection_by_uuid)); g_object_unref (connection_new); return NULL; } @@ -189,9 +188,17 @@ update_connection (SCPluginKeyfile *self, } if (connection_by_uuid) { + const char *old_path; + /* An existing connection changed. */ - nm_log_info (LOGD_SETTINGS, "updating %s", full_path); + old_path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection_by_uuid)); + if (!g_strcmp0 (old_path, nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection_new)))) + nm_log_info (LOGD_SETTINGS, "keyfile: update "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); + else if (old_path) + nm_log_info (LOGD_SETTINGS, "keyfile: rename \"%s\" to "NM_KEYFILE_CONNECTION_LOG_FMT, old_path, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); + else + nm_log_info (LOGD_SETTINGS, "keyfile: update and persist "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection_by_uuid), NM_CONNECTION (connection_new), @@ -206,7 +213,7 @@ update_connection (SCPluginKeyfile *self, g_object_unref (connection_new); return connection_by_uuid; } else { - nm_log_info (LOGD_SETTINGS, "new connection %s", full_path); + nm_log_info (LOGD_SETTINGS, "keyfile: new connection "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_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, @@ -376,7 +383,7 @@ read_connections (NMSystemConfigInterface *config) dir = g_dir_open (KEYFILE_DIR, 0, &error); if (!dir) { - nm_log_warn (LOGD_SETTINGS, "Cannot read directory '%s': (%d) %s", + nm_log_warn (LOGD_SETTINGS, "keyfile: cannot read directory '%s': (%d) %s", KEYFILE_DIR, error ? error->code : -1, error && error->message ? error->message : "(unknown)"); @@ -489,6 +496,7 @@ add_connection (NMSystemConfigInterface *config, added = (NMSettingsConnection *) nm_keyfile_connection_new (connection, path, error); if (added) { + nm_log_info (LOGD_SETTINGS, "keyfile: add "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG ((NMKeyfileConnection *) added)); g_hash_table_insert (priv->connections, g_strdup (nm_connection_get_uuid (NM_CONNECTION (added))), added); @@ -555,7 +563,7 @@ get_unmanaged_specs (NMSystemConfigInterface *config) } else if (!strncmp (udis[i], "interface-name:", 15) && nm_utils_iface_valid_name (udis[i] + 15)) { specs = g_slist_append (specs, udis[i]); } else { - nm_log_warn (LOGD_SETTINGS, "Error in file '%s': invalid unmanaged-devices entry: '%s'", priv->conf_file, udis[i]); + nm_log_warn (LOGD_SETTINGS, "keyfile: error in file '%s': invalid unmanaged-devices entry: '%s'", priv->conf_file, udis[i]); g_free (udis[i]); } } @@ -565,7 +573,7 @@ get_unmanaged_specs (NMSystemConfigInterface *config) out: if (error) { - nm_log_warn (LOGD_SETTINGS, "%s", error->message); + nm_log_warn (LOGD_SETTINGS, "keyfile: error getting unmanaged specs: %s", error->message); g_error_free (error); } if (key_file) @@ -593,7 +601,7 @@ plugin_get_hostname (SCPluginKeyfile *plugin) out: if (error) { - nm_log_warn (LOGD_SETTINGS, "%s", error->message); + nm_log_warn (LOGD_SETTINGS, "keyfile: error getting hostname: %s", error->message); g_error_free (error); } if (key_file) @@ -640,7 +648,7 @@ plugin_set_hostname (SCPluginKeyfile *plugin, const char *hostname) out: if (error) { - nm_log_warn (LOGD_SETTINGS, "%s", error->message); + nm_log_warn (LOGD_SETTINGS, "keyfile: error setting hostname: %s", error->message); g_error_free (error); } g_free (data); diff --git a/src/settings/plugins/keyfile/utils.h b/src/settings/plugins/keyfile/utils.h index 1a7c2502b0..d153367103 100644 --- a/src/settings/plugins/keyfile/utils.h +++ b/src/settings/plugins/keyfile/utils.h @@ -23,6 +23,13 @@ #include #include "common.h" +#include "NetworkManagerUtils.h" + +#define NM_KEYFILE_CONNECTION_LOG_PATH(path) str_if_set (path,"in-memory") +#define NM_KEYFILE_CONNECTION_LOG_FMT "%s (%s,\"%s\")" +#define NM_KEYFILE_CONNECTION_LOG_ARG(con) NM_KEYFILE_CONNECTION_LOG_PATH (nm_settings_connection_get_filename ((NMSettingsConnection *) (con))), nm_connection_get_uuid ((NMConnection *) (con)), nm_connection_get_id ((NMConnection *) (con)) +#define NM_KEYFILE_CONNECTION_LOG_FMTD "%s (%s,\"%s\",%p)" +#define NM_KEYFILE_CONNECTION_LOG_ARGD(con) NM_KEYFILE_CONNECTION_LOG_PATH (nm_settings_connection_get_filename ((NMSettingsConnection *) (con))), nm_connection_get_uuid ((NMConnection *) (con)), nm_connection_get_id ((NMConnection *) (con)), (con) gboolean nm_keyfile_plugin_utils_should_ignore_file (const char *filename); From 8c97626918357210940250a325b3d94919e0c9c6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 10 Dec 2014 11:23:20 +0100 Subject: [PATCH 11/19] keyfile: reuse duplicate check from update_connection() in add_connection() Also during add_connection() we must take special care of not "adding" a connection with a conflicting UUID. In that case we want to fallback to "update". update_connection() already does all the checks, so call update_connection() from add_connection(). (cherry picked from commit db5c4ce64ffd32b339dd59aae6394396d800d4f3) --- src/settings/plugins/keyfile/plugin.c | 156 ++++++++++++++++---------- 1 file changed, 99 insertions(+), 57 deletions(-) diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index 25a4630c01..2a5820226e 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -45,6 +45,7 @@ #include "writer.h" #include "common.h" #include "utils.h" +#include "gsystem-local-alloc.h" static char *plugin_get_hostname (SCPluginKeyfile *plugin); static void system_config_interface_init (NMSystemConfigInterface *system_config_interface_class); @@ -119,12 +120,24 @@ find_by_path (SCPluginKeyfile *self, const char *path) /* update_connection: * @self: the plugin instance + * @source: if %NULL, this re-reads the connection from @full_path + * and updates it. When passing @source, this adds a connection from + * memory. * @full_path: the filename of the keyfile to be loaded * @connection: an existing connection that might be updated. * If given, @connection must be an existing connection that is currently * owned by the plugin. + * @protect_existing_connection: if %TRUE, and !@connection, we don't allow updating + * an existing connection with the same UUID. + * If %TRUE and @connection, allow updating only if the reload would modify + * @connection (without changing its UUID) or if we would create a new connection. + * In other words, if this paramter is %TRUE, we only allow creating a + * new connection (with an unseen UUID) or updating the passed in @connection + * (whereas the UUID cannot change). + * Note, that this allows for @connection to be replaced by a new connection. * @protected_connections: (allow-none): if given, we only update an * existing connection if it is not contained in this hash. + * @error: error in case of failure * * Loads a connection from file @full_path. This can both be used to * load a connection initially or to update an existing connection. @@ -138,23 +151,37 @@ find_by_path (SCPluginKeyfile *self, const char *path) * */ static NMKeyfileConnection * update_connection (SCPluginKeyfile *self, + NMConnection *source, const char *full_path, NMKeyfileConnection *connection, - GHashTable *protected_connections) + gboolean protect_existing_connection, + GHashTable *protected_connections, + GError **error) { SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (self); NMKeyfileConnection *connection_new; NMKeyfileConnection *connection_by_uuid; - GError *error = NULL; + GError *local = NULL; const char *uuid; - connection_new = nm_keyfile_connection_new (NULL, full_path, &error); + g_return_val_if_fail (!source || NM_IS_CONNECTION (source), NULL); + g_return_val_if_fail (full_path || source, NULL); + + if (full_path) + nm_log_dbg (LOGD_SETTINGS, "keyfile: loading from file \"%s\"...", full_path); + + connection_new = nm_keyfile_connection_new (source, full_path, &local); if (!connection_new) { /* Error; remove the connection */ - nm_log_warn (LOGD_SETTINGS, "keyfile: error loading connection from file %s: %s", full_path, error->message); - g_clear_error (&error); - if (connection) + if (source) + nm_log_warn (LOGD_SETTINGS, "keyfile: error creating connection %s: %s", nm_connection_get_uuid (source), local->message); + else + nm_log_warn (LOGD_SETTINGS, "keyfile: error loading connection from file %s: %s", full_path, local->message); + if ( connection + && !protect_existing_connection + && (!protected_connections || !g_hash_table_contains (protected_connections, connection))) remove_connection (self, connection); + g_propagate_error (error, local); return NULL; } @@ -163,64 +190,91 @@ update_connection (SCPluginKeyfile *self, if ( connection && connection != connection_by_uuid) { + + if ( (protect_existing_connection && connection_by_uuid != NULL) + || (protected_connections && g_hash_table_contains (protected_connections, connection))) { + NMKeyfileConnection *conflicting = (protect_existing_connection && connection_by_uuid != NULL) ? connection_by_uuid : connection; + + if (source) + nm_log_warn (LOGD_SETTINGS, "keyfile: cannot update protected "NM_KEYFILE_CONNECTION_LOG_FMT" connection due to conflicting UUID %s", NM_KEYFILE_CONNECTION_LOG_ARG (conflicting), uuid); + else + nm_log_warn (LOGD_SETTINGS, "keyfile: cannot load %s due to conflicting UUID for "NM_KEYFILE_CONNECTION_LOG_FMT, full_path, NM_KEYFILE_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; + } + /* The new connection has a different UUID then the original one. * Remove @connection. */ remove_connection (self, connection); } if ( connection_by_uuid - && protected_connections - && g_hash_table_contains (protected_connections, connection_by_uuid)) { - nm_log_warn (LOGD_SETTINGS, "keyfile: cannot load %s due to conflicting UUID for "NM_KEYFILE_CONNECTION_LOG_FMT, full_path, NM_KEYFILE_CONNECTION_LOG_ARG (connection_by_uuid)); + && ( (!connection && protect_existing_connection) + || (protected_connections && g_hash_table_contains (protected_connections, connection_by_uuid)))) { + if (source) + nm_log_warn (LOGD_SETTINGS, "keyfile: cannot update connection due to conflicting UUID for "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection_by_uuid)); + else + nm_log_warn (LOGD_SETTINGS, "keyfile: cannot load %s due to conflicting UUID for "NM_KEYFILE_CONNECTION_LOG_FMT, full_path, NM_KEYFILE_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; } - if ( connection_by_uuid - && 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)) { - /* Nothing to do... except updating the path. */ - nm_settings_connection_set_filename (NM_SETTINGS_CONNECTION (connection_by_uuid), full_path); - g_object_unref (connection_new); - return connection_by_uuid; - } - if (connection_by_uuid) { const char *old_path; - /* An existing connection changed. */ - old_path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection_by_uuid)); - if (!g_strcmp0 (old_path, nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection_new)))) - nm_log_info (LOGD_SETTINGS, "keyfile: update "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); - else if (old_path) - nm_log_info (LOGD_SETTINGS, "keyfile: rename \"%s\" to "NM_KEYFILE_CONNECTION_LOG_FMT, old_path, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); - else - nm_log_info (LOGD_SETTINGS, "keyfile: update and persist "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); - if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection_by_uuid), - NM_CONNECTION (connection_new), - FALSE, /* don't set Unsaved */ - "keyfile-update", - &error)) { - /* Shouldn't ever get here as 'connection_new' was verified by the reader already */ - g_assert_not_reached (); + if (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)) { + /* Nothing to do... except updating the path. */ + if (old_path && g_strcmp0 (old_path, full_path) != 0) + nm_log_info (LOGD_SETTINGS, "keyfile: rename \"%s\" to "NM_KEYFILE_CONNECTION_LOG_FMT" without other changes", old_path, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); + } else { + /* An existing connection changed. */ + if (source) + nm_log_info (LOGD_SETTINGS, "keyfile: update "NM_KEYFILE_CONNECTION_LOG_FMT" from %s", NM_KEYFILE_CONNECTION_LOG_ARG (connection_new), NM_KEYFILE_CONNECTION_LOG_PATH (old_path)); + else if (!g_strcmp0 (old_path, nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection_new)))) + nm_log_info (LOGD_SETTINGS, "keyfile: update "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); + else if (old_path) + nm_log_info (LOGD_SETTINGS, "keyfile: rename \"%s\" to "NM_KEYFILE_CONNECTION_LOG_FMT, old_path, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); + else + nm_log_info (LOGD_SETTINGS, "keyfile: update and persist "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); + + if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection_by_uuid), + NM_CONNECTION (connection_new), + FALSE, /* don't set Unsaved */ + "keyfile-update", + &local)) { + /* Shouldn't ever get here as 'connection_new' was verified by the reader already */ + g_assert_not_reached (); + } + g_assert_no_error (local); } - g_assert_no_error (error); nm_settings_connection_set_filename (NM_SETTINGS_CONNECTION (connection_by_uuid), full_path); g_object_unref (connection_new); return connection_by_uuid; } else { - nm_log_info (LOGD_SETTINGS, "keyfile: new connection "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); + if (source) + nm_log_info (LOGD_SETTINGS, "keyfile: add connection "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG (connection_new)); + else + nm_log_info (LOGD_SETTINGS, "keyfile: new connection "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_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); - g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection_new); + 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. */ + g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection_new); + } return connection_new; } } @@ -243,6 +297,8 @@ dir_changed (GFileMonitor *monitor, return; } + nm_log_dbg (LOGD_SETTINGS, "dir_changed(%s) = %d", full_path, event_type); + connection = find_by_path (self, full_path); switch (event_type) { @@ -252,7 +308,7 @@ dir_changed (GFileMonitor *monitor, break; case G_FILE_MONITOR_EVENT_CREATED: case G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT: - update_connection (SC_PLUGIN_KEYFILE (config), full_path, connection, NULL); + update_connection (SC_PLUGIN_KEYFILE (config), NULL, full_path, connection, TRUE, NULL, NULL); break; default: break; @@ -412,7 +468,7 @@ read_connections (NMSystemConfigInterface *config) g_hash_table_destroy (paths); for (i = 0; i < filenames->len; i++) { - connection = update_connection (self, filenames->pdata[i], NULL, alive_connections); + connection = update_connection (self, NULL, filenames->pdata[i], NULL, FALSE, alive_connections, NULL); if (connection) g_hash_table_add (alive_connections, connection); } @@ -467,7 +523,7 @@ load_connection (NMSystemConfigInterface *config, if (nm_keyfile_plugin_utils_should_ignore_file (filename + dir_len + 1)) return FALSE; - connection = update_connection (self, filename, find_by_path (self, filename), NULL); + connection = update_connection (self, NULL, filename, find_by_path (self, filename), TRUE, NULL, NULL); return (connection != NULL); } @@ -485,27 +541,13 @@ add_connection (NMSystemConfigInterface *config, GError **error) { SCPluginKeyfile *self = SC_PLUGIN_KEYFILE (config); - SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (self); - NMSettingsConnection *added = NULL; - char *path = NULL; + gs_free char *path = NULL; if (save_to_disk) { if (!nm_keyfile_plugin_write_connection (connection, NULL, &path, error)) return NULL; } - - added = (NMSettingsConnection *) nm_keyfile_connection_new (connection, path, error); - if (added) { - nm_log_info (LOGD_SETTINGS, "keyfile: add "NM_KEYFILE_CONNECTION_LOG_FMT, NM_KEYFILE_CONNECTION_LOG_ARG ((NMKeyfileConnection *) added)); - g_hash_table_insert (priv->connections, - g_strdup (nm_connection_get_uuid (NM_CONNECTION (added))), - added); - g_signal_connect (added, NM_SETTINGS_CONNECTION_REMOVED, - G_CALLBACK (connection_removed_cb), - self); - } - g_free (path); - return added; + return NM_SETTINGS_CONNECTION (update_connection (self, connection, path, NULL, FALSE, NULL, error)); } static gboolean From 6cd264b0d5861f00a6411702cb245207ecec6262 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 13 Jan 2015 14:09:23 +0100 Subject: [PATCH 12/19] keyfile: ignore GFileMonitor change flags in dir_changed() When writing a file (for example with `sed -i`) a temporary file might be created and removed quickly. This causes spurious events in dir_changed(). (cherry picked from commit 8ba8a55cfa7169d0ca7e03decbfe99983ad04c32) --- src/settings/plugins/keyfile/plugin.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index 2a5820226e..872de4bb8a 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -290,25 +290,28 @@ dir_changed (GFileMonitor *monitor, SCPluginKeyfile *self = SC_PLUGIN_KEYFILE (config); NMKeyfileConnection *connection; char *full_path; + gboolean exists; full_path = g_file_get_path (file); if (nm_keyfile_plugin_utils_should_ignore_file (full_path)) { g_free (full_path); return; } + exists = g_file_test (full_path, G_FILE_TEST_EXISTS); - nm_log_dbg (LOGD_SETTINGS, "dir_changed(%s) = %d", full_path, event_type); + nm_log_dbg (LOGD_SETTINGS, "dir_changed(%s) = %d; file %s", full_path, event_type, exists ? "exists" : "does not exist"); connection = find_by_path (self, full_path); switch (event_type) { case G_FILE_MONITOR_EVENT_DELETED: - if (connection) + if (!exists && connection) remove_connection (SC_PLUGIN_KEYFILE (config), connection); break; case G_FILE_MONITOR_EVENT_CREATED: case G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT: - update_connection (SC_PLUGIN_KEYFILE (config), NULL, full_path, connection, TRUE, NULL, NULL); + if (exists) + update_connection (SC_PLUGIN_KEYFILE (config), NULL, full_path, connection, TRUE, NULL, NULL); break; default: break; From 7510e67f0ae02a89151cec9c38608c4842be6c77 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Dec 2014 19:02:19 +0100 Subject: [PATCH 13/19] settings: avoid duplicate UUID in settings When adding a connection to NMSettings we did not check for duplicate connection UUIDs (which could for example happen if two different plugins report a conflicting UUID). Also, we would not check that an already added connection changes it's UUID. Both could lead to have duplicate connections (by UUID). Avoid that two ways: - when adding a connection to NMSettings, ensure that we don't add a conflicting UUID. Otherwise just bail out and do nothing. - when modifying a connection that is already added to NMSettings, enforce that the UUID cannot change. Otherwise fail with error. For ifcfg-rh plugin this situation still can happen during reload. In this case error out and refuse to update the connection. After all, the user configured invalid UUIDs. https://bugzilla.redhat.com/show_bug.cgi?id=1171751 (cherry picked from commit 7b807b11cca284e3edb26273a3ea575c7cd25a67) --- src/settings/nm-settings-connection.c | 9 +++++++++ src/settings/nm-settings.c | 18 ++++++++++++++++++ src/settings/plugins/ifcfg-rh/plugin.c | 24 ++++++++++++++++++++++-- src/settings/plugins/ifnet/plugin.c | 3 ++- src/settings/plugins/keyfile/plugin.c | 3 ++- 5 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index b5e1e56812..ec2052f0a0 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -461,6 +461,15 @@ nm_settings_connection_replace_settings (NMSettingsConnection *self, if (!nm_connection_normalize (new_connection, NULL, NULL, error)) return FALSE; + if ( nm_connection_get_path (NM_CONNECTION (self)) + && g_strcmp0 (nm_connection_get_uuid (NM_CONNECTION (self)), nm_connection_get_uuid (new_connection)) != 0) { + /* Updating the UUID is not allowed once the path is exported. */ + g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, + "connection %s cannot change the UUID from %s to %s", nm_connection_get_id (NM_CONNECTION (self)), + nm_connection_get_uuid (NM_CONNECTION (self)), nm_connection_get_uuid (new_connection)); + return FALSE; + } + /* Do nothing if there's nothing to update */ if (nm_connection_compare (NM_CONNECTION (self), new_connection, diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index bc123b4686..9d40b06e89 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -887,6 +887,7 @@ claim_connection (NMSettings *self, GHashTableIter iter; gpointer data; char *path; + NMSettingsConnection *existing; g_return_if_fail (NM_IS_SETTINGS_CONNECTION (connection)); g_return_if_fail (nm_connection_get_path (NM_CONNECTION (connection)) == NULL); @@ -905,6 +906,23 @@ claim_connection (NMSettings *self, return; } + existing = nm_settings_get_connection_by_uuid (self, nm_connection_get_uuid (NM_CONNECTION (connection))); + if (existing) { + /* Cannot add duplicate connections per UUID. Just return without action and + * log a warning. + * + * This means, that plugins must not provide duplicate connections (UUID). + * In fact, none of the plugins currently would do that. + * + * But globaly, over different setting plugins, there could be duplicates + * without the individual plugins being aware. Don't handle that at all, just + * error out. That should not happen unless the admin misconfigured the system + * to create conflicting connections. */ + nm_log_warn (LOGD_SETTINGS, "plugin provided duplicate connection with UUID %s", + nm_connection_get_uuid (NM_CONNECTION (connection))); + return; + } + /* Read timestamp from look-aside file and put it into the connection's data */ nm_settings_connection_read_and_fill_timestamp (connection); diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index e608a29f88..97d3a64692 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -129,6 +129,7 @@ _internal_new_connection (SCPluginIfcfg *self, SCPluginIfcfgPrivate *priv = SC_PLUGIN_IFCFG_GET_PRIVATE (self); NMIfcfgConnection *connection; const char *cid; + const char *uuid; if (!source) nm_log_info (LOGD_SETTINGS, "parsing %s ... ", path); @@ -137,11 +138,20 @@ _internal_new_connection (SCPluginIfcfg *self, 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 (nm_connection_get_uuid (NM_CONNECTION (connection))), + g_strdup (uuid), connection); nm_log_info (LOGD_SETTINGS, " read connection '%s'", cid); g_signal_connect (connection, NM_SETTINGS_CONNECTION_REMOVED, @@ -302,6 +312,15 @@ connection_new_or_changed (SCPluginIfcfg *self, return; } + if (g_strcmp0 (nm_connection_get_uuid (NM_CONNECTION (existing)), nm_connection_get_uuid (NM_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(). */ + nm_log_warn (LOGD_SETTINGS, "UUID changes are not supported. Cannot update connection %s (%s)", nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (new)), nm_connection_get_uuid (NM_CONNECTION (new))); + g_object_unref (new); + return; + } + nm_log_info (LOGD_SETTINGS, "updating %s", path); g_object_set (existing, NM_IFCFG_CONNECTION_UNMANAGED_SPEC, new_unmanaged, @@ -342,7 +361,8 @@ connection_new_or_changed (SCPluginIfcfg *self, FALSE, /* don't set Unsaved */ "ifcfg-rh-update", &error)) { - /* Shouldn't ever get here as 'new' was verified by the reader already */ + /* Shouldn't ever get here as 'new' was verified by the reader already + * and the UUID did not change. */ g_assert_not_reached (); } g_assert_no_error (error); diff --git a/src/settings/plugins/ifnet/plugin.c b/src/settings/plugins/ifnet/plugin.c index 218e44aa08..33711639da 100644 --- a/src/settings/plugins/ifnet/plugin.c +++ b/src/settings/plugins/ifnet/plugin.c @@ -315,7 +315,8 @@ reload_connections (NMSystemConfigInterface *config) FALSE, /* don't set Unsaved */ "ifnet-update", &error)) { - /* Shouldn't ever get here as 'new' was verified by the reader already */ + /* Shouldn't ever get here as 'new' was verified by the reader already + * and the UUID did not change. */ g_assert_not_reached (); } g_assert_no_error (error); diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index 872de4bb8a..fcdb3c3cf0 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -251,7 +251,8 @@ update_connection (SCPluginKeyfile *self, FALSE, /* don't set Unsaved */ "keyfile-update", &local)) { - /* Shouldn't ever get here as 'connection_new' was verified by the reader already */ + /* 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); From 0516b55de2a71de5c0041880494259037ea12b6b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 Dec 2014 14:12:13 +0100 Subject: [PATCH 14/19] ifcfg-rh: add logging macros _LOGX() to plugin.c (cherry picked from commit bbaa243e3168fca56d793d66f784857875582bfb) --- src/settings/plugins/ifcfg-rh/plugin.c | 56 +++++++++++++++++--------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index 97d3a64692..aa495495ed 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -62,6 +62,24 @@ #define DBUS_SERVICE_NAME "com.redhat.ifcfgrh1" #define DBUS_OBJECT_PATH "/com/redhat/ifcfgrh1" + +#define _LOG_DEFAULT_DOMAIN LOGD_SETTINGS + +#define _LOG(level, domain, ...) \ + G_STMT_START { \ + nm_log ((level), (domain), \ + "%s" _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + "ifcfg-rh: " \ + _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + } G_STMT_END + +#define _LOGT(...) _LOG (LOGL_TRACE, _LOG_DEFAULT_DOMAIN, __VA_ARGS__) +#define _LOGD(...) _LOG (LOGL_DEBUG, _LOG_DEFAULT_DOMAIN, __VA_ARGS__) +#define _LOGI(...) _LOG (LOGL_INFO, _LOG_DEFAULT_DOMAIN, __VA_ARGS__) +#define _LOGW(...) _LOG (LOGL_WARN, _LOG_DEFAULT_DOMAIN, __VA_ARGS__) +#define _LOGE(...) _LOG (LOGL_ERR, _LOG_DEFAULT_DOMAIN, __VA_ARGS__) + + static gboolean impl_ifcfgrh_get_ifcfg_details (SCPluginIfcfg *plugin, const char *in_ifcfg, const char **out_uuid, @@ -132,7 +150,7 @@ _internal_new_connection (SCPluginIfcfg *self, const char *uuid; if (!source) - nm_log_info (LOGD_SETTINGS, "parsing %s ... ", path); + _LOGI ("parsing %s ... ", path); connection = nm_ifcfg_connection_new (source, path, error); if (!connection) @@ -153,7 +171,7 @@ _internal_new_connection (SCPluginIfcfg *self, g_hash_table_insert (priv->connections, g_strdup (uuid), connection); - nm_log_info (LOGD_SETTINGS, " read connection '%s'", cid); + _LOGI (" read connection '%s'", cid); g_signal_connect (connection, NM_SETTINGS_CONNECTION_REMOVED, G_CALLBACK (connection_removed_cb), self); @@ -168,10 +186,10 @@ _internal_new_connection (SCPluginIfcfg *self, device_id++; else device_id = spec; - nm_log_warn (LOGD_SETTINGS, " Ignoring connection '%s' / device '%s' due to NM_CONTROLLED=no.", + _LOGW (" Ignoring connection '%s' / device '%s' due to NM_CONTROLLED=no.", cid, device_id); } else if (nm_ifcfg_connection_get_unrecognized_spec (connection)) { - nm_log_warn (LOGD_SETTINGS, " Ignoring connection '%s' of unrecognized type.", cid); + _LOGW (" Ignoring connection '%s' of unrecognized type.", cid); } /* watch changes of ifcfg hardlinks */ @@ -263,7 +281,7 @@ connection_new_or_changed (SCPluginIfcfg *self, existing = find_by_uuid_from_path (self, path); if (existing) { const char *old_path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (existing)); - nm_log_info (LOGD_SETTINGS, "renaming %s -> %s", old_path, path); + _LOGI ("renaming %s -> %s", old_path, path); if (out_old_path) *out_old_path = g_strdup (old_path); nm_settings_connection_set_filename (NM_SETTINGS_CONNECTION (existing), path); @@ -287,7 +305,7 @@ connection_new_or_changed (SCPluginIfcfg *self, new = (NMIfcfgConnection *) nm_ifcfg_connection_new (NULL, path, NULL); if (!new) { /* errors reading connection; remove it */ - nm_log_info (LOGD_SETTINGS, "removed %s.", path); + _LOGI ("removed %s.", path); remove_connection (self, existing); return; } @@ -316,12 +334,12 @@ connection_new_or_changed (SCPluginIfcfg *self, /* 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(). */ - nm_log_warn (LOGD_SETTINGS, "UUID changes are not supported. Cannot update connection %s (%s)", nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (new)), nm_connection_get_uuid (NM_CONNECTION (new))); + _LOGW ("UUID changes are not supported. Cannot update connection %s (%s)", nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (new)), nm_connection_get_uuid (NM_CONNECTION (new))); g_object_unref (new); return; } - nm_log_info (LOGD_SETTINGS, "updating %s", path); + _LOGI ("updating %s", path); g_object_set (existing, NM_IFCFG_CONNECTION_UNMANAGED_SPEC, new_unmanaged, NM_IFCFG_CONNECTION_UNRECOGNIZED_SPEC, new_unrecognized, @@ -349,10 +367,10 @@ connection_new_or_changed (SCPluginIfcfg *self, const char *cid = nm_connection_get_id (NM_CONNECTION (new)); if (old_unmanaged /* && !new_unmanaged */) { - nm_log_info (LOGD_SETTINGS, "Managing connection '%s' and its device because NM_CONTROLLED was true.", cid); + _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, existing); } else if (old_unrecognized /* && !new_unrecognized */) { - nm_log_info (LOGD_SETTINGS, "Managing connection '%s' because it is now a recognized type.", cid); + _LOGI ("Managing connection '%s' because it is now a recognized type.", cid); g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, existing); } @@ -404,7 +422,7 @@ ifcfg_dir_changed (GFileMonitor *monitor, connection = find_by_path (plugin, ifcfg_path); switch (event_type) { case G_FILE_MONITOR_EVENT_DELETED: - nm_log_info (LOGD_SETTINGS, "removed %s.", ifcfg_path); + _LOGI ("removed %s.", ifcfg_path); if (connection) remove_connection (plugin, connection); break; @@ -454,7 +472,7 @@ read_connections (SCPluginIfcfg *plugin) dir = g_dir_open (IFCFG_DIR, 0, &err); if (!dir) { - nm_log_warn (LOGD_SETTINGS, "Could not read directory '%s': %s", IFCFG_DIR, err->message); + _LOGW ("Could not read directory '%s': %s", IFCFG_DIR, err->message); g_error_free (err); return; } @@ -496,7 +514,7 @@ read_connections (SCPluginIfcfg *plugin) g_hash_table_iter_init (&iter, oldconns); while (g_hash_table_iter_next (&iter, &key, &value)) { - nm_log_info (LOGD_SETTINGS, "removed %s.", (char *)key); + _LOGI ("removed %s.", (char *)key); g_hash_table_iter_remove (&iter); remove_connection (plugin, value); } @@ -648,7 +666,7 @@ plugin_get_hostname (SCPluginIfcfg *plugin) network = svOpenFile (SC_NETWORK_FILE, NULL); if (!network) { - nm_log_warn (LOGD_SETTINGS, "Could not get hostname: failed to read " SC_NETWORK_FILE); + _LOGW ("Could not get hostname: failed to read " SC_NETWORK_FILE); return NULL; } @@ -700,7 +718,7 @@ plugin_set_hostname (SCPluginIfcfg *plugin, const char *hostname) #endif if (!ret) { - nm_log_warn (LOGD_SETTINGS, "Could not save hostname: failed to create/open " HOSTNAME_FILE); + _LOGW ("Could not save hostname: failed to create/open " HOSTNAME_FILE); g_free (hostname_eol); return FALSE; } @@ -869,7 +887,7 @@ sc_plugin_ifcfg_init (SCPluginIfcfg *plugin) priv->bus = dbus_g_bus_get (DBUS_BUS_SYSTEM, &error); if (!priv->bus) { - nm_log_warn (LOGD_SETTINGS, "Couldn't connect to D-Bus: %s", error->message); + _LOGW ("Couldn't connect to D-Bus: %s", error->message); g_clear_error (&error); } else { DBusConnection *tmp; @@ -890,10 +908,10 @@ sc_plugin_ifcfg_init (SCPluginIfcfg *plugin) G_TYPE_INVALID, G_TYPE_UINT, &result, G_TYPE_INVALID)) { - nm_log_warn (LOGD_SETTINGS, "Couldn't acquire D-Bus service: %s", error->message); + _LOGW ("Couldn't acquire D-Bus service: %s", error->message); g_clear_error (&error); } else if (result != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) { - nm_log_warn (LOGD_SETTINGS, "Couldn't acquire ifcfgrh1 D-Bus service (already taken)"); + _LOGW ("Couldn't acquire ifcfgrh1 D-Bus service (already taken)"); } else success = TRUE; } @@ -1055,7 +1073,7 @@ nm_system_config_factory (void) dbus_g_connection_register_g_object (priv->bus, DBUS_OBJECT_PATH, G_OBJECT (singleton)); - nm_log_info (LOGD_SETTINGS, "Acquired D-Bus service %s", DBUS_SERVICE_NAME); + _LOGI ("Acquired D-Bus service %s", DBUS_SERVICE_NAME); } else g_object_ref (singleton); From d6bd8cc49615eda88e791dda99619eb00ae71f19 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 10 Dec 2014 21:35:39 +0100 Subject: [PATCH 15/19] ifcfg-rh/trivial: rename connection_new_or_changed() and variable The ifcfg-rh implementation should be similar to the one from keyfile. Rename the variables and function that have the same meaning. Do this trivial commit first, before starting refactoring. (cherry picked from commit a609dd12d3019fe57ac39c09a14828aa963cc7e9) --- src/settings/plugins/ifcfg-rh/plugin.c | 126 +++++++++++++------------ 1 file changed, 66 insertions(+), 60 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index aa495495ed..9003a208dd 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -88,10 +88,13 @@ static gboolean impl_ifcfgrh_get_ifcfg_details (SCPluginIfcfg *plugin, #include "nm-ifcfg-rh-glue.h" -static void connection_new_or_changed (SCPluginIfcfg *plugin, - const char *path, - NMIfcfgConnection *existing, - char **out_old_path); +static void update_connection (SCPluginIfcfg *plugin, + NMConnection *source, + const char *full_path, + NMIfcfgConnection *connection, + GHashTable *protected_connections, + char **out_old_path, + GError **error); static void system_config_interface_init (NMSystemConfigInterface *system_config_interface_class); @@ -128,7 +131,7 @@ connection_ifcfg_changed (NMIfcfgConnection *connection, gpointer user_data) path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection)); g_return_if_fail (path != NULL); - connection_new_or_changed (plugin, path, connection, NULL); + update_connection (plugin, NULL, path, connection, NULL, NULL, NULL); } static void @@ -140,7 +143,7 @@ connection_removed_cb (NMSettingsConnection *obj, gpointer user_data) static NMIfcfgConnection * _internal_new_connection (SCPluginIfcfg *self, - const char *path, + const char *full_path, NMConnection *source, GError **error) { @@ -150,9 +153,9 @@ _internal_new_connection (SCPluginIfcfg *self, const char *uuid; if (!source) - _LOGI ("parsing %s ... ", path); + _LOGI ("parsing %s ... ", full_path); - connection = nm_ifcfg_connection_new (source, path, error); + connection = nm_ifcfg_connection_new (source, full_path, error); if (!connection) return NULL; @@ -258,134 +261,137 @@ find_by_uuid_from_path (SCPluginIfcfg *self, const char *path) } static void -connection_new_or_changed (SCPluginIfcfg *self, - const char *path, - NMIfcfgConnection *existing, - char **out_old_path) +update_connection (SCPluginIfcfg *self, + NMConnection *source, + const char *full_path, + NMIfcfgConnection *connection, + GHashTable *protected_connections, + char **out_old_path, + GError **error) { SCPluginIfcfgPrivate *priv = SC_PLUGIN_IFCFG_GET_PRIVATE (self); - NMIfcfgConnection *new; - GError *error = NULL; + NMIfcfgConnection *connection_new; + GError *local = NULL; const char *new_unmanaged = NULL, *old_unmanaged = NULL; const char *new_unrecognized = NULL, *old_unrecognized = NULL; gboolean unmanaged_changed, unrecognized_changed; g_return_if_fail (self != NULL); - g_return_if_fail (path != NULL); + g_return_if_fail (full_path != NULL); if (out_old_path) *out_old_path = NULL; - if (!existing) { + if (!connection) { /* See if it's a rename */ - existing = find_by_uuid_from_path (self, path); - if (existing) { - const char *old_path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (existing)); - _LOGI ("renaming %s -> %s", old_path, path); + 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 (existing), path); + nm_settings_connection_set_filename (NM_SETTINGS_CONNECTION (connection), full_path); } } - if (!existing) { + if (!connection) { /* New connection */ - new = _internal_new_connection (self, path, NULL, NULL); - if (new) { - if (nm_ifcfg_connection_get_unmanaged_spec (new)) + 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 (new)) + 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, new); + g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection_new); } return; } - new = (NMIfcfgConnection *) nm_ifcfg_connection_new (NULL, path, NULL); - if (!new) { + connection_new = (NMIfcfgConnection *) nm_ifcfg_connection_new (NULL, full_path, NULL); + if (!connection_new) { /* errors reading connection; remove it */ - _LOGI ("removed %s.", path); - remove_connection (self, existing); + _LOGI ("removed %s.", full_path); + remove_connection (self, connection); return; } /* Successfully read connection changes */ - old_unmanaged = nm_ifcfg_connection_get_unmanaged_spec (NM_IFCFG_CONNECTION (existing)); - new_unmanaged = nm_ifcfg_connection_get_unmanaged_spec (NM_IFCFG_CONNECTION (new)); + 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)); unmanaged_changed = g_strcmp0 (old_unmanaged, new_unmanaged); - old_unrecognized = nm_ifcfg_connection_get_unrecognized_spec (NM_IFCFG_CONNECTION (existing)); - new_unrecognized = nm_ifcfg_connection_get_unrecognized_spec (NM_IFCFG_CONNECTION (new)); + 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)); unrecognized_changed = g_strcmp0 (old_unrecognized, new_unrecognized); if ( !unmanaged_changed && !unrecognized_changed - && nm_connection_compare (NM_CONNECTION (existing), - NM_CONNECTION (new), + && 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 (new); + g_object_unref (connection_new); return; } - if (g_strcmp0 (nm_connection_get_uuid (NM_CONNECTION (existing)), nm_connection_get_uuid (NM_CONNECTION (new))) != 0) { + 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 (new)), nm_connection_get_uuid (NM_CONNECTION (new))); - g_object_unref (new); + _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; } - _LOGI ("updating %s", path); - g_object_set (existing, + _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 (existing); + g_object_ref (connection); /* Unexport the connection by telling the settings service it's * been removed. */ - nm_settings_connection_signal_remove (NM_SETTINGS_CONNECTION (existing)); + 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 (existing), NULL); + 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 (existing))), - existing); + g_strdup (nm_connection_get_uuid (NM_CONNECTION (connection))), + connection); } } else { - const char *cid = nm_connection_get_id (NM_CONNECTION (new)); + 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, existing); + 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, existing); + g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection); } - if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (existing), - NM_CONNECTION (new), + if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection), + NM_CONNECTION (connection_new), FALSE, /* don't set Unsaved */ "ifcfg-rh-update", - &error)) { - /* Shouldn't ever get here as 'new' was verified by the reader already + &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 (error); + g_assert_no_error (local); } - g_object_unref (new); + g_object_unref (connection_new); if (unmanaged_changed) g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_UNMANAGED_SPECS_CHANGED); @@ -429,7 +435,7 @@ ifcfg_dir_changed (GFileMonitor *monitor, case G_FILE_MONITOR_EVENT_CREATED: case G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT: /* Update or new */ - connection_new_or_changed (plugin, ifcfg_path, connection, NULL); + update_connection (plugin, NULL, ifcfg_path, connection, NULL, NULL, NULL); break; default: break; @@ -499,7 +505,7 @@ read_connections (SCPluginIfcfg *plugin) connection = g_hash_table_lookup (oldconns, full_path); g_hash_table_remove (oldconns, full_path); - connection_new_or_changed (plugin, full_path, connection, &old_path); + update_connection (plugin, NULL, full_path, connection, NULL, &old_path, NULL); if (old_path) { g_hash_table_remove (oldconns, old_path); @@ -565,7 +571,7 @@ load_connection (NMSystemConfigInterface *config, return FALSE; connection = find_by_path (plugin, filename); - connection_new_or_changed (plugin, filename, connection, NULL); + update_connection (plugin, NULL, filename, connection, NULL, NULL, NULL); if (!connection) connection = find_by_path (plugin, filename); From 4b5d3f160d4c8bd26202ba29d3753c6be3b82190 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 Dec 2014 14:38:03 +0100 Subject: [PATCH 16/19] ifcfg-rh: sort paths in read_connections() Presort the files in read_connections() as we do it for keyfile. This alone has not much consequences. Do this patch first, to keep the next patches more self-contained. (cherry picked from commit 0cf00ff3aaf4381585ea1f620f22b283b4fa47cb) --- src/settings/plugins/ifcfg-rh/plugin.c | 70 +++++++++++++++++++++++--- 1 file changed, 63 insertions(+), 7 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index 9003a208dd..eaa3aea2d9 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -464,6 +464,43 @@ setup_ifcfg_monitoring (SCPluginIfcfg *plugin) } } +static GHashTable * +_paths_from_connections (GHashTable *connections) +{ + GHashTableIter iter; + NMIfcfgConnection *connection; + GHashTable *paths = g_hash_table_new (g_str_hash, g_str_equal); + + g_hash_table_iter_init (&iter, connections); + while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &connection)) { + const char *path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection)); + + if (path) + g_hash_table_add (paths, (void *) path); + } + return paths; +} + +static int +_sort_paths (const char **f1, const char **f2, GHashTable *paths) +{ + struct stat st; + gboolean c1, c2; + gint64 m1, m2; + + c1 = !!g_hash_table_contains (paths, *f1); + c2 = !!g_hash_table_contains (paths, *f2); + if (c1 != c2) + return c1 ? -1 : 1; + + m1 = stat (*f1, &st) == 0 ? (gint64) st.st_mtime : G_MININT64; + m2 = stat (*f2, &st) == 0 ? (gint64) st.st_mtime : G_MININT64; + if (m1 != m2) + return m1 > m2 ? -1 : 1; + + return strcmp (*f1, *f2); +} + static void read_connections (SCPluginIfcfg *plugin) { @@ -475,6 +512,9 @@ read_connections (SCPluginIfcfg *plugin) GHashTableIter iter; gpointer key, value; NMIfcfgConnection *connection; + guint i; + GPtrArray *filenames; + GHashTable *paths; dir = g_dir_open (IFCFG_DIR, 0, &err); if (!dir) { @@ -491,8 +531,9 @@ read_connections (SCPluginIfcfg *plugin) g_hash_table_insert (oldconns, g_strdup (ifcfg_path), value); } + filenames = g_ptr_array_new_with_free_func (g_free); while ((item = g_dir_read_name (dir))) { - char *full_path, *old_path; + char *full_path; if (utils_should_ignore_file (item, TRUE)) continue; @@ -501,8 +542,27 @@ read_connections (SCPluginIfcfg *plugin) full_path = g_build_filename (IFCFG_DIR, item, NULL); if (!utils_get_ifcfg_name (full_path, TRUE)) - goto next; + g_free (full_path); + else + g_ptr_array_add (filenames, full_path); + } + g_dir_close (dir); + /* While reloading, we don't replace connections that we already loaded while + * iterating over the files. + * + * To have sensible, reproducible behavior, sort the paths by last modification + * time prefering older files. + */ + paths = _paths_from_connections (priv->connections); + g_ptr_array_sort_with_data (filenames, (GCompareDataFunc) _sort_paths, paths); + 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); @@ -511,12 +571,8 @@ read_connections (SCPluginIfcfg *plugin) g_hash_table_remove (oldconns, old_path); g_free (old_path); } - - next: - g_free (full_path); } - - g_dir_close (dir); + g_ptr_array_free (filenames, TRUE); g_hash_table_iter_init (&iter, oldconns); while (g_hash_table_iter_next (&iter, &key, &value)) { From 581caca9f21fcb4428bdc51bb21072f711c469ce Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 Dec 2014 14:49:36 +0100 Subject: [PATCH 17/19] 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 (cherry picked from commit 0c6349c627745d625a73505032807e7c70e2f710) --- src/settings/plugins/ifcfg-rh/plugin.c | 463 +++++++++--------- .../ifcfg-rh/tests/test-ifcfg-rh-utils.c | 2 +- src/settings/plugins/ifcfg-rh/utils.h | 7 + 3 files changed, 238 insertions(+), 234 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,58 +190,211 @@ 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); + g_return_val_if_fail (!source || NM_IS_CONNECTION (source), NULL); + g_return_val_if_fail (full_path || source, NULL); - if (out_old_path) - *out_old_path = NULL; + if (full_path) + _LOGD ("loading from file \"%s\"...", full_path); - 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); - } + /* 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) { + 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 + _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; + } + + /* The new connection has a different UUID then the original one that we + * are about to update. Remove @connection. */ + remove_connection (self, connection); + } + + /* 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; + } + + /* 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); + + 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 (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); + } + } + + 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 { + + /******************************************************* + * 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)) @@ -305,98 +402,8 @@ update_connection (SCPluginIfcfg *self, else g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection_new); } - return; + return connection_new; } - - connection_new = (NMIfcfgConnection *) nm_ifcfg_connection_new (NULL, full_path, NULL); - if (!connection_new) { - /* errors reading connection; remove it */ - _LOGI ("removed %s.", full_path); - remove_connection (self, connection); - return; - } - - /* Successfully read connection changes */ - - 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)); - 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)); - 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; - } - - _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); - } - } 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 (); - } - g_assert_no_error (local); - } - 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 #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); From 76ee139107d1d181d6db8ac1b0bcc452be2d42dd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 Dec 2014 00:40:43 +0100 Subject: [PATCH 18/19] ifcfg-rh: don't reload connection in connection_ifcfg_changed() if monitoring is not enabled This was not really an error, because NMIfcfgConnection would not watch the files if monitoring is not enabled. Still do it, because it feels more correct. (cherry picked from commit 236226a590a99c737293b2fe0df547276a7a1955) --- src/settings/plugins/ifcfg-rh/plugin.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index 06c3c9f0c7..b7efa8643a 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -128,15 +128,22 @@ typedef struct { static void connection_ifcfg_changed (NMIfcfgConnection *connection, gpointer user_data) { - SCPluginIfcfg *plugin = SC_PLUGIN_IFCFG (user_data); + SCPluginIfcfg *self = SC_PLUGIN_IFCFG (user_data); + SCPluginIfcfgPrivate *priv = SC_PLUGIN_IFCFG_GET_PRIVATE (self); const char *path; path = nm_settings_connection_get_filename (NM_SETTINGS_CONNECTION (connection)); g_return_if_fail (path != 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); + if (!priv->ifcfg_monitor) { + _LOGD ("connection_ifcfg_changed("NM_IFCFG_CONNECTION_LOG_FMTD"): %s", NM_IFCFG_CONNECTION_LOG_ARGD (connection), "ignore event"); + return; + } + + _LOGD ("connection_ifcfg_changed("NM_IFCFG_CONNECTION_LOG_FMTD"): %s", NM_IFCFG_CONNECTION_LOG_ARGD (connection), "reload"); + + update_connection (self, NULL, path, connection, TRUE, NULL, NULL); } static void From 06b3e3ae6b60ed0efedc129a7833aac7f6f832c9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 13 Jan 2015 12:23:48 +0100 Subject: [PATCH 19/19] settings: update year in copyright text of plugin-info for keyfile and ifcfg-rh (cherry picked from commit 4475f59bce48b05f64e7977ac233f61682ee7ef5) --- src/settings/plugins/ifcfg-rh/common.h | 2 +- src/settings/plugins/keyfile/common.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/common.h b/src/settings/plugins/ifcfg-rh/common.h index 7736d2632e..0ec355ee4e 100644 --- a/src/settings/plugins/ifcfg-rh/common.h +++ b/src/settings/plugins/ifcfg-rh/common.h @@ -41,7 +41,7 @@ #define IFCFG_DIR SYSCONFDIR"/sysconfig/network-scripts" #define IFCFG_PLUGIN_NAME "ifcfg-rh" -#define IFCFG_PLUGIN_INFO "(c) 2007 - 2013 Red Hat, Inc. To report bugs please use the NetworkManager mailing list." +#define IFCFG_PLUGIN_INFO "(c) 2007 - 2015 Red Hat, Inc. To report bugs please use the NetworkManager mailing list." #define TYPE_ETHERNET "Ethernet" #define TYPE_WIRELESS "Wireless" diff --git a/src/settings/plugins/keyfile/common.h b/src/settings/plugins/keyfile/common.h index db6569e54a..7bde4bf3a9 100644 --- a/src/settings/plugins/keyfile/common.h +++ b/src/settings/plugins/keyfile/common.h @@ -24,7 +24,7 @@ #include #define KEYFILE_PLUGIN_NAME "keyfile" -#define KEYFILE_PLUGIN_INFO "(c) 2007 - 2013 Red Hat, Inc. To report bugs please use the NetworkManager mailing list." +#define KEYFILE_PLUGIN_INFO "(c) 2007 - 2015 Red Hat, Inc. To report bugs please use the NetworkManager mailing list." #define KEYFILE_DIR NMCONFDIR "/system-connections"