[PATCH 4/6] bridge: support bridging slaves

[Thomas Graf <tgraf redhat com>]

Allows to attach any connection to a bridge using the BRIDGE= key.
IP configuration is optional for bridge components but not
prohibited. Test case included.
---
 src/settings/plugins/ifcfg-rh/reader.c             |   52 +++----
 .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c         |  155 +++++++++++++++++++-
 src/settings/plugins/ifcfg-rh/utils.c              |    5 +-
 src/settings/plugins/ifcfg-rh/writer.c             |    2 +
 4 files changed, 175 insertions(+), 39 deletions(-)

diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c
index 8d490bf..2ea63ea 100644
--- a/src/settings/plugins/ifcfg-rh/reader.c
+++ b/src/settings/plugins/ifcfg-rh/reader.c
@@ -169,6 +169,24 @@ make_connection_setting (const char *file,
 	g_object_set (s_con, NM_SETTING_CONNECTION_ZONE, zone, NULL);
 	g_free (zone);
 
+	value = svGetValue (ifcfg, "BRIDGE", FALSE);
+	if (value) {
+		const char *bridge;
+
+		if ((bridge = nm_setting_connection_get_master (s_con))) {
+			PLUGIN_WARN (IFCFG_PLUGIN_NAME,
+			             "     warning: Already configured as slave of %s. "
+			             "Ignoring BRIDGE=\"%s\"", bridge, value);
+			g_free (value);
+		}
+
+		g_object_set (s_con, NM_SETTING_CONNECTION_MASTER, value, NULL);
+		g_object_set (s_con, NM_SETTING_CONNECTION_SLAVE_TYPE,
+		              NM_SETTING_BRIDGE_SETTING_NAME, NULL);
+		g_free (value);
+
+	}
+
 	return NM_SETTING (s_con);
 }
 
@@ -4035,11 +4053,6 @@ vlan_connection_from_ifcfg (const char *file,
 	return connection;
 }
 
-enum {
-	IGNORE_REASON_NONE = 0x00,
-	IGNORE_REASON_BRIDGE = 0x01,
-};
-
 NMConnection *
 connection_from_file (const char *filename,
                       const char *network_file,  /* for unit tests only */
@@ -4054,13 +4067,12 @@ connection_from_file (const char *filename,
 {
 	NMConnection *connection = NULL;
 	shvarFile *parsed;
-	char *type, *nmc = NULL, *bootproto, *tmp;
+	char *type, *nmc = NULL, *bootproto;
 	NMSetting *s_ip4, *s_ip6;
 	const char *ifcfg_name = NULL;
 	gboolean nm_controlled = TRUE;
 	gboolean can_disable_ip4 = FALSE;
 	GError *error = NULL;
-	guint32 ignore_reason = IGNORE_REASON_NONE;
 
 	g_return_val_if_fail (filename != NULL, NULL);
 	g_return_val_if_fail (unmanaged != NULL, NULL);
@@ -4159,14 +4171,6 @@ connection_from_file (const char *filename,
 		goto done;
 	}
 
-	/* Ignore BRIDGE= connections for now too (rh #619863) */
-	tmp = svGetValue (parsed, "BRIDGE", FALSE);
-	if (tmp) {
-		g_free (tmp);
-		nm_controlled = FALSE;
-		ignore_reason = IGNORE_REASON_BRIDGE;
-	}
-
 	/* Construct the connection */
 	if (!strcasecmp (type, TYPE_ETHERNET))
 		connection = wired_connection_from_ifcfg (filename, parsed, nm_controlled, unmanaged, &error);
@@ -4193,24 +4197,8 @@ connection_from_file (const char *filename,
 	g_free (type);
 
 	/* Don't bother reading the connection fully if it's unmanaged or ignored */
-	if (!connection || *unmanaged || ignore_reason) {
-		if (connection && !*unmanaged) {
-			/* However,BRIDGE and VLAN connections that don't have HWADDR won't
-			 * be unmanaged because the unmanaged state is keyed off HWADDR.
-			 * They willl still be tagged 'ignore' from code that checks BRIDGE
-			 * and VLAN above.  Since they aren't marked unmanaged, kill them
-			 * completely.
-			 */
-			if (ignore_reason) {
-				g_object_unref (connection);
-				connection = NULL;
-				g_set_error (&error, IFCFG_PLUGIN_ERROR, 0,
-				             "%s connections are not yet supported",
-				             ignore_reason == IGNORE_REASON_BRIDGE ? "Bridge" : "VLAN");
-			}
-		}
+	if (!connection || *unmanaged)
 		goto done;
-	}
 
 	s_ip6 = make_ip6_setting (parsed, network_file, iscsiadm_path, &error);
 	if (error) {
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 fa25dce..8570338 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -11980,6 +11980,7 @@ static void
 test_read_bridge_component (void)
 {
 	NMConnection *connection;
+	NMSettingConnection *s_con;
 	char *unmanaged = NULL;
 	char *keyfile = NULL;
 	char *routefile = NULL;
@@ -12000,11 +12001,21 @@ test_read_bridge_component (void)
 	ASSERT (connection != NULL,
 	        "bridge-component-read", "unexpected failure reading %s", TEST_IFCFG_BRIDGE_COMPONENT);
 
-	ASSERT (unmanaged != NULL,
-	        "bridge-component-read", "missing unmanaged spec from %s", TEST_IFCFG_BRIDGE_COMPONENT);
+	ASSERT (nm_connection_verify (connection, &error),
+	        "bridge-component-read", "failed to verify %s: %s", TEST_IFCFG_BRIDGE_COMPONENT, error->message);
+
+	s_con = nm_connection_get_setting_connection (connection);
+	ASSERT (s_con != NULL,
+	        "bridge-component-read", "failed to verify %s: missing %s setting",
+	        TEST_IFCFG_BRIDGE_COMPONENT, NM_SETTING_CONNECTION_SETTING_NAME);
 
-	ASSERT (g_strcmp0 (unmanaged, "mac:00:22:15:59:62:97") == 0,
-	        "bridge-component-read", "unexpected unmanaged spec from %s", TEST_IFCFG_BRIDGE_COMPONENT);
+	ASSERT (g_strcmp0 (nm_setting_connection_get_master (s_con), "br0") == 0,
+	        "bridge-component-read", "failed to verify %s: master is not br0",
+	        TEST_IFCFG_BRIDGE_COMPONENT);
+
+	ASSERT (g_strcmp0 (nm_setting_connection_get_slave_type (s_con), NM_SETTING_BRIDGE_SETTING_NAME) == 0,
+	        "bridge-component-read", "failed to verify %s: slave-type is not bridge",
+	        TEST_IFCFG_BRIDGE_COMPONENT);
 
 	g_free (unmanaged);
 	g_free (keyfile);
@@ -12013,6 +12024,139 @@ test_read_bridge_component (void)
 	g_object_unref (connection);
 }
 
+static void
+test_write_bridge_component (void)
+{
+	NMConnection *connection;
+	NMConnection *reread;
+	NMSettingConnection *s_con;
+	NMSettingWired *s_wired;
+	NMSettingIP4Config *s_ip4;
+	NMSettingIP6Config *s_ip6;
+	static unsigned char tmpmac[] = { 0x31, 0x33, 0x33, 0x37, 0xbe, 0xcd };
+	GByteArray *mac;
+	guint32 mtu = 1492;
+	char *uuid;
+	gboolean success;
+	GError *error = NULL;
+	char *testfile = NULL;
+	char *unmanaged = NULL;
+	char *keyfile = NULL;
+	char *routefile = NULL;
+	char *route6file = NULL;
+	gboolean ignore_error = FALSE;
+
+	connection = nm_connection_new ();
+	ASSERT (connection != NULL,
+	        "bridge-component-write", "failed to allocate new connection");
+
+	/* Connection setting */
+	s_con = (NMSettingConnection *) nm_setting_connection_new ();
+	ASSERT (s_con != NULL,
+	        "bridge-component-write", "failed to allocate new %s setting",
+	        NM_SETTING_CONNECTION_SETTING_NAME);
+	nm_connection_add_setting (connection, NM_SETTING (s_con));
+
+	uuid = nm_utils_uuid_generate ();
+	g_object_set (s_con,
+	              NM_SETTING_CONNECTION_ID, "Test Write Bridge Component",
+	              NM_SETTING_CONNECTION_UUID, uuid,
+	              NM_SETTING_CONNECTION_AUTOCONNECT, TRUE,
+	              NM_SETTING_CONNECTION_TYPE, NM_SETTING_WIRED_SETTING_NAME,
+				  NM_SETTING_CONNECTION_MASTER, "br0",
+				  NM_SETTING_CONNECTION_SLAVE_TYPE, NM_SETTING_BRIDGE_SETTING_NAME,
+	              NULL);
+	g_free (uuid);
+
+	/* Wired setting */
+	s_wired = (NMSettingWired *) nm_setting_wired_new ();
+	ASSERT (s_wired != NULL,
+	        "bridge-component-write", "failed to allocate new %s setting",
+	        NM_SETTING_WIRED_SETTING_NAME);
+	nm_connection_add_setting (connection, NM_SETTING (s_wired));
+
+	mac = g_byte_array_sized_new (sizeof (tmpmac));
+	g_byte_array_append (mac, &tmpmac[0], sizeof (tmpmac));
+
+	g_object_set (s_wired,
+	              NM_SETTING_WIRED_MAC_ADDRESS, mac,
+	              NM_SETTING_WIRED_MTU, mtu,
+	              NULL);
+	g_byte_array_free (mac, TRUE);
+
+	/* IP4 setting */
+	s_ip4 = (NMSettingIP4Config *) nm_setting_ip4_config_new ();
+	ASSERT (s_ip4 != NULL,
+			"bridge-component-write", "failed to allocate new %s setting",
+			NM_SETTING_IP4_CONFIG_SETTING_NAME);
+	nm_connection_add_setting (connection, NM_SETTING (s_ip4));
+
+	g_object_set (s_ip4,
+	              NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_DISABLED,
+	              NULL);
+
+	/* IP6 setting */
+	s_ip6 = (NMSettingIP6Config *) nm_setting_ip6_config_new ();
+	ASSERT (s_ip6 != NULL,
+	        "bridge-component-write", "failed to allocate new %s setting",
+	        NM_SETTING_IP6_CONFIG_SETTING_NAME);
+	nm_connection_add_setting (connection, NM_SETTING (s_ip6));
+
+	g_object_set (s_ip6,
+	              NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE,
+	              NM_SETTING_IP6_CONFIG_MAY_FAIL, TRUE,
+	              NULL);
+
+	ASSERT (nm_connection_verify (connection, &error) == TRUE,
+	        "bridge-component-write", "failed to verify connection: %s",
+	        (error && error->message) ? error->message : "(unknown)");
+
+	/* Save the ifcfg */
+	success = writer_new_connection (connection,
+	                                 TEST_SCRATCH_DIR "/network-scripts/",
+	                                 &testfile,
+	                                 &error);
+	ASSERT (success == TRUE,
+	        "bridge-component-write", "failed to write connection to disk: %s",
+	        (error && error->message) ? error->message : "(unknown)");
+
+	ASSERT (testfile != NULL,
+	        "bridge-component-write", "didn't get ifcfg file path back after writing connection");
+
+	/* re-read the connection for comparison */
+	reread = connection_from_file (testfile,
+	                               NULL,
+	                               TYPE_ETHERNET,
+	                               NULL,
+	                               &unmanaged,
+	                               &keyfile,
+	                               &routefile,
+	                               &route6file,
+	                               &error,
+	                               &ignore_error);
+	unlink (testfile);
+
+	ASSERT (reread != NULL,
+	        "bridge-component-write-reread", "failed to read %s: %s", testfile, error->message);
+
+	ASSERT (nm_connection_verify (reread, &error),
+	        "bridge-component-write-reread-verify", "failed to verify %s: %s", testfile, error->message);
+
+	ASSERT (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT) == TRUE,
+	        "bridge-component-write", "written and re-read connection weren't the same.");
+
+	if (route6file)
+		unlink (route6file);
+
+	g_free (testfile);
+	g_free (unmanaged);
+	g_free (keyfile);
+	g_free (routefile);
+	g_free (route6file);
+	g_object_unref (connection);
+	g_object_unref (reread);
+}
+
 #define TEST_IFCFG_VLAN_INTERFACE TEST_IFCFG_DIR"/network-scripts/ifcfg-test-vlan-interface"
 
 static void
@@ -13056,13 +13200,14 @@ int main (int argc, char **argv)
 
 	test_read_bridge_main ();
 	test_write_bridge_main ();
+	test_read_bridge_component ();
+	test_write_bridge_component ();
 
 	/* Stuff we expect to fail for now */
 	test_write_wired_pppoe ();
 	test_write_vpn ();
 	test_write_mobile_broadband (TRUE);
 	test_write_mobile_broadband (FALSE);
-	test_read_bridge_component ();
 
 	base = g_path_get_basename (argv[0]);
 	fprintf (stdout, "%s: SUCCESS\n", base);
diff --git a/src/settings/plugins/ifcfg-rh/utils.c b/src/settings/plugins/ifcfg-rh/utils.c
index 0b7ddf3..156f90a 100644
--- a/src/settings/plugins/ifcfg-rh/utils.c
+++ b/src/settings/plugins/ifcfg-rh/utils.c
@@ -451,8 +451,9 @@ utils_disabling_ip4_config_allowed (NMConnection *connection)
 	s_con = nm_connection_get_setting_connection (connection);
 	g_assert (s_con);
 
-	/* bonding slaves are allowed to have no ip configuration */
-	if (nm_setting_connection_is_slave_type (s_con, NM_SETTING_BOND_SETTING_NAME))
+	/* bonding and bridging slaves are allowed to have no ip configuration */
+	if (   nm_setting_connection_is_slave_type (s_con, NM_SETTING_BOND_SETTING_NAME)
+	    || nm_setting_connection_is_slave_type (s_con, NM_SETTING_BRIDGE_SETTING_NAME))
 		return TRUE;
 
 	return FALSE;
diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c
index 7e4ce77..b6b1371 100644
--- a/src/settings/plugins/ifcfg-rh/writer.c
+++ b/src/settings/plugins/ifcfg-rh/writer.c
@@ -1345,6 +1345,8 @@ write_connection_setting (NMSettingConnection *s_con, shvarFile *ifcfg)
 	if (master) {
 		if (nm_setting_connection_is_slave_type (s_con, NM_SETTING_BOND_SETTING_NAME))
 			svSetValue (ifcfg, "MASTER", master, FALSE);
+		else if (nm_setting_connection_is_slave_type (s_con, NM_SETTING_BRIDGE_SETTING_NAME))
+			svSetValue (ifcfg, "BRIDGE", master, FALSE);
 	}
 }
 
-- 
1.7.7.6