libnm-core: fix NMSettingConnection:verify() not to modify interface-name

verify() used to modify interface-name of the base settings. This is
discouraged, because verify() should not touch the connection.

For libnm-core we can change behavior and only modify the connection
in normalize().

Also, be more strict not to verify() sucessfully on invalid
interface-name.

Signed-off-by: Thomas Haller <thaller@redhat.com>

6 files changed, 52 insertions, 118 deletions

diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c
index 8ff956e298..8a7aa10cb8 100644
--- a/libnm-core/nm-connection.c
+++ b/libnm-core/nm-connection.c
@@ -798,10 +798,7 @@ _nm_connection_verify (NMConnection *connection, GError **error)
 	g_hash_table_iter_init (&iter, priv->settings);
 	while (g_hash_table_iter_next (&iter, NULL, &value)) {
 		/* Order NMSettingConnection so that it will be verified first.
-		 * The reason is, that NMSettingConnection:verify() modifies the connection
-		 * by setting NMSettingConnection:interface_name. So we want to call that
-		 * verify() first, because the order can affect the outcome.
-		 * Another reason is, that errors in this setting might be more fundamental
+		 * The reason is, that errors in this setting might be more fundamental
 		 * and should be checked and reported with higher priority.
 		 * Another reason is, that some settings look especially at the
 		 * NMSettingConnection, so they find it first in the all_settings list. */
diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c
index 28e5adf7de..b605d8289c 100644
--- a/libnm-core/nm-setting-connection.c
+++ b/libnm-core/nm-setting-connection.c
@@ -752,7 +752,6 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
 	NMSettingConnectionPrivate *priv = NM_SETTING_CONNECTION_GET_PRIVATE (setting);
 	gboolean is_slave;
 	const char *slave_setting_type = NULL;
-	GSList *iter;
 	NMSetting *normerr_base_type = NULL;
 	const char *normerr_slave_setting_type = NULL;
 	const char *normerr_missing_slave_type = NULL;
@@ -792,38 +791,6 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
 		return FALSE;
 	}
 
-	/* FIXME: previously, verify() set the NMSettingConnection:interface_name property,
-	 * thus modifying the setting. verify() should not do this, but keep this not to change
-	 * behaviour.
-	 */
-	if (!priv->interface_name) {
-		for (iter = all_settings; iter; iter = iter->next) {
-			NMSetting *s_current = iter->data;
-			char *virtual_iface_name = NULL;
-
-			if (NM_IS_SETTING_BOND (s_current))
-				g_object_get (s_current, NM_SETTING_BOND_INTERFACE_NAME, &virtual_iface_name, NULL);
-			else if (NM_IS_SETTING_BRIDGE (s_current))
-				g_object_get (s_current, NM_SETTING_BRIDGE_INTERFACE_NAME, &virtual_iface_name, NULL);
-			else if (NM_IS_SETTING_TEAM (s_current))
-				g_object_get (s_current, NM_SETTING_TEAM_INTERFACE_NAME, &virtual_iface_name, NULL);
-			else if (NM_IS_SETTING_VLAN (s_current))
-				g_object_get (s_current, NM_SETTING_VLAN_INTERFACE_NAME, &virtual_iface_name, NULL);
-			/* For NMSettingInfiniband, virtual_iface_name has no backing field.
-			 * No need to set the (unset) interface_name to the default value.
-			 **/
-
-			if (virtual_iface_name) {
-				if (nm_utils_iface_valid_name (virtual_iface_name)) {
-					/* found a new interface name. */
-					priv->interface_name = virtual_iface_name;
-					break;
-				}
-				g_free (virtual_iface_name);
-			}
-		}
-	}
-
 	if (priv->interface_name) {
 		if (!nm_utils_iface_valid_name (priv->interface_name)) {
 			g_set_error (error,
diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c
index e6305f6ecb..a29b6678ea 100644
--- a/libnm-core/nm-setting.c
+++ b/libnm-core/nm-setting.c
@@ -1424,7 +1424,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name,
 			                     e_invalid_property,
 			                     _("property is invalid"));
 			g_prefix_error (error, "%s.%s: ", setting_name, setting_property);
-			return NM_SETTING_VERIFY_NORMALIZABLE;
+			return NM_SETTING_VERIFY_NORMALIZABLE_ERROR;
 		}
 		return NM_SETTING_VERIFY_SUCCESS;
 	}
@@ -1459,7 +1459,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name,
 		                     NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY,
 		                     _("property is missing"));
 		g_prefix_error (error, "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_INTERFACE_NAME);
-		return NM_SETTING_VERIFY_NORMALIZABLE;
+		return NM_SETTING_VERIFY_NORMALIZABLE_ERROR;
 	}
 	if (!nm_utils_iface_valid_name (con_name)) {
 		/* NMSettingConnection:interface_name is invalid, we cannot normalize it. */
@@ -1477,19 +1477,17 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name,
 		                     e_missing_property,
 		                     _("property is missing"));
 		g_prefix_error (error, "%s.%s: ", setting_name, setting_property);
-		return NM_SETTING_VERIFY_NORMALIZABLE;
+		return NM_SETTING_VERIFY_NORMALIZABLE_ERROR;
 	}
 	if (strcmp (con_name, interface_name) != 0) {
 		/* con_name and interface_name are different. It can be normalized by setting interface_name
 		 * to con_name. */
 		g_set_error_literal (error,
 		                     error_quark,
-		                     e_missing_property,
+		                     e_invalid_property,
 		                     _("property is invalid"));
 		g_prefix_error (error, "%s.%s: ", setting_name, setting_property);
-		/* we would like to make this a NORMALIZEABLE_ERROR, but that might
-		 * break older connections. */
-		return NM_SETTING_VERIFY_NORMALIZABLE;
+		return NM_SETTING_VERIFY_NORMALIZABLE_ERROR;
 	}
 
 	return NM_SETTING_VERIFY_SUCCESS;
diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c
index 8e01677294..2d26440b91 100644
--- a/libnm-core/tests/test-general.c
+++ b/libnm-core/tests/test-general.c
@@ -2562,43 +2562,27 @@ test_setting_old_uuid (void)
 	g_assert (success == TRUE);
 }
 
-/*
- * nm_connection_verify() modifies the connection by setting
- * the interface-name property to the virtual_iface_name of
- * the type specific settings.
- *
- * It would be preferable of verify() not to touch the connection,
- * but as it is now, stick with it and test it.
- **/
 static void
-test_connection_verify_sets_interface_name (void)
+test_connection_normalize_connection_interface_name (void)
 {
 	NMConnection *con;
 	NMSettingConnection *s_con;
 	NMSettingBond *s_bond;
-	GError *error = NULL;
-	gboolean success;
 
-	s_con = (NMSettingConnection *) nm_setting_connection_new ();
-	g_object_set (G_OBJECT (s_con),
-	              NM_SETTING_CONNECTION_ID, "test1",
-	              NM_SETTING_CONNECTION_UUID, "22001632-bbb4-4616-b277-363dce3dfb5b",
-	              NM_SETTING_CONNECTION_TYPE, NM_SETTING_BOND_SETTING_NAME,
-	              NULL);
-	s_bond = (NMSettingBond *) nm_setting_bond_new ();
+	con = nmtst_create_minimal_connection ("test1",
+	                                       "22001632-bbb4-4616-b277-363dce3dfb5b",
+	                                       NM_SETTING_BOND_SETTING_NAME,
+	                                       &s_con);
+
+	s_bond = nm_connection_get_setting_bond (con);
 	g_object_set (G_OBJECT (s_bond),
 	              NM_SETTING_BOND_INTERFACE_NAME, "bond-x",
 	              NULL);
 
-	con = nm_simple_connection_new ();
-	nm_connection_add_setting (con, NM_SETTING (s_con));
-	nm_connection_add_setting (con, NM_SETTING (s_bond));
-
 	g_assert_cmpstr (nm_connection_get_interface_name (con), ==, NULL);
 
 	/* for backward compatiblity, normalizes the interface name */
-	success = nm_connection_verify (con, &error);
-	g_assert (success && !error);
+	nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY);
 
 	g_assert_cmpstr (nm_connection_get_interface_name (con), ==, "bond-x");
 
@@ -2611,68 +2595,61 @@ test_connection_verify_sets_interface_name (void)
 static void
 test_connection_normalize_virtual_iface_name (void)
 {
-	NMConnection *con;
+	gs_unref_object NMConnection *con = NULL;
 	NMSettingConnection *s_con;
 	NMSettingVlan *s_vlan;
-	NMSetting *setting;
-	GError *error = NULL;
-	gboolean success;
 	const char *IFACE_NAME = "iface";
 	const char *IFACE_VIRT = "iface-X";
-	gboolean modified = FALSE;
 
-	con = nm_simple_connection_new ();
+	con = nmtst_create_minimal_connection ("test1",
+	                                       "22001632-bbb4-4616-b277-363dce3dfb5b",
+	                                       NM_SETTING_VLAN_SETTING_NAME,
+	                                       &s_con);
 
-	setting = nm_setting_ip4_config_new ();
-	g_object_set (setting,
-	              NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO,
-	              NULL);
-	nm_connection_add_setting (con, setting);
+	nm_connection_add_setting (con,
+	    g_object_new (NM_TYPE_SETTING_IP4_CONFIG,
+	                  NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO,
+	                  NULL));
 
-	setting = nm_setting_ip6_config_new ();
-	g_object_set (setting,
-	              NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_AUTO,
-	              NM_SETTING_IP6_CONFIG_MAY_FAIL, TRUE,
-	              NULL);
-	nm_connection_add_setting (con, setting);
+	nm_connection_add_setting (con,
+	    g_object_new (NM_TYPE_SETTING_IP6_CONFIG,
+	                  NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO,
+	                  NULL));
+
+	s_vlan = nm_connection_get_setting_vlan (con);
 
-	s_con = (NMSettingConnection *) nm_setting_connection_new ();
-	g_object_set (G_OBJECT (s_con),
-	              NM_SETTING_CONNECTION_ID, "test1",
-	              NM_SETTING_CONNECTION_UUID, "22001632-bbb4-4616-b277-363dce3dfb5b",
-	              NM_SETTING_CONNECTION_TYPE, NM_SETTING_VLAN_SETTING_NAME,
-	              NM_SETTING_CONNECTION_INTERFACE_NAME, IFACE_NAME,
-	              NULL);
-	s_vlan = (NMSettingVlan *) nm_setting_vlan_new ();
 	g_object_set (G_OBJECT (s_vlan),
-	              NM_SETTING_VLAN_INTERFACE_NAME, IFACE_VIRT,
 	              NM_SETTING_VLAN_PARENT, "eth0",
 	              NULL);
 
-	nm_connection_add_setting (con, NM_SETTING (s_con));
-	nm_connection_add_setting (con, NM_SETTING (s_vlan));
+	g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_INTERFACE_NAME, IFACE_NAME, NULL);
+	g_object_set (G_OBJECT (s_vlan), NM_SETTING_VLAN_INTERFACE_NAME, IFACE_VIRT, NULL);
 
 	g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME);
 	g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_VIRT);
-
-	/* for backward compatiblity, normalizes the interface name */
-	success = nm_connection_verify (con, &error);
-	g_assert (success && !error);
-
+	nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_VLAN_ERROR, NM_SETTING_VLAN_ERROR_INVALID_PROPERTY);
 	g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME);
-	g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_VIRT);
+	g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_NAME);
+
 
-	success = nm_connection_normalize (con, NULL, &modified, &error);
-	g_assert (success && !error);
-	g_assert (modified);
+	g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_INTERFACE_NAME, IFACE_NAME, NULL);
+	g_object_set (G_OBJECT (s_vlan), NM_SETTING_VLAN_INTERFACE_NAME, NULL, NULL);
 
 	g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME);
+	g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, NULL);
+	nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_VLAN_ERROR, NM_SETTING_VLAN_ERROR_MISSING_PROPERTY);
+	g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME);
 	g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_NAME);
 
-	success = nm_connection_verify (con, &error);
-	g_assert (success && !error);
 
-	g_object_unref (con);
+	g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_INTERFACE_NAME, NULL, NULL);
+	g_object_set (G_OBJECT (s_vlan), NM_SETTING_VLAN_INTERFACE_NAME, IFACE_NAME, NULL);
+
+	g_assert_cmpstr (nm_connection_get_interface_name (con), ==, NULL);
+	g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_NAME);
+	nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY);
+	g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME);
+	g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_NAME);
 }
 
 static void
@@ -3142,7 +3119,7 @@ int main (int argc, char **argv)
 	g_test_add_func ("/core/general/test_connection_replace_settings", test_connection_replace_settings);
 	g_test_add_func ("/core/general/test_connection_replace_settings_from_connection", test_connection_replace_settings_from_connection);
 	g_test_add_func ("/core/general/test_connection_new_from_hash", test_connection_new_from_hash);
-	g_test_add_func ("/core/general/test_connection_verify_sets_interface_name", test_connection_verify_sets_interface_name);
+	g_test_add_func ("/core/general/test_connection_normalize_connection_interface_name", test_connection_normalize_connection_interface_name);
 	g_test_add_func ("/core/general/test_connection_normalize_virtual_iface_name", test_connection_normalize_virtual_iface_name);
 	g_test_add_func ("/core/general/test_connection_normalize_type", test_connection_normalize_type);
 	g_test_add_func ("/core/general/test_connection_normalize_slave_type_1", test_connection_normalize_slave_type_1);
diff --git a/libnm-util/nm-setting.c b/libnm-util/nm-setting.c
index b479bf4aa0..b943369e2e 100644
--- a/libnm-util/nm-setting.c
+++ b/libnm-util/nm-setting.c
@@ -1340,7 +1340,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name,
 		 * to con_name. */
 		g_set_error_literal (error,
 		                     error_quark,
-		                     e_missing_property,
+		                     e_invalid_property,
 		                     _("property is invalid"));
 		g_prefix_error (error, "%s.%s: ", setting_name, setting_property);
 		/* we would like to make this a NORMALIZEABLE_ERROR, but that might
diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
index d917159efe..f835064ce2 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -12346,8 +12346,7 @@ test_write_bridge_main (void)
 	              NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE,
 	              NULL);
 
-	g_assert (nm_connection_verify (connection, &error));
-	g_assert_no_error (error);
+	nmtst_assert_connection_verifies_after_normalization (connection, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY);
 
 	/* Save the ifcfg */
 	success = writer_new_connection (connection,
@@ -13131,9 +13130,7 @@ test_write_bond_main (void)
 	              NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE,
 	              NULL);
 
-	ASSERT (nm_connection_verify (connection, &error) == TRUE,
-	        "bond-main-write", "failed to verify connection: %s",
-	        (error && error->message) ? error->message : "(unknown)");
+	nmtst_assert_connection_verifies_after_normalization (connection, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY);
 
 	/* Save the ifcfg */
 	success = writer_new_connection (connection,
@@ -14216,9 +14213,7 @@ test_write_team_master (void)
 	s_wired = (NMSettingWired *) nm_setting_wired_new ();
 	nm_connection_add_setting (connection, NM_SETTING (s_wired));
 
-	success = nm_connection_verify (connection, &error);
-	g_assert_no_error (error);
-	g_assert (success);
+	nmtst_assert_connection_verifies_after_normalization (connection, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY);
 
 	/* Save the ifcfg */
 	success = writer_new_connection (connection,