Commit 2bc6598b authored by Antonio Cardace's avatar Antonio Cardace

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: d595f784 ('libnm: add libnm/libnm-core (part 1)')
parent 4dcfa48c
Pipeline #111385 failed with stages
in 0 seconds
...@@ -179,6 +179,16 @@ NM_UTILS_STRING_TABLE_LOOKUP_STRUCT_DEFINE ( ...@@ -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: * nm_setting_bond_get_num_options:
* @setting: the #NMSettingBond * @setting: the #NMSettingBond
...@@ -598,6 +608,26 @@ _nm_setting_bond_option_supported (const char *option, NMBondMode mode) ...@@ -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)); 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 static gboolean
verify (NMSetting *setting, NMConnection *connection, GError **error) verify (NMSetting *setting, NMConnection *connection, GError **error)
{ {
...@@ -616,12 +646,11 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) ...@@ -616,12 +646,11 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
NMBondMode bond_mode; NMBondMode bond_mode;
guint i; guint i;
const NMUtilsNamedValue *n; const NMUtilsNamedValue *n;
const char *value;
miimon = atoi (nm_setting_bond_get_option_default (self, NM_SETTING_BOND_OPTION_MIIMON)); miimon = _atoi (bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_MIIMON));
arp_interval = atoi (nm_setting_bond_get_option_default (self, NM_SETTING_BOND_OPTION_ARP_INTERVAL)); arp_interval = _atoi (bond_get_option_or_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_grat_arp = _atoi (bond_get_option_or_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)); num_unsol_na = _atoi (bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA));
_ensure_options_idx_cache (priv); _ensure_options_idx_cache (priv);
...@@ -642,30 +671,18 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) ...@@ -642,30 +671,18 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
} }
} }
value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_MIIMON); /* Option restrictions:
if (value) *
miimon = atoi (value); * arp_interval conflicts miimon > 0
value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_ARP_INTERVAL); * arp_interval conflicts [ alb, tlb ]
if (value) * arp_validate does not work with [ BOND_MODE_8023AD, BOND_MODE_TLB, BOND_MODE_ALB ]
arp_interval = atoi (value); * downdelay needs miimon
value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP); * updelay needs miimon
if (value) * primary needs [ active-backup, tlb, alb ]
num_grat_arp = atoi (value); *
value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA); * clearing miimon requires that arp_interval be 0, but clearing
if (value) * arp_interval doesn't require miimon to be 0
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;
}
/* Verify bond mode */ /* Verify bond mode */
mode_orig = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_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) ...@@ -684,8 +701,12 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
NM_CONNECTION_ERROR, NM_CONNECTION_ERROR,
NM_CONNECTION_ERROR_INVALID_PROPERTY, NM_CONNECTION_ERROR_INVALID_PROPERTY,
_("'%s' is not a valid value for '%s'"), _("'%s' is not a valid value for '%s'"),
value, NM_SETTING_BOND_OPTION_MODE); mode_orig,
g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); NM_SETTING_BOND_OPTION_MODE);
g_prefix_error (error,
"%s.%s: ",
NM_SETTING_BOND_SETTING_NAME,
NM_SETTING_BOND_OPTIONS);
return FALSE; return FALSE;
} }
mode_new = nm_utils_bond_mode_int_to_string (mode); mode_new = nm_utils_bond_mode_int_to_string (mode);
...@@ -698,12 +719,34 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) ...@@ -698,12 +719,34 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
NM_CONNECTION_ERROR, NM_CONNECTION_ERROR,
NM_CONNECTION_ERROR_INVALID_PROPERTY, NM_CONNECTION_ERROR_INVALID_PROPERTY,
_("'%s=%s' is incompatible with '%s > 0'"), _("'%s=%s' is incompatible with '%s > 0'"),
NM_SETTING_BOND_OPTION_MODE, mode_new, NM_SETTING_BOND_OPTION_ARP_INTERVAL); NM_SETTING_BOND_OPTION_MODE,
g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); 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; 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); primary = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_PRIMARY);
if (NM_IN_STRSET (mode_new, "active-backup")) { if (NM_IN_STRSET (mode_new, "active-backup")) {
GError *tmp_error = NULL; GError *tmp_error = NULL;
...@@ -745,21 +788,26 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) ...@@ -745,21 +788,26 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
} }
if (miimon == 0) { if (miimon == 0) {
/* updelay and downdelay can only be used with miimon */ gpointer delayopt;
if (g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_UPDELAY)) {
/* 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, g_set_error (error,
NM_CONNECTION_ERROR, NM_CONNECTION_ERROR,
NM_CONNECTION_ERROR_INVALID_PROPERTY, 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); 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); g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS);
return FALSE; 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, g_set_error (error,
NM_CONNECTION_ERROR, NM_CONNECTION_ERROR,
NM_CONNECTION_ERROR_INVALID_PROPERTY, 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); 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); g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS);
return FALSE; return FALSE;
......
...@@ -586,6 +586,15 @@ test_bond_verify (void) ...@@ -586,6 +586,15 @@ test_bond_verify (void)
test_verify_options (TRUE, test_verify_options (TRUE,
"mode", "802.3ad", "mode", "802.3ad",
"ad_actor_system", "ae:00:11:33:44:55"); "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 static void
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment