From c863d2ad0fb27d1ea39c4035e0683a86cfb911f3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Jul 2015 10:50:07 +0200 Subject: [PATCH 1/5] keyfile: fix memleak of path in commit_changes() --- src/settings/plugins/keyfile/nm-keyfile-connection.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/settings/plugins/keyfile/nm-keyfile-connection.c b/src/settings/plugins/keyfile/nm-keyfile-connection.c index 8fb94302d7..11762ee602 100644 --- a/src/settings/plugins/keyfile/nm-keyfile-connection.c +++ b/src/settings/plugins/keyfile/nm-keyfile-connection.c @@ -104,8 +104,10 @@ commit_changes (NMSettingsConnection *connection, } /* Update the filename if it changed */ - if (path) + if (path) { nm_settings_connection_set_filename (connection, path); + g_free (path); + } NM_SETTINGS_CONNECTION_CLASS (nm_keyfile_connection_parent_class)->commit_changes (connection, callback, From c0b03debc8dca9b68f9762a98773eb3c3e81e40e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Jul 2015 10:50:42 +0200 Subject: [PATCH 2/5] keyfile: add info logging when updating connection --- .../plugins/keyfile/nm-keyfile-connection.c | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/settings/plugins/keyfile/nm-keyfile-connection.c b/src/settings/plugins/keyfile/nm-keyfile-connection.c index 11762ee602..c631faee5c 100644 --- a/src/settings/plugins/keyfile/nm-keyfile-connection.c +++ b/src/settings/plugins/keyfile/nm-keyfile-connection.c @@ -33,6 +33,9 @@ #include "reader.h" #include "writer.h" #include "common.h" +#include "utils.h" +#include "gsystem-local-alloc.h" +#include "nm-logging.h" G_DEFINE_TYPE (NMKeyfileConnection, nm_keyfile_connection, NM_TYPE_SETTINGS_CONNECTION) @@ -104,11 +107,26 @@ commit_changes (NMSettingsConnection *connection, } /* Update the filename if it changed */ - if (path) { + if ( path + && g_strcmp0 (path, nm_settings_connection_get_filename (connection)) != 0) { + gs_free char *old_path = g_strdup (nm_settings_connection_get_filename (connection)); + nm_settings_connection_set_filename (connection, path); - g_free (path); + if (old_path) { + nm_log_info (LOGD_SETTINGS, "keyfile: update "NM_KEYFILE_CONNECTION_LOG_FMT" and rename from \"%s\"", + NM_KEYFILE_CONNECTION_LOG_ARG (connection), + old_path); + } else { + nm_log_info (LOGD_SETTINGS, "keyfile: update "NM_KEYFILE_CONNECTION_LOG_FMT" and persist connection", + NM_KEYFILE_CONNECTION_LOG_ARG (connection)); + } + } else { + nm_log_info (LOGD_SETTINGS, "keyfile: update "NM_KEYFILE_CONNECTION_LOG_FMT, + NM_KEYFILE_CONNECTION_LOG_ARG (connection)); } + g_free (path); + NM_SETTINGS_CONNECTION_CLASS (nm_keyfile_connection_parent_class)->commit_changes (connection, callback, user_data); From d5d34ec107a3a101debb57d16487e67cca2159da Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Jul 2015 11:08:31 +0200 Subject: [PATCH 3/5] keyfile: refactor return paths in _internal_write_connection() --- src/settings/plugins/keyfile/writer.c | 43 ++++++++++++--------------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/src/settings/plugins/keyfile/writer.c b/src/settings/plugins/keyfile/writer.c index b6a8786dc4..ffe941bbac 100644 --- a/src/settings/plugins/keyfile/writer.c +++ b/src/settings/plugins/keyfile/writer.c @@ -34,6 +34,7 @@ #include "common.h" #include "utils.h" #include "nm-keyfile-internal.h" +#include "gsystem-local-alloc.h" typedef struct { @@ -237,10 +238,9 @@ _internal_write_connection (NMConnection *connection, GError **error) { GKeyFile *key_file; - char *data; + gs_free char *data = NULL; gsize len; - gboolean success = FALSE; - char *path; + gs_free char *path = NULL; const char *id; WriteInfo info = { 0 }; GError *local_err = NULL; @@ -312,8 +312,7 @@ _internal_write_connection (NMConnection *connection, /* this really should not happen, we tried hard to find an unused name... bail out. */ g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, "could not find suitable keyfile file name (%s already used)", path); - g_free (path); - goto out; + return FALSE; } /* Both our preferred path based on connection id and id-uuid are taken. * Fallback to @existing_path */ @@ -334,8 +333,7 @@ _internal_write_connection (NMConnection *connection, "%s.%d: error writing to file '%s': %s", __FILE__, __LINE__, path, local_err->message); g_error_free (local_err); - g_free (path); - goto out; + return FALSE; } if (chown (path, owner_uid, owner_grp) < 0) { @@ -343,25 +341,22 @@ _internal_write_connection (NMConnection *connection, "%s.%d: error chowning '%s': %d", __FILE__, __LINE__, path, errno); unlink (path); - } else { - if (chmod (path, S_IRUSR | S_IWUSR) < 0) { - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "%s.%d: error setting permissions on '%s': %d", __FILE__, - __LINE__, path, errno); - unlink (path); - } else { - if (out_path && g_strcmp0 (existing_path, path)) { - *out_path = path; /* pass path out to caller */ - path = NULL; - } - success = TRUE; - } + return FALSE; } - g_free (path); -out: - g_free (data); - return success; + if (chmod (path, S_IRUSR | S_IWUSR) < 0) { + g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, + "%s.%d: error setting permissions on '%s': %d", __FILE__, + __LINE__, path, errno); + unlink (path); + return FALSE; + } + + if (out_path && g_strcmp0 (existing_path, path)) { + *out_path = path; /* pass path out to caller */ + path = NULL; + } + return TRUE; } gboolean From 238cb02ed672834f2e04f6c30829c3ea9a769e67 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Jul 2015 11:17:25 +0200 Subject: [PATCH 4/5] keyfile: cleanup error messages in _internal_write_connection() A GError should contain a nice, human readable error message. The file:line prefix looks ugly. Also, the error messages are already systemwide unique. So a user can easily grep for them and locate the origin. --- src/settings/plugins/keyfile/writer.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/settings/plugins/keyfile/writer.c b/src/settings/plugins/keyfile/writer.c index ffe941bbac..e962ed5522 100644 --- a/src/settings/plugins/keyfile/writer.c +++ b/src/settings/plugins/keyfile/writer.c @@ -244,6 +244,7 @@ _internal_write_connection (NMConnection *connection, const char *id; WriteInfo info = { 0 }; GError *local_err = NULL; + int errsv; g_return_val_if_fail (!out_path || !*out_path, FALSE); g_return_val_if_fail (keyfile_dir && keyfile_dir[0] == '/', FALSE); @@ -330,24 +331,26 @@ _internal_write_connection (NMConnection *connection, g_file_set_contents (path, data, len, &local_err); if (local_err) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "%s.%d: error writing to file '%s': %s", __FILE__, __LINE__, + "error writing to file '%s': %s", path, local_err->message); g_error_free (local_err); return FALSE; } if (chown (path, owner_uid, owner_grp) < 0) { + errsv = errno; g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "%s.%d: error chowning '%s': %d", __FILE__, __LINE__, - path, errno); + "error chowning '%s': %s (%d)", + path, g_strerror (errsv), errsv); unlink (path); return FALSE; } if (chmod (path, S_IRUSR | S_IWUSR) < 0) { + errsv = errno; g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "%s.%d: error setting permissions on '%s': %d", __FILE__, - __LINE__, path, errno); + "error setting permissions on '%s': %s (%d)", + path, g_strerror (errsv), errsv); unlink (path); return FALSE; } From 6a5657896f305eb1d624fd2687d3b9c1181dcbac Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Jul 2015 10:34:10 +0200 Subject: [PATCH 5/5] keyfile: rename keyfile when user changes connection id Originally, if you change the ID of a connection, the existing keyfile will not be renamed. That means after renaming a connection, it's keyfile name will mismatch. Now, when th user modifies a connection via D-Bus and changes the connection it, rename the file. https://bugzilla.gnome.org/show_bug.cgi?id=740738 --- src/nm-manager.c | 4 +++- src/settings/nm-settings-connection.c | 14 +++++++++++--- src/settings/nm-settings-connection.h | 7 +++++++ .../plugins/ifcfg-rh/nm-ifcfg-connection.c | 5 +++-- src/settings/plugins/ifnet/nm-ifnet-connection.c | 5 +++-- src/settings/plugins/ifupdown/plugin.c | 2 +- .../plugins/keyfile/nm-keyfile-connection.c | 4 ++++ src/settings/plugins/keyfile/plugin.c | 2 +- src/settings/plugins/keyfile/writer.c | 6 +++++- src/settings/plugins/keyfile/writer.h | 1 + 10 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index cd73125213..d8072d1d69 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -3221,7 +3221,9 @@ activation_add_done (NMSettings *self, nm_active_connection_set_connection (info->active, NM_CONNECTION (new_connection)); if (_internal_activate_generic (info->manager, info->active, &local)) { - nm_settings_connection_commit_changes (new_connection, NULL, NULL); + nm_settings_connection_commit_changes (new_connection, + NM_SETTINGS_CONNECTION_COMMIT_REASON_USER_ACTION | NM_SETTINGS_CONNECTION_COMMIT_REASON_ID_CHANGED, + NULL, NULL); dbus_g_method_return (context, nm_connection_get_path (NM_CONNECTION (new_connection)), nm_active_connection_get_path (info->active)); diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index cc0a5b6b53..c25dfac01a 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -593,9 +593,14 @@ replace_and_commit (NMSettingsConnection *self, gpointer user_data) { GError *error = NULL; + NMSettingsConnectionCommitReason commit_reason = NM_SETTINGS_CONNECTION_COMMIT_REASON_USER_ACTION; + + if (g_strcmp0 (nm_connection_get_id (NM_CONNECTION (self)), + nm_connection_get_id (new_connection)) != 0) + commit_reason |= NM_SETTINGS_CONNECTION_COMMIT_REASON_ID_CHANGED; if (nm_settings_connection_replace_settings (self, new_connection, TRUE, "replace-and-commit-disk", &error)) - nm_settings_connection_commit_changes (self, callback, user_data); + nm_settings_connection_commit_changes (self, commit_reason, callback, user_data); else { g_assert (error); if (callback) @@ -618,6 +623,7 @@ nm_settings_connection_replace_and_commit (NMSettingsConnection *self, static void commit_changes (NMSettingsConnection *self, + NMSettingsConnectionCommitReason commit_reason, NMSettingsConnectionCommitFunc callback, gpointer user_data) { @@ -633,6 +639,7 @@ commit_changes (NMSettingsConnection *self, void nm_settings_connection_commit_changes (NMSettingsConnection *self, + NMSettingsConnectionCommitReason commit_reason, NMSettingsConnectionCommitFunc callback, gpointer user_data) { @@ -640,6 +647,7 @@ nm_settings_connection_commit_changes (NMSettingsConnection *self, if (NM_SETTINGS_CONNECTION_GET_CLASS (self)->commit_changes) { NM_SETTINGS_CONNECTION_GET_CLASS (self)->commit_changes (self, + commit_reason, callback ? callback : ignore_cb, user_data); } else { @@ -916,7 +924,7 @@ agent_secrets_done_cb (NMAgentManager *manager, setting_name, call_id); - nm_settings_connection_commit_changes (self, new_secrets_commit_cb, NULL); + nm_settings_connection_commit_changes (self, NM_SETTINGS_CONNECTION_COMMIT_REASON_NONE, new_secrets_commit_cb, NULL); } else { _LOGD ("(%s:%u) new agent secrets processed", setting_name, @@ -1743,7 +1751,7 @@ dbus_clear_secrets_auth_cb (NMSettingsConnection *self, /* Tell agents to remove secrets for this connection */ nm_agent_manager_delete_secrets (priv->agent_mgr, NM_CONNECTION (self)); - nm_settings_connection_commit_changes (self, clear_secrets_cb, context); + nm_settings_connection_commit_changes (self, NM_SETTINGS_CONNECTION_COMMIT_REASON_NONE, clear_secrets_cb, context); } } diff --git a/src/settings/nm-settings-connection.h b/src/settings/nm-settings-connection.h index 512112f032..0d3963d3df 100644 --- a/src/settings/nm-settings-connection.h +++ b/src/settings/nm-settings-connection.h @@ -78,6 +78,11 @@ typedef enum NM_SETTINGS_CONNECTION_FLAGS_ALL = ((__NM_SETTINGS_CONNECTION_FLAGS_LAST - 1) << 1) - 1, } NMSettingsConnectionFlags; +typedef enum { /*< skip >*/ + NM_SETTINGS_CONNECTION_COMMIT_REASON_NONE = 0, + NM_SETTINGS_CONNECTION_COMMIT_REASON_USER_ACTION = (1LL << 0), + NM_SETTINGS_CONNECTION_COMMIT_REASON_ID_CHANGED = (1LL << 1), +} NMSettingsConnectionCommitReason; typedef struct _NMSettingsConnectionClass NMSettingsConnectionClass; @@ -103,6 +108,7 @@ struct _NMSettingsConnectionClass { gpointer user_data); void (*commit_changes) (NMSettingsConnection *self, + NMSettingsConnectionCommitReason commit_reason, NMSettingsConnectionCommitFunc callback, gpointer user_data); @@ -117,6 +123,7 @@ struct _NMSettingsConnectionClass { GType nm_settings_connection_get_type (void); void nm_settings_connection_commit_changes (NMSettingsConnection *self, + NMSettingsConnectionCommitReason commit_reason, NMSettingsConnectionCommitFunc callback, gpointer user_data); diff --git a/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c b/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c index a18920c685..277e9d7a20 100644 --- a/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c +++ b/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c @@ -365,6 +365,7 @@ replace_and_commit (NMSettingsConnection *connection, static void commit_changes (NMSettingsConnection *connection, + NMSettingsConnectionCommitReason commit_reason, NMSettingsConnectionCommitFunc callback, gpointer user_data) { @@ -392,7 +393,7 @@ commit_changes (NMSettingsConnection *connection, /* Don't bother writing anything out if in-memory and on-disk data are the same */ if (same) { /* But chain up to parent to handle success - emits updated signal */ - NM_SETTINGS_CONNECTION_CLASS (nm_ifcfg_connection_parent_class)->commit_changes (connection, callback, user_data); + NM_SETTINGS_CONNECTION_CLASS (nm_ifcfg_connection_parent_class)->commit_changes (connection, commit_reason, callback, user_data); return; } } @@ -415,7 +416,7 @@ commit_changes (NMSettingsConnection *connection, if (success) { /* Chain up to parent to handle success */ - NM_SETTINGS_CONNECTION_CLASS (nm_ifcfg_connection_parent_class)->commit_changes (connection, callback, user_data); + NM_SETTINGS_CONNECTION_CLASS (nm_ifcfg_connection_parent_class)->commit_changes (connection, commit_reason, callback, user_data); } else { /* Otherwise immediate error */ callback (connection, error, user_data); diff --git a/src/settings/plugins/ifnet/nm-ifnet-connection.c b/src/settings/plugins/ifnet/nm-ifnet-connection.c index 693e653288..b31246408e 100644 --- a/src/settings/plugins/ifnet/nm-ifnet-connection.c +++ b/src/settings/plugins/ifnet/nm-ifnet-connection.c @@ -102,8 +102,9 @@ nm_ifnet_connection_get_conn_name (NMIfnetConnection *connection) static void commit_changes (NMSettingsConnection *connection, + NMSettingsConnectionCommitReason commit_reason, NMSettingsConnectionCommitFunc callback, - gpointer user_data) + gpointer user_data) { GError *error = NULL; NMIfnetConnectionPrivate *priv = NM_IFNET_CONNECTION_GET_PRIVATE (connection); @@ -139,7 +140,7 @@ commit_changes (NMSettingsConnection *connection, g_free (priv->conn_name); priv->conn_name = new_name; - NM_SETTINGS_CONNECTION_CLASS (nm_ifnet_connection_parent_class)->commit_changes (connection, callback, user_data); + NM_SETTINGS_CONNECTION_CLASS (nm_ifnet_connection_parent_class)->commit_changes (connection, commit_reason, callback, user_data); nm_log_info (LOGD_SETTINGS, "Successfully updated %s", priv->conn_name); } else { nm_log_warn (LOGD_SETTINGS, "Failed to update %s", diff --git a/src/settings/plugins/ifupdown/plugin.c b/src/settings/plugins/ifupdown/plugin.c index c3cb180b58..d67405b037 100644 --- a/src/settings/plugins/ifupdown/plugin.c +++ b/src/settings/plugins/ifupdown/plugin.c @@ -193,7 +193,7 @@ bind_device_to_connection (SCPluginIfupdown *self, g_object_set (s_wifi, NM_SETTING_WIRELESS_MAC_ADDRESS, address, NULL); } - nm_settings_connection_commit_changes (NM_SETTINGS_CONNECTION (exported), NULL, NULL); + nm_settings_connection_commit_changes (NM_SETTINGS_CONNECTION (exported), NM_SETTINGS_CONNECTION_COMMIT_REASON_NONE, NULL, NULL); } static void diff --git a/src/settings/plugins/keyfile/nm-keyfile-connection.c b/src/settings/plugins/keyfile/nm-keyfile-connection.c index c631faee5c..744ae9ccf3 100644 --- a/src/settings/plugins/keyfile/nm-keyfile-connection.c +++ b/src/settings/plugins/keyfile/nm-keyfile-connection.c @@ -91,6 +91,7 @@ nm_keyfile_connection_new (NMConnection *source, static void commit_changes (NMSettingsConnection *connection, + NMSettingsConnectionCommitReason commit_reason, NMSettingsConnectionCommitFunc callback, gpointer user_data) { @@ -99,6 +100,8 @@ commit_changes (NMSettingsConnection *connection, if (!nm_keyfile_plugin_write_connection (NM_CONNECTION (connection), nm_settings_connection_get_filename (connection), + NM_FLAGS_ALL (commit_reason, NM_SETTINGS_CONNECTION_COMMIT_REASON_USER_ACTION + | NM_SETTINGS_CONNECTION_COMMIT_REASON_ID_CHANGED), &path, &error)) { callback (connection, error, user_data); @@ -128,6 +131,7 @@ commit_changes (NMSettingsConnection *connection, g_free (path); NM_SETTINGS_CONNECTION_CLASS (nm_keyfile_connection_parent_class)->commit_changes (connection, + commit_reason, callback, user_data); } diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index 57b0d094fb..71f407dd57 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -514,7 +514,7 @@ add_connection (NMSystemConfigInterface *config, gs_free char *path = NULL; if (save_to_disk) { - if (!nm_keyfile_plugin_write_connection (connection, NULL, &path, error)) + if (!nm_keyfile_plugin_write_connection (connection, NULL, FALSE, &path, error)) return NULL; } return NM_SETTINGS_CONNECTION (update_connection (self, connection, path, NULL, FALSE, NULL, error)); diff --git a/src/settings/plugins/keyfile/writer.c b/src/settings/plugins/keyfile/writer.c index e962ed5522..e6ee7f97cc 100644 --- a/src/settings/plugins/keyfile/writer.c +++ b/src/settings/plugins/keyfile/writer.c @@ -234,6 +234,7 @@ _internal_write_connection (NMConnection *connection, uid_t owner_uid, pid_t owner_grp, const char *existing_path, + gboolean force_rename, char **out_path, GError **error) { @@ -268,7 +269,7 @@ _internal_write_connection (NMConnection *connection, /* If we have existing file path, use it. Else generate one from * connection's ID. */ - if (existing_path != NULL) { + if (existing_path != NULL && !force_rename) { path = g_strdup (existing_path); } else { char *filename_escaped = nm_keyfile_plugin_utils_escape_filename (id); @@ -365,6 +366,7 @@ _internal_write_connection (NMConnection *connection, gboolean nm_keyfile_plugin_write_connection (NMConnection *connection, const char *existing_path, + gboolean force_rename, char **out_path, GError **error) { @@ -372,6 +374,7 @@ nm_keyfile_plugin_write_connection (NMConnection *connection, KEYFILE_DIR, 0, 0, existing_path, + force_rename, out_path, error); } @@ -388,6 +391,7 @@ nm_keyfile_plugin_write_test_connection (NMConnection *connection, keyfile_dir, owner_uid, owner_grp, NULL, + FALSE, out_path, error); } diff --git a/src/settings/plugins/keyfile/writer.h b/src/settings/plugins/keyfile/writer.h index 8b08812d86..95885106df 100644 --- a/src/settings/plugins/keyfile/writer.h +++ b/src/settings/plugins/keyfile/writer.h @@ -27,6 +27,7 @@ gboolean nm_keyfile_plugin_write_connection (NMConnection *connection, const char *existing_path, + gboolean force_rename, char **out_path, GError **error);