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().

6 files changed, 75 insertions, 38 deletions

diff --git a/clients/cli/connections.c b/clients/cli/connections.c
index b0f04f4017..91f9b05a48 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;
-
-	if (strcmp (option->option, "mode") == 0) {
-		value = nmc_bond_validate_mode (value, error);
-		if (!value)
-			return FALSE;
+	name = g_strdup (option->option);
+	for (p = name; p[0]; p++) {
+		if (p[0] == '-')
+			p[0] = '_';
+	}
 
-		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 (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);
 
-		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)
+		return FALSE;
 
-	if (!success) {
-		g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT,
-		             _("Error: error adding bond option '%s=%s'."),
-		             option->option, value);
+	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"));
+			}
+		}
 	}
 
 	return success;
diff --git a/clients/common/nm-meta-setting-desc.c b/clients/common/nm-meta-setting-desc.c
index 0ea82dfa28..5a574adea1 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 2468972092..68c03d0541 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 5e98d212c8..66c4cafd89 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 d74a39f3d7..aeddf2f9c5 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 4e38f474e8..df5c878019 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);