From c07f3b518c42397e452125bcd2d73e3a8459fef6 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Thu, 20 Feb 2020 15:38:01 +0100 Subject: [PATCH 1/5] 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, From 57bdf68088a8b4f5f5799ebcec97dfbb3d45fbab Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Fri, 21 Feb 2020 11:43:41 +0100 Subject: [PATCH 2/5] nm-setting-bond: let 'miimon' and 'arp_interval' coexist for verify() Fix 'miimon' and 'arp_interval' validation, they can both be set indeed, the kernel does not impose this limitation, nevertheless is sensible to keep the defaults as previously (miimon=100, arp_interval=0). Also add unit test. --- libnm-core/nm-setting-bond.c | 11 ----------- libnm-core/tests/test-setting.c | 10 ++++++++++ 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index 0d62ff3e6..b3c196a95 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -661,17 +661,6 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) 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) { - 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 */ mode_orig = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_MODE); diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index 9809ef39d..71f484bab 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -595,6 +595,16 @@ test_bond_verify (void) "mode", "0", "downdelay", "0", "updelay", "0"); + test_verify_options (TRUE, + "mode", "0", + "miimon", "100", + "arp_ip_target", "1.1.1.1", + "arp_interval", "200"); + test_verify_options (TRUE, + "mode", "0", + "downdelay", "100", + "arp_ip_target", "1.1.1.1", + "arp_interval", "200"); } static void From 9bd07336ef16f014b35f66a7d0d03bc49703b039 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Fri, 21 Feb 2020 11:41:07 +0100 Subject: [PATCH 3/5] bond: bond options logic rework Add '_nm_setting_bond_get_option_or_default()' and move all the custom policies applied by NM for bond options in there. One such example of a custom policy is to set 'miimon' to 0 (instead of its default value of 100) if 'arp_interval' is explicitly enabled and 'miimon' is not. This means removing every piece of logic from nm_setting_bond_add_option() which used to clear out 'arp_interval' and 'arp_ip_target' if 'miimon' was set or clear out 'miimon' along with 'downdelay', 'updelay' and 'miimon' if 'arp_interval' was set. This behaviour is a bug since the kernel allow setting any combination of this options for bonds and NetworkManager should not limit the user to do so. Also use 'set_bond_attr_or_default()' instead of 'set_bond_attr()' as the former calls '_nm_setting_bond_get_option_or_default()' to implement the right logic to retrieve bond options according to current bond configuration. --- libnm-core/nm-core-internal.h | 7 +- libnm-core/nm-setting-bond.c | 363 ++++++++++++++++++++++------------ src/devices/nm-device-bond.c | 182 ++++++----------- 3 files changed, 308 insertions(+), 244 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 04d61df92..babf7dfd4 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -494,12 +494,9 @@ typedef enum { NM_BOND_OPTION_TYPE_IFNAME, } NMBondOptionType; -NMBondOptionType -_nm_setting_bond_get_option_type (NMSettingBond *setting, const char *name); +NMBondOptionType _nm_setting_bond_get_option_type (NMSettingBond *setting, const char *name); - -const char* -bond_get_option_or_default (NMSettingBond *self, const char *option); +const char* nm_setting_bond_get_option_or_default (NMSettingBond *self, const char *option); /*****************************************************************************/ diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index b3c196a95..0d34086ba 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -179,6 +179,152 @@ NM_UTILS_STRING_TABLE_LOOKUP_STRUCT_DEFINE ( /*****************************************************************************/ +#define BIT(x) (((guint32) 1) << (x)) + +static +NM_UTILS_STRING_TABLE_LOOKUP_DEFINE ( + _bond_option_unsupp_mode, + guint32, + { ; }, + { return 0; }, + { NM_SETTING_BOND_OPTION_ACTIVE_SLAVE, ~(BIT (NM_BOND_MODE_ACTIVEBACKUP) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, + { NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO, ~(BIT (NM_BOND_MODE_8023AD)) }, + { NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM, ~(BIT (NM_BOND_MODE_8023AD)) }, + { NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY, ~(BIT (NM_BOND_MODE_8023AD)) }, + { NM_SETTING_BOND_OPTION_ARP_INTERVAL, (BIT (NM_BOND_MODE_8023AD) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, + { NM_SETTING_BOND_OPTION_ARP_IP_TARGET, (BIT (NM_BOND_MODE_8023AD) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, + { NM_SETTING_BOND_OPTION_ARP_VALIDATE, (BIT (NM_BOND_MODE_8023AD) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, + { NM_SETTING_BOND_OPTION_LACP_RATE, ~(BIT (NM_BOND_MODE_8023AD)) }, + { NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE, ~(BIT (NM_BOND_MODE_ROUNDROBIN)) }, + { NM_SETTING_BOND_OPTION_PRIMARY, ~(BIT (NM_BOND_MODE_ACTIVEBACKUP) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, + { NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB, ~(BIT (NM_BOND_MODE_TLB)) }, +) + +gboolean +_nm_setting_bond_option_supported (const char *option, NMBondMode mode) +{ + nm_assert (option); + nm_assert (_NM_INT_NOT_NEGATIVE (mode) && mode < 32); + + return !NM_FLAGS_ANY (_bond_option_unsupp_mode (option), BIT (mode)); +} + +static const char* +_bond_get_option (NMSettingBond *self, + const char *option) +{ + g_return_val_if_fail (NM_IS_SETTING_BOND (self), NULL); + g_return_val_if_fail (option, NULL); + + return g_hash_table_lookup (NM_SETTING_BOND_GET_PRIVATE (self)->options, option); +} + +static const char* +_bond_get_option_default (NMSettingBond *self, + const char *option) +{ + const OptionMeta *option_meta; + + g_return_val_if_fail (NM_IS_SETTING_BOND (self), NULL); + + option_meta = _get_option_meta (option); + + g_return_val_if_fail (option_meta, NULL); + + return option_meta->val; +} + +static const char* +_bond_get_option_or_default (NMSettingBond *self, + const char *option) +{ + const char *value; + + value = _bond_get_option (self, option); + if (!value) { + value = _bond_get_option_default (self, option); + } + return value; +} + +static const char* +_bond_get_option_normalized (NMSettingBond* self, + const char* option, + gboolean get_default_only) +{ + const char *arp_interval_str; + const char *mode_str; + gint64 arp_interval; + NMBondMode mode; + const char *value = NULL; + + g_return_val_if_fail (NM_IS_SETTING_BOND (self), NULL); + g_return_val_if_fail (option, NULL); + + mode_str = _bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_MODE); + mode = _nm_setting_bond_mode_from_string (mode_str); + g_return_val_if_fail (mode != NM_BOND_MODE_UNKNOWN, NULL); + + if (!_nm_setting_bond_option_supported (option, mode)) + return NULL; + + /* Apply custom NetworkManager policies here */ + if (!get_default_only) { + if (NM_IN_STRSET (option, + NM_SETTING_BOND_OPTION_UPDELAY, + NM_SETTING_BOND_OPTION_DOWNDELAY, + NM_SETTING_BOND_OPTION_MIIMON)) { + /* if arp_interval is explicitly set and miimon is not, then disable miimon + * (and related updelay and downdelay) as recommended by the kernel docs */ + arp_interval_str = _bond_get_option (self, NM_SETTING_BOND_OPTION_ARP_INTERVAL); + arp_interval = _nm_utils_ascii_str_to_int64 (arp_interval_str, 10, 0, G_MAXINT, 0); + + if (!arp_interval || _bond_get_option (self, NM_SETTING_BOND_OPTION_MIIMON)) { + value = _bond_get_option (self, option); + } else { + return NULL; + } + } else if (NM_IN_STRSET (option, + NM_SETTING_BOND_OPTION_NUM_GRAT_ARP, + NM_SETTING_BOND_OPTION_NUM_UNSOL_NA)) { + /* just get one of the 2, at kernel level they're the same bond option */ + value = _bond_get_option (self, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP); + if (!value) { + value = _bond_get_option (self, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA); + } + } else { + value = _bond_get_option (self, option); + } + } + + if (!value) { + /* Apply rules that change the default value of an option */ + if (nm_streq (option, NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM)) { + /* The default value depends on the current mode */ + if (NM_IN_STRSET (mode_str, "4", "802.3ad")) + return "00:00:00:00:00:00"; + else + return ""; + } else { + return _bond_get_option_or_default (self, option); + } + } + + return value; +} + +const char* +nm_setting_bond_get_option_or_default (NMSettingBond *self, + const char *option) +{ + g_return_val_if_fail (NM_IS_SETTING_BOND (self), NULL); + g_return_val_if_fail (option, NULL); + + return _bond_get_option_normalized (self, + option, + FALSE); +} + static int _atoi (const char *value) { @@ -403,7 +549,7 @@ nm_setting_bond_get_option_by_name (NMSettingBond *setting, if (!nm_setting_bond_validate_option (name, NULL)) return NULL; - return g_hash_table_lookup (NM_SETTING_BOND_GET_PRIVATE (setting)->options, name); + return _bond_get_option (setting, name); } /** @@ -440,19 +586,6 @@ nm_setting_bond_add_option (NMSettingBond *setting, nm_clear_g_free (&priv->options_idx_cache); g_hash_table_insert (priv->options, g_strdup (name), g_strdup (value)); - if (nm_streq (name, NM_SETTING_BOND_OPTION_MIIMON)) { - if (!nm_streq (value, "0")) { - g_hash_table_remove (priv->options, NM_SETTING_BOND_OPTION_ARP_INTERVAL); - g_hash_table_remove (priv->options, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); - } - } else if (nm_streq (name, NM_SETTING_BOND_OPTION_ARP_INTERVAL)) { - if (!nm_streq (value, "0")) { - g_hash_table_remove (priv->options, NM_SETTING_BOND_OPTION_MIIMON); - g_hash_table_remove (priv->options, NM_SETTING_BOND_OPTION_DOWNDELAY); - g_hash_table_remove (priv->options, NM_SETTING_BOND_OPTION_UPDELAY); - } - } - _notify (setting, PROP_OPTIONS); return TRUE; @@ -517,25 +650,15 @@ nm_setting_bond_get_valid_options (NMSettingBond *setting) const char * nm_setting_bond_get_option_default (NMSettingBond *setting, const char *name) { - const OptionMeta *option_meta; - const char *mode; - g_return_val_if_fail (NM_IS_SETTING_BOND (setting), NULL); - option_meta = _get_option_meta (name); - - g_return_val_if_fail (option_meta, NULL); - - if (nm_streq (name, NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM)) { - /* The default value depends on the current mode */ - mode = nm_setting_bond_get_option_by_name (setting, NM_SETTING_BOND_OPTION_MODE); - if (NM_IN_STRSET (mode, "4", "802.3ad")) - return "00:00:00:00:00:00"; - else - return ""; + if (!name) { + return NULL; } - return option_meta->val; + return _bond_get_option_normalized (setting, + name, + TRUE); } /** @@ -575,49 +698,6 @@ NM_UTILS_STRING_TABLE_LOOKUP_DEFINE ( /*****************************************************************************/ -#define BIT(x) (((guint32) 1) << (x)) - -static -NM_UTILS_STRING_TABLE_LOOKUP_DEFINE ( - _bond_option_unsupp_mode, - guint32, - { ; }, - { return 0; }, - { NM_SETTING_BOND_OPTION_ACTIVE_SLAVE, ~(BIT (NM_BOND_MODE_ACTIVEBACKUP) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, - { NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO, ~(BIT (NM_BOND_MODE_8023AD)) }, - { NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM, ~(BIT (NM_BOND_MODE_8023AD)) }, - { NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY, ~(BIT (NM_BOND_MODE_8023AD)) }, - { NM_SETTING_BOND_OPTION_ARP_INTERVAL, (BIT (NM_BOND_MODE_8023AD) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, - { NM_SETTING_BOND_OPTION_ARP_IP_TARGET, (BIT (NM_BOND_MODE_8023AD) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, - { NM_SETTING_BOND_OPTION_ARP_VALIDATE, (BIT (NM_BOND_MODE_8023AD) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, - { NM_SETTING_BOND_OPTION_LACP_RATE, ~(BIT (NM_BOND_MODE_8023AD)) }, - { NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE, ~(BIT (NM_BOND_MODE_ROUNDROBIN)) }, - { NM_SETTING_BOND_OPTION_PRIMARY, ~(BIT (NM_BOND_MODE_ACTIVEBACKUP) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, - { NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB, ~(BIT (NM_BOND_MODE_TLB)) }, -) - -gboolean -_nm_setting_bond_option_supported (const char *option, NMBondMode mode) -{ - nm_assert (option); - nm_assert (_NM_INT_NOT_NEGATIVE (mode) && mode < 32); - - 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) { @@ -650,20 +730,32 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR_INVALID_PROPERTY, _("invalid option '%s' or its value '%s'"), n->name, n->value_str); - 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; } } } - 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)); + 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)); + /* Option restrictions: + * + * arp_interval conflicts [ alb, tlb ] + * arp_interval needs arp_ip_target + * 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 ] + */ /* Verify bond mode */ - mode_orig = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_MODE); + mode_orig = _bond_get_option (self, NM_SETTING_BOND_OPTION_MODE); if (!mode_orig) { g_set_error (error, NM_CONNECTION_ERROR, @@ -679,8 +771,12 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s' is not a valid value for '%s'"), - mode_orig, 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); @@ -693,13 +789,18 @@ 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; } } - primary = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_PRIMARY); + primary = _bond_get_option (self, NM_SETTING_BOND_OPTION_PRIMARY); if (NM_IN_STRSET (mode_new, "active-backup")) { GError *tmp_error = NULL; @@ -709,21 +810,22 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s' is not valid for the '%s' option: %s"), primary, NM_SETTING_BOND_OPTION_PRIMARY, tmp_error->message); - 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); g_error_free (tmp_error); return FALSE; } - } else { - if (primary) { - g_set_error (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' option is only valid for '%s=%s'"), - NM_SETTING_BOND_OPTION_PRIMARY, - NM_SETTING_BOND_OPTION_MODE, "active-backup"); - g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); - return FALSE; - } + } else if (primary) { + g_set_error (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' option is only valid for '%s=%s'"), + NM_SETTING_BOND_OPTION_PRIMARY, + NM_SETTING_BOND_OPTION_MODE, "active-backup"); + g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); + return FALSE; } if ( connection @@ -734,34 +836,38 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s=%s' is not a valid configuration for '%s'"), NM_SETTING_BOND_OPTION_MODE, mode_new, NM_SETTING_INFINIBAND_SETTING_NAME); - 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; } } if (miimon == 0) { - 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) { + if (_atoi (_bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_UPDELAY))) { g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%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); + g_prefix_error (error, + "%s.%s: ", + NM_SETTING_BOND_SETTING_NAME, + NM_SETTING_BOND_OPTIONS); return FALSE; } - delayopt = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_DOWNDELAY); - if (delayopt && _atoi (delayopt) > 0) { + if (_atoi (_bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_DOWNDELAY))) { g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%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); + g_prefix_error (error, + "%s.%s: ", + NM_SETTING_BOND_SETTING_NAME, + NM_SETTING_BOND_OPTIONS); return FALSE; } } @@ -769,7 +875,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) /* arp_ip_target can only be used with arp_interval, and must * contain a comma-separated list of IPv4 addresses. */ - arp_ip_target = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); + arp_ip_target = _bond_get_option (self, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); if (arp_interval > 0) { char **addrs; guint32 addr; @@ -779,8 +885,12 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s' option requires '%s' option to be set"), - NM_SETTING_BOND_OPTION_ARP_INTERVAL, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); - g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); + NM_SETTING_BOND_OPTION_ARP_INTERVAL, + NM_SETTING_BOND_OPTION_ARP_IP_TARGET); + g_prefix_error (error, + "%s.%s: ", + NM_SETTING_BOND_SETTING_NAME, + NM_SETTING_BOND_OPTIONS); return FALSE; } @@ -791,7 +901,9 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s' option is empty"), NM_SETTING_BOND_OPTION_ARP_IP_TARGET); - 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); g_strfreev (addrs); return FALSE; } @@ -802,8 +914,12 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s' is not a valid IPv4 address for '%s' option"), - NM_SETTING_BOND_OPTION_ARP_IP_TARGET, addrs[i]); - g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS); + NM_SETTING_BOND_OPTION_ARP_IP_TARGET, + addrs[i]); + g_prefix_error (error, + "%s.%s: ", + NM_SETTING_BOND_SETTING_NAME, + NM_SETTING_BOND_OPTIONS); g_strfreev (addrs); return FALSE; } @@ -815,13 +931,16 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s' option requires '%s' option to be set"), - NM_SETTING_BOND_OPTION_ARP_IP_TARGET, 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_ARP_IP_TARGET, + NM_SETTING_BOND_OPTION_ARP_INTERVAL); + g_prefix_error (error, "%s.%s: ", + NM_SETTING_BOND_SETTING_NAME, + NM_SETTING_BOND_OPTIONS); return FALSE; } } - lacp_rate = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_LACP_RATE); + lacp_rate = _bond_get_option (self, NM_SETTING_BOND_OPTION_LACP_RATE); if ( lacp_rate && !nm_streq0 (mode_new, "802.3ad") && !NM_IN_STRSET (lacp_rate, "0", "slow")) { @@ -834,8 +953,8 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) return FALSE; } - 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) + if ( _bond_get_option (self, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP) + && _bond_get_option (self, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA) && num_grat_arp != num_unsol_na) { g_set_error (error, NM_CONNECTION_ERROR, @@ -872,7 +991,10 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) NM_CONNECTION_ERROR_INVALID_PROPERTY, _("'%s' option is not valid with mode '%s'"), n->name, mode_new); - 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 NM_SETTING_VERIFY_NORMALIZABLE; } } @@ -887,7 +1009,6 @@ options_equal_asym (NMSettingBond *s_bond, NMSettingBond *s_bond2, NMSettingCompareFlags flags) { - GHashTable *options2 = NM_SETTING_BOND_GET_PRIVATE (s_bond2)->options; GHashTableIter iter; const char *key, *value, *value2; @@ -904,17 +1025,17 @@ options_equal_asym (NMSettingBond *s_bond, continue; } - value2 = g_hash_table_lookup (options2, key); + value2 = _bond_get_option (s_bond2, key); if (!value2) { if (nm_streq (key, "num_grat_arp")) - value2 = g_hash_table_lookup (options2, "num_unsol_na"); + value2 = _bond_get_option (s_bond2, "num_unsol_na"); else if (nm_streq (key, "num_unsol_na")) - value2 = g_hash_table_lookup (options2, "num_grat_arp"); + value2 = _bond_get_option (s_bond2, "num_grat_arp"); } if (!value2) - value2 = nm_setting_bond_get_option_default (s_bond2, key); + value2 = _bond_get_option_default (s_bond2, key); if (!nm_streq (value, value2)) return FALSE; } diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index 347d63324..a038a6ffc 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -70,16 +70,16 @@ complete_connection (NMDevice *device, /*****************************************************************************/ static gboolean -set_bond_attr (NMDevice *device, NMBondMode mode, const char *attr, const char *value) +_set_bond_attr (NMDevice *device, const char *attr, const char *value) { NMDeviceBond *self = NM_DEVICE_BOND (device); - gboolean ret; int ifindex = nm_device_get_ifindex (device); + gboolean ret; - if (!_nm_setting_bond_option_supported (attr, mode)) - return FALSE; - - ret = nm_platform_sysctl_master_set_option (nm_device_get_platform (device), ifindex, attr, value); + ret = nm_platform_sysctl_master_set_option (nm_device_get_platform (device), + ifindex, + attr, + value); if (!ret) _LOGW (LOGD_PLATFORM, "failed to set bonding attribute '%s' to '%s'", attr, value); return ret; @@ -119,8 +119,10 @@ update_connection (NMDevice *device, NMConnection *connection) /* Read bond options from sysfs and update the Bond setting to match */ options = nm_setting_bond_get_valid_options (s_bond); for (; *options; options++) { - gs_free char *value = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), ifindex, *options); char *p; + gs_free char *value = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), + ifindex, + *options); if ( value && _nm_setting_bond_get_option_type (s_bond, *options) == NM_BOND_OPTION_TYPE_BOTH) { @@ -181,138 +183,86 @@ set_arp_targets (NMDevice *device, gs_free char *tmp = NULL; tmp = g_strdup_printf ("%s%s", prefix, value_v[i]); - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_ARP_IP_TARGET, tmp); + _set_bond_attr (device, NM_SETTING_BOND_OPTION_ARP_IP_TARGET, tmp); } } +/* + * Sets bond attribute stored in the option hashtable or + * the default value if no value was set. + */ static void -set_simple_option (NMDevice *device, - NMBondMode mode, - NMSettingBond *s_bond, - const char *opt) +set_bond_attr_or_default (NMDevice *device, + NMSettingBond *s_bond, + const char *opt) { - const char *value; + NMDeviceBond *self = NM_DEVICE_BOND (device); + const char *value = nm_setting_bond_get_option_or_default (s_bond, opt); - value = nm_setting_bond_get_option_by_name (s_bond, opt); - if (!value) - value = nm_setting_bond_get_option_default (s_bond, opt); - set_bond_attr (device, mode, opt, value); + if (value) { + _set_bond_attr (device, opt, value); + } else { + _LOGD (LOGD_BOND, "bond option %s rejected due to incompatibility", opt); + } } static gboolean apply_bonding_config (NMDeviceBond *self) { NMDevice *device = NM_DEVICE (self); - NMSettingBond *s_bond; int ifindex = nm_device_get_ifindex (device); - const char *mode_str, *value; - char *contents; - gboolean set_arp_interval = TRUE; + NMSettingBond *s_bond; NMBondMode mode; - - /* 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 - */ + const char *mode_str; + const char *value; + char *contents; s_bond = nm_device_get_applied_setting (device, NM_TYPE_SETTING_BOND); - g_return_val_if_fail (s_bond, FALSE); - mode_str = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MODE); - if (!mode_str) - mode_str = "balance-rr"; - + mode_str = nm_setting_bond_get_option_or_default (s_bond, NM_SETTING_BOND_OPTION_MODE); mode = _nm_setting_bond_mode_from_string (mode_str); - if (mode == NM_BOND_MODE_UNKNOWN) { - _LOGW (LOGD_BOND, "unknown bond mode '%s'", mode_str); - return FALSE; - } + g_return_val_if_fail (mode != NM_BOND_MODE_UNKNOWN, FALSE); /* Set mode first, as some other options (e.g. arp_interval) are valid * only for certain modes. */ + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_MODE); - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_MODE, mode_str); - - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MIIMON); - if (value && atoi (value)) { - /* clear arp interval */ - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_ARP_INTERVAL, "0"); - set_arp_interval = FALSE; - - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_MIIMON, value); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_UPDELAY); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_DOWNDELAY); - } else if (!value) { - /* If not given, and arp_interval is not given or disabled, default to 100 */ - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL); - if (_nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXUINT32, 0) == 0) - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_MIIMON, "100"); - } - - if (set_arp_interval) { - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL); - /* Just let miimon get cleared automatically; even setting miimon to - * 0 (disabled) clears arp_interval. - */ - } - - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_ARP_VALIDATE); - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_ARP_VALIDATE, value ?: "0"); - - /* Primary */ - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_PRIMARY); - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_PRIMARY, value ?: ""); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_MIIMON); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_UPDELAY); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_DOWNDELAY); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ARP_VALIDATE); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_PRIMARY); /* ARP targets: clear and initialize the list */ - contents = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), ifindex, + contents = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), + ifindex, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); set_arp_targets (device, mode, contents, " \n", "-"); - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); + value = nm_setting_bond_get_option_or_default (s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); set_arp_targets (device, mode, value, ",", "+"); g_free (contents); - /* AD actor system: don't set if empty */ - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM); - if (value) - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM, value); - - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_AD_SELECT); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_FAIL_OVER_MAC); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_LACP_RATE); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_LP_INTERVAL); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_MIN_LINKS); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_PRIMARY_RESELECT); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_RESEND_IGMP); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_USE_CARRIER); - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY); - - /* num_grat_arp and num_unsol_na are actually the same attribute - * on kernel side and their value in the bond setting is guaranteed - * to be equal. Write only one of the two. - */ - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP); - if (value) - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP, value); - else - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA); - + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_AD_SELECT); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_FAIL_OVER_MAC); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_LACP_RATE); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_LP_INTERVAL); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_MIN_LINKS); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_PRIMARY_RESELECT); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_RESEND_IGMP); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_USE_CARRIER); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP); return TRUE; } @@ -369,8 +319,9 @@ enslave_slave (NMDevice *device, const char *active; if (s_bond) { - active = nm_setting_bond_get_option_by_name (s_bond, "active_slave"); - if (active && nm_streq0 (active, nm_device_get_iface (slave))) { + active = nm_setting_bond_get_option_or_default (s_bond, + NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); + if (nm_streq0 (active, nm_device_get_iface (slave))) { nm_platform_sysctl_master_set_option (nm_device_get_platform (device), nm_device_get_ifindex (device), "active_slave", @@ -568,19 +519,14 @@ reapply_connection (NMDevice *device, NMConnection *con_old, NMConnection *con_n s_bond = nm_connection_get_setting_bond (con_new); g_return_if_fail (s_bond); - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MODE); - if (!value) - value = "balance-rr"; - + value = nm_setting_bond_get_option_or_default (s_bond, NM_SETTING_BOND_OPTION_MODE); mode = _nm_setting_bond_mode_from_string (value); g_return_if_fail (mode != NM_BOND_MODE_UNKNOWN); /* Primary */ - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_PRIMARY); - set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_PRIMARY, value ?: ""); - + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_PRIMARY); /* Active slave */ - set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); + set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); } /*****************************************************************************/ From b868fee9cb7b09b1e5d1c269f0591cb036813d79 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 3 Mar 2020 11:36:16 +0100 Subject: [PATCH 4/5] nm-setting-bond: add API to libnm to get the normalized bond option value Add 'nm_setting_bond_get_option_normalized()', the purpose of this API is to retrieve a bond option normalized value which is the option that NetworkManager will actually apply to the bond when activating the connection, this takes into account default values for some options that NM assumes. For example, if you create a connection: $ nmcli c add type bond con-name nm-bond ifname bond0 bond.options mode=0 Calling 'nm_setting_bond_get_option_normalized(s_bond, "miimon")' would return "100" as even if not specified NetworkManager enables miimon for bond connections. Another example: $ nmcli c add type bond con-name nm-bond ifname bond0 bond.options mode=0,arp_interval=100 Calling 'nm_setting_bond_get_option_normalized(s_bond, "miimon")' would return NULL in this case because NetworkManager disables miimon if 'arp_interval' is set explicitly but 'miimon' is not. --- libnm-core/nm-setting-bond.c | 23 +++++++++++++++++++++++ libnm-core/nm-setting-bond.h | 4 ++++ libnm/libnm.ver | 1 + 3 files changed, 28 insertions(+) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index 0d34086ba..f53f2c8cf 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -661,6 +661,29 @@ nm_setting_bond_get_option_default (NMSettingBond *setting, const char *name) TRUE); } +/** + * nm_setting_bond_get_option_normalized: + * @setting: the #NMSettingBond + * @name: the name of the option + * + * Since: 1.24 + * + * Returns: the value of the bond option after normalization, which is what NetworkManager + * will actually apply when activating the connection. %NULL if the option won't be applied + * to the connection. + **/ +const char * +nm_setting_bond_get_option_normalized (NMSettingBond *setting, + const char *name) +{ + g_return_val_if_fail (NM_IS_SETTING_BOND (setting), NULL); + g_return_val_if_fail (name, NULL); + + return _bond_get_option_normalized (setting, + name, + FALSE); +} + /** * nm_setting_bond_get_option_type: * @setting: the #NMSettingBond diff --git a/libnm-core/nm-setting-bond.h b/libnm-core/nm-setting-bond.h index c17babdc8..fc2a37353 100644 --- a/libnm-core/nm-setting-bond.h +++ b/libnm-core/nm-setting-bond.h @@ -94,6 +94,10 @@ const char **nm_setting_bond_get_valid_options (NMSettingBond *setting); const char * nm_setting_bond_get_option_default (NMSettingBond *setting, const char *name); +NM_AVAILABLE_IN_1_24 +const char * nm_setting_bond_get_option_normalized (NMSettingBond *setting, + const char *name); + G_END_DECLS #endif /* __NM_SETTING_BOND_H__ */ diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 9229a1fc7..ce266bec6 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1678,6 +1678,7 @@ global: nm_secret_agent_old_get_dbus_connection; nm_secret_agent_old_get_dbus_name_owner; nm_secret_agent_old_get_main_context; + nm_setting_bond_get_option_normalized; nm_setting_vrf_get_table; nm_setting_vrf_get_type; nm_setting_vrf_new; From 40ea0335d0ce2a4390abc41c9735a5b805773a28 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Wed, 4 Mar 2020 16:22:37 +0100 Subject: [PATCH 5/5] nmtui: only set 'mode' and enable 'miimon' when setting up a new connection When creating a new connection before the user gets the chance to modify the bond options let's just initialize 'mode' and 'miimon' (with related 'updelay' and 'downdelay'). Initializing also 'arp_interval', 'arp_ip_target' and 'primary' doesn't make much sense as by default they're disabled or contain empty values. --- clients/tui/nm-editor-utils.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/clients/tui/nm-editor-utils.c b/clients/tui/nm-editor-utils.c index 83fd8d3b7..420193617 100644 --- a/clients/tui/nm-editor-utils.c +++ b/clients/tui/nm-editor-utils.c @@ -56,24 +56,19 @@ bond_connection_setup_func (NMConnection *connection, NMSetting *s_hw) { NMSettingBond *s_bond = NM_SETTING_BOND (s_hw); - const char *def, *cur; + guint i; + const char *value; static const char *const options[] = { - NM_SETTING_BOND_OPTION_ARP_INTERVAL, - NM_SETTING_BOND_OPTION_ARP_IP_TARGET, - NM_SETTING_BOND_OPTION_DOWNDELAY, - NM_SETTING_BOND_OPTION_MIIMON, NM_SETTING_BOND_OPTION_MODE, - NM_SETTING_BOND_OPTION_PRIMARY, + NM_SETTING_BOND_OPTION_MIIMON, + NM_SETTING_BOND_OPTION_DOWNDELAY, NM_SETTING_BOND_OPTION_UPDELAY, }; - guint i; - /* Only add options supported by the UI */ for (i = 0; i < G_N_ELEMENTS (options); i++) { - def = nm_setting_bond_get_option_default (s_bond, options[i]); - cur = nm_setting_bond_get_option_by_name (s_bond, options[i]); - if (!nm_streq0 (def, cur)) - nm_setting_bond_add_option (s_bond, options[i], def); + value = nm_setting_bond_get_option_default (s_bond, options[i]); + if (value) + nm_setting_bond_add_option (s_bond, options[i], value); } }