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.

3 files changed, 308 insertions, 244 deletions

diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h
index 04d61df920..babf7dfd40 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 b3c196a95d..0d34086ba9 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 347d633248..a038a6ffc6 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);
 }
 
 /*****************************************************************************/