From 20e60deccb023917f89bd3d5433295a72af67566 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 23 May 2012 16:19:28 +0200 Subject: [PATCH] ifcfg-rh: add support for reading/writing bridge port connections 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 | 176 ++++++++++++------ .../ifcfg-test-bridge-component | 2 +- .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 133 ++++++++++++- src/settings/plugins/ifcfg-rh/utils.c | 3 +- src/settings/plugins/ifcfg-rh/writer.c | 57 +++++- 5 files changed, 299 insertions(+), 72 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c index 41b8549bd5..bbe8343608 100644 --- a/src/settings/plugins/ifcfg-rh/reader.c +++ b/src/settings/plugins/ifcfg-rh/reader.c @@ -45,6 +45,7 @@ #include #include #include +#include #include #include "wifi-utils.h" @@ -199,6 +200,23 @@ make_connection_setting (const char *file, g_strfreev (items); } + 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); } @@ -3715,8 +3733,13 @@ bond_connection_from_ifcfg (const char *file, return connection; } +typedef void (*BridgeOptFunc) (NMSetting *setting, + gboolean stp, + const char *key, + const char *value); + static void -handle_bridge_option (NMSettingBridge *s_bridge, +handle_bridge_option (NMSetting *setting, gboolean stp, const char *key, const char *value) @@ -3727,32 +3750,59 @@ handle_bridge_option (NMSettingBridge *s_bridge, if (stp == FALSE) { PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: 'priority' invalid when STP is disabled"); } else if (get_uint (value, &u)) - g_object_set (s_bridge, NM_SETTING_BRIDGE_PRIORITY, u, NULL); + g_object_set (setting, NM_SETTING_BRIDGE_PRIORITY, u, NULL); else PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: invalid priority value '%s'", value); } else if (!strcmp (key, "hello_time")) { if (stp == FALSE) { PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: 'hello_time' invalid when STP is disabled"); } else if (get_uint (value, &u)) - g_object_set (s_bridge, NM_SETTING_BRIDGE_HELLO_TIME, u, NULL); + g_object_set (setting, NM_SETTING_BRIDGE_HELLO_TIME, u, NULL); else PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: invalid hello_time value '%s'", value); } else if (!strcmp (key, "max_age")) { if (stp == FALSE) { PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: 'max_age' invalid when STP is disabled"); } else if (get_uint (value, &u)) - g_object_set (s_bridge, NM_SETTING_BRIDGE_MAX_AGE, u, NULL); + g_object_set (setting, NM_SETTING_BRIDGE_MAX_AGE, u, NULL); else PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: invalid max_age value '%s'", value); } else if (!strcmp (key, "ageing_time")) { if (get_uint (value, &u)) - g_object_set (s_bridge, NM_SETTING_BRIDGE_AGEING_TIME, u, NULL); + g_object_set (setting, NM_SETTING_BRIDGE_AGEING_TIME, u, NULL); else PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: invalid ageing_time value '%s'", value); } else PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: unhandled bridge option '%s'", key); } +static void +handle_bridging_opts (NMSetting *setting, + gboolean stp, + const char *value, + BridgeOptFunc func) +{ + char **items, **iter; + + items = g_strsplit_set (value, " ", -1); + for (iter = items; iter && *iter; iter++) { + if (strlen (*iter)) { + char **keys, *key, *val; + + keys = g_strsplit_set (*iter, "=", 2); + if (keys && *keys) { + key = *keys; + val = *(keys + 1); + if (val && strlen(key) && strlen(val)) + func (setting, stp, key, val); + } + + g_strfreev (keys); + } + } + g_strfreev (items); +} + static NMSetting * make_bridge_setting (shvarFile *ifcfg, const char *file, @@ -3802,26 +3852,8 @@ make_bridge_setting (shvarFile *ifcfg, value = svGetValue (ifcfg, "BRIDGING_OPTS", FALSE); if (value) { - char **items, **iter; - - items = g_strsplit_set (value, " ", -1); - for (iter = items; iter && *iter; iter++) { - if (strlen (*iter)) { - char **keys, *key, *val; - - keys = g_strsplit_set (*iter, "=", 2); - if (keys && *keys) { - key = *keys; - val = *(keys + 1); - if (val && strlen(key) && strlen(val)) - handle_bridge_option (s_bridge, stp, key, val); - } - - g_strfreev (keys); - } - } + handle_bridging_opts (NM_SETTING (s_bridge), stp, value, handle_bridge_option); g_free (value); - g_strfreev (items); } return (NMSetting *) s_bridge; @@ -3876,6 +3908,57 @@ bridge_connection_from_ifcfg (const char *file, return connection; } +static void +handle_bridge_port_option (NMSetting *setting, + gboolean stp, + const char *key, + const char *value) +{ + guint32 u = 0; + + if (!strcmp (key, "priority")) { + if (get_uint (value, &u)) + g_object_set (setting, NM_SETTING_BRIDGE_PORT_PRIORITY, u, NULL); + else + PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: invalid priority value '%s'", value); + } else if (!strcmp (key, "path_cost")) { + if (get_uint (value, &u)) + g_object_set (setting, NM_SETTING_BRIDGE_PORT_PATH_COST, u, NULL); + else + PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: invalid path_cost value '%s'", value); + } else if (!strcmp (key, "hairpin_mode")) { + if (!strcasecmp (value, "on") || !strcasecmp (value, "yes") || !strcmp (value, "1")) + g_object_set (setting, NM_SETTING_BRIDGE_PORT_HAIRPIN_MODE, TRUE, NULL); + else if (!strcasecmp (value, "off") || !strcasecmp (value, "no")) + g_object_set (setting, NM_SETTING_BRIDGE_PORT_HAIRPIN_MODE, FALSE, NULL); + else + PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: invalid hairpin_mode value '%s'", value); + } else + PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: unhandled bridge port option '%s'", key); +} + +static NMSetting * +make_bridge_port_setting (shvarFile *ifcfg, GError **error) +{ + NMSetting *s_port; + char *value; + + value = svGetValue (ifcfg, "BRIDGE", FALSE); + if (!value) + return NULL; + g_free (value); + + s_port = nm_setting_bridge_port_new (); + + value = svGetValue (ifcfg, "BRIDGING_OPTS", FALSE); + if (value) { + handle_bridging_opts (s_port, FALSE, value, handle_bridge_port_option); + g_free (value); + } + + return s_port; +} + static gboolean is_bond_device (const char *name, shvarFile *parsed) { @@ -4108,11 +4191,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 */ @@ -4127,13 +4205,12 @@ connection_from_file (const char *filename, { NMConnection *connection = NULL; shvarFile *parsed; - char *type, *nmc = NULL, *bootproto, *tmp; - NMSetting *s_ip4, *s_ip6; + char *type, *nmc = NULL, *bootproto; + NMSetting *s_ip4, *s_ip6, *s_port; 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); @@ -4232,14 +4309,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); @@ -4266,24 +4335,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) { @@ -4313,6 +4366,15 @@ connection_from_file (const char *filename, } else if (s_ip4) nm_connection_add_setting (connection, s_ip4); + /* Bridge port? */ + s_port = make_bridge_port_setting (parsed, &error); + if (error) { + g_object_unref (connection); + connection = NULL; + goto done; + } else if (s_port) + nm_connection_add_setting (connection, s_port); + /* iSCSI / ibft connections are read-only since their settings are * stored in NVRAM and can only be changed in BIOS. */ diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-bridge-component b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-bridge-component index f586637ec3..24b5122188 100644 --- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-bridge-component +++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-bridge-component @@ -2,4 +2,4 @@ DEVICE=eth0 HWADDR=00:22:15:59:62:97 ONBOOT=no BRIDGE=br0 - +BRIDGING_OPTS="priority=28 hairpin_mode=1 path_cost=100" 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 e2fe28cc70..2b8df270a0 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -12089,12 +12089,15 @@ static void test_read_bridge_component (void) { NMConnection *connection; + NMSettingConnection *s_con; + NMSettingBridgePort *s_port; char *unmanaged = NULL; char *keyfile = NULL; char *routefile = NULL; char *route6file = NULL; gboolean ignore_error = FALSE; GError *error = NULL; + gboolean success; connection = connection_from_file (TEST_IFCFG_BRIDGE_COMPONENT, NULL, @@ -12106,14 +12109,22 @@ test_read_bridge_component (void) &route6file, &error, &ignore_error); - ASSERT (connection != NULL, - "bridge-component-read", "unexpected failure reading %s", TEST_IFCFG_BRIDGE_COMPONENT); + g_assert (connection); - ASSERT (unmanaged != NULL, - "bridge-component-read", "missing unmanaged spec from %s", TEST_IFCFG_BRIDGE_COMPONENT); + success = nm_connection_verify (connection, &error); + g_assert_no_error (error); + g_assert (success); - ASSERT (g_strcmp0 (unmanaged, "mac:00:22:15:59:62:97") == 0, - "bridge-component-read", "unexpected unmanaged spec from %s", TEST_IFCFG_BRIDGE_COMPONENT); + s_con = nm_connection_get_setting_connection (connection); + g_assert (s_con); + g_assert_cmpstr (nm_setting_connection_get_master (s_con), ==, "br0"); + g_assert_cmpstr (nm_setting_connection_get_slave_type (s_con), ==, NM_SETTING_BRIDGE_SETTING_NAME); + + s_port = nm_connection_get_setting_bridge_port (connection); + g_assert (s_port); + g_assert (nm_setting_bridge_port_get_hairpin_mode (s_port)); + g_assert_cmpuint (nm_setting_bridge_port_get_priority (s_port), ==, 28); + g_assert_cmpuint (nm_setting_bridge_port_get_path_cost (s_port), ==, 100); g_free (unmanaged); g_free (keyfile); @@ -12122,6 +12133,113 @@ 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; + NMSetting *s_port; + 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 (); + g_assert (connection); + + /* Connection setting */ + s_con = (NMSettingConnection *) nm_setting_connection_new (); + g_assert (s_con); + 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 (); + g_assert (s_wired); + 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); + + /* Bridge port */ + s_port = nm_setting_bridge_port_new (); + nm_connection_add_setting (connection, s_port); + g_object_set (s_port, + NM_SETTING_BRIDGE_PORT_PRIORITY, 50, + NM_SETTING_BRIDGE_PORT_PATH_COST, 33, + NULL); + + success = nm_connection_verify (connection, &error); + g_assert_no_error (error); + g_assert (success); + + /* Save the ifcfg */ + success = writer_new_connection (connection, + TEST_SCRATCH_DIR "/network-scripts/", + &testfile, + &error); + g_assert_no_error (error); + g_assert (success); + g_assert (testfile); + + /* re-read the connection for comparison */ + reread = connection_from_file (testfile, + NULL, + TYPE_ETHERNET, + NULL, + &unmanaged, + &keyfile, + &routefile, + &route6file, + &error, + &ignore_error); + unlink (testfile); + + g_assert (reread); + + success = nm_connection_verify (reread, &error); + g_assert_no_error (error); + + g_assert (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT)); + + 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 @@ -13306,13 +13424,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 4e0afb96f3..04df667135 100644 --- a/src/settings/plugins/ifcfg-rh/utils.c +++ b/src/settings/plugins/ifcfg-rh/utils.c @@ -454,7 +454,8 @@ utils_ignore_ip_config (NMConnection *connection) /* bonding slaves have no IP configuration, and the system * scripts just ignore it if it's there. */ - if (nm_setting_connection_is_slave_type (s_con, NM_SETTING_BOND_SETTING_NAME)) + 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 3817ec8b27..f0ee023ba5 100644 --- a/src/settings/plugins/ifcfg-rh/writer.c +++ b/src/settings/plugins/ifcfg-rh/writer.c @@ -1289,13 +1289,13 @@ write_bonding_setting (NMConnection *connection, shvarFile *ifcfg, GError **erro } static guint32 -get_bridge_default (NMSettingBridge *s_br, const char *prop) +get_setting_default (NMSetting *setting, const char *prop) { GParamSpec *pspec; GValue val = { 0 }; guint32 ret = 0; - pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (s_br), prop); + pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (setting), prop); g_assert (pspec); g_value_init (&val, pspec->value_type); g_param_value_set_default (pspec, &val); @@ -1339,7 +1339,7 @@ write_bridge_setting (NMConnection *connection, shvarFile *ifcfg, GError **error svSetValue (ifcfg, "STP", "yes", FALSE); i = nm_setting_bridge_get_forward_delay (s_bridge); - if (i && i != get_bridge_default (s_bridge, NM_SETTING_BRIDGE_FORWARD_DELAY)) { + if (i && i != get_setting_default (NM_SETTING (s_bridge), NM_SETTING_BRIDGE_FORWARD_DELAY)) { s = g_strdup_printf ("%u", i); svSetValue (ifcfg, "DELAY", s, FALSE); g_free (s); @@ -1348,14 +1348,14 @@ write_bridge_setting (NMConnection *connection, shvarFile *ifcfg, GError **error g_string_append_printf (opts, "priority=%u", nm_setting_bridge_get_priority (s_bridge)); i = nm_setting_bridge_get_hello_time (s_bridge); - if (i && i != get_bridge_default (s_bridge, NM_SETTING_BRIDGE_HELLO_TIME)) { + if (i && i != get_setting_default (NM_SETTING (s_bridge), NM_SETTING_BRIDGE_HELLO_TIME)) { if (opts->len) g_string_append_c (opts, ' '); g_string_append_printf (opts, "hello_time=%u", i); } i = nm_setting_bridge_get_max_age (s_bridge); - if (i && i != get_bridge_default (s_bridge, NM_SETTING_BRIDGE_MAX_AGE)) { + if (i && i != get_setting_default (NM_SETTING (s_bridge), NM_SETTING_BRIDGE_MAX_AGE)) { if (opts->len) g_string_append_c (opts, ' '); g_string_append_printf (opts, "max_age=%u", i); @@ -1363,7 +1363,7 @@ write_bridge_setting (NMConnection *connection, shvarFile *ifcfg, GError **error } i = nm_setting_bridge_get_ageing_time (s_bridge); - if (i != get_bridge_default (s_bridge, NM_SETTING_BRIDGE_AGEING_TIME)) { + if (i != get_setting_default (NM_SETTING (s_bridge), NM_SETTING_BRIDGE_AGEING_TIME)) { if (opts->len) g_string_append_c (opts, ' '); g_string_append_printf (opts, "ageing_time=%u", i); @@ -1378,6 +1378,46 @@ write_bridge_setting (NMConnection *connection, shvarFile *ifcfg, GError **error return TRUE; } +static gboolean +write_bridge_port_setting (NMConnection *connection, shvarFile *ifcfg, GError **error) +{ + NMSettingBridgePort *s_port; + guint32 i; + GString *opts; + + s_port = nm_connection_get_setting_bridge_port (connection); + if (!s_port) + return TRUE; + + svSetValue (ifcfg, "BRIDGING_OPTS", NULL, FALSE); + + /* Bridge options */ + opts = g_string_sized_new (32); + + i = nm_setting_bridge_port_get_priority (s_port); + if (i && i != get_setting_default (NM_SETTING (s_port), NM_SETTING_BRIDGE_PORT_PRIORITY)) + g_string_append_printf (opts, "priority=%u", i); + + i = nm_setting_bridge_port_get_path_cost (s_port); + if (i && i != get_setting_default (NM_SETTING (s_port), NM_SETTING_BRIDGE_PORT_PATH_COST)) { + if (opts->len) + g_string_append_c (opts, ' '); + g_string_append_printf (opts, "path_cost=%u", i); + } + + if (nm_setting_bridge_port_get_hairpin_mode (s_port)) { + if (opts->len) + g_string_append_c (opts, ' '); + g_string_append_printf (opts, "hairpin_mode=1"); + } + + if (opts->len) + svSetValue (ifcfg, "BRIDGING_OPTS", opts->str, FALSE); + g_string_free (opts, TRUE); + + return TRUE; +} + static void write_connection_setting (NMSettingConnection *s_con, shvarFile *ifcfg) { @@ -1419,6 +1459,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); } /* secondary connection UUIDs */ @@ -2159,6 +2201,9 @@ write_connection (NMConnection *connection, goto out; } + if (!write_bridge_port_setting (connection, ifcfg, error)) + goto out; + if (!utils_ignore_ip_config (connection)) { svSetValue (ifcfg, "DHCP_HOSTNAME", NULL, FALSE);