From 563ead4974fa00ccc0f46dd042fcd76fee99ee82 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 22 Nov 2017 14:01:43 +0100 Subject: [PATCH 01/10] libnm-core: document bridge.mac-address as deprecated The description already says that. Also add the deprecation tag. --- clients/common/settings-docs.c.in | 2 +- libnm-core/nm-setting-bridge.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/clients/common/settings-docs.c.in b/clients/common/settings-docs.c.in index 3d6f012bf7..79cbb715b1 100644 --- a/clients/common/settings-docs.c.in +++ b/clients/common/settings-docs.c.in @@ -118,7 +118,7 @@ #define DESCRIBE_DOC_NM_SETTING_BRIDGE_FORWARD_DELAY N_("The Spanning Tree Protocol (STP) forwarding delay, in seconds.") #define DESCRIBE_DOC_NM_SETTING_BRIDGE_GROUP_FORWARD_MASK N_("A mask of group addresses to forward. Usually, group addresses in the range from 01:80:C2:00:00:00 to 01:80:C2:00:00:0F are not forwarded according to standards. This property is a mask of 16 bits, each corresponding to a group address in that range that must be forwarded. The mask can't have bits 0, 1 or 2 set because they are used for STP, MAC pause frames and LACP.") #define DESCRIBE_DOC_NM_SETTING_BRIDGE_HELLO_TIME N_("The Spanning Tree Protocol (STP) hello time, in seconds.") -#define DESCRIBE_DOC_NM_SETTING_BRIDGE_MAC_ADDRESS N_("If specified, the MAC address of bridge. When creating a new bridge, this MAC address will be set. If this field is left unspecified, the \"ethernet.cloned-mac-address\" is referred instead to generate the initial MAC address. Note that setting \"ethernet.cloned-mac-address\" anyway overwrites the MAC address of the bridge later while activating the bridge. Hence, this property is deprecated.") +#define DESCRIBE_DOC_NM_SETTING_BRIDGE_MAC_ADDRESS N_("If specified, the MAC address of bridge. When creating a new bridge, this MAC address will be set. If this field is left unspecified, the \"ethernet.cloned-mac-address\" is referred instead to generate the initial MAC address. Note that setting \"ethernet.cloned-mac-address\" anyway overwrites the MAC address of the bridge later while activating the bridge. Hence, this property is deprecated. Deprecated: 1") #define DESCRIBE_DOC_NM_SETTING_BRIDGE_MAX_AGE N_("The Spanning Tree Protocol (STP) maximum message age, in seconds.") #define DESCRIBE_DOC_NM_SETTING_BRIDGE_MULTICAST_SNOOPING N_("Controls whether IGMP snooping is enabled for this bridge. Note that if snooping was automatically disabled due to hash collisions, the system may refuse to enable the feature until the collisions are resolved.") #define DESCRIBE_DOC_NM_SETTING_BRIDGE_NAME N_("The setting's name, which uniquely identifies the setting within the connection. Each setting type has a name unique to that type, for example \"ppp\" or \"wireless\" or \"wired\".") diff --git a/libnm-core/nm-setting-bridge.c b/libnm-core/nm-setting-bridge.c index 5330038425..7f944bc8e0 100644 --- a/libnm-core/nm-setting-bridge.c +++ b/libnm-core/nm-setting-bridge.c @@ -432,6 +432,8 @@ nm_setting_bridge_class_init (NMSettingBridgeClass *setting_class) * "ethernet.cloned-mac-address" anyway overwrites the MAC address of * the bridge later while activating the bridge. Hence, this property * is deprecated. + * + * Deprecated: 1.12: Use the ethernet.cloned-mac-address property instead. **/ /* ---keyfile--- * property: mac-address From 56a02c9baf79ed49c84111cc2f94eb1998482f49 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 22 Nov 2017 13:54:56 +0100 Subject: [PATCH 02/10] ifcfg-rh: read wired properties for bridge connections A bridge connection can have ethernet settings, read them from the ifcfg file. --- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 12 ++++++++++++ src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 8 ++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index b1cf80ad91..18d07c117d 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -4805,6 +4805,8 @@ bridge_connection_from_ifcfg (const char *file, NMConnection *connection = NULL; NMSetting *con_setting = NULL; NMSetting *bridge_setting = NULL; + NMSetting *wired_setting = NULL; + NMSetting8021x *s_8021x = NULL; g_return_val_if_fail (file != NULL, NULL); g_return_val_if_fail (ifcfg != NULL, NULL); @@ -4827,6 +4829,16 @@ bridge_connection_from_ifcfg (const char *file, } nm_connection_add_setting (connection, bridge_setting); + wired_setting = make_wired_setting (ifcfg, file, &s_8021x, error); + if (!wired_setting) { + g_object_unref (connection); + return NULL; + } + nm_connection_add_setting (connection, wired_setting); + + if (s_8021x) + nm_connection_add_setting (connection, NM_SETTING (s_8021x)); + return connection; } 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 10a0f68396..e8864d2d6f 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -7440,8 +7440,8 @@ test_write_bridge_main (void) gs_unref_object NMConnection *reread = NULL; NMSettingConnection *s_con; NMSettingBridge *s_bridge; - NMSettingIPConfig *s_ip4; - NMSettingIPConfig *s_ip6; + NMSettingIPConfig *s_ip4, *s_ip6; + NMSettingWired *s_wired; NMIPAddress *addr; static const char *mac = "31:33:33:37:be:cd"; GError *error = NULL; @@ -7493,6 +7493,10 @@ test_write_bridge_main (void) NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NULL); + /* Wired setting */ + s_wired = (NMSettingWired *) nm_setting_wired_new (); + nm_connection_add_setting (connection, NM_SETTING (s_wired)); + nm_connection_add_setting (connection, nm_setting_proxy_new ()); nmtst_assert_connection_verifies_without_normalization (connection); From fb191fc282395e30df6c2c4f332ba4e8b2e5ff2a Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 22 Nov 2017 13:55:53 +0100 Subject: [PATCH 03/10] ifcfg-rh: use distinct variables for bridge and wired mac address Currently both bridge.mac-address and ethernet.cloned-mac-address get written to the same MACADDR ifcfg-rh variable; the ethernet property wins if both are present. When one property is set and the connection is saved (and thus reread) both properties are populated with the same value. This is wrong because, even if the properties have the same meaning, the setting plugin should not read something different from what was written. Also consider that after the following steps: $ nmcli con mod c ethernet.cloned-mac-address 00:11:22:33:44:55 $ nmcli con mod c ethernet.cloned-mac-address "" the connection will still have the new mac address set in the bridge.mac-address property, which is certainly unexpected. In general, mapping multiple properties to the same variable is harmful and must be avoided. Therefore, let's use a different variable for bridge.mac-address. This changes behavior, but not so much: - connections that have MACADDR set will behave as before; the only difference will be that the MAC will be present in the wired setting instead of the bridge one; - initscripts compatibility is not relevant because MACADDR for bridges was a NM extension; - if someone creates a new connection and sets bridge.mac-address NM will set the BRIDGE_MACADDR property instead of MACADDR. But this shouldn't be a big concern as bridge.mac-address is documented as deprecated and should not be used for new connections. https://bugzilla.redhat.com/show_bug.cgi?id=1516659 --- libnm-core/nm-setting-bridge.c | 4 ++-- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 2 +- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 2 +- src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 5 ++++- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/libnm-core/nm-setting-bridge.c b/libnm-core/nm-setting-bridge.c index 7f944bc8e0..187d71d3fe 100644 --- a/libnm-core/nm-setting-bridge.c +++ b/libnm-core/nm-setting-bridge.c @@ -445,10 +445,10 @@ nm_setting_bridge_class_init (NMSettingBridgeClass *setting_class) * ---end--- * ---ifcfg-rh--- * property: mac-address - * variable: MACADDR(+) + * variable: BRIDGE_MACADDR(+) * description: MAC address of the bridge. Note that this requires a recent * kernel support, originally introduced in 3.15 upstream kernel) - * MACADDR for bridges is an NM extension. + * BRIDGE_MACADDR for bridges is an NM extension. * ---end--- */ g_object_class_install_property diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 18d07c117d..bdafbd4261 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -4750,7 +4750,7 @@ make_bridge_setting (shvarFile *ifcfg, s_bridge = NM_SETTING_BRIDGE (nm_setting_bridge_new ()); - value = svGetValueStr_cp (ifcfg, "MACADDR"); + value = svGetValueStr_cp (ifcfg, "BRIDGE_MACADDR"); if (value) { value = g_strstrip (value); g_object_set (s_bridge, NM_SETTING_BRIDGE_MAC_ADDRESS, value, NULL); diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index c277cb2ad5..d5cbcd3555 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -1408,7 +1408,7 @@ write_bridge_setting (NMConnection *connection, shvarFile *ifcfg, gboolean *wire svUnsetValue (ifcfg, "DELAY"); mac = nm_setting_bridge_get_mac_address (s_bridge); - svSetValueStr (ifcfg, "MACADDR", mac); + svSetValueStr (ifcfg, "BRIDGE_MACADDR", mac); /* Bridge options */ opts = g_string_sized_new (32); 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 e8864d2d6f..7df0c4efad 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -7403,6 +7403,7 @@ test_read_bridge_main (void) { NMConnection *connection; NMSettingBridge *s_bridge; + NMSettingWired *s_wired; const char *mac; char expected_mac_address[ETH_ALEN] = { 0x00, 0x16, 0x41, 0x11, 0x22, 0x33 }; @@ -7425,7 +7426,9 @@ test_read_bridge_main (void) g_assert (!nm_setting_bridge_get_multicast_snooping (s_bridge)); /* MAC address */ - mac = nm_setting_bridge_get_mac_address (s_bridge); + s_wired = nm_connection_get_setting_wired (connection); + g_assert (s_wired); + mac = nm_setting_wired_get_cloned_mac_address (s_wired); g_assert (mac); g_assert (nm_utils_hwaddr_matches (mac, -1, expected_mac_address, ETH_ALEN)); From 3a67b496ca80e2b2043705021fd379ad6385cbbe Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 23 Nov 2017 15:20:52 +0100 Subject: [PATCH 04/10] ifcfg-rh: avoid string copies in make_bridge_setting() Also, don't g_strstrip(value) for BRIDGE_MACADDR. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index bdafbd4261..c8c62002ad 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -4734,30 +4734,30 @@ make_bridge_setting (shvarFile *ifcfg, const char *file, GError **error) { - NMSettingBridge *s_bridge; - char *value; + gs_unref_object NMSettingBridge *s_bridge = NULL; + gs_free char *value_to_free = NULL; + const char *value; guint32 u; gboolean stp = FALSE; gboolean stp_set = FALSE; - value = svGetValueStr_cp (ifcfg, "DEVICE"); + value = svGetValueStr (ifcfg, "DEVICE", &value_to_free); if (!value) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "mandatory DEVICE keyword missing"); return NULL; } - g_free (value); + nm_clear_g_free (&value_to_free); s_bridge = NM_SETTING_BRIDGE (nm_setting_bridge_new ()); - value = svGetValueStr_cp (ifcfg, "BRIDGE_MACADDR"); + value = svGetValueStr (ifcfg, "BRIDGE_MACADDR", &value_to_free); if (value) { - value = g_strstrip (value); g_object_set (s_bridge, NM_SETTING_BRIDGE_MAC_ADDRESS, value, NULL); - g_free (value); + nm_clear_g_free (&value_to_free); } - value = svGetValueStr_cp (ifcfg, "STP"); + value = svGetValueStr (ifcfg, "STP", &value_to_free); if (value) { if (!strcasecmp (value, "on") || !strcasecmp (value, "yes")) { g_object_set (s_bridge, NM_SETTING_BRIDGE_STP, TRUE, NULL); @@ -4768,7 +4768,7 @@ make_bridge_setting (shvarFile *ifcfg, stp_set = TRUE; } else PARSE_WARNING ("invalid STP value '%s'", value); - g_free (value); + nm_clear_g_free (&value_to_free); } if (!stp_set) { @@ -4776,7 +4776,7 @@ make_bridge_setting (shvarFile *ifcfg, g_object_set (s_bridge, NM_SETTING_BRIDGE_STP, FALSE, NULL); } - value = svGetValueStr_cp (ifcfg, "DELAY"); + value = svGetValueStr (ifcfg, "DELAY", &value_to_free); if (value) { if (stp) { if (get_uint (value, &u)) @@ -4785,16 +4785,16 @@ make_bridge_setting (shvarFile *ifcfg, PARSE_WARNING ("invalid forward delay value '%s'", value); } else PARSE_WARNING ("DELAY invalid when STP is disabled"); - g_free (value); + nm_clear_g_free (&value_to_free); } - value = svGetValueStr_cp (ifcfg, "BRIDGING_OPTS"); + value = svGetValueStr (ifcfg, "BRIDGING_OPTS", &value_to_free); if (value) { handle_bridging_opts (NM_SETTING (s_bridge), stp, value, handle_bridge_option); - g_free (value); + nm_clear_g_free (&value_to_free); } - return (NMSetting *) s_bridge; + return (NMSetting *) g_steal_pointer (&s_bridge); } static NMConnection * From 12788db4ee9d442e44f67a568e9f3cb92238113d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 23 Nov 2017 15:21:44 +0100 Subject: [PATCH 05/10] ifcfg-rh/trivial: rename get_uint() to get_uint32() --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index c8c62002ad..96e70f7141 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -78,7 +78,7 @@ /*****************************************************************************/ static gboolean -get_uint (const char *str, guint32 *value) +get_uint32 (const char *str, guint32 *value) { gint64 tmp; @@ -4666,37 +4666,37 @@ handle_bridge_option (NMSetting *setting, if (!strcmp (key, "priority")) { if (stp == FALSE) PARSE_WARNING ("'priority' invalid when STP is disabled"); - else if (get_uint (value, &u)) + else if (get_uint32 (value, &u)) g_object_set (setting, NM_SETTING_BRIDGE_PRIORITY, u, NULL); else PARSE_WARNING ("invalid priority value '%s'", value); } else if (!strcmp (key, "hello_time")) { if (stp == FALSE) PARSE_WARNING ("'hello_time' invalid when STP is disabled"); - else if (get_uint (value, &u)) + else if (get_uint32 (value, &u)) g_object_set (setting, NM_SETTING_BRIDGE_HELLO_TIME, u, NULL); else PARSE_WARNING ("invalid hello_time value '%s'", value); } else if (!strcmp (key, "max_age")) { if (stp == FALSE) PARSE_WARNING ("'max_age' invalid when STP is disabled"); - else if (get_uint (value, &u)) + else if (get_uint32 (value, &u)) g_object_set (setting, NM_SETTING_BRIDGE_MAX_AGE, u, NULL); else PARSE_WARNING ("invalid max_age value '%s'", value); } else if (!strcmp (key, "ageing_time")) { - if (get_uint (value, &u)) + if (get_uint32 (value, &u)) g_object_set (setting, NM_SETTING_BRIDGE_AGEING_TIME, u, NULL); else PARSE_WARNING ("invalid ageing_time value '%s'", value); } else if (!strcmp (key, "multicast_snooping")) { - if (get_uint (value, &u)) + if (get_uint32 (value, &u)) g_object_set (setting, NM_SETTING_BRIDGE_MULTICAST_SNOOPING, (gboolean) u, NULL); else PARSE_WARNING ("invalid multicast_snooping value '%s'", value); } else if (!strcmp (key, "group_fwd_mask")) { - if (get_uint (value, &u) && u <= 0xFFFF && !NM_FLAGS_ANY (u, 7)) + if (get_uint32 (value, &u) && u <= 0xFFFF && !NM_FLAGS_ANY (u, 7)) g_object_set (setting, NM_SETTING_BRIDGE_GROUP_FORWARD_MASK, (gboolean) u, NULL); else @@ -4779,7 +4779,7 @@ make_bridge_setting (shvarFile *ifcfg, value = svGetValueStr (ifcfg, "DELAY", &value_to_free); if (value) { if (stp) { - if (get_uint (value, &u)) + if (get_uint32 (value, &u)) g_object_set (s_bridge, NM_SETTING_BRIDGE_FORWARD_DELAY, u, NULL); else PARSE_WARNING ("invalid forward delay value '%s'", value); @@ -4851,12 +4851,12 @@ handle_bridge_port_option (NMSetting *setting, guint32 u = 0; if (!strcmp (key, "priority")) { - if (get_uint (value, &u)) + if (get_uint32 (value, &u)) g_object_set (setting, NM_SETTING_BRIDGE_PORT_PRIORITY, u, NULL); else PARSE_WARNING ("invalid priority value '%s'", value); } else if (!strcmp (key, "path_cost")) { - if (get_uint (value, &u)) + if (get_uint32 (value, &u)) g_object_set (setting, NM_SETTING_BRIDGE_PORT_PATH_COST, u, NULL); else PARSE_WARNING ("invalid path_cost value '%s'", value); From 30ce598fb5a667387856c435e2ba5e38434e52d7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 23 Nov 2017 15:29:15 +0100 Subject: [PATCH 06/10] ifcfg-rh: fix range and size when parsing integer values in reader --- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 96e70f7141..bbc5a9e418 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -4661,13 +4661,14 @@ handle_bridge_option (NMSetting *setting, const char *key, const char *value) { + gint64 v; guint32 u = 0; if (!strcmp (key, "priority")) { if (stp == FALSE) PARSE_WARNING ("'priority' invalid when STP is disabled"); - else if (get_uint32 (value, &u)) - g_object_set (setting, NM_SETTING_BRIDGE_PRIORITY, u, NULL); + else if ((v = _nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXUINT16, -1)) != -1) + g_object_set (setting, NM_SETTING_BRIDGE_PRIORITY, (guint) v, NULL); else PARSE_WARNING ("invalid priority value '%s'", value); } else if (!strcmp (key, "hello_time")) { From 901520af85492b947e9a1a392b739e1c69c6e5c4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 23 Nov 2017 15:52:07 +0100 Subject: [PATCH 07/10] libnm: move bridge min/max defines to header file --- libnm-core/nm-core-internal.h | 14 +++++++++++++ libnm-core/nm-setting-bridge.c | 38 +++++++++++----------------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 5144a05077..de39deabd9 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -80,6 +80,20 @@ #include "nm-core-types-internal.h" #include "nm-vpn-editor-plugin.h" +/* IEEE 802.1D-1998 timer values */ +#define NM_BR_MIN_HELLO_TIME 1 +#define NM_BR_MAX_HELLO_TIME 10 + +#define NM_BR_MIN_FORWARD_DELAY 2 +#define NM_BR_MAX_FORWARD_DELAY 30 + +#define NM_BR_MIN_MAX_AGE 6 +#define NM_BR_MAX_MAX_AGE 40 + +/* IEEE 802.1D-1998 Table 7.4 */ +#define NM_BR_MIN_AGEING_TIME 0 +#define NM_BR_MAX_AGEING_TIME 1000000 + /* NM_SETTING_COMPARE_FLAG_INFERRABLE: check whether a device-generated * connection can be replaced by a already-defined connection. This flag only * takes into account properties marked with the %NM_SETTING_PARAM_INFERRABLE diff --git a/libnm-core/nm-setting-bridge.c b/libnm-core/nm-setting-bridge.c index 187d71d3fe..112b499f31 100644 --- a/libnm-core/nm-setting-bridge.c +++ b/libnm-core/nm-setting-bridge.c @@ -213,20 +213,6 @@ nm_setting_bridge_get_multicast_snooping (NMSettingBridge *setting) return NM_SETTING_BRIDGE_GET_PRIVATE (setting)->multicast_snooping; } -/* IEEE 802.1D-1998 timer values */ -#define BR_MIN_HELLO_TIME 1 -#define BR_MAX_HELLO_TIME 10 - -#define BR_MIN_FORWARD_DELAY 2 -#define BR_MAX_FORWARD_DELAY 30 - -#define BR_MIN_MAX_AGE 6 -#define BR_MAX_MAX_AGE 40 - -/* IEEE 802.1D-1998 Table 7.4 */ -#define BR_MIN_AGEING_TIME 0 -#define BR_MAX_AGEING_TIME 1000000 - static inline gboolean check_range (guint32 val, guint32 min, @@ -265,32 +251,32 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) } if (!check_range (priv->forward_delay, - BR_MIN_FORWARD_DELAY, - BR_MAX_FORWARD_DELAY, + NM_BR_MIN_FORWARD_DELAY, + NM_BR_MAX_FORWARD_DELAY, !priv->stp, NM_SETTING_BRIDGE_FORWARD_DELAY, error)) return FALSE; if (!check_range (priv->hello_time, - BR_MIN_HELLO_TIME, - BR_MAX_HELLO_TIME, + NM_BR_MIN_HELLO_TIME, + NM_BR_MAX_HELLO_TIME, !priv->stp, NM_SETTING_BRIDGE_HELLO_TIME, error)) return FALSE; if (!check_range (priv->max_age, - BR_MIN_MAX_AGE, - BR_MAX_MAX_AGE, + NM_BR_MIN_MAX_AGE, + NM_BR_MAX_MAX_AGE, !priv->stp, NM_SETTING_BRIDGE_MAX_AGE, error)) return FALSE; if (!check_range (priv->ageing_time, - BR_MIN_AGEING_TIME, - BR_MAX_AGEING_TIME, + NM_BR_MIN_AGEING_TIME, + NM_BR_MAX_AGEING_TIME, !priv->stp, NM_SETTING_BRIDGE_AGEING_TIME, error)) @@ -524,7 +510,7 @@ nm_setting_bridge_class_init (NMSettingBridgeClass *setting_class) g_object_class_install_property (object_class, PROP_FORWARD_DELAY, g_param_spec_uint (NM_SETTING_BRIDGE_FORWARD_DELAY, "", "", - 0, BR_MAX_FORWARD_DELAY, 15, + 0, NM_BR_MAX_FORWARD_DELAY, 15, G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_INFERRABLE | @@ -546,7 +532,7 @@ nm_setting_bridge_class_init (NMSettingBridgeClass *setting_class) g_object_class_install_property (object_class, PROP_HELLO_TIME, g_param_spec_uint (NM_SETTING_BRIDGE_HELLO_TIME, "", "", - 0, BR_MAX_HELLO_TIME, 2, + 0, NM_BR_MAX_HELLO_TIME, 2, G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_INFERRABLE | @@ -568,7 +554,7 @@ nm_setting_bridge_class_init (NMSettingBridgeClass *setting_class) g_object_class_install_property (object_class, PROP_MAX_AGE, g_param_spec_uint (NM_SETTING_BRIDGE_MAX_AGE, "", "", - 0, BR_MAX_MAX_AGE, 20, + 0, NM_BR_MAX_MAX_AGE, 20, G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_INFERRABLE | @@ -590,7 +576,7 @@ nm_setting_bridge_class_init (NMSettingBridgeClass *setting_class) g_object_class_install_property (object_class, PROP_AGEING_TIME, g_param_spec_uint (NM_SETTING_BRIDGE_AGEING_TIME, "", "", - 0, BR_MAX_AGEING_TIME, 300, + NM_BR_MIN_AGEING_TIME, NM_BR_MAX_AGEING_TIME, 300, G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_INFERRABLE | From 5befde7d7d56ac3d893cc75c0ee1e633a105f7fb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 23 Nov 2017 16:01:24 +0100 Subject: [PATCH 08/10] shared: add nm_g_object_set_property_*() helper --- shared/nm-utils/nm-shared-utils.c | 26 ++++++++++++++++++++++++++ shared/nm-utils/nm-shared-utils.h | 10 ++++++++++ 2 files changed, 36 insertions(+) diff --git a/shared/nm-utils/nm-shared-utils.c b/shared/nm-utils/nm-shared-utils.c index 79d1e6aa9c..2e42dc0cac 100644 --- a/shared/nm-utils/nm-shared-utils.c +++ b/shared/nm-utils/nm-shared-utils.c @@ -845,6 +845,32 @@ nm_g_object_set_property (GObject *object, return TRUE; } +gboolean +nm_g_object_set_property_boolean (GObject *object, + const gchar *property_name, + gboolean value, + GError **error) +{ + nm_auto_unset_gvalue GValue gvalue = { 0 }; + + g_value_init (&gvalue, G_TYPE_BOOLEAN); + g_value_set_boolean (&gvalue, !!value); + return nm_g_object_set_property (object, property_name, &gvalue, error); +} + +gboolean +nm_g_object_set_property_uint (GObject *object, + const gchar *property_name, + guint value, + GError **error) +{ + nm_auto_unset_gvalue GValue gvalue = { 0 }; + + g_value_init (&gvalue, G_TYPE_UINT); + g_value_set_uint (&gvalue, value); + return nm_g_object_set_property (object, property_name, &gvalue, error); +} + GParamSpec * nm_g_object_class_find_property_from_gtype (GType gtype, const char *property_name) diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index 1e80b35a93..d6d829cd57 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -386,6 +386,16 @@ gboolean nm_g_object_set_property (GObject *object, const GValue *value, GError **error); +gboolean nm_g_object_set_property_boolean (GObject *object, + const gchar *property_name, + gboolean value, + GError **error); + +gboolean nm_g_object_set_property_uint (GObject *object, + const gchar *property_name, + guint value, + GError **error); + GParamSpec *nm_g_object_class_find_property_from_gtype (GType gtype, const char *property_name); From ff239c16524d20b8c914c3b72a518e50f8b586f5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 23 Nov 2017 16:30:31 +0100 Subject: [PATCH 09/10] ifcfg-rh: check integer value when reading handle_bridge_option() We cannot just call g_object_set() with an integer that is out of bound. Otherwise, glib will warn. We can use nm_g_object_set_property*() to return an error without asserting. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 100 +++++++++++------- 1 file changed, 59 insertions(+), 41 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index bbc5a9e418..07f0e3146e 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -4661,49 +4661,67 @@ handle_bridge_option (NMSetting *setting, const char *key, const char *value) { + static const struct { + const char *key; + const char *property_name; + gboolean only_with_stp; + } m/*etadata*/[] = { + { "priority", NM_SETTING_BRIDGE_PRIORITY, TRUE }, + { "hello_time", NM_SETTING_BRIDGE_HELLO_TIME, TRUE }, + { "max_age", NM_SETTING_BRIDGE_MAX_AGE, TRUE }, + { "ageing_time", NM_SETTING_BRIDGE_AGEING_TIME, FALSE }, + { "multicast_snooping", NM_SETTING_BRIDGE_MULTICAST_SNOOPING, FALSE }, + { "group_fwd_mask", NM_SETTING_BRIDGE_GROUP_FORWARD_MASK, FALSE }, + }; + const char *error_message = NULL; + int i; gint64 v; - guint32 u = 0; - if (!strcmp (key, "priority")) { - if (stp == FALSE) - PARSE_WARNING ("'priority' invalid when STP is disabled"); - else if ((v = _nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXUINT16, -1)) != -1) - g_object_set (setting, NM_SETTING_BRIDGE_PRIORITY, (guint) v, NULL); - else - PARSE_WARNING ("invalid priority value '%s'", value); - } else if (!strcmp (key, "hello_time")) { - if (stp == FALSE) - PARSE_WARNING ("'hello_time' invalid when STP is disabled"); - else if (get_uint32 (value, &u)) - g_object_set (setting, NM_SETTING_BRIDGE_HELLO_TIME, u, NULL); - else - PARSE_WARNING ("invalid hello_time value '%s'", value); - } else if (!strcmp (key, "max_age")) { - if (stp == FALSE) - PARSE_WARNING ("'max_age' invalid when STP is disabled"); - else if (get_uint32 (value, &u)) - g_object_set (setting, NM_SETTING_BRIDGE_MAX_AGE, u, NULL); - else - PARSE_WARNING ("invalid max_age value '%s'", value); - } else if (!strcmp (key, "ageing_time")) { - if (get_uint32 (value, &u)) - g_object_set (setting, NM_SETTING_BRIDGE_AGEING_TIME, u, NULL); - else - PARSE_WARNING ("invalid ageing_time value '%s'", value); - } else if (!strcmp (key, "multicast_snooping")) { - if (get_uint32 (value, &u)) - g_object_set (setting, NM_SETTING_BRIDGE_MULTICAST_SNOOPING, - (gboolean) u, NULL); - else - PARSE_WARNING ("invalid multicast_snooping value '%s'", value); - } else if (!strcmp (key, "group_fwd_mask")) { - if (get_uint32 (value, &u) && u <= 0xFFFF && !NM_FLAGS_ANY (u, 7)) - g_object_set (setting, NM_SETTING_BRIDGE_GROUP_FORWARD_MASK, - (gboolean) u, NULL); - else - PARSE_WARNING ("invalid group_fwd_mask value '%s'", value); - } else - PARSE_WARNING ("unhandled bridge option '%s'", key); + for (i = 0; i < G_N_ELEMENTS (m); i++) { + GParamSpec *param_spec; + + if (!nm_streq (key, m[i].key)) + continue; + if (m[i].only_with_stp && !stp) { + PARSE_WARNING ("'%s' invalid when STP is disabled", key); + return; + } + + param_spec = g_object_class_find_property (G_OBJECT_GET_CLASS (setting), m[i].property_name); + switch (param_spec->value_type) { + case G_TYPE_BOOLEAN: + v = _nm_utils_ascii_str_to_int64 (value, 10, 0, 1, -1); + if (v == -1) { + error_message = g_strerror (errno); + goto warn; + } + if (!nm_g_object_set_property_boolean (G_OBJECT (setting), m[i].property_name, v, NULL)) { + error_message = "number is out of range"; + goto warn; + } + return; + case G_TYPE_UINT: + v = _nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXUINT, -1); + if (v == -1) { + error_message = g_strerror (errno); + goto warn; + } + if (!nm_g_object_set_property_uint (G_OBJECT (setting), m[i].property_name, v, NULL)) { + error_message = "number is out of range"; + goto warn; + } + return; + default: + nm_assert_not_reached (); + continue; + } + +warn: + PARSE_WARNING ("invalid %s value '%s': %s", key, value, error_message); + return; + } + + PARSE_WARNING ("unhandled bridge option '%s'", key); } static void From b074fd23b4f0833c2abfe95e817f5f7851012902 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 23 Nov 2017 16:30:31 +0100 Subject: [PATCH 10/10] ifcfg-rh: check integer value for other bridge options --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 127 ++++++++---------- 1 file changed, 55 insertions(+), 72 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 07f0e3146e..50d0bc723a 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -77,18 +77,6 @@ /*****************************************************************************/ -static gboolean -get_uint32 (const char *str, guint32 *value) -{ - gint64 tmp; - - tmp = _nm_utils_ascii_str_to_int64 (str, 0, 0, G_MAXUINT32, -1); - if (tmp == -1) - return FALSE; - *value = tmp; - return TRUE; -} - static void check_if_bond_slave (shvarFile *ifcfg, NMSettingConnection *s_con) @@ -4650,28 +4638,43 @@ team_connection_from_ifcfg (const char *file, return connection; } +typedef enum { + BRIDGE_OPT_TYPE_MAIN, + BRIDGE_OPT_TYPE_OPTION, + BRIDGE_OPT_TYPE_PORT_MAIN, + BRIDGE_OPT_TYPE_PORT_OPTION, +} BridgeOptType; + typedef void (*BridgeOptFunc) (NMSetting *setting, gboolean stp, const char *key, - const char *value); + const char *value, + BridgeOptType opt_type); static void handle_bridge_option (NMSetting *setting, gboolean stp, const char *key, - const char *value) + const char *value, + BridgeOptType opt_type) { static const struct { const char *key; const char *property_name; + BridgeOptType opt_type; gboolean only_with_stp; + gboolean extended_bool; } m/*etadata*/[] = { - { "priority", NM_SETTING_BRIDGE_PRIORITY, TRUE }, - { "hello_time", NM_SETTING_BRIDGE_HELLO_TIME, TRUE }, - { "max_age", NM_SETTING_BRIDGE_MAX_AGE, TRUE }, - { "ageing_time", NM_SETTING_BRIDGE_AGEING_TIME, FALSE }, - { "multicast_snooping", NM_SETTING_BRIDGE_MULTICAST_SNOOPING, FALSE }, - { "group_fwd_mask", NM_SETTING_BRIDGE_GROUP_FORWARD_MASK, FALSE }, + { "DELAY", NM_SETTING_BRIDGE_FORWARD_DELAY, BRIDGE_OPT_TYPE_MAIN, .only_with_stp = TRUE }, + { "priority", NM_SETTING_BRIDGE_PRIORITY, BRIDGE_OPT_TYPE_OPTION, .only_with_stp = TRUE }, + { "hello_time", NM_SETTING_BRIDGE_HELLO_TIME, BRIDGE_OPT_TYPE_OPTION, .only_with_stp = TRUE }, + { "max_age", NM_SETTING_BRIDGE_MAX_AGE, BRIDGE_OPT_TYPE_OPTION, .only_with_stp = TRUE }, + { "ageing_time", NM_SETTING_BRIDGE_AGEING_TIME, BRIDGE_OPT_TYPE_OPTION }, + { "multicast_snooping", NM_SETTING_BRIDGE_MULTICAST_SNOOPING, BRIDGE_OPT_TYPE_OPTION }, + { "group_fwd_mask", NM_SETTING_BRIDGE_GROUP_FORWARD_MASK, BRIDGE_OPT_TYPE_OPTION }, + { "priority", NM_SETTING_BRIDGE_PORT_PRIORITY, BRIDGE_OPT_TYPE_PORT_OPTION }, + { "path_cost", NM_SETTING_BRIDGE_PORT_PATH_COST, BRIDGE_OPT_TYPE_PORT_OPTION }, + { "hairpin_mode", NM_SETTING_BRIDGE_PORT_HAIRPIN_MODE, BRIDGE_OPT_TYPE_PORT_OPTION, .extended_bool = TRUE, }, }; const char *error_message = NULL; int i; @@ -4680,6 +4683,8 @@ handle_bridge_option (NMSetting *setting, for (i = 0; i < G_N_ELEMENTS (m); i++) { GParamSpec *param_spec; + if (opt_type != m[i].opt_type) + continue; if (!nm_streq (key, m[i].key)) continue; if (m[i].only_with_stp && !stp) { @@ -4690,10 +4695,21 @@ handle_bridge_option (NMSetting *setting, param_spec = g_object_class_find_property (G_OBJECT_GET_CLASS (setting), m[i].property_name); switch (param_spec->value_type) { case G_TYPE_BOOLEAN: - v = _nm_utils_ascii_str_to_int64 (value, 10, 0, 1, -1); - if (v == -1) { - error_message = g_strerror (errno); - goto warn; + if (m[i].extended_bool) { + if (!strcasecmp (value, "on") || !strcasecmp (value, "yes") || !strcmp (value, "1")) + v = TRUE; + else if (!strcasecmp (value, "off") || !strcasecmp (value, "no")) + v = FALSE; + else { + error_message = "is not a boolean"; + goto warn; + } + } else { + v = _nm_utils_ascii_str_to_int64 (value, 10, 0, 1, -1); + if (v == -1) { + error_message = g_strerror (errno); + goto warn; + } } if (!nm_g_object_set_property_boolean (G_OBJECT (setting), m[i].property_name, v, NULL)) { error_message = "number is out of range"; @@ -4728,7 +4744,8 @@ static void handle_bridging_opts (NMSetting *setting, gboolean stp, const char *value, - BridgeOptFunc func) + BridgeOptFunc func, + BridgeOptType opt_type) { gs_free const char **items = NULL; const char *const *iter; @@ -4743,7 +4760,7 @@ handle_bridging_opts (NMSetting *setting, key = *keys; val = *(keys + 1); if (val && key[0] && val[0]) - func (setting, stp, key, val); + func (setting, stp, key, val, opt_type); } } } @@ -4756,7 +4773,6 @@ make_bridge_setting (shvarFile *ifcfg, gs_unref_object NMSettingBridge *s_bridge = NULL; gs_free char *value_to_free = NULL; const char *value; - guint32 u; gboolean stp = FALSE; gboolean stp_set = FALSE; @@ -4797,19 +4813,13 @@ make_bridge_setting (shvarFile *ifcfg, value = svGetValueStr (ifcfg, "DELAY", &value_to_free); if (value) { - if (stp) { - if (get_uint32 (value, &u)) - g_object_set (s_bridge, NM_SETTING_BRIDGE_FORWARD_DELAY, u, NULL); - else - PARSE_WARNING ("invalid forward delay value '%s'", value); - } else - PARSE_WARNING ("DELAY invalid when STP is disabled"); + handle_bridge_option (NM_SETTING (s_bridge), stp, "DELAY", value, BRIDGE_OPT_TYPE_MAIN); nm_clear_g_free (&value_to_free); } value = svGetValueStr (ifcfg, "BRIDGING_OPTS", &value_to_free); if (value) { - handle_bridging_opts (NM_SETTING (s_bridge), stp, value, handle_bridge_option); + handle_bridging_opts (NM_SETTING (s_bridge), stp, value, handle_bridge_option, BRIDGE_OPT_TYPE_OPTION); nm_clear_g_free (&value_to_free); } @@ -4861,54 +4871,27 @@ 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_uint32 (value, &u)) - g_object_set (setting, NM_SETTING_BRIDGE_PORT_PRIORITY, u, NULL); - else - PARSE_WARNING ("invalid priority value '%s'", value); - } else if (!strcmp (key, "path_cost")) { - if (get_uint32 (value, &u)) - g_object_set (setting, NM_SETTING_BRIDGE_PORT_PATH_COST, u, NULL); - else - PARSE_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 - PARSE_WARNING ("invalid hairpin_mode value '%s'", value); - } else - PARSE_WARNING ("unhandled bridge port option '%s'", key); -} - static NMSetting * make_bridge_port_setting (shvarFile *ifcfg) { NMSetting *s_port = NULL; - char *value; + gs_free char *value_to_free = NULL; + const char *value; g_return_val_if_fail (ifcfg != NULL, FALSE); - value = svGetValueStr_cp (ifcfg, "BRIDGE_UUID"); + value = svGetValueStr (ifcfg, "BRIDGE_UUID", &value_to_free); if (!value) - value = svGetValueStr_cp (ifcfg, "BRIDGE"); + value = svGetValueStr (ifcfg, "BRIDGE", &value_to_free); if (value) { - g_free (value); + nm_clear_g_free (&value_to_free); s_port = nm_setting_bridge_port_new (); - value = svGetValueStr_cp (ifcfg, "BRIDGING_OPTS"); - if (value) - handle_bridging_opts (s_port, FALSE, value, handle_bridge_port_option); - g_free (value); + value = svGetValueStr (ifcfg, "BRIDGING_OPTS", &value_to_free); + if (value) { + handle_bridging_opts (s_port, FALSE, value, handle_bridge_option, BRIDGE_OPT_TYPE_PORT_OPTION); + nm_clear_g_free (&value_to_free); + } } return s_port;