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

@@ -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;

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,
 	    ),

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__ */

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

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

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);