libnm,core: fix handling miimon and arp_interval as conflicting kernel options

We use sysfs API for setting bond options. Note that the miimon and
arp_interval settings conflict each other, and whenever setting one
of these sysfs values, the other one gets reset. That means,
NetworkManager needs to mediate and handle a profile which has both
these options set.

Before 1.24, the libnm API nm_setting_bond_add_option() API would mangle
the content of the bond settings, to clear the respective other fields
when setting miimon/arp_interval. That also had the effect that the
settings plugins, weren't able to read such (conflicting) settings
back from disk (but they would write them to disk). If a keyfile
specified both miimon and arp_interval keys, then it would depend on
their order in the keyfile which wins.
It is wrong that a libnm property setter mangles the option in such a way,
especially, because you still could set the NM_SETTING_BOND_OPTIONS
property directly and bypass this. So, since 1.24, you can create
profiles that have conflicting options.

Also, we can now not start to reject such settings as invalid, because that
would be an API break. Instead, just make sure that when one of the
settings is set, that the other one consistently gets deactivated.

Also, before 1.24 already, NMDeviceBond would mediate whether to either set
miimon or arp_interval settings. Despite that the keyfile reader would
mangle the settings, it would also prefer miimon over arp_interval,
if both were set.

This mechanism was broken since we switch to _bond_get_option_normalized()
for that. As a consequence, NetworkManager would try to set both the
conflicting options. Fix that.

1 files changed, 26 insertions, 30 deletions

diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c
index 2ef7075f93..4e38f474e8 100644
--- a/libnm-core/nm-setting-bond.c
+++ b/libnm-core/nm-setting-bond.c
@@ -251,9 +251,7 @@ _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;
 
@@ -262,6 +260,7 @@ _bond_get_option_normalized (NMSettingBond* self,
 
 	mode_str = _bond_get_option_or_default (self, NM_SETTING_BOND_OPTION_MODE);
 	mode = _nm_setting_bond_mode_from_string (mode_str);
+
 	if (mode == NM_BOND_MODE_UNKNOWN) {
 		/* the mode is unknown, consequently, there is no normalized/default
 		 * value either. */
@@ -274,46 +273,44 @@ _bond_get_option_normalized (NMSettingBond* self,
 	/* 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)) {
+		                  NM_SETTING_BOND_OPTION_ARP_INTERVAL,
+		                  NM_SETTING_BOND_OPTION_ARP_IP_TARGET)) {
+			int 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;
+			miimon = _nm_utils_ascii_str_to_int64 (_bond_get_option (self, NM_SETTING_BOND_OPTION_MIIMON),
+			                                       10, 0, G_MAXINT, 0);
+			if (miimon != 0) {
+				/* miimon is enabled. arp_interval values are unset. */
+				if (nm_streq (option, NM_SETTING_BOND_OPTION_ARP_INTERVAL))
+					return "0";
+				return "";
 			}
+			value = _bond_get_option (self, option);
 		} 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) {
+			if (!value)
 				value = _bond_get_option (self, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA);
-			}
-		} else {
+		} else
 			value = _bond_get_option (self, option);
-		}
+
+		if (value)
+			return value;
 	}
 
-	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);
-		}
+	/* 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 (mode == NM_BOND_MODE_8023AD)
+			return "00:00:00:00:00:00";
+		return "";
 	}
 
-	return value;
+	return _bond_get_option_or_default (self, option);
 }
 
 const char*
@@ -686,9 +683,8 @@ nm_setting_bond_get_option_default (NMSettingBond *setting, const char *name)
 {
 	g_return_val_if_fail (NM_IS_SETTING_BOND (setting), NULL);
 
-	if (!name) {
+	if (!name)
 		return NULL;
-	}
 
 	return _bond_get_option_normalized (setting,
 	                                    name,