bond: handle bond options more gracefully

Support new bonding options and set them carefully. The options cannot
be set arbitrarily because they interfere with each other.

This commit is forward-ported from rhel-6.5, see patch
rh901662-bond-more-options.patch, originally written by Dan Williams.

https://bugzilla.redhat.com/show_bug.cgi?id=901662
https://bugzilla.redhat.com/show_bug.cgi?id=905532

Co-Authored-By: Dan Williams <dcbw@redhat.com>
Signed-off-by: Thomas Haller <thaller@redhat.com>

4 files changed, 286 insertions, 146 deletions

diff --git a/libnm-util/nm-setting-bond.c b/libnm-util/nm-setting-bond.c
index 8d5b9c0d7a..12f7941d0b 100644
--- a/libnm-util/nm-setting-bond.c
+++ b/libnm-util/nm-setting-bond.c
@@ -23,6 +23,9 @@
 
 #include <string.h>
 #include <stdlib.h>
+#include <errno.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
 #include <dbus/dbus-glib.h>
 #include <glib/gi18n.h>
 
@@ -81,19 +84,43 @@ enum {
 	LAST_PROP
 };
 
+enum {
+	TYPE_INT,
+	TYPE_STR,
+	TYPE_BOTH,
+	TYPE_IP,
+};
+
 typedef struct {
 	const char *opt;
 	const char *val;
+	guint opt_type;
+	guint min;
+	guint max;
+	char *list[10];
 } BondDefault;
 
 static const BondDefault defaults[] = {
-	{ NM_SETTING_BOND_OPTION_MODE,          "balance-rr" },
-	{ NM_SETTING_BOND_OPTION_MIIMON,        "100"        },
-	{ NM_SETTING_BOND_OPTION_DOWNDELAY,     "0"          },
-	{ NM_SETTING_BOND_OPTION_UPDELAY,       "0"          },
-	{ NM_SETTING_BOND_OPTION_ARP_INTERVAL,  "0"          },
-	{ NM_SETTING_BOND_OPTION_ARP_IP_TARGET, ""           },
-	{ NM_SETTING_BOND_OPTION_PRIMARY,       ""           },
+	{ NM_SETTING_BOND_OPTION_MODE,             "balance-rr", TYPE_BOTH, 0, 6,
+	  { "balance-rr", "active-backup", "balance-xor", "broadcast", "802.3ad", "balance-tlb", "balance-alb", NULL } },
+	{ NM_SETTING_BOND_OPTION_MIIMON,           "100",        TYPE_INT, 0, G_MAXINT },
+	{ NM_SETTING_BOND_OPTION_DOWNDELAY,        "0",          TYPE_INT, 0, G_MAXINT },
+	{ NM_SETTING_BOND_OPTION_UPDELAY,          "0",          TYPE_INT, 0, G_MAXINT },
+	{ NM_SETTING_BOND_OPTION_ARP_INTERVAL,     "0",          TYPE_INT, 0, G_MAXINT },
+	{ NM_SETTING_BOND_OPTION_ARP_IP_TARGET,    "",           TYPE_IP },
+	{ NM_SETTING_BOND_OPTION_ARP_VALIDATE,     "0",          TYPE_BOTH, 0, 3,
+	  { "none", "active", "backup", "all", NULL } },
+	{ NM_SETTING_BOND_OPTION_PRIMARY,          "",           TYPE_STR },
+	{ NM_SETTING_BOND_OPTION_PRIMARY_RESELECT, "0",          TYPE_BOTH, 0, 2,
+	  { "always", "better", "failure", NULL } },
+	{ NM_SETTING_BOND_OPTION_FAIL_OVER_MAC,    "0",          TYPE_BOTH, 0, 2,
+	  { "none", "active", "follow", NULL } },
+	{ NM_SETTING_BOND_OPTION_USE_CARRIER,      "1",          TYPE_INT, 0, 1 },
+	{ NM_SETTING_BOND_OPTION_AD_SELECT,        "0",          TYPE_BOTH, 0, 2,
+	  { "stable", "bandwidth", "count", NULL } },
+	{ NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY, "0",          TYPE_BOTH, 0, 2,
+	  { "layer2", "layer3+4", "layer2+3", NULL } },
+	{ NM_SETTING_BOND_OPTION_RESEND_IGMP,      "1",          TYPE_INT, 0, 255 },
 };
 
 /**
@@ -192,7 +219,61 @@ nm_setting_bond_get_option (NMSettingBond *setting,
 }
 
 static gboolean
-validate_option (const char *name)
+validate_int (const char *name, const char *value, const BondDefault *def)
+{
+	glong num;
+	guint i;
+
+	for (i = 0; i < strlen (value); i++) {
+		if (!g_ascii_isdigit (value[i]) && value[i] != '-')
+			return FALSE;
+	}
+
+	errno = 0;
+	num = strtol (value, NULL, 10);
+	if (errno)
+		return FALSE;
+	if (num < def->min || num > def->max)
+		return FALSE;
+
+	return TRUE;
+}
+
+static gboolean
+validate_list (const char *name, const char *value, const BondDefault *def)
+{
+	guint i;
+
+	for (i = 0; def->list && i < G_N_ELEMENTS (def->list) && def->list[i]; i++) {
+		if (g_strcmp0 (def->list[i], value) == 0)
+			return TRUE;
+	}
+
+	/* empty validation list means all values pass */
+	return (def->list == NULL || def->list[0] == NULL) ? TRUE : FALSE;
+}
+
+static gboolean
+validate_ip (const char *name, const char *value)
+{
+	char **ips, **iter;
+	gboolean success = TRUE;
+	struct in_addr addr;
+
+	if (!value || !value[0])
+		return FALSE;
+
+	ips = g_strsplit_set (value, ",", 0);
+	for (iter = ips; iter && *iter && success; iter++)
+		success = !!inet_aton (*iter, &addr);
+	g_strfreev (ips);
+
+	return success;
+}
+
+/* If value is NULL, validates name only */
+static gboolean
+validate_option (const char *name, const char *value)
 {
 	guint i;
 
@@ -200,8 +281,21 @@ validate_option (const char *name)
 	g_return_val_if_fail (name[0] != '\0', FALSE);
 
 	for (i = 0; i < G_N_ELEMENTS (defaults); i++) {
-		if (g_strcmp0 (defaults[i].opt, name) == 0)
-			return TRUE;
+		if (g_strcmp0 (defaults[i].opt, name) == 0) {
+			if (value == NULL)
+				return TRUE;
+			else if (defaults[i].opt_type == TYPE_INT)
+				return validate_int (name, value, &defaults[i]);
+			else if (defaults[i].opt_type == TYPE_STR)
+				return validate_list (name, value, &defaults[i]);
+			else if (defaults[i].opt_type == TYPE_BOTH)
+				return    validate_int (name, value, &defaults[i])
+				       || validate_list (name, value, &defaults[i]);
+			else if (defaults[i].opt_type == TYPE_IP)
+				return validate_ip (name, value);
+
+			return FALSE;
+		}
 	}
 	return FALSE;
 }
@@ -222,7 +316,7 @@ nm_setting_bond_get_option_by_name (NMSettingBond *setting,
                                     const char *name)
 {
 	g_return_val_if_fail (NM_IS_SETTING_BOND (setting), NULL);
-	g_return_val_if_fail (validate_option (name), NULL);
+	g_return_val_if_fail (validate_option (name, NULL), NULL);
 
 	return g_hash_table_lookup (NM_SETTING_BOND_GET_PRIVATE (setting)->options, name);
 }
@@ -246,17 +340,13 @@ gboolean nm_setting_bond_add_option (NMSettingBond *setting,
                                      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 (validate_option (name, value), 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 (priv->options, g_strdup (name), g_strdup (value));
 
 	if (   !strcmp (name, NM_SETTING_BOND_OPTION_MIIMON)
@@ -293,7 +383,7 @@ nm_setting_bond_remove_option (NMSettingBond *setting,
 	gboolean found;
 
 	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 (validate_option (name, NULL), FALSE);
 
 	found = g_hash_table_remove (NM_SETTING_BOND_GET_PRIVATE (setting)->options, name);
 	if (found)
@@ -338,7 +428,7 @@ nm_setting_bond_get_option_default (NMSettingBond *setting, const char *name)
 	guint i;
 
 	g_return_val_if_fail (NM_IS_SETTING_BOND (setting), NULL);
-	g_return_val_if_fail (validate_option (name), NULL);
+	g_return_val_if_fail (validate_option (name, NULL), NULL);
 
 	for (i = 0; i < G_N_ELEMENTS (defaults); i++) {
 		if (g_strcmp0 (defaults[i].opt, name) == 0)
@@ -386,10 +476,7 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
 
 	g_hash_table_iter_init (&iter, priv->options);
 	while (g_hash_table_iter_next (&iter, (gpointer) &key, (gpointer) &value)) {
-		if (   !validate_option (key)
-		    || !value[0]
-		    || (strlen (value) > 200)
-		    || strchr (value, ' ')) {
+		if (!value[0] || !validate_option (key, value)) {
 			g_set_error (error,
 			             NM_SETTING_BOND_ERROR,
 			             NM_SETTING_BOND_ERROR_INVALID_OPTION,
@@ -589,7 +676,6 @@ nm_setting_bond_init (NMSettingBond *setting)
 
 	/* 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");
 }
 
 static void
diff --git a/libnm-util/nm-setting-bond.h b/libnm-util/nm-setting-bond.h
index ba833a9b9d..06fcf28eae 100644
--- a/libnm-util/nm-setting-bond.h
+++ b/libnm-util/nm-setting-bond.h
@@ -59,13 +59,20 @@ GQuark nm_setting_bond_error_quark (void);
 #define NM_SETTING_BOND_OPTIONS "options"
 
 /* Valid options for the 'options' property */
-#define NM_SETTING_BOND_OPTION_MODE          "mode"
-#define NM_SETTING_BOND_OPTION_MIIMON        "miimon"
-#define NM_SETTING_BOND_OPTION_DOWNDELAY     "downdelay"
-#define NM_SETTING_BOND_OPTION_UPDELAY       "updelay"
-#define NM_SETTING_BOND_OPTION_ARP_INTERVAL  "arp_interval"
-#define NM_SETTING_BOND_OPTION_ARP_IP_TARGET "arp_ip_target"
-#define NM_SETTING_BOND_OPTION_PRIMARY       "primary"
+#define NM_SETTING_BOND_OPTION_MODE             "mode"
+#define NM_SETTING_BOND_OPTION_MIIMON           "miimon"
+#define NM_SETTING_BOND_OPTION_DOWNDELAY        "downdelay"
+#define NM_SETTING_BOND_OPTION_UPDELAY          "updelay"
+#define NM_SETTING_BOND_OPTION_ARP_INTERVAL     "arp_interval"
+#define NM_SETTING_BOND_OPTION_ARP_IP_TARGET    "arp_ip_target"
+#define NM_SETTING_BOND_OPTION_ARP_VALIDATE     "arp_validate"
+#define NM_SETTING_BOND_OPTION_PRIMARY          "primary"
+#define NM_SETTING_BOND_OPTION_PRIMARY_RESELECT "primary_reselect"
+#define NM_SETTING_BOND_OPTION_FAIL_OVER_MAC    "fail_over_mac"
+#define NM_SETTING_BOND_OPTION_USE_CARRIER      "use_carrier"
+#define NM_SETTING_BOND_OPTION_AD_SELECT        "ad_select"
+#define NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY "xmit_hash_policy"
+#define NM_SETTING_BOND_OPTION_RESEND_IGMP      "resend_igmp"
 
 typedef struct {
 	NMSetting parent;
diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c
index b43d41d4dc..f05a07df61 100644
--- a/src/devices/nm-device-bond.c
+++ b/src/devices/nm-device-bond.c
@@ -24,6 +24,8 @@
 #include <glib/gi18n.h>
 
 #include <netinet/ether.h>
+#include <errno.h>
+#include <stdlib.h>
 
 #include "gsystem-local-alloc.h"
 #include "nm-device-bond.h"
@@ -180,150 +182,179 @@ match_l2_config (NMDevice *self, NMConnection *connection)
 
 /******************************************************************/
 
-typedef struct {
-	const char *name;
-	const char *default_value;
-} Option;
-
-static const Option master_options[] = {
-	{ "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 gboolean
-option_valid_for_nm_setting (NMSettingBond *s_bond, const Option *option)
+set_bond_attr (const char *iface, const char *attr, const char *value)
 {
-	const char **valid_opts = nm_setting_bond_get_valid_options (s_bond);
-
-	for (; *valid_opts; valid_opts++)
-		if (!strcmp (option->name, *valid_opts))
-			return TRUE;
-
-	return FALSE;
+	char file[FILENAME_MAX];
+	gboolean ret;
+
+	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': %d", iface, attr, value, errno);
+	}
+	return ret;
 }
 
 static void
-remove_bonding_entries (NMDevice *device, const char *option)
+set_arp_targets (const char *iface,
+                 const char *value,
+                 const char *delim,
+                 const char *prefix)
 {
-	int ifindex = nm_device_get_ifindex (device);
-	const char *ifname = nm_device_get_iface (device);
-	gs_free char *value = nm_platform_master_get_option (ifindex, option);
-	char **entries, **entry;
-	char cmd[20];
-
-	g_return_if_fail (value);
-
-	entries = g_strsplit (value, " ", -1);
-	for (entry = entries; *entry; entry++) {
-		snprintf (cmd, sizeof (cmd), "-%s", g_strstrip (*entry));
-		if (!nm_platform_master_set_option (ifindex, option, cmd))
-			nm_log_warn (LOGD_HW, "(%s): failed to remove entry '%s' from '%s'",
-			             ifname, *entry, option);
+	char **items, **iter, *tmp;
+
+	items = g_strsplit_set (value, delim, 0);
+	for (iter = items; iter && *iter; iter++) {
+		if (*iter[0]) {
+			tmp = g_strdup_printf ("%s%s", prefix, *iter);
+			set_bond_attr (iface, "arp_ip_target", tmp);
+			g_free (tmp);
+		}
 	}
-	g_strfreev (entries);
+	g_strfreev (items);
 }
 
-static gboolean
-apply_bonding_config (NMDevice *device, NMSettingBond *s_bond)
+static void
+set_simple_option (const char *iface,
+                   const char *attr,
+                   NMSettingBond *s_bond,
+                   const char *opt)
 {
-	int ifindex = nm_device_get_ifindex (device);
-	const char *ifname = nm_device_get_iface (device);
-	static const Option *option;
-	const char *value;
-
-	g_return_val_if_fail (ifindex, FALSE);
-
-	/* Remove old slaves and arp_ip_targets */
-	remove_bonding_entries (device, "arp_ip_target");
-	remove_bonding_entries (device, "slaves");
-
-	/* Apply config/defaults */
-	for (option = master_options; option->name; option++) {
-		gs_free char *old_value = NULL;
-		char *space;
-
-		value = NULL;
-		if (option_valid_for_nm_setting (s_bond, option))
-			value = nm_setting_bond_get_option_by_name (s_bond, option->name);
-		if (!value)
-			value = option->default_value;
-
-		old_value = nm_platform_master_get_option (ifindex, option->name);
-		/* FIXME: This could be handled in nm-platform. */
-		space = strchr (old_value, ' ');
-		if (space)
-			*space = '\0';
-
-		if (g_strcmp0 (value, old_value)) {
-			if (!nm_platform_master_set_option (ifindex, option->name, value))
-				nm_log_warn (LOGD_HW, "(%s): failed to set bonding attribute "
-				             "'%s' to '%s'", ifname, option->name, value);
-		}
+	const char *value, *def;
+
+	value = nm_setting_bond_get_option_by_name (s_bond, opt);
+	def = nm_setting_bond_get_option_default (s_bond, opt);
+	set_bond_attr (iface, attr, value ? value : def);
+}
+
+static NMActStageReturn
+apply_bonding_config (NMDevice *device)
+{
+	NMConnection *connection;
+	NMSettingBond *s_bond;
+	const char *iface = nm_device_get_ip_iface (device);
+	const char *mode, *value;
+	char *path, *contents;
+	gboolean set_arp_interval = TRUE;
+
+	/* Option restrictions:
+	 *
+	 * arp_interval conflicts miimon > 0
+	 * arp_interval conflicts [ alb, tlb ]
+	 * arp_validate needs [ active-backup ]
+	 * 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
+	 */
+
+	connection = nm_device_get_connection (device);
+	g_assert (connection);
+	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";
+
+	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 (iface, "arp_interval", "0");
+		set_arp_interval = FALSE;
+
+		set_bond_attr (iface, "miimon", value);
+		set_simple_option (iface, "updelay", s_bond, NM_SETTING_BOND_OPTION_UPDELAY);
+		set_simple_option (iface, "downdelay", 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;
+
+		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 (iface, "miimon", "100");
 	}
 
-	/* Handle arp_ip_target */
-	value = nm_setting_bond_get_option_by_name (s_bond, "arp_ip_target");
-	if (value) {
-		char **addresses, **address;
+	/* The stuff after 'mode' requires the given mode or doesn't care */
+	set_bond_attr (iface, "mode", mode);
 
-		addresses = g_strsplit (value, ",", -1);
-		for (address = addresses; *address; address++) {
-			char cmd[20];
+	/* arp_interval not compatible with ALB, TLB */
+	if (g_strcmp0 (mode, "balance-alb") == 0 || g_strcmp0 (mode, "balance-tlb") == 0)
+		set_arp_interval = FALSE;
 
-			snprintf (cmd, sizeof (cmd), "+%s", g_strstrip (*address));
-			if (!nm_platform_master_set_option (ifindex, "arp_ip_target", cmd)){
-				nm_log_warn (LOGD_HW, "(%s): failed to add arp_ip_target '%s'",
-				             ifname, *address);
-			}
-		}
-		g_strfreev (addresses);
+	if (set_arp_interval) {
+		set_simple_option (iface, "arp_interval", s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL);
+
+		/* Just let miimon get cleared automatically; even setting miimon to
+		 * 0 (disabled) clears arp_interval.
+		 */
 	}
 
-	return TRUE;
+	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
+	    && g_strcmp0 (value, "0") != 0
+	    && g_strcmp0 (value, "none") != 0
+	    && g_strcmp0 (mode, "active-backup") == 0)
+		set_bond_attr (iface, "arp_validate", value);
+	else
+		set_bond_attr (iface, "arp_validate", "0");
+
+	if (   g_strcmp0 (mode, "active-backup") == 0
+	    || g_strcmp0 (mode, "balance-alb") == 0
+	    || g_strcmp0 (mode, "balance-tlb") == 0) {
+		value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_PRIMARY);
+		set_bond_attr (iface, "primary", value ? value : "");
+	}
+
+	/* Clear ARP targets */
+	path = g_strdup_printf ("/sys/class/net/%s/bonding/arp_ip_target", iface);
+	if (g_file_get_contents (path, &contents, NULL, NULL)) {
+		set_arp_targets (iface, contents, " \n", "-");
+		g_free (contents);
+	}
+	g_free (path);
+
+	/* Add new ARP targets */
+	value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET);
+	if (value)
+		set_arp_targets (iface, value, ",", "+");
+
+	set_simple_option (iface, "primary_reselect", s_bond, NM_SETTING_BOND_OPTION_PRIMARY_RESELECT);
+	set_simple_option (iface, "fail_over_mac", s_bond, NM_SETTING_BOND_OPTION_FAIL_OVER_MAC);
+	set_simple_option (iface, "use_carrier", s_bond, NM_SETTING_BOND_OPTION_USE_CARRIER);
+	set_simple_option (iface, "ad_select", s_bond, NM_SETTING_BOND_OPTION_AD_SELECT);
+	set_simple_option (iface, "xmit_hash_policy", s_bond, NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY);
+	set_simple_option (iface, "resend_igmp", s_bond, NM_SETTING_BOND_OPTION_RESEND_IGMP);
+
+	return NM_ACT_STAGE_RETURN_SUCCESS;
 }
 
+
 static NMActStageReturn
 act_stage1_prepare (NMDevice *dev, NMDeviceStateReason *reason)
 {
 	NMActStageReturn ret = NM_ACT_STAGE_RETURN_SUCCESS;
-	NMConnection *connection;
-	NMSettingBond *s_bond;
 	gboolean no_firmware = FALSE;
 
 	g_return_val_if_fail (reason != NULL, NM_ACT_STAGE_RETURN_FAILURE);
 
 	ret = NM_DEVICE_CLASS (nm_device_bond_parent_class)->act_stage1_prepare (dev, reason);
-	if (ret == NM_ACT_STAGE_RETURN_SUCCESS) {
-		connection = nm_device_get_connection (dev);
-		g_assert (connection);
-		s_bond = nm_connection_get_setting_bond (connection);
-		g_assert (s_bond);
+	if (ret != NM_ACT_STAGE_RETURN_SUCCESS)
+		return ret;
 
-		/* Interface must be down to set bond options */
-		nm_device_take_down (dev, TRUE);
+	/* Interface must be down to set bond options */
+	nm_device_take_down (dev, TRUE);
+	ret = apply_bonding_config (dev);
+	nm_device_bring_up (dev, TRUE, &no_firmware);
 
-		if (!apply_bonding_config (dev, s_bond))
-			ret = NM_ACT_STAGE_RETURN_FAILURE;
-
-		nm_device_bring_up (dev, TRUE, &no_firmware);
-	}
 	return ret;
 }
 
diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c
index 0957e82a5c..de53e401c9 100644
--- a/src/settings/plugins/ifcfg-rh/reader.c
+++ b/src/settings/plugins/ifcfg-rh/reader.c
@@ -3722,8 +3722,24 @@ handle_bond_option (NMSettingBond *s_bond,
                     const char *key,
                     const char *value)
 {
-	if (!nm_setting_bond_add_option (s_bond, key, value))
+	char *sanitized = NULL, *j;
+	const char *p = value;
+
+	/* Remove any quotes or +/- from arp_ip_target */
+	if (!g_strcmp0 (key, NM_SETTING_BOND_OPTION_ARP_IP_TARGET) && value && value[0]) {
+		if (*p == '\'' || *p == '"')
+			p++;
+		j = sanitized = g_malloc0 (strlen (p) + 1);
+		while (*p) {
+			if (*p != '+' && *p != '-' && *p != '\'' && *p != '"')
+				*j++ = *p;
+			p++;
+		}
+	}
+
+	if (!nm_setting_bond_add_option (s_bond, key, sanitized ? sanitized : value))
 		PLUGIN_WARN (IFCFG_PLUGIN_NAME, "    warning: invalid bonding option '%s'", key);
+	g_free (sanitized);
 }
 
 static NMSetting *