From b55578bf6e6ed449b03c87e6ca6ab38e9787583b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 9 Jul 2020 19:17:27 +0200 Subject: [PATCH] cli: fix alternating miimon/arp_interval settings for bond options in nmcli Before 1.24, nm_setting_bond_add_option() would clear miimon/arp_interval settings when the respective other was set. That was no longer done, with the effect that enabling (for example) miimon on a bond profile that has arp_interval enabled, sets both conflicting options. That is not a severe problem, because the profile still validates. However, at runtime only one of the settings can be actually configured. Fix that, by restoring the previous behavior for the client. But note that this time it's implemented in the client, and not in libnm's nm_setting_bond_add_option(). --- clients/cli/connections.c | 55 ++++++++----------- clients/common/nm-meta-setting-desc.c | 25 +++++++-- clients/common/nm-meta-setting-desc.h | 7 +++ .../nm-libnm-core-utils.c | 19 +++++++ .../nm-libnm-core-utils.h | 4 ++ libnm-core/nm-setting-bond.c | 3 +- 6 files changed, 75 insertions(+), 38 deletions(-) diff --git a/clients/cli/connections.c b/clients/cli/connections.c index b0f04f401..91f9b05a4 100644 --- a/clients/cli/connections.c +++ b/clients/cli/connections.c @@ -4273,45 +4273,36 @@ set_bond_option (NmCli *nmc, NMConnection *con, const OptionInfo *option, const { NMSettingBond *s_bond; gboolean success; + gs_free char *name = NULL; + char *p; s_bond = nm_connection_get_setting_bond (con); g_return_val_if_fail (s_bond, FALSE); - if (!value) - return TRUE; + name = g_strdup (option->option); + for (p = name; p[0]; p++) { + if (p[0] == '-') + p[0] = '_'; + } - if (strcmp (option->option, "mode") == 0) { - value = nmc_bond_validate_mode (value, error); - if (!value) - return FALSE; + if (nm_str_is_empty (value)) { + nm_setting_bond_remove_option (s_bond, name); + success = TRUE; + } else + success = _nm_meta_setting_bond_add_option (NM_SETTING (s_bond), name, value, error); - if (g_strcmp0 (value, "active-backup") == 0) { - const char *primary[] = { "primary", NULL }; - enable_options (NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS, primary); + if (!success) + return FALSE; + + if (success) { + if (nm_streq (name, NM_SETTING_BOND_OPTION_MODE)) { + value = nmc_bond_validate_mode (value, error); + if (nm_streq (value, "active-backup")) { + enable_options (NM_SETTING_BOND_SETTING_NAME, + NM_SETTING_BOND_OPTIONS, + NM_MAKE_STRV ("primary")); + } } - - success = nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_MODE, value); - } else if (strcmp (option->option, "primary") == 0) - success = nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_PRIMARY, value); - else if (strcmp (option->option, "miimon") == 0) - success = nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_MIIMON, value); - else if (strcmp (option->option, "downdelay") == 0) - success = nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_DOWNDELAY, value); - else if (strcmp (option->option, "updelay") == 0) - success = nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_UPDELAY, value); - else if (strcmp (option->option, "arp-interval") == 0) - success = nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL, value); - else if (strcmp (option->option, "arp-ip-target") == 0) - success = nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET, value); - else if (strcmp (option->option, "lacp-rate") == 0) - success = nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_LACP_RATE, value); - else - g_return_val_if_reached (FALSE); - - if (!success) { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("Error: error adding bond option '%s=%s'."), - option->option, value); } return success; diff --git a/clients/common/nm-meta-setting-desc.c b/clients/common/nm-meta-setting-desc.c index 0ea82dfa2..5a574adea 100644 --- a/clients/common/nm-meta-setting-desc.c +++ b/clients/common/nm-meta-setting-desc.c @@ -2357,8 +2357,8 @@ _get_fcn_bond_options (ARGS_GET_FCN) RETURN_STR_TO_FREE (g_string_free (str, FALSE)); } -static gboolean -_optionlist_set_fcn_bond_options (NMSetting *setting, +gboolean +_nm_meta_setting_bond_add_option (NMSetting *setting, const char *name, const char *value, GError **error) @@ -2366,8 +2366,14 @@ _optionlist_set_fcn_bond_options (NMSetting *setting, gs_free char *tmp_value = NULL; char *p; - if (!value) { - nm_setting_bond_remove_option (NM_SETTING_BOND (setting), name); + if ( !value + || !value[0]) { + if (!nm_setting_bond_remove_option (NM_SETTING_BOND (setting), name)) { + nm_utils_error_set (error, NM_UTILS_ERROR_INVALID_ARGUMENT, + _("failed to unset bond option \"%s\""), + name); + return FALSE; + } return TRUE; } @@ -2388,6 +2394,15 @@ _optionlist_set_fcn_bond_options (NMSetting *setting, name); return FALSE; } + + if (nm_streq (name, NM_SETTING_BOND_OPTION_ARP_INTERVAL)) { + if (_nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXINT, 0) > 0) + _nm_setting_bond_remove_options_miimon (NM_SETTING_BOND (setting)); + } else if (nm_streq (name, NM_SETTING_BOND_OPTION_MIIMON)) { + if (_nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXINT, 0) > 0) + _nm_setting_bond_remove_options_arp_interval (NM_SETTING_BOND (setting)); + } + return TRUE; } @@ -4883,7 +4898,7 @@ static const NMMetaPropertyInfo property_info_BOND_OPTIONS = ), .property_typ_data = DEFINE_PROPERTY_TYP_DATA ( PROPERTY_TYP_DATA_SUBTYPE (optionlist, - .set_fcn = _optionlist_set_fcn_bond_options, + .set_fcn = _nm_meta_setting_bond_add_option, ), .nested = &nm_meta_property_typ_data_bond, ), diff --git a/clients/common/nm-meta-setting-desc.h b/clients/common/nm-meta-setting-desc.h index 246897209..68c03d054 100644 --- a/clients/common/nm-meta-setting-desc.h +++ b/clients/common/nm-meta-setting-desc.h @@ -524,4 +524,11 @@ extern const NMMetaPropertyTypDataNested nm_meta_property_typ_data_bond; /*****************************************************************************/ +gboolean _nm_meta_setting_bond_add_option (NMSetting *setting, + const char *name, + const char *value, + GError **error); + +/*****************************************************************************/ + #endif /* __NM_META_SETTING_DESC_H__ */ diff --git a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c index 5e98d212c..66c4cafd8 100644 --- a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c +++ b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c @@ -16,6 +16,25 @@ nm_utils_bond_option_arp_ip_targets_split (const char *arp_ip_target) return nm_utils_strsplit_set_full (arp_ip_target, ",", NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP); } +void +_nm_setting_bond_remove_options_miimon (NMSettingBond *s_bond) +{ + g_return_if_fail (NM_IS_SETTING_BOND (s_bond)); + + nm_setting_bond_remove_option (s_bond, NM_SETTING_BOND_OPTION_MIIMON); + nm_setting_bond_remove_option (s_bond, NM_SETTING_BOND_OPTION_UPDELAY); + nm_setting_bond_remove_option (s_bond, NM_SETTING_BOND_OPTION_DOWNDELAY); +} + +void +_nm_setting_bond_remove_options_arp_interval (NMSettingBond *s_bond) +{ + g_return_if_fail (NM_IS_SETTING_BOND (s_bond)); + + nm_setting_bond_remove_option (s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL); + nm_setting_bond_remove_option (s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); +} + /*****************************************************************************/ gboolean diff --git a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h index d74a39f3d..aeddf2f9c 100644 --- a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h +++ b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h @@ -5,6 +5,7 @@ /****************************************************************************/ +#include "nm-setting-bond.h" #include "nm-setting-bridge.h" #include "nm-setting-connection.h" #include "nm-setting-ip-config.h" @@ -48,6 +49,9 @@ NM_AUTO_DEFINE_FCN0 (NMWireGuardPeer *, _nm_auto_unref_wgpeer, nm_wireguard_peer const char **nm_utils_bond_option_arp_ip_targets_split (const char *arp_ip_target); +void _nm_setting_bond_remove_options_miimon (NMSettingBond *s_bond); +void _nm_setting_bond_remove_options_arp_interval (NMSettingBond *s_bond); + /*****************************************************************************/ static inline guint32 diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index 4e38f474e..df5c87801 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -609,7 +609,8 @@ nm_setting_bond_add_option (NMSettingBond *setting, g_return_val_if_fail (NM_IS_SETTING_BOND (setting), FALSE); - if (!value || !nm_setting_bond_validate_option (name, value)) + if ( !value + || !nm_setting_bond_validate_option (name, value)) return FALSE; priv = NM_SETTING_BOND_GET_PRIVATE (setting);