bonding fixes

[Dan Winship <danw gnome org>]

>From 7765a481979af6aad48dee4918b63caf4351c5f2 Mon Sep 17 00:00:00 2001
From: Dan Winship <danw gnome org>
Date: Tue, 20 Mar 2012 13:06:22 -0400
Subject: [PATCH 1/4] core: fix NMDeviceBond:dispose() to chain up

---
 src/nm-device-bond.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/nm-device-bond.c b/src/nm-device-bond.c
index 710de4c..0d99fd7 100644
--- a/src/nm-device-bond.c
+++ b/src/nm-device-bond.c
@@ -45,7 +45,6 @@ G_DEFINE_TYPE (NMDeviceBond, nm_device_bond, NM_TYPE_DEVICE_WIRED)
 #define NM_BOND_ERROR (nm_bond_error_quark ())
 
 typedef struct {
-	gboolean disposed;
 	GSList *slaves;
 } NMDeviceBondPrivate;
 
@@ -506,15 +505,12 @@ dispose (GObject *object)
 	NMDeviceBondPrivate *priv = NM_DEVICE_BOND_GET_PRIVATE (self);
 	GSList *iter;
 
-	if (priv->disposed) {
-		G_OBJECT_CLASS (nm_device_bond_parent_class)->dispose (object);
-		return;
-	}
-	priv->disposed = TRUE;
-
 	for (iter = priv->slaves; iter; iter = g_slist_next (iter))
 		release_slave (NM_DEVICE (self), ((SlaveInfo *) iter->data)->slave);
 	g_slist_free (priv->slaves);
+	priv->slaves = NULL;
+
+	G_OBJECT_CLASS (nm_device_bond_parent_class)->dispose (object);
 }
 
 static void
-- 
1.7.7.6

>From 11b4614d94e4dbab2cabd69762509a4c12b98a03 Mon Sep 17 00:00:00 2001
From: Dan Winship <danw gnome org>
Date: Thu, 22 Mar 2012 12:39:16 -0400
Subject: [PATCH 2/4] libnm-util: fix an NMSettingBond bug

NMSettingBond sets the "miimon" option to "100" by default, but this
means that when reading in a saved configuration with "arp_interval"
set, it would end up with both "miimon" and "arp_interval" set, which
is invalid. Fix this by clearing "miimon" if "arp_interval" is set,
and vice versa.
---
 libnm-util/nm-setting-bond.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/libnm-util/nm-setting-bond.c b/libnm-util/nm-setting-bond.c
index 50cad2d..0e9bb0f 100644
--- a/libnm-util/nm-setting-bond.c
+++ b/libnm-util/nm-setting-bond.c
@@ -237,17 +237,26 @@ gboolean nm_setting_bond_add_option (NMSettingBond *setting,
                                      const char *name,
                                      const char *value)
 {
+	NMSettingBondPrivate *priv;
 	size_t value_len;
 
 	g_return_val_if_fail (NM_IS_SETTING_BOND (setting), FALSE);
 	g_return_val_if_fail (validate_option (name), FALSE);
 	g_return_val_if_fail (value != NULL, FALSE);
 
+	priv = NM_SETTING_BOND_GET_PRIVATE (setting);
+
 	value_len = strlen (value);
 	g_return_val_if_fail (value_len > 0 && value_len < 200, FALSE);
 
-	g_hash_table_insert (NM_SETTING_BOND_GET_PRIVATE (setting)->options,
-	                     g_strdup (name), g_strdup (value));
+	g_hash_table_insert (priv->options, g_strdup (name), g_strdup (value));
+
+	if (!strcmp (name, NM_SETTING_BOND_OPTION_MIIMON)) {
+		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 (!strcmp (name, NM_SETTING_BOND_OPTION_ARP_INTERVAL))
+		g_hash_table_remove (priv->options, NM_SETTING_BOND_OPTION_MIIMON);
+
 	return TRUE;
 }
 
-- 
1.7.7.6

>From d719b6af2fa42a198369c96f7a2e133541560656 Mon Sep 17 00:00:00 2001
From: Dan Winship <danw gnome org>
Date: Tue, 20 Mar 2012 09:39:57 -0400
Subject: [PATCH 3/4] libnm-util: improve NMSettingBond:verify()

Add NM_SETTING_BOND_ERROR_INVALID_OPTION and
NM_SETTING_BOND_ERROR_MISSING_OPTION error codes so we can better
distinguish errors in different options, and add checks for various
incompatible sets of options.
---
 libnm-util/nm-setting-bond.c |  136 +++++++++++++++++++++++++++++++++++++++---
 libnm-util/nm-setting-bond.h |    2 +
 2 files changed, 130 insertions(+), 8 deletions(-)

diff --git a/libnm-util/nm-setting-bond.c b/libnm-util/nm-setting-bond.c
index 0e9bb0f..9f9f438 100644
--- a/libnm-util/nm-setting-bond.c
+++ b/libnm-util/nm-setting-bond.c
@@ -23,6 +23,7 @@
 
 #include <string.h>
 #include <ctype.h>
+#include <stdlib.h>
 #include <dbus/dbus-glib.h>
 
 #include "nm-setting-bond.h"
@@ -353,6 +354,15 @@ dev_valid_name(const char *name)
 	return TRUE;
 }
 
+static gint
+find_setting_by_name (gconstpointer a, gconstpointer b)
+{
+	NMSetting *setting = NM_SETTING (a);
+	const char *str = (const char *) b;
+
+	return strcmp (nm_setting_get_name (setting), str);
+}
+
 static gboolean
 verify (NMSetting *setting, GSList *all_settings, GError **error)
 {
@@ -367,6 +377,8 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
 	                              "balance-tlb",
 	                              "balance-alb",
 	                              NULL };
+	int miimon = 0, arp_interval = 0;
+	const char *arp_ip_target = NULL;
 
 	if (!priv->interface_name || !strlen(priv->interface_name)) {
 		g_set_error (error,
@@ -388,24 +400,131 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
 	while (g_hash_table_iter_next (&iter, (gpointer) &key, (gpointer) &value)) {
 		if (   !validate_option (key)
 		    || !value[0]
-		    || (strlen (value) > 200)) {
+		    || (strlen (value) > 200)
+		    || strchr (value, ' ')) {
+			g_set_error (error,
+			             NM_SETTING_BOND_ERROR,
+			             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+			             key);
+			return FALSE;
+		}
+	}
+
+	value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_MIIMON);
+	if (value)
+		miimon = atoi (value);
+	value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_ARP_INTERVAL);
+	if (value)
+		arp_interval = atoi (value);
+
+	/* Can only set one of miimon and arp_interval */
+	if (miimon > 0 && arp_interval > 0) {
+		g_set_error (error,
+		             NM_SETTING_BOND_ERROR,
+		             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+		             NM_SETTING_BOND_OPTION_ARP_INTERVAL);
+	}
+
+	value = g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_MODE);
+	if (!value) {
+		g_set_error (error,
+		             NM_SETTING_BOND_ERROR,
+		             NM_SETTING_BOND_ERROR_MISSING_OPTION,
+		             NM_SETTING_BOND_OPTION_MODE);
+		return FALSE;
+	}
+	if (!_nm_utils_string_in_list (value, valid_modes)) {
+		g_set_error (error,
+		             NM_SETTING_BOND_ERROR,
+		             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+		             NM_SETTING_BOND_OPTION_MODE);
+		return FALSE;
+	}
+
+	/* Make sure mode is compatible with other settings */
+	if (   strcmp (value, "balance-alb") == 0
+	    || strcmp (value, "balance-tlb") == 0) {
+		if (arp_interval > 0) {
+			g_set_error (error,
+			             NM_SETTING_BOND_ERROR,
+			             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+			             NM_SETTING_BOND_OPTION_ARP_INTERVAL);
+		}
+	}
+	if (g_slist_find_custom (all_settings, NM_SETTING_INFINIBAND_SETTING_NAME, find_setting_by_name)) {
+		if (strcmp (value, "active-backup") != 0) {
 			g_set_error (error,
 			             NM_SETTING_BOND_ERROR,
-			             NM_SETTING_BOND_ERROR_INVALID_PROPERTY,
-			             NM_SETTING_BOND_OPTIONS);
+			             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+			             NM_SETTING_BOND_OPTION_MODE);
 			return FALSE;
 		}
+	}
 
-		if (   (g_strcmp0 (key, "mode") == 0)
-		    && !_nm_utils_string_in_list (value, valid_modes)) {
+	if (miimon == 0) {
+		/* updelay and downdelay can only be used with miimon */
+		if (g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_UPDELAY)) {
 			g_set_error (error,
 			             NM_SETTING_BOND_ERROR,
-			             NM_SETTING_BOND_ERROR_INVALID_PROPERTY,
-			             NM_SETTING_BOND_OPTIONS);
+			             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+			             NM_SETTING_BOND_OPTION_UPDELAY);
 			return FALSE;
 		}
+		if (g_hash_table_lookup (priv->options, NM_SETTING_BOND_OPTION_DOWNDELAY)) {
+			g_set_error (error,
+			             NM_SETTING_BOND_ERROR,
+			             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+			             NM_SETTING_BOND_OPTION_DOWNDELAY);
+			return FALSE;
+		}
+	}
+
+	/* 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);
+	if (arp_interval > 0) {
+		char **addrs;
+		guint32 addr;
+		int i;
 
-		/* XXX: Validate arp-ip-target */
+		if (!arp_ip_target) {
+			g_set_error (error,
+			             NM_SETTING_BOND_ERROR,
+			             NM_SETTING_BOND_ERROR_MISSING_OPTION,
+			             NM_SETTING_BOND_OPTION_ARP_IP_TARGET);
+			return FALSE;
+		}
+
+		addrs = g_strsplit (arp_ip_target, ",", -1);
+		if (!addrs[0]) {
+			g_strfreev (addrs);
+			g_set_error (error,
+			             NM_SETTING_BOND_ERROR,
+			             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+			             NM_SETTING_BOND_OPTION_ARP_IP_TARGET);
+			return FALSE;
+		}
+
+		for (i = 0; addrs[i]; i++) {
+			if (!inet_pton (AF_INET, addrs[i], &addr)) {
+				g_strfreev (addrs);
+				g_set_error (error,
+				             NM_SETTING_BOND_ERROR,
+				             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+				             NM_SETTING_BOND_OPTION_ARP_IP_TARGET);
+				return FALSE;
+			}
+		}
+		g_strfreev (addrs);
+	} else {
+		if (arp_ip_target) {
+			g_set_error (error,
+			             NM_SETTING_BOND_ERROR,
+			             NM_SETTING_BOND_ERROR_INVALID_OPTION,
+			             NM_SETTING_BOND_OPTION_ARP_IP_TARGET);
+			return FALSE;
+		}
 	}
 
 	return TRUE;
@@ -430,6 +549,7 @@ nm_setting_bond_init (NMSettingBond *setting)
 	priv->options = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
 
 	/* Default values: */
+	nm_setting_bond_add_option (setting, NM_SETTING_BOND_OPTION_MODE, "balance-rr");
 	nm_setting_bond_add_option (setting, NM_SETTING_BOND_OPTION_MIIMON, "100");
 }
 
diff --git a/libnm-util/nm-setting-bond.h b/libnm-util/nm-setting-bond.h
index 1904505..9d494d3 100644
--- a/libnm-util/nm-setting-bond.h
+++ b/libnm-util/nm-setting-bond.h
@@ -48,6 +48,8 @@ typedef enum {
 	NM_SETTING_BOND_ERROR_UNKNOWN = 0,      /*< nick=UnknownError >*/
 	NM_SETTING_BOND_ERROR_INVALID_PROPERTY, /*< nick=InvalidProperty >*/
 	NM_SETTING_BOND_ERROR_MISSING_PROPERTY, /*< nick=MissingProperty >*/
+	NM_SETTING_BOND_ERROR_INVALID_OPTION,   /*< nick=InvalidOption >*/
+	NM_SETTING_BOND_ERROR_MISSING_OPTION,   /*< nick=MissingOption >*/
 } NMSettingBondError;
 
 #define NM_SETTING_BOND_ERROR nm_setting_bond_error_quark ()
-- 
1.7.7.6

>From b779dd666e1b3e80633c317dffbb06b556267602 Mon Sep 17 00:00:00 2001
From: Dan Winship <danw gnome org>
Date: Tue, 20 Mar 2012 11:16:01 -0400
Subject: [PATCH 4/4] core: do a better job of applying bond configuration

Reset all known bond options to their default values, not just the
ones that NMSettingBond allows overriding. Also, remove any bond
slaves that were already attached to the bond before we managed it.

Only update bond parameters that need to be updated. In particular,
setting either arp_interval or miimon to 0 has the side effect of also
setting the other one to 0, so don't do that if it's already 0.

Fix the handling of arp_ip_target; the sysfs arp_ip_target node does
not work the same way as the ifcfg BONDING_OPTS line (which is what
the code was assuming before).
---
 src/nm-system.c |  154 +++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 105 insertions(+), 49 deletions(-)

diff --git a/src/nm-system.c b/src/nm-system.c
index ed75b3f..7368dc3 100644
--- a/src/nm-system.c
+++ b/src/nm-system.c
@@ -1226,70 +1226,126 @@ nm_system_device_set_priority (int ifindex,
 	}
 }
 
-static gboolean
-set_bond_attr (const char *iface, const char *attr, const char *value)
+static const struct {
+	const char *option;
+	const char *default_value;
+} bonding_defaults[] = {
+	{ "mode", "balance-rr" },
+	{ "arp_interval", "0" },
+	{ "miimon", "0" },
+
+	{ "ad_select", "stable" },
+	{ "arp_validate", "none" },
+	{ "downdelay", "0" },
+	{ "fail_over_mac", "none" },
+	{ "lacp_rate", "slow" },
+	{ "min_links", "0" },
+	{ "num_grat_arp", "1" },
+	{ "num_unsol_na", "1" },
+	{ "primary", "" },
+	{ "primary_reselect", "always" },
+	{ "resend_igmp", "1" },
+	{ "updelay", "0" },
+	{ "use_carrier", "1" },
+	{ "xmit_hash_policy", "layer2" },
+	{ NULL, NULL }
+};
+
+static void
+remove_bonding_entries (const char *iface, const char *path)
 {
-	char file[FILENAME_MAX];
+	char cmd[20];
+	char *value, **entries;
 	gboolean ret;
+	int i;
+
+	if (!g_file_get_contents (path, &value, NULL, NULL))
+		return;
 
-	snprintf (file, sizeof (file), "/sys/class/net/%s/bonding/%s", iface, attr);
-	ret = nm_utils_do_sysctl (file, value);
-	if (!ret) {
-		nm_log_warn (LOGD_HW, "(%s): failed to set bonding attribute "
-		             "'%s' to '%s'", iface, attr, value);
+	entries = g_strsplit (value, " ", -1);
+	for (i = 0; entries[i]; i++) {
+		snprintf (cmd, sizeof (cmd), "-%s", g_strstrip (entries[i]));
+		ret = nm_utils_do_sysctl (path, cmd);
+		if (!ret) {
+			nm_log_warn (LOGD_HW, "(%s): failed to remove entry '%s' from '%s'",
+			             iface, entries[i], path);
+		}
 	}
+	g_strfreev (entries);
+}
 
-	return ret;
+static gboolean
+option_valid_for_nm_setting (const char *option, const char **valid_opts)
+{
+	while (*valid_opts) {
+		if (strcmp (option, *valid_opts) == 0)
+			return TRUE;
+		valid_opts++;
+	}
+	return FALSE;
 }
 
 gboolean
 nm_system_apply_bonding_config (const char *iface, NMSettingBond *s_bond)
 {
-	const char **opts, **iter;
+	const char **valid_opts;
+	const char *option, *value;
+	char path[FILENAME_MAX];
+	char *current, *space;
+	gboolean ret;
+	int i;
 
 	g_return_val_if_fail (iface != NULL, FALSE);
 
-	/*
-	 * FIXME:
-	 *
-	 * ifup-eth contains code to append targets if the value is prefixed
-	 * with '+':
-	 *
-	 *  if [ "${key}" = "arp_ip_target" -a "${value:0:1}" != "+" ]; then
-	 *  OLDIFS=$IFS;
-	 *  IFS=',';
-	 *  for arp_ip in $value; do
-	 *      if ! grep -q $arp_ip /sys/class/net/${DEVICE}/bonding/$key; then
-	 *          echo +$arp_ip > /sys/class/net/${DEVICE}/bonding/$key
-	 *      fi
-	 *  done
-	 *
-	 * Not sure if this is actually being used and it seems dangerous as
-	 * the result is pretty much unforeseeable.
-	 */
-
-	/* Set bonding options; if the setting didn't specify a value for the
-	 * option then use the default value to ensure there's no leakage of
-	 * options from any previous connections to this one.
-	 */
-	opts = nm_setting_bond_get_valid_options (s_bond);
-	for (iter = opts; iter && *iter; iter++) {
-		const char *value;
-		gboolean is_default = FALSE;
-
-		value = nm_setting_bond_get_option_by_name (s_bond, *iter);
-		if (!value) {
-			value = nm_setting_bond_get_option_default (s_bond, *iter);  /* use the default value */
-			is_default = TRUE;
+	/* Remove old slaves and arp_ip_targets */
+	snprintf (path, sizeof (path), "/sys/class/net/%s/bonding/arp_ip_target", iface);
+	remove_bonding_entries (iface, path);
+	snprintf (path, sizeof (path), "/sys/class/net/%s/bonding/slaves", iface);
+	remove_bonding_entries (iface, path);
+
+	/* Apply config/defaults */
+	valid_opts = nm_setting_bond_get_valid_options (s_bond);
+	for (i = 0; bonding_defaults[i].option; i++) {
+		option = bonding_defaults[i].option;
+		if (option_valid_for_nm_setting (option, valid_opts))
+			value = nm_setting_bond_get_option_by_name (s_bond, option);
+		else
+			value = NULL;
+		if (!value)
+			value = bonding_defaults[i].default_value;
+
+		snprintf (path, sizeof (path), "/sys/class/net/%s/bonding/%s", iface, option);
+		if (g_file_get_contents (path, &current, NULL, NULL)) {
+			g_strstrip (current);
+			space = strchr (current, ' ');
+			if (space)
+				*space = '\0';
+			if (strcmp (current, value) != 0) {
+				ret = nm_utils_do_sysctl (path, value);
+				if (!ret) {
+					nm_log_warn (LOGD_HW, "(%s): failed to set bonding attribute "
+					             "'%s' to '%s'", iface, option, value);
+				}
+			}
 		}
+	}
 
-		nm_log_dbg (LOGD_DEVICE, "(%s): setting bond option '%s' to %s'%s'",
-			        iface,
-			        *iter,
-			        is_default ? "default " : "",
-			        value);
-
-		set_bond_attr (iface, *iter, value);
+	/* Handle arp_ip_target */
+	value = nm_setting_bond_get_option_by_name (s_bond, "arp_ip_target");
+	if (value) {
+		char **addresses, cmd[20];
+
+		snprintf (path, sizeof (path), "/sys/class/net/%s/bonding/arp_ip_target", iface);
+		addresses = g_strsplit (value, ",", -1);
+		for (i = 0; addresses[i]; i++) {
+			snprintf (cmd, sizeof (cmd), "+%s", g_strstrip (addresses[i]));
+			ret = nm_utils_do_sysctl (path, cmd);
+			if (!ret) {
+				nm_log_warn (LOGD_HW, "(%s): failed to add arp_ip_target '%s'",
+				             iface, addresses[i]);
+			}
+		}
+		g_strfreev (addresses);
 	}
 
 	return TRUE;
-- 
1.7.7.6