bond: merge branch 'bg/bond-options-validate-bgo767776'

https://bugzilla.gnome.org/show_bug.cgi?id=767776

3 files changed, 158 insertions, 79 deletions

diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h
index 956c910a87..d29eb23cb5 100644
--- a/libnm-core/nm-core-internal.h
+++ b/libnm-core/nm-core-internal.h
@@ -307,6 +307,22 @@ _nm_setting_bond_get_option_type (NMSettingBond *setting, const char *name);
 
 /***********************************************************/
 
+typedef enum {
+	NM_BOND_MODE_UNKNOWN = 0,
+	NM_BOND_MODE_ROUNDROBIN,
+	NM_BOND_MODE_ACTIVEBACKUP,
+	NM_BOND_MODE_XOR,
+	NM_BOND_MODE_BROADCAST,
+	NM_BOND_MODE_8023AD,
+	NM_BOND_MODE_TLB,
+	NM_BOND_MODE_ALB,
+} NMBondMode;
+
+NMBondMode _nm_setting_bond_mode_from_string (const char *str);
+gboolean _nm_setting_bond_option_supported (const char *option, NMBondMode mode);
+
+/***********************************************************/
+
 gboolean _nm_utils_inet6_is_token (const struct in6_addr *in6addr);
 
 #endif
diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c
index e008878db8..a82d0d792c 100644
--- a/libnm-core/nm-setting-bond.c
+++ b/libnm-core/nm-setting-bond.c
@@ -92,7 +92,7 @@ static const BondDefault defaults[] = {
 	  { "slow", "fast", NULL } },
 	{ NM_SETTING_BOND_OPTION_ACTIVE_SLAVE,     "",           NM_BOND_OPTION_TYPE_IFNAME },
 	{ NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO,"65535",      NM_BOND_OPTION_TYPE_INT, 1, 65535 },
-	{ NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM,  "",           NM_BOND_OPTION_TYPE_MAC },
+	{ NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM,  NULL,         NM_BOND_OPTION_TYPE_MAC },
 	{ NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY, "0",          NM_BOND_OPTION_TYPE_INT, 0, 1023},
 	{ NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE,"0",          NM_BOND_OPTION_TYPE_INT, 0, 1},
 	{ NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS,  "any",        NM_BOND_OPTION_TYPE_BOTH, 0, 1, {"any", "all"}},
@@ -427,11 +427,22 @@ nm_setting_bond_get_valid_options  (NMSettingBond *setting)
 const char *
 nm_setting_bond_get_option_default (NMSettingBond *setting, const char *name)
 {
+	const char *mode;
 	guint i;
 
 	g_return_val_if_fail (NM_IS_SETTING_BOND (setting), NULL);
 	g_return_val_if_fail (nm_setting_bond_validate_option (name, NULL), 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_streq0 (mode, "4")
+		    || nm_streq0 (mode, "802.3ad"))
+			return "00:00:00:00:00:00";
+		else
+			return "";
+	}
+
 	for (i = 0; i < G_N_ELEMENTS (defaults); i++) {
 		if (g_strcmp0 (defaults[i].opt, name) == 0)
 			return defaults[i].val;
@@ -463,6 +474,60 @@ _nm_setting_bond_get_option_type (NMSettingBond *setting, const char *name)
 	g_assert_not_reached ();
 }
 
+NMBondMode
+_nm_setting_bond_mode_from_string (const char *str)
+{
+	g_return_val_if_fail (str, NM_BOND_MODE_UNKNOWN);
+
+	if (nm_streq (str, "balance-rr"))
+		return NM_BOND_MODE_ROUNDROBIN;
+	if (nm_streq (str, "active-backup"))
+		return NM_BOND_MODE_ACTIVEBACKUP;
+	if (nm_streq (str, "balance-xor"))
+		return NM_BOND_MODE_XOR;
+	if (nm_streq (str, "broadcast"))
+		return NM_BOND_MODE_BROADCAST;
+	if (nm_streq (str, "802.3ad"))
+		return NM_BOND_MODE_8023AD;
+	if (nm_streq (str, "balance-tlb"))
+		return NM_BOND_MODE_TLB;
+	if (nm_streq (str, "balance-alb"))
+		return NM_BOND_MODE_ALB;
+
+	return NM_BOND_MODE_UNKNOWN;
+}
+
+#define BIT(x) (1 << (x))
+
+const static struct {
+	const char *option;
+	NMBondMode unsupp_modes;
+} bond_unsupp_modes[] = {
+	{ NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE, ~(BIT (NM_BOND_MODE_ROUNDROBIN)) },
+	{ 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_ARP_INTERVAL,      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_PRIMARY,           ~(BIT (NM_BOND_MODE_ACTIVEBACKUP) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) },
+	{ 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_TLB_DYNAMIC_LB,    ~(BIT (NM_BOND_MODE_TLB)) },
+	{ 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)) },
+};
+
+gboolean
+_nm_setting_bond_option_supported (const char *option, NMBondMode mode)
+{
+	guint i;
+
+	for (i = 0; i < G_N_ELEMENTS (bond_unsupp_modes); i++) {
+		if (nm_streq (option, bond_unsupp_modes[i].option))
+		    return !NM_FLAGS_HAS (bond_unsupp_modes[i].unsupp_modes, BIT (mode));
+	}
+
+	return TRUE;
+}
+
 static gboolean
 verify (NMSetting *setting, NMConnection *connection, GError **error)
 {
diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c
index 9c98c8ea84..3f17849b0a 100644
--- a/src/devices/nm-device-bond.c
+++ b/src/devices/nm-device-bond.c
@@ -15,7 +15,7 @@
  * with this program; if not, write to the Free Software Foundation, Inc.,
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  *
- * Copyright 2011 - 2012 Red Hat, Inc.
+ * Copyright 2011 - 2016 Red Hat, Inc.
  */
 
 #include "nm-default.h"
@@ -118,12 +118,15 @@ complete_connection (NMDevice *device,
 /******************************************************************/
 
 static gboolean
-set_bond_attr (NMDevice *device, const char *attr, const char *value)
+set_bond_attr (NMDevice *device, NMBondMode mode, const char *attr, const char *value)
 {
 	NMDeviceBond *self = NM_DEVICE_BOND (device);
 	gboolean ret;
 	int ifindex = nm_device_get_ifindex (device);
 
+	if (!_nm_setting_bond_option_supported (attr, mode))
+		return FALSE;
+
 	ret = nm_platform_sysctl_master_set_option (NM_PLATFORM_GET, ifindex, attr, value);
 	if (!ret)
 		_LOGW (LOGD_HW, "failed to set bonding attribute '%s' to '%s'", attr, value);
@@ -134,10 +137,10 @@ set_bond_attr (NMDevice *device, const char *attr, const char *value)
 static gboolean
 ignore_if_zero (const char *option, const char *value)
 {
-	if (strcmp (option, "arp_interval") &&
-	    strcmp (option, "miimon") &&
-	    strcmp (option, "downdelay") &&
-	    strcmp (option, "updelay"))
+	if (!NM_IN_STRSET (option, NM_SETTING_BOND_OPTION_ARP_INTERVAL,
+	                           NM_SETTING_BOND_OPTION_DOWNDELAY,
+	                           NM_SETTING_BOND_OPTION_MIIMON,
+	                           NM_SETTING_BOND_OPTION_UPDELAY))
 		return FALSE;
 
 	return g_strcmp0 (value, "0") == 0 ? TRUE : FALSE;
@@ -173,7 +176,7 @@ update_connection (NMDevice *device, NMConnection *connection)
 		    && !ignore_if_zero (*options, value)
 		    && !nm_streq0 (value, defvalue)) {
 			/* Replace " " with "," for arp_ip_targets from the kernel */
-			if (strcmp (*options, "arp_ip_target") == 0) {
+			if (strcmp (*options, NM_SETTING_BOND_OPTION_ARP_IP_TARGET) == 0) {
 				for (p = value; *p; p++) {
 					if (*p == ' ')
 						*p = ',';
@@ -201,6 +204,7 @@ master_update_slave_connection (NMDevice *self,
 
 static void
 set_arp_targets (NMDevice *device,
+                 NMBondMode mode,
                  const char *value,
                  const char *delim,
                  const char *prefix)
@@ -214,7 +218,7 @@ set_arp_targets (NMDevice *device,
 	for (iter = items; iter && *iter; iter++) {
 		if (*iter[0]) {
 			tmp = g_strdup_printf ("%s%s", prefix, *iter);
-			set_bond_attr (device, "arp_ip_target", tmp);
+			set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_ARP_IP_TARGET, tmp);
 			g_free (tmp);
 		}
 	}
@@ -223,7 +227,7 @@ set_arp_targets (NMDevice *device,
 
 static void
 set_simple_option (NMDevice *device,
-                   const char *attr,
+                   NMBondMode mode,
                    NMSettingBond *s_bond,
                    const char *opt)
 {
@@ -232,18 +236,20 @@ set_simple_option (NMDevice *device,
 	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, attr, value);
+	set_bond_attr (device, mode, opt, value);
 }
 
 static NMActStageReturn
 apply_bonding_config (NMDevice *device)
 {
+	NMDeviceBond *self = NM_DEVICE_BOND (device);
 	NMConnection *connection;
 	NMSettingBond *s_bond;
 	int ifindex = nm_device_get_ifindex (device);
-	const char *mode, *value;
+	const char *mode_str, *value;
 	char *contents;
 	gboolean set_arp_interval = TRUE;
+	NMBondMode mode;
 
 	/* Option restrictions:
 	 *
@@ -263,98 +269,90 @@ apply_bonding_config (NMDevice *device)
 	s_bond = nm_connection_get_setting_bond (connection);
 	g_assert (s_bond);
 
-	mode = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MODE);
-	if (mode == NULL)
-		mode = "balance-rr";
+	mode_str = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MODE);
+	if (!mode_str)
+		mode_str = "balance-rr";
+
+	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 NM_ACT_STAGE_RETURN_FAILURE;
+	}
+
+	/* Set mode first, as some other options (e.g. arp_interval) are valid
+	 * only for certain modes.
+	 */
+
+	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, "arp_interval", "0");
+		set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_ARP_INTERVAL, "0");
 		set_arp_interval = FALSE;
 
-		set_bond_attr (device, "miimon", value);
-		set_simple_option (device, "updelay", s_bond, NM_SETTING_BOND_OPTION_UPDELAY);
-		set_simple_option (device, "downdelay", s_bond, NM_SETTING_BOND_OPTION_DOWNDELAY);
+		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, default to 100 */
-		long int val_int;
-		char *end;
-
+		/* 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);
-		errno = 0;
-		val_int = strtol (value ? value : "0", &end, 10);
-		if (!value || (val_int == 0 && errno == 0 && *end == '\0'))
-			set_bond_attr (device, "miimon", "100");
+		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");
 	}
 
-	/* The stuff after 'mode' requires the given mode or doesn't care */
-	set_bond_attr (device, "mode", mode);
-
-	/* arp_interval not compatible with ALB, TLB */
-	if (NM_IN_STRSET (mode, "balance-alb", "balance-tlb"))
-		set_arp_interval = FALSE;
-
 	if (set_arp_interval) {
-		set_simple_option (device, "arp_interval", s_bond, NM_SETTING_BOND_OPTION_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.
 		 */
 	}
 
+	/* ARP validate: value > 0 only valid in active-backup mode */
 	value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_ARP_VALIDATE);
-	/* arp_validate > 0 only valid in active-backup mode */
 	if (   value
 	    && !nm_streq (value, "0")
 	    && !nm_streq (value, "none")
-	    && nm_streq (mode, "active-backup"))
-		set_bond_attr (device, "arp_validate", value);
+	    && mode == NM_BOND_MODE_ACTIVEBACKUP)
+		set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_ARP_VALIDATE, value);
 	else
-		set_bond_attr (device, "arp_validate", "0");
-
-	if (NM_IN_STRSET (mode, "active-backup", "balance-alb", "balance-tlb")) {
-		value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_PRIMARY);
-		set_bond_attr (device, "primary", value ? value : "");
-		set_simple_option (device, "lp_interval", s_bond, NM_SETTING_BOND_OPTION_LP_INTERVAL);
-	}
+		set_bond_attr (device, mode, NM_SETTING_BOND_OPTION_ARP_VALIDATE, "0");
 
-	/* Clear ARP targets */
-	contents = nm_platform_sysctl_master_get_option (NM_PLATFORM_GET, ifindex, "arp_ip_target");
-	set_arp_targets (device, contents, " \n", "-");
-	g_free (contents);
+	/* 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 ? value : "");
 
-	/* Add new ARP targets */
+	/* ARP targets: clear and initialize the list */
+	contents = nm_platform_sysctl_master_get_option (NM_PLATFORM_GET, 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);
-	set_arp_targets (device, value, ",", "+");
-
-	set_simple_option (device, "primary_reselect", s_bond, NM_SETTING_BOND_OPTION_PRIMARY_RESELECT);
-	set_simple_option (device, "fail_over_mac", s_bond, NM_SETTING_BOND_OPTION_FAIL_OVER_MAC);
-	set_simple_option (device, "use_carrier", s_bond, NM_SETTING_BOND_OPTION_USE_CARRIER);
-	set_simple_option (device, "ad_select", s_bond, NM_SETTING_BOND_OPTION_AD_SELECT);
-	set_simple_option (device, "xmit_hash_policy", s_bond, NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY);
-	set_simple_option (device, "resend_igmp", s_bond, NM_SETTING_BOND_OPTION_RESEND_IGMP);
-	set_simple_option (device, "active_slave", s_bond, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE);
-	set_simple_option (device, "all_slaves_active", s_bond, NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE);
-	set_simple_option (device, "num_grat_arp", s_bond, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP);
-	set_simple_option (device, "num_unsol_na", s_bond, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA);
-
-	if (nm_streq (mode, "802.3ad")) {
-		set_simple_option (device, "lacp_rate", s_bond, NM_SETTING_BOND_OPTION_LACP_RATE);
-		set_simple_option (device, "ad_actor_sys_prio", s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO);
-		set_simple_option (device, "ad_actor_system", s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM);
-		set_simple_option (device, "ad_user_port_key", s_bond, NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY);
-		set_simple_option (device, "min_links", s_bond, NM_SETTING_BOND_OPTION_MIN_LINKS);
-	}
-
-	if (nm_streq (mode, "active-backup"))
-		set_simple_option (device, "arp_all_targets", s_bond, NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS);
-
-	if (nm_streq (mode, "balance-rr"))
-		set_simple_option (device, "packets_per_slave", s_bond, NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE);
+	set_arp_targets (device, mode, value, ",", "+");
+	g_free (contents);
 
-	if (nm_streq (mode, "balance-tlb"))
-		set_simple_option (device, "tlb_dynamic_lb", s_bond, NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB);
+	/* 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_NUM_GRAT_ARP);
+	set_simple_option (device, mode, s_bond, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA);
+	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);
 
 	return NM_ACT_STAGE_RETURN_SUCCESS;
 }