fixup! keyfile: add keyfile.rename-file config optionth/keyfile-write-connection-bgo740738

Revert previous commit, and implmenet:

    https://bugzilla.gnome.org/show_bug.cgi?id=740738#c20:

    > On the other other hand, I tend to assume that if I have a
    > connection named
    > "foobar", that it's going to be in
    > /etc/sysconfig/network-scripts/ifcfg-foobar,
    > and I find it confusing when it's not.

    My feeling is that if you explicitly rename the connection through NM
    (which most people won't do, and most people on servers won't do
    either), then yeah, I think it should get renamed.  We should obviously
    make sure it only happens when explicitly requested, of course (eg, NM
    should *not* rename when it reads a connection off disk where the ID !=
    filename), and these patches do that AFAICT.

    > How about if we rename the file if its old filename matches its old
    > ID, but
    > keep the old filename otherwise?

    I would agree with that, it sounds like a nice compromise.

It's only implemented for keyfile (for now).

12 files changed, 39 insertions, 25 deletions

diff --git a/man/NetworkManager.conf.xml.in b/man/NetworkManager.conf.xml.in
index f0e95b7fc7..4d4c8a293f 100644
--- a/man/NetworkManager.conf.xml.in
+++ b/man/NetworkManager.conf.xml.in
@@ -353,14 +353,6 @@ unmanaged-devices=mac:00:22:68:1c:59:b1;mac:00:1E:65:30:D1:C4;interface-name:eth
 	  </para>
 	  </listitem>
 	</varlistentry>
-	<varlistentry>
-	  <term><varname>rename-file</varname></term>
-	  <listitem><para>Boolean key which indicates whether keyfile plugin
-	  should rename an existing keyfile when updating the connection. By
-	  default, keyfile does not rename the file which means that if you change
-	  the connection id, the file name and connection name can mismatch.
-	  </para></listitem>
-	</varlistentry>
       </variablelist>
     </para>
   </refsect1>
diff --git a/src/nm-config.h b/src/nm-config.h
index 6b5dd28daa..870d4e4fe5 100644
--- a/src/nm-config.h
+++ b/src/nm-config.h
@@ -66,7 +66,6 @@ G_BEGIN_DECLS
 #define NM_CONFIG_KEYFILE_KEY_IFNET_AUTO_REFRESH            "auto_refresh"
 #define NM_CONFIG_KEYFILE_KEY_IFNET_MANAGED                 "managed"
 #define NM_CONFIG_KEYFILE_KEY_IFUPDOWN_MANAGED              "managed"
-#define NM_CONFIG_KEYFILE_KEY_KEYFILE_RENAME_FILE           "rename-file"
 
 #define NM_CONFIG_KEYFILE_KEYPREFIX_WAS                     ".was."
 #define NM_CONFIG_KEYFILE_KEYPREFIX_SET                     ".set."
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 b13caef2ea..e6ee7f97cc 100644
--- a/src/settings/plugins/keyfile/writer.c
+++ b/src/settings/plugins/keyfile/writer.c
@@ -33,7 +33,6 @@
 #include "writer.h"
 #include "common.h"
 #include "utils.h"
-#include "nm-config.h"
 #include "nm-keyfile-internal.h"
 #include "gsystem-local-alloc.h"
 
@@ -235,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)
 {
@@ -269,11 +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
-	    && !nm_config_data_get_value_boolean (NM_CONFIG_GET_DATA,
-	                                          NM_CONFIG_KEYFILE_GROUP_KEYFILE,
-	                                          NM_CONFIG_KEYFILE_KEY_KEYFILE_RENAME_FILE,
-	                                          FALSE)) {
+	if (existing_path != NULL && !force_rename) {
 		path = g_strdup (existing_path);
 	} else {
 		char *filename_escaped = nm_keyfile_plugin_utils_escape_filename (id);
@@ -370,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)
 {
@@ -377,6 +374,7 @@ nm_keyfile_plugin_write_connection (NMConnection *connection,
 	                                   KEYFILE_DIR,
 	                                   0, 0,
 	                                   existing_path,
+	                                   force_rename,
 	                                   out_path,
 	                                   error);
 }
@@ -393,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);