From ec0edc76b557cb901364382ffc6c50fd2453a4d9 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 20 Dec 2018 18:10:49 +0100 Subject: [PATCH 1/5] 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. (cherry picked from commit cad905fce263518168f521cd1ab3c84828c3d5c1) --- 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)); /** From 5a49f7ee775778a1588ef770b99fdaceb0fb446b Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 3 Jan 2019 14:19:21 +0100 Subject: [PATCH 2/5] 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. (cherry picked from commit 0f6fe2a38ae23a814a424ea653a0ed78ccd01c6f) --- 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 7dc57e502d..9d2d0f2cf7 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)); From 6a0f828405684aceae2ade6b73b44567e27e2c57 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 3 Jan 2019 11:40:13 +0100 Subject: [PATCH 3/5] 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. (cherry picked from commit 30d97445347c264a2d3ec0c56ee461e558c7a7d6) --- 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 e79de95cd4..3dfd8ae4a2 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); From 8d0b71fdbcc86221cf8c5a9a0b2bcf80b5b66995 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 3 Jan 2019 14:53:02 +0100 Subject: [PATCH 4/5] device: use bool instead of gboolean in the bridge options struct just to save some bytes of memory (gboolean --> typef gint) (cherry picked from commit ede6b65abf6cf161d64c8826df0c5b41afd13a42) --- 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 3dfd8ae4a2..650500b342 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[] = { From ec0e52cdc804694f7b06c3c69207577f9df7a588 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 3 Jan 2019 12:02:41 +0100 Subject: [PATCH 5/5] 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. (cherry picked from commit abc40618f1bab6a347d0ec6abf39a4fb8ed3d563) --- 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 650500b342..68e1ac282e 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) {