libnm-core: normalize invalid bridge|team slave-port settingsth/ifcfg-reread-rh1427482

Having a bridge-port/team-port setting for a connection that
has a different slave-type makes no sense. Such a configuration
shall be considered invalid, and be fixed by normalization.

Note that there is already a normalization the other way around,
when you omit the "slave-type" but a "master" and one(!) port-type
setting is present, the slave-type is automatically determined
based on the port-type.

The use of this is of course to modify an existing slave connection
to make it a non-slave. Then the invalid port settings should be
automatically removed.

Previously, ifcfg-rh writer would write the "BRIDGING_OPTS" setting
without a "BRIDGE". The reader would then (correctly) ignore the
bridge-port. Avoid that altogehter, by requiring the connection to
strictly verify.

4 files changed, 77 insertions, 17 deletions

diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c
index 9c51bc0af4..fa195402e3 100644
--- a/libnm-core/nm-connection.c
+++ b/libnm-core/nm-connection.c
@@ -132,15 +132,15 @@ nm_connection_add_setting (NMConnection *connection, NMSetting *setting)
  * Removes the #NMSetting with the given #GType from the #NMConnection.  This
  * operation dereferences the #NMSetting object.
  **/
-void
-nm_connection_remove_setting (NMConnection *connection, GType setting_type)
+gboolean
+_nm_connection_remove_setting (NMConnection *connection, GType setting_type)
 {
 	NMConnectionPrivate *priv;
 	NMSetting *setting;
 	const char *setting_name;
 
-	g_return_if_fail (NM_IS_CONNECTION (connection));
-	g_return_if_fail (g_type_is_a (setting_type, NM_TYPE_SETTING));
+	g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE);
+	g_return_val_if_fail (g_type_is_a (setting_type, NM_TYPE_SETTING), FALSE);
 
 	priv = NM_CONNECTION_GET_PRIVATE (connection);
 	setting_name = g_type_name (setting_type);
@@ -149,7 +149,23 @@ nm_connection_remove_setting (NMConnection *connection, GType setting_type)
 		g_signal_handlers_disconnect_by_func (setting, setting_changed_cb, connection);
 		g_hash_table_remove (priv->settings, setting_name);
 		g_signal_emit (connection, signals[CHANGED], 0);
+		return TRUE;
 	}
+	return FALSE;
+}
+
+/**
+ * nm_connection_remove_setting:
+ * @connection: a #NMConnection
+ * @setting_type: the #GType of the setting object to remove
+ *
+ * Removes the #NMSetting with the given #GType from the #NMConnection.  This
+ * operation dereferences the #NMSetting object.
+ **/
+void
+nm_connection_remove_setting (NMConnection *connection, GType setting_type)
+{
+	_nm_connection_remove_setting (connection, setting_type);
 }
 
 /**
@@ -994,6 +1010,26 @@ _normalize_required_settings (NMConnection *self, GHashTable *parameters)
 	return FALSE;
 }
 
+static gboolean
+_normalize_invalid_slave_port_settings (NMConnection *self, GHashTable *parameters)
+{
+	NMSettingConnection *s_con = nm_connection_get_setting_connection (self);
+	const char *slave_type;
+	gboolean changed = FALSE;
+
+	slave_type = nm_setting_connection_get_slave_type (s_con);
+
+	if (   !nm_streq0 (slave_type, NM_SETTING_BRIDGE_SETTING_NAME)
+	    && _nm_connection_remove_setting (self, NM_TYPE_SETTING_BRIDGE_PORT))
+		changed = TRUE;
+
+	if (   !nm_streq0 (slave_type, NM_SETTING_TEAM_SETTING_NAME)
+	    && _nm_connection_remove_setting (self, NM_TYPE_SETTING_TEAM_PORT))
+		changed = TRUE;
+
+	return changed;
+}
+
 /**
  * nm_connection_verify:
  * @connection: the #NMConnection to verify
@@ -1242,6 +1278,7 @@ nm_connection_normalize (NMConnection *connection,
 	was_modified |= _normalize_connection_type (connection);
 	was_modified |= _normalize_connection_slave_type (connection);
 	was_modified |= _normalize_required_settings (connection, parameters);
+	was_modified |= _normalize_invalid_slave_port_settings (connection, parameters);
 	was_modified |= _normalize_ip_config (connection, parameters);
 	was_modified |= _normalize_ethernet_link_neg (connection);
 	was_modified |= _normalize_infiniband_mtu (connection, parameters);
diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h
index 6d2170904b..ac292bfc18 100644
--- a/libnm-core/nm-core-internal.h
+++ b/libnm-core/nm-core-internal.h
@@ -136,6 +136,8 @@ typedef enum {
 
 NMSettingVerifyResult _nm_connection_verify (NMConnection *connection, GError **error);
 
+gboolean _nm_connection_remove_setting (NMConnection *connection, GType setting_type);
+
 NMConnection *_nm_simple_connection_new_from_dbus (GVariant      *dict,
                                                    NMSettingParseFlags parse_flags,
                                                    GError       **error);
diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c
index ac8bdf2d2d..376da69f4e 100644
--- a/libnm-core/nm-setting-connection.c
+++ b/libnm-core/nm-setting-connection.c
@@ -857,7 +857,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 {
 	NMSettingConnectionPrivate *priv = NM_SETTING_CONNECTION_GET_PRIVATE (setting);
 	gboolean is_slave;
-	const char *slave_setting_type = NULL;
+	const char *slave_setting_type;
 	NMSetting *normerr_base_type = NULL;
 	const char *normerr_slave_setting_type = NULL;
 	const char *normerr_missing_slave_type = NULL;
@@ -958,16 +958,17 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 	}
 
 	is_slave = FALSE;
-	if (priv->slave_type)
+	slave_setting_type = NULL;
+	if (priv->slave_type) {
 		is_slave = _nm_setting_slave_type_is_valid (priv->slave_type, &slave_setting_type);
-
-	if (priv->slave_type && !is_slave) {
-		g_set_error (error,
-		             NM_CONNECTION_ERROR,
-		             NM_CONNECTION_ERROR_INVALID_PROPERTY,
-		             _("Unknown slave type '%s'"), priv->slave_type);
-		g_prefix_error (error, "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_SLAVE_TYPE);
-		return FALSE;
+		if (!is_slave) {
+			g_set_error (error,
+			             NM_CONNECTION_ERROR,
+			             NM_CONNECTION_ERROR_INVALID_PROPERTY,
+			             _("Unknown slave type '%s'"), priv->slave_type);
+			g_prefix_error (error, "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_SLAVE_TYPE);
+			return FALSE;
+		}
 	}
 
 	if (is_slave) {
@@ -1063,6 +1064,24 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 		return NM_SETTING_VERIFY_NORMALIZABLE_ERROR;
 	}
 
+	if (connection) {
+		gboolean has_bridge_port = FALSE;
+
+		if (   (   !nm_streq0 (priv->slave_type, NM_SETTING_BRIDGE_SETTING_NAME)
+		        && (has_bridge_port = !!nm_connection_get_setting_by_name (connection, NM_SETTING_BRIDGE_PORT_SETTING_NAME)))
+		    || (   !nm_streq0 (priv->slave_type, NM_SETTING_TEAM_SETTING_NAME)
+		        && nm_connection_get_setting_by_name (connection, NM_SETTING_TEAM_PORT_SETTING_NAME))) {
+			g_set_error (error,
+			             NM_CONNECTION_ERROR,
+			             NM_CONNECTION_ERROR_INVALID_SETTING,
+			             _("A slave connection with '%s' set to '%s' cannot have a '%s' setting"),
+			             NM_SETTING_CONNECTION_SLAVE_TYPE, priv->slave_type ?: "",
+			             has_bridge_port ? NM_SETTING_BRIDGE_PORT_SETTING_NAME : NM_SETTING_TEAM_PORT_SETTING_NAME);
+			g_prefix_error (error, "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_SLAVE_TYPE);
+			return NM_SETTING_VERIFY_NORMALIZABLE_ERROR;
+		}
+	}
+
 	return TRUE;
 }
 
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 bc021ed14f..58c50492db 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -1928,10 +1928,12 @@ test_clear_master (void)
 	g_assert_cmpstr (nm_setting_connection_get_master (s_con), ==, NULL);
 	g_assert_cmpstr (nm_setting_connection_get_slave_type (s_con), ==, NULL);
 
+	nmtst_assert_connection_verifies_after_normalization (connection, 0, 0);
+
 	/* 4. update the connection on disk */
-	_writer_update_connection_FIXME (connection,
-	                                 TEST_SCRATCH_DIR "/network-scripts/",
-	                                 testfile);
+	_writer_update_connection (connection,
+	                           TEST_SCRATCH_DIR "/network-scripts/",
+	                           testfile);
 	keyfile = utils_get_keys_path (testfile);
 	g_assert (!g_file_test (keyfile, G_FILE_TEST_EXISTS));