From c07f3b518c42397e452125bcd2d73e3a8459fef6 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Thu, 20 Feb 2020 15:38:01 +0100 Subject: [PATCH] nm-setting-bond: if unset consider bond options with default values for validation Doing 'verify()' with options such as 'miimon' and 'num_grat_arp' set to arbitrary values it's not consistent with what NetworkManager does to bond options when activating the bond through 'apply_bonding_config()' (at a later stage) because the said values do not correspond to what the default values for those options are. This leads to an inconsistency with the 'miimon' parameter for example, where 'verify()' is done while assuming it's 0 if not set but its default value is actually 100. Fixes: 8775c25c3318 ('libnm: verify bond option in defined order') --- libnm-core/nm-core-internal.h | 4 +++ libnm-core/nm-setting-bond.c | 46 ++++++++++++++++++++--------------- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 444338abf..04d61df92 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -497,6 +497,10 @@ typedef enum { NMBondOptionType _nm_setting_bond_get_option_type (NMSettingBond *setting, const char *name); + +const char* +bond_get_option_or_default (NMSettingBond *self, const char *option); + /*****************************************************************************/ /* nm_connection_get_uuid() asserts against NULL, which is the right thing to diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index e72116c99..0d62ff3e6 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -605,15 +605,29 @@ _nm_setting_bond_option_supported (const char *option, NMBondMode mode) return !NM_FLAGS_ANY (_bond_option_unsupp_mode (option), BIT (mode)); } +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) { + NMSettingBond *self = NM_SETTING_BOND (setting); NMSettingBondPrivate *priv = NM_SETTING_BOND_GET_PRIVATE (setting); int mode; - int miimon = 0; - int arp_interval = 0; - int num_grat_arp = -1; - int num_unsol_na = -1; + int miimon; + int arp_interval; + int num_grat_arp; + int num_unsol_na; const char *mode_orig; const char *mode_new; const char *arp_ip_target = NULL; @@ -622,7 +636,6 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NMBondMode bond_mode; guint i; const NMUtilsNamedValue *n; - const char *value; _ensure_options_idx_cache (priv); @@ -643,18 +656,10 @@ 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); + 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)); /* Can only set one of miimon and arp_interval */ if (miimon > 0 && arp_interval > 0) { @@ -685,7 +690,7 @@ 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); + mode_orig, NM_SETTING_BOND_OPTION_MODE); g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); return FALSE; } @@ -840,8 +845,9 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) return FALSE; } - if ( (num_grat_arp != -1 && num_unsol_na != -1) - && (num_grat_arp != num_unsol_na)) { + if ( g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP) + && g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA) + && num_grat_arp != num_unsol_na) { g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY,