From cad905fce263518168f521cd1ab3c84828c3d5c1 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 20 Dec 2018 18:10:49 +0100 Subject: core: remove NM_SETTING_PARAM_INFERRABLE flag from bridge-port.path-cost In NetworkManager we have a default port path-cost equal to 100. In the linux kernel the default port cost depends upon the interface speed: 2 for 10Gb, 4 for 1Gb, 19 for 100Mb and 100 for 10Mb (or when the interface speed is not available, like current virtio_net driver). Allow NetworkManager to assume bridge port connections also when the path-cost differs: this will allow us to assume bridge ports created outside NetworkManager (e.g. in initrd) that will likely have a different "cost" value. --- libnm-core/nm-setting-bridge-port.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libnm-core/nm-setting-bridge-port.c b/libnm-core/nm-setting-bridge-port.c index 4104f8c5cc..86ac9172a4 100644 --- a/libnm-core/nm-setting-bridge-port.c +++ b/libnm-core/nm-setting-bridge-port.c @@ -264,7 +264,6 @@ nm_setting_bridge_port_class_init (NMSettingBridgePortClass *klass) 0, BR_MAX_PATH_COST, 100, G_PARAM_READWRITE | G_PARAM_CONSTRUCT | - NM_SETTING_PARAM_INFERRABLE | G_PARAM_STATIC_STRINGS)); /** -- cgit v1.2.1 From 0f6fe2a38ae23a814a424ea653a0ed78ccd01c6f Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 3 Jan 2019 14:19:21 +0100 Subject: core: move bridge port min/max/default values to core-internal We have bridge min/max/default values in core-internal. Do the same for bridge port ones. We will soon use those values to enforce limits when assuming a bridge port configuration. --- libnm-core/nm-core-internal.h | 5 +++++ libnm-core/nm-setting-bridge-port.c | 10 ++-------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 1e7ec9bcbc..ae43d27545 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -101,6 +101,11 @@ #define NM_BR_MIN_AGEING_TIME 0 #define NM_BR_MAX_AGEING_TIME 1000000 +#define NM_BR_PORT_MAX_PRIORITY 63 +#define NM_BR_PORT_DEF_PRIORITY 32 + +#define NM_BR_PORT_MAX_PATH_COST 65535 + /* 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-port.c b/libnm-core/nm-setting-bridge-port.c index 86ac9172a4..bc9c3e6fec 100644 --- a/libnm-core/nm-setting-bridge-port.c +++ b/libnm-core/nm-setting-bridge-port.c @@ -104,12 +104,6 @@ nm_setting_bridge_port_get_hairpin_mode (NMSettingBridgePort *setting) /*****************************************************************************/ -#define BR_MAX_PORT_PRIORITY 63 -#define BR_DEF_PRIORITY 32 - -#define BR_MIN_PATH_COST 1 -#define BR_MAX_PATH_COST 65535 - static gboolean verify (NMSetting *setting, NMConnection *connection, GError **error) { @@ -238,7 +232,7 @@ nm_setting_bridge_port_class_init (NMSettingBridgePortClass *klass) g_object_class_install_property (object_class, PROP_PRIORITY, g_param_spec_uint (NM_SETTING_BRIDGE_PORT_PRIORITY, "", "", - 0, BR_MAX_PORT_PRIORITY, BR_DEF_PRIORITY, + 0, NM_BR_PORT_MAX_PRIORITY, NM_BR_PORT_DEF_PRIORITY, G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_INFERRABLE | @@ -261,7 +255,7 @@ nm_setting_bridge_port_class_init (NMSettingBridgePortClass *klass) g_object_class_install_property (object_class, PROP_PATH_COST, g_param_spec_uint (NM_SETTING_BRIDGE_PORT_PATH_COST, "", "", - 0, BR_MAX_PATH_COST, 100, + 0, NM_BR_PORT_MAX_PATH_COST, 100, G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS)); -- cgit v1.2.1 From 30d97445347c264a2d3ec0c56ee461e558c7a7d6 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 3 Jan 2019 11:40:13 +0100 Subject: device: always enforce bridge properties limits ...also when the connection is created at NetworkManager startup to map an already configured bridge. Ensure the device has configuration values that fall inside NetworkManager boundaries, otherwise map the value with a default. --- src/devices/nm-device-bridge.c | 81 +++++++++++++++++++++++++++++++----------- 1 file changed, 60 insertions(+), 21 deletions(-) diff --git a/src/devices/nm-device-bridge.c b/src/devices/nm-device-bridge.c index be53f30dc3..efbf58c583 100644 --- a/src/devices/nm-device-bridge.c +++ b/src/devices/nm-device-bridge.c @@ -168,26 +168,51 @@ complete_connection (NMDevice *device, typedef struct { const char *name; const char *sysname; + uint nm_min; + uint nm_max; + uint nm_default; gboolean default_if_zero; gboolean user_hz_compensate; } Option; static const Option master_options[] = { - { NM_SETTING_BRIDGE_STP, "stp_state", FALSE, FALSE }, - { NM_SETTING_BRIDGE_PRIORITY, "priority", TRUE, FALSE }, - { NM_SETTING_BRIDGE_FORWARD_DELAY, "forward_delay", TRUE, TRUE }, - { NM_SETTING_BRIDGE_HELLO_TIME, "hello_time", TRUE, TRUE }, - { NM_SETTING_BRIDGE_MAX_AGE, "max_age", TRUE, TRUE }, - { NM_SETTING_BRIDGE_AGEING_TIME, "ageing_time", TRUE, TRUE }, - { NM_SETTING_BRIDGE_GROUP_FORWARD_MASK, "group_fwd_mask", TRUE, FALSE }, - { NM_SETTING_BRIDGE_MULTICAST_SNOOPING, "multicast_snooping", FALSE, FALSE }, + { NM_SETTING_BRIDGE_STP, "stp_state", + 0, 1, 1, + FALSE, FALSE }, + { NM_SETTING_BRIDGE_PRIORITY, "priority", + 0, G_MAXUINT16, 0x8000, + TRUE, FALSE }, + { NM_SETTING_BRIDGE_FORWARD_DELAY, "forward_delay", + 0, NM_BR_MAX_FORWARD_DELAY, 15, + TRUE, TRUE }, + { NM_SETTING_BRIDGE_HELLO_TIME, "hello_time", + 0, NM_BR_MAX_HELLO_TIME, 2, + TRUE, TRUE }, + { NM_SETTING_BRIDGE_MAX_AGE, "max_age", + 0, NM_BR_MAX_MAX_AGE, 20, + TRUE, TRUE }, + { NM_SETTING_BRIDGE_AGEING_TIME, "ageing_time", + NM_BR_MIN_AGEING_TIME, NM_BR_MAX_AGEING_TIME, 300, + TRUE, TRUE }, + { NM_SETTING_BRIDGE_GROUP_FORWARD_MASK, "group_fwd_mask", + 0, 0xFFFF, 0, + TRUE, FALSE }, + { NM_SETTING_BRIDGE_MULTICAST_SNOOPING, "multicast_snooping", + 0, 1, 1, + FALSE, FALSE }, { NULL, NULL } }; static const Option slave_options[] = { - { NM_SETTING_BRIDGE_PORT_PRIORITY, "priority", TRUE, FALSE }, - { NM_SETTING_BRIDGE_PORT_PATH_COST, "path_cost", TRUE, FALSE }, - { NM_SETTING_BRIDGE_PORT_HAIRPIN_MODE, "hairpin_mode", FALSE, FALSE }, + { NM_SETTING_BRIDGE_PORT_PRIORITY, "priority", + 0, NM_BR_PORT_MAX_PRIORITY, NM_BR_PORT_DEF_PRIORITY, + TRUE, FALSE }, + { NM_SETTING_BRIDGE_PORT_PATH_COST, "path_cost", + 0, NM_BR_PORT_MAX_PATH_COST, 100, + TRUE, FALSE }, + { NM_SETTING_BRIDGE_PORT_HAIRPIN_MODE, "hairpin_mode", + 0, 1, 0, + FALSE, FALSE }, { NULL, NULL } }; @@ -283,15 +308,22 @@ update_connection (NMDevice *device, NMConnection *connection) for (option = master_options; option->name; option++) { gs_free char *str = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), ifindex, option->sysname); - int value; + uint value; if (str) { - value = strtol (str, NULL, 10); - /* See comments in set_sysfs_uint() about centiseconds. */ - if (option->user_hz_compensate) + if (option->user_hz_compensate) { + value = _nm_utils_ascii_str_to_int64 (str, 10, + option->nm_min * 100, + option->nm_max * 100, + option->nm_default * 100); value /= 100; - + } else { + value = _nm_utils_ascii_str_to_int64 (str, 10, + option->nm_min, + option->nm_max, + option->nm_default); + } g_object_set (s_bridge, option->name, value, NULL); } else _LOGW (LOGD_BRIDGE, "failed to read bridge setting '%s'", option->sysname); @@ -322,15 +354,22 @@ master_update_slave_connection (NMDevice *device, for (option = slave_options; option->name; option++) { gs_free char *str = nm_platform_sysctl_slave_get_option (nm_device_get_platform (device), ifindex_slave, option->sysname); - int value; + uint value; if (str) { - value = strtol (str, NULL, 10); - /* See comments in set_sysfs_uint() about centiseconds. */ - if (option->user_hz_compensate) + if (option->user_hz_compensate) { + value = _nm_utils_ascii_str_to_int64 (str, 10, + option->nm_min * 100, + option->nm_max * 100, + option->nm_default * 100); value /= 100; - + } else { + value = _nm_utils_ascii_str_to_int64 (str, 10, + option->nm_min, + option->nm_max, + option->nm_default); + } g_object_set (s_port, option->name, value, NULL); } else _LOGW (LOGD_BRIDGE, "failed to read bridge port setting '%s'", option->sysname); -- cgit v1.2.1 From ede6b65abf6cf161d64c8826df0c5b41afd13a42 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 3 Jan 2019 14:53:02 +0100 Subject: device: use bool instead of gboolean in the bridge options struct just to save some bytes of memory (gboolean --> typef gint) --- src/devices/nm-device-bridge.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-device-bridge.c b/src/devices/nm-device-bridge.c index efbf58c583..02c5df0c51 100644 --- a/src/devices/nm-device-bridge.c +++ b/src/devices/nm-device-bridge.c @@ -171,8 +171,8 @@ typedef struct { uint nm_min; uint nm_max; uint nm_default; - gboolean default_if_zero; - gboolean user_hz_compensate; + bool default_if_zero; + bool user_hz_compensate; } Option; static const Option master_options[] = { -- cgit v1.2.1 From abc40618f1bab6a347d0ec6abf39a4fb8ed3d563 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 3 Jan 2019 12:02:41 +0100 Subject: device: when assuming a bridge ignore stp options if stp is disabled When STP is disabled, the bridge parameters 'priority', 'forward-delay', 'hello-time' and 'max-age' are irrelevant. We already skip them when loading a connection profile from a ifcfg file. Do the same when generating a connection from a configured device, in order to possibly assume the connection. --- src/devices/nm-device-bridge.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/devices/nm-device-bridge.c b/src/devices/nm-device-bridge.c index 02c5df0c51..4c8921c071 100644 --- a/src/devices/nm-device-bridge.c +++ b/src/devices/nm-device-bridge.c @@ -173,33 +173,34 @@ typedef struct { uint nm_default; bool default_if_zero; bool user_hz_compensate; + bool only_with_stp; } Option; static const Option master_options[] = { - { NM_SETTING_BRIDGE_STP, "stp_state", + { NM_SETTING_BRIDGE_STP, "stp_state", /* this must stay as the first item */ 0, 1, 1, - FALSE, FALSE }, + FALSE, FALSE, FALSE }, { NM_SETTING_BRIDGE_PRIORITY, "priority", 0, G_MAXUINT16, 0x8000, - TRUE, FALSE }, + TRUE, FALSE, TRUE }, { NM_SETTING_BRIDGE_FORWARD_DELAY, "forward_delay", 0, NM_BR_MAX_FORWARD_DELAY, 15, - TRUE, TRUE }, + TRUE, TRUE, TRUE}, { NM_SETTING_BRIDGE_HELLO_TIME, "hello_time", 0, NM_BR_MAX_HELLO_TIME, 2, - TRUE, TRUE }, + TRUE, TRUE, TRUE }, { NM_SETTING_BRIDGE_MAX_AGE, "max_age", 0, NM_BR_MAX_MAX_AGE, 20, - TRUE, TRUE }, + TRUE, TRUE, TRUE }, { NM_SETTING_BRIDGE_AGEING_TIME, "ageing_time", NM_BR_MIN_AGEING_TIME, NM_BR_MAX_AGEING_TIME, 300, - TRUE, TRUE }, + TRUE, TRUE, FALSE }, { NM_SETTING_BRIDGE_GROUP_FORWARD_MASK, "group_fwd_mask", 0, 0xFFFF, 0, - TRUE, FALSE }, + TRUE, FALSE, FALSE }, { NM_SETTING_BRIDGE_MULTICAST_SNOOPING, "multicast_snooping", 0, 1, 1, - FALSE, FALSE }, + FALSE, FALSE, FALSE }, { NULL, NULL } }; @@ -300,16 +301,29 @@ update_connection (NMDevice *device, NMConnection *connection) NMSettingBridge *s_bridge = nm_connection_get_setting_bridge (connection); int ifindex = nm_device_get_ifindex (device); const Option *option; + gs_free char *stp = NULL; + int stp_value; if (!s_bridge) { s_bridge = (NMSettingBridge *) nm_setting_bridge_new (); nm_connection_add_setting (connection, (NMSetting *) s_bridge); } - for (option = master_options; option->name; option++) { + option = master_options; + nm_assert (nm_streq (option->sysname, "stp_state")); + + stp = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), ifindex, option->sysname); + stp_value = _nm_utils_ascii_str_to_int64 (stp, 10, option->nm_min, option->nm_max, option->nm_default); + g_object_set (s_bridge, option->name, stp_value, NULL); + option++; + + for (; option->name; option++) { gs_free char *str = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), ifindex, option->sysname); uint value; + if (!stp_value && option->only_with_stp) + continue; + if (str) { /* See comments in set_sysfs_uint() about centiseconds. */ if (option->user_hz_compensate) { -- cgit v1.2.1