From f9721385ce8d296b9ee26b02d25f69e1e374074f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 10 Jun 2020 17:30:35 +0200 Subject: [PATCH] libnm: don't require birdge multicast_snooping with multicast_router auto,enabled This does not match kernel behavior. You seem to be able to configure multicast_snooping=auto,enabled with multicast_snooping off just fine. Possibly this constrained was inspired by `ip link`, which says: mcast_router MULTICAST_ROUTER - set bridge's multicast router if IGMP snooping is enabled. But kernel doesn't enforce this: ip link delete br0 2>/dev/null; \ ip link add br0 type bridge mcast_router 1 mcast_snooping 0; \ grep ^ /sys/devices/virtual/net/br0/bridge/{multicast_router,multicast_snooping} gives: /sys/devices/virtual/net/br0/bridge/multicast_router:1 /sys/devices/virtual/net/br0/bridge/multicast_snooping:0 We probably should not implement additional constrains on top of what kernel does, if the conditions are that obscure. Fixes: e01d3b4c2b89 ('nm-setting-bridge: add 'multicast-router' bridge option') --- clients/common/settings-docs.h.in | 2 +- libnm-core/nm-setting-bridge.c | 40 ++++++++++--------------------- libnm-core/tests/test-setting.c | 4 ++-- 3 files changed, 16 insertions(+), 30 deletions(-) diff --git a/clients/common/settings-docs.h.in b/clients/common/settings-docs.h.in index a937f4ba6d..93472d160d 100644 --- a/clients/common/settings-docs.h.in +++ b/clients/common/settings-docs.h.in @@ -128,7 +128,7 @@ #define DESCRIBE_DOC_NM_SETTING_BRIDGE_MULTICAST_QUERY_INTERVAL N_("Interval (in deciseconds) between queries sent by the bridge after the end of the startup phase.") #define DESCRIBE_DOC_NM_SETTING_BRIDGE_MULTICAST_QUERY_RESPONSE_INTERVAL N_("Set the Max Response Time/Max Response Delay (in deciseconds) for IGMP/MLD queries sent by the bridge.") #define DESCRIBE_DOC_NM_SETTING_BRIDGE_MULTICAST_QUERY_USE_IFADDR N_("If enabled the bridge's own IP address is used as the source address for IGMP queries otherwise the default of 0.0.0.0 is used.") -#define DESCRIBE_DOC_NM_SETTING_BRIDGE_MULTICAST_ROUTER N_("Sets bridge's multicast router. multicast-snooping must be enabled for this option to work. Supported values are: 'auto', 'disabled', 'enabled'. If not specified the default value is 'auto'.") +#define DESCRIBE_DOC_NM_SETTING_BRIDGE_MULTICAST_ROUTER N_("Sets bridge's multicast router. Multicast-snooping must be enabled for this option to work. Supported values are: 'auto', 'disabled', 'enabled'. If not specified the default value is 'auto'.") #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_MULTICAST_STARTUP_QUERY_COUNT N_("Set the number of IGMP queries to send during startup phase.") #define DESCRIBE_DOC_NM_SETTING_BRIDGE_MULTICAST_STARTUP_QUERY_INTERVAL N_("Sets the time (in deciseconds) between queries sent out at startup to determine membership information.") diff --git a/libnm-core/nm-setting-bridge.c b/libnm-core/nm-setting-bridge.c index 18aaff2a5e..2ead27393e 100644 --- a/libnm-core/nm-setting-bridge.c +++ b/libnm-core/nm-setting-bridge.c @@ -1272,31 +1272,17 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) return FALSE; } - if (priv->multicast_router) { - if (!NM_IN_STRSET (priv->multicast_router, - "auto", - "enabled", - "disabled")) { - g_set_error_literal (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("is not a valid option")); - g_prefix_error (error, "%s.%s: ", NM_SETTING_BRIDGE_SETTING_NAME, NM_SETTING_BRIDGE_MULTICAST_ROUTER); - return FALSE; - } - - if ( NM_IN_STRSET (priv->multicast_router, - "auto", - "enabled") - && !priv->multicast_snooping) { - g_set_error (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' option requires '%s' option to be enabled"), - NM_SETTING_BRIDGE_MULTICAST_ROUTER, NM_SETTING_BRIDGE_MULTICAST_SNOOPING); - g_prefix_error (error, "%s.%s: ", NM_SETTING_BRIDGE_SETTING_NAME, NM_SETTING_BRIDGE_MULTICAST_ROUTER); - return FALSE; - } + if (!NM_IN_STRSET (priv->multicast_router, + NULL, + "auto", + "enabled", + "disabled")) { + g_set_error_literal (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("is not a valid option")); + g_prefix_error (error, "%s.%s: ", NM_SETTING_BRIDGE_SETTING_NAME, NM_SETTING_BRIDGE_MULTICAST_ROUTER); + return FALSE; } if (!nm_utils_is_power_of_two (priv->multicast_hash_max)) { @@ -1999,8 +1985,8 @@ nm_setting_bridge_class_init (NMSettingBridgeClass *klass) /** * NMSettingBridge:multicast-router: * - * Sets bridge's multicast router. - * multicast-snooping must be enabled for this option to work. + * Sets bridge's multicast router. Multicast-snooping must be enabled + * for this option to work. * * Supported values are: 'auto', 'disabled', 'enabled'. * If not specified the default value is 'auto'. diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index 3bae728d5e..b8587a7d7f 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -2197,10 +2197,10 @@ test_bridge_verify (void) /* multicast-router */ test_verify_options_bridge (FALSE, "multicast-router", "nonsense"); - test_verify_options_bridge (FALSE, + test_verify_options_bridge (TRUE, "multicast-snooping", "no", "multicast-router", "auto"); - test_verify_options_bridge (FALSE, + test_verify_options_bridge (TRUE, "multicast-snooping", "no", "multicast-router", "enabled"); test_verify_options_bridge (TRUE,