From 2bc6598bce7cafb6cf3d3f852de745416d4f9acb Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Fri, 21 Feb 2020 11:43:41 +0100 Subject: [PATCH] nm-setting-bond: only validate '[up|down]delay' option when miimon is enabled Just looking at the hashtable entry of 'updelay' and 'downdelay' options is wrong, we have to inspect their values to check if they're actually enabled or not. Otherwise bond connections with valid settings will fail when created: $ nmcli c add type bond ifname bond99 bond.options miimon=0,updelay=0,mode=0 Error: Failed to add 'bond-bond99' connection: bond.options: 'updelay' option requires 'miimon' option to be set Also add unit test for bond options when miimon=0 and [up|down]delay=0 https://bugzilla.redhat.com/show_bug.cgi?id=1805184 Fixes: d595f7843e31 ('libnm: add libnm/libnm-core (part 1)') --- libnm-core/nm-setting-bond.c | 124 ++++++++++++++++++++++---------- libnm-core/tests/test-setting.c | 9 +++ 2 files changed, 95 insertions(+), 38 deletions(-) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index 988334f1aa..0c6aa05def 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -179,6 +179,16 @@ NM_UTILS_STRING_TABLE_LOOKUP_STRUCT_DEFINE ( /*****************************************************************************/ +static int +_atoi (const char *value) +{ + int v; + + v = _nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXINT, -1); + nm_assert (v >= 0); + return v; +}; + /** * nm_setting_bond_get_num_options: * @setting: the #NMSettingBond @@ -598,6 +608,26 @@ _nm_setting_bond_option_supported (const char *option, NMBondMode mode) return !NM_FLAGS_ANY (_bond_option_unsupp_mode (option), BIT (mode)); } +static gboolean +bond_option_is_set (NMSettingBond *self, + const char *option) +{ + return g_hash_table_lookup (NM_SETTING_BOND_GET_PRIVATE (self)->options, option) != NULL; +} + +static const char* +bond_get_option_or_default (NMSettingBond *self, + const char *option) +{ + const char *value; + + value = g_hash_table_lookup (NM_SETTING_BOND_GET_PRIVATE (self)->options, option); + if (!value) + return nm_setting_bond_get_option_default (self, option); + + return value; +} + static gboolean verify (NMSetting *setting, NMConnection *connection, GError **error) { @@ -616,12 +646,11 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NMBondMode bond_mode; guint i; const NMUtilsNamedValue *n; - const char *value; - miimon = atoi (nm_setting_bond_get_option_default (self, NM_SETTING_BOND_OPTION_MIIMON)); - arp_interval = atoi (nm_setting_bond_get_option_default (self, NM_SETTING_BOND_OPTION_ARP_INTERVAL)); - num_grat_arp = atoi (nm_setting_bond_get_option_default (self, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP)); - num_unsol_na = atoi (nm_setting_bond_get_option_default (self, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA)); + miimon = _atoi (bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_MIIMON)); + arp_interval = _atoi (bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_ARP_INTERVAL)); + num_grat_arp = _atoi (bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP)); + num_unsol_na = _atoi (bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA)); _ensure_options_idx_cache (priv); @@ -642,30 +671,18 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) } } - value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_MIIMON); - if (value) - miimon = atoi (value); - value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_ARP_INTERVAL); - if (value) - arp_interval = atoi (value); - value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP); - if (value) - num_grat_arp = atoi (value); - value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA); - if (value) - num_unsol_na = atoi (value); - - /* Can only set one of miimon and arp_interval */ - if (miimon > 0 && arp_interval > 0) { - g_set_error (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("only one of '%s' and '%s' can be set"), - NM_SETTING_BOND_OPTION_MIIMON, - NM_SETTING_BOND_OPTION_ARP_INTERVAL); - g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); - return FALSE; - } + /* Option restrictions: + * + * arp_interval conflicts miimon > 0 + * arp_interval conflicts [ alb, tlb ] + * arp_validate does not work with [ BOND_MODE_8023AD, BOND_MODE_TLB, BOND_MODE_ALB ] + * downdelay needs miimon + * updelay needs miimon + * primary needs [ active-backup, tlb, alb ] + * + * clearing miimon requires that arp_interval be 0, but clearing + * arp_interval doesn't require miimon to be 0 + */ /* Verify bond mode */ mode_orig = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_MODE); @@ -684,8 +701,12 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s' is not a valid value for '%s'"), - value, NM_SETTING_BOND_OPTION_MODE); - g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); + mode_orig, + NM_SETTING_BOND_OPTION_MODE); + g_prefix_error (error, + "%s.%s: ", + NM_SETTING_BOND_SETTING_NAME, + NM_SETTING_BOND_OPTIONS); return FALSE; } mode_new = nm_utils_bond_mode_int_to_string (mode); @@ -698,12 +719,34 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s=%s' is incompatible with '%s > 0'"), - NM_SETTING_BOND_OPTION_MODE, mode_new, NM_SETTING_BOND_OPTION_ARP_INTERVAL); - g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); + NM_SETTING_BOND_OPTION_MODE, + mode_new, + NM_SETTING_BOND_OPTION_ARP_INTERVAL); + g_prefix_error (error, + "%s.%s: ", + NM_SETTING_BOND_SETTING_NAME, + NM_SETTING_BOND_OPTIONS); return FALSE; } } + /* if 'miimon' is not explicitely set and 'arp_interval' is 'miimon' is considered disabled */ + if (arp_interval > 0 && !bond_option_is_set (self, NM_SETTING_BOND_OPTION_MIIMON)) { + miimon = 0; + } + + /* Can only set one of miimon and arp_interval */ + if (miimon > 0 && arp_interval > 0) { + g_set_error (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("only one of '%s' and '%s' can be set"), + NM_SETTING_BOND_OPTION_MIIMON, + NM_SETTING_BOND_OPTION_ARP_INTERVAL); + g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); + return FALSE; + } + primary = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_PRIMARY); if (NM_IN_STRSET (mode_new, "active-backup")) { GError *tmp_error = NULL; @@ -745,21 +788,26 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) } if (miimon == 0) { - /* updelay and downdelay can only be used with miimon */ - if (g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_UPDELAY)) { + gpointer delayopt; + + /* updelay and downdelay need miimon to be enabled to be valid */ + delayopt = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_UPDELAY); + if (delayopt && _atoi (delayopt) > 0) { g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' option requires '%s' option to be set"), + _("'%s' option requires '%s' option to be enabled"), NM_SETTING_BOND_OPTION_UPDELAY, NM_SETTING_BOND_OPTION_MIIMON); g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); return FALSE; } - if (g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_DOWNDELAY)) { + + delayopt = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_DOWNDELAY); + if (delayopt && _atoi (delayopt) > 0) { g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' option requires '%s' option to be set"), + _("'%s' option requires '%s' option to be enabled"), NM_SETTING_BOND_OPTION_DOWNDELAY, NM_SETTING_BOND_OPTION_MIIMON); g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); return FALSE; diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index c2cf5edd03..9809ef39dd 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -586,6 +586,15 @@ test_bond_verify (void) test_verify_options (TRUE, "mode", "802.3ad", "ad_actor_system", "ae:00:11:33:44:55"); + test_verify_options (TRUE, + "mode", "0", + "miimon", "0", + "updelay", "0", + "downdelay", "0"); + test_verify_options (TRUE, + "mode", "0", + "downdelay", "0", + "updelay", "0"); } static void -- GitLab