bond: add proper properties to NMSettingBondth/wip/bondprobsth/bondprobs

Make NMSettingBond have individual properties like other settings
types.

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

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

5 files changed, 177 insertions, 191 deletions

diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c
index 8d9939b096..f09cfbd893 100644
--- a/src/devices/nm-device-bond.c
+++ b/src/devices/nm-device-bond.c
@@ -200,14 +200,24 @@ set_bond_attr (NMDevice *device, const char *attr, const char *value)
 	return ret;
 }
 
+static gboolean
+set_bond_attr_int (NMDevice *device, const char *attr, int value)
+{
+	char buf[20];
+
+	snprintf (buf, sizeof (buf), "%d", value);
+	buf[sizeof (buf) -1] = 0;
+	return set_bond_attr (device, attr, buf);
+}
+
 /* Ignore certain bond options if they are zero (off/disabled) */
 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 (strcmp (option, NM_SETTING_BOND_OPTION_ARP_INTERVAL) &&
+	    strcmp (option, NM_SETTING_BOND_OPTION_MIIMON) &&
+	    strcmp (option, NM_SETTING_BOND_OPTION_DOWNDELAY) &&
+	    strcmp (option, NM_SETTING_BOND_OPTION_UPDELAY))
 		return FALSE;
 
 	return g_strcmp0 (value, "0") == 0 ? TRUE : FALSE;
@@ -219,72 +229,46 @@ update_connection (NMDevice *device, NMConnection *connection)
 	NMSettingBond *s_bond = nm_connection_get_setting_bond (connection);
 	const char *ifname = nm_device_get_iface (device);
 	int ifindex = nm_device_get_ifindex (device);
-	const char **options;
+	const char *const*options;
 
 	if (!s_bond) {
 		s_bond = (NMSettingBond *) nm_setting_bond_new ();
-		nm_connection_add_setting (connection, (NMSetting *) s_bond);
 		g_object_set (s_bond, NM_SETTING_BOND_INTERFACE_NAME, ifname, NULL);
+		nm_connection_add_setting (connection, (NMSetting *) s_bond);
 	}
 
 	/* Read bond options from sysfs and update the Bond setting to match */
-	options = nm_setting_bond_get_valid_options (s_bond);
+	options = nm_setting_bond_get_kernel_names ();
 	while (options && *options) {
 		gs_free char *value = nm_platform_master_get_option (ifindex, *options);
-		const char *defvalue = nm_setting_bond_get_option_default (s_bond, *options);
-
-		if (value && !ignore_if_zero (*options, value) && (g_strcmp0 (value, defvalue) != 0)) {
-			/* Replace " " with "," for arp_ip_targets from the kernel */
-			if (strcmp (*options, "arp_ip_target") == 0) {
-				char *p = value;
-
-				while (p && *p) {
-					if (*p == ' ')
-						*p = ',';
-					p++;
-				}
-			}
 
-			nm_setting_bond_add_option (s_bond, *options, value);
-		}
+		/* FIXME: why this check for ignore_if_zero? */
+		if (value && !ignore_if_zero (*options, value)) {
+			if (!nm_setting_bond_set_string (s_bond, *options, value))
+				nm_log_dbg (LOGD_BOND, "Could not set bonding option %s from sysfs value '%s'", *options, value);
+		} else
+			nm_setting_bond_set_default (s_bond, *options);
 		options++;
 	}
 }
 
 static void
 set_arp_targets (NMDevice *device,
-                 const char *value,
-                 const char *delim,
+                 const char *const *values,
                  const char *prefix)
 {
-	char **items, **iter, *tmp;
+	char *tmp;
 
-	if (!value || !*value)
+	if (!values)
 		return;
 
-	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 (device, "arp_ip_target", tmp);
+	for (; *values; values++) {
+		if (*values[0]) {
+			tmp = g_strconcat (prefix, *values, NULL);
+			set_bond_attr (device, NM_SETTING_BOND_OPTION_ARP_IP_TARGET, tmp);
 			g_free (tmp);
 		}
 	}
-	g_strfreev (items);
-}
-
-static void
-set_simple_option (NMDevice *device,
-                   const char *attr,
-                   NMSettingBond *s_bond,
-                   const char *opt)
-{
-	const char *value;
-
-	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);
 }
 
 static NMActStageReturn
@@ -294,6 +278,7 @@ apply_bonding_config (NMDevice *device)
 	NMSettingBond *s_bond;
 	int ifindex = nm_device_get_ifindex (device);
 	const char *mode, *value;
+	int ivalue;
 	char *contents;
 	gboolean set_arp_interval = TRUE;
 
@@ -315,78 +300,65 @@ 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 = nm_setting_bond_get_mode (s_bond);
+	g_assert (mode && !mode[0]);
 
-	value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MIIMON);
-	if (value && atoi (value)) {
+	ivalue = nm_setting_bond_get_miimon (s_bond);
+	if (ivalue > 0) {
 		/* clear arp interval */
-		set_bond_attr (device, "arp_interval", "0");
+		set_bond_attr (device, 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);
-	} 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 (device, "miimon", "100");
+		set_bond_attr_int (device, NM_SETTING_BOND_OPTION_MIIMON, ivalue);
+		set_bond_attr_int (device, NM_SETTING_BOND_OPTION_UPDELAY, nm_setting_bond_get_updelay (s_bond));
+		set_bond_attr_int (device, NM_SETTING_BOND_OPTION_DOWNDELAY, nm_setting_bond_get_downdelay (s_bond));
 	}
 
 	/* The stuff after 'mode' requires the given mode or doesn't care */
-	set_bond_attr (device, "mode", mode);
+	set_bond_attr (device, NM_SETTING_BOND_OPTION_MODE, mode);
 
 	/* arp_interval not compatible with ALB, TLB */
-	if (g_strcmp0 (mode, "balance-alb") == 0 || g_strcmp0 (mode, "balance-tlb") == 0)
+	if (__NM_SETTING_BOND_MODE_IS_balance_alb (mode) || __NM_SETTING_BOND_MODE_IS_balance_tlb (mode))
 		set_arp_interval = FALSE;
 
 	if (set_arp_interval) {
-		set_simple_option (device, "arp_interval", s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL);
+		set_bond_attr_int (device, NM_SETTING_BOND_OPTION_ARP_INTERVAL, nm_setting_bond_get_arp_interval (s_bond));
 
 		/* 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);
-	/* 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 (device, "arp_validate", value);
-	else
-		set_bond_attr (device, "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 (device, "primary", value ? value : "");
+	value = nm_setting_bond_get_arp_validate (s_bond);
+	set_bond_attr (device, NM_SETTING_BOND_OPTION_ARP_VALIDATE, value);
+
+	if (   __NM_SETTING_BOND_MODE_IS_active_backup (mode)
+	    || __NM_SETTING_BOND_MODE_IS_balance_alb (mode)
+	    || __NM_SETTING_BOND_MODE_IS_balance_tlb (mode)) {
+		value = nm_setting_bond_get_primary (s_bond);
+		set_bond_attr (device, NM_SETTING_BOND_OPTION_PRIMARY, value ? value : "");
 	}
 
 	/* Clear ARP targets */
-	contents = nm_platform_master_get_option (ifindex, "arp_ip_target");
-	set_arp_targets (device, contents, " \n", "-");
+	contents = nm_platform_master_get_option (ifindex, NM_SETTING_BOND_OPTION_ARP_IP_TARGET);
+	if (contents && contents[0]) {
+		char **items;
+
+		items = g_strsplit_set (contents, " \n", 0);
+		set_arp_targets (device, (const char *const*) items, "-");
+		g_strfreev (items);
+	}
 	g_free (contents);
 
 	/* Add new ARP targets */
-	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_arp_targets (device, nm_setting_bond_get_arp_ip_target (s_bond), "+");
+
+	set_bond_attr     (device, NM_SETTING_BOND_OPTION_PRIMARY_RESELECT, nm_setting_bond_get_primary_reselect (s_bond));
+	set_bond_attr     (device, NM_SETTING_BOND_OPTION_FAIL_OVER_MAC,    nm_setting_bond_get_fail_over_mac (s_bond));
+	set_bond_attr_int (device, NM_SETTING_BOND_OPTION_USE_CARRIER,      nm_setting_bond_get_use_carrier (s_bond));
+	set_bond_attr     (device, NM_SETTING_BOND_OPTION_AD_SELECT,        nm_setting_bond_get_ad_select (s_bond));
+	set_bond_attr     (device, NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY, nm_setting_bond_get_xmit_hash_policy (s_bond));
+	set_bond_attr_int (device, NM_SETTING_BOND_OPTION_RESEND_IGMP,      nm_setting_bond_get_resend_igmp (s_bond));
 
 	return NM_ACT_STAGE_RETURN_SUCCESS;
 }
diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c
index e2cff0b2cf..b46daf9ca1 100644
--- a/src/settings/plugins/ifcfg-rh/reader.c
+++ b/src/settings/plugins/ifcfg-rh/reader.c
@@ -63,10 +63,10 @@
 #include "reader.h"
 
 #define PLUGIN_PRINT(pname, fmt, args...) \
-	{ g_message ("   " pname ": " fmt, ##args); }
+    G_STMT_START { g_message ("   " pname ": " fmt, ##args); } G_STMT_END
 
 #define PLUGIN_WARN(pname, fmt, args...) \
-	{ g_warning ("   " pname ": " fmt, ##args); }
+    G_STMT_START { g_warning ("   " pname ": " fmt, ##args); } G_STMT_END
 
 static gboolean
 get_int (const char *str, int *value)
@@ -4059,29 +4059,93 @@ infiniband_connection_from_ifcfg (const char *file,
 	return connection;
 }
 
+typedef void (*IfcfgRhOptFunc) (NMSetting *setting,
+                                const char *key,
+                                const char *value,
+                                gpointer data);
+
+static void
+handle_ifcfg_rh_opts (NMSetting *setting,
+                      const char *value,
+                      IfcfgRhOptFunc func,
+                      gpointer data)
+{
+	char **items, **iter;
+
+	g_return_if_fail (value);
+	if (!value || !*value)
+		return;
+
+	items = g_strsplit_set (value, " ", -1);
+	for (iter = items; *iter; iter++) {
+		if (**iter) {
+			char **keys, *key, *val;
+
+			keys = g_strsplit_set (*iter, "=", 2);
+			if (*keys) {
+				key = keys[0];
+				val = keys[1];
+				if (val && *key && *val)
+					func (setting, key, val, data);
+			}
+
+			g_strfreev (keys);
+		}
+	}
+	g_strfreev (items);
+}
+
 static void
-handle_bond_option (NMSettingBond *s_bond,
+handle_bond_option (NMSetting *setting,
                     const char *key,
-                    const char *value)
+                    const char *value,
+                    gpointer data)
 {
-	char *sanitized = NULL, *j;
-	const char *p = value;
+	NMSettingBond *s_bond = NM_SETTING_BOND (setting);
+	const char *value0 = value;
+	char *value_cleanup = NULL;
 
-	/* 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++;
+	g_return_if_fail (key);
+
+	if (!strcmp (key, NM_SETTING_BOND_OPTION_ARP_IP_TARGET)) {
+		char **arp_ip_target = NULL;
+		char *sanitized = NULL, *j;
+		const char *p = value;
+
+		/* Remove any quotes or +/- from arp_ip_target */
+		if (value[0]) {
+			if (*p == '\'' || *p == '"')
+				p++;
+			j = sanitized = g_malloc0 (strlen (p) + 1);
+			while (*p) {
+				if (*p != '+' && *p != '-' && *p != '\'' && *p != '"')
+					*j++ = *p;
+				p++;
+			}
+			value = sanitized;
 		}
+
+		if (value && value[0]) {
+			arp_ip_target = g_strsplit (value, ",", -1);
+			/* don't set the GObject property to arp_ip_target, because we want to reuse the
+			 * validation from nm_setting_bond_set_string. */
+			value_cleanup = g_strjoinv (",", arp_ip_target);
+			value = value_cleanup;
+			g_strfreev (arp_ip_target);
+		} else
+			value = NULL;
+		g_free (sanitized);
 	}
 
-	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);
+	if (!nm_setting_bond_set_string (s_bond, key, value)) {
+		if (!nm_setting_bond_get_property_name (key))
+			PLUGIN_WARN (IFCFG_PLUGIN_NAME, "    warning: unrecognized bond property '%s'", key);
+		else
+			PLUGIN_WARN (IFCFG_PLUGIN_NAME, "    warning: invalid bond property '%s'='%s'", key, value0);
+		return;
+	}
+
+	g_free (value_cleanup);
 }
 
 static NMSetting *
@@ -4105,26 +4169,8 @@ make_bond_setting (shvarFile *ifcfg,
 
 	value = svGetValue (ifcfg, "BONDING_OPTS", FALSE);
 	if (value) {
-		char **items, **iter;
-
-		items = g_strsplit_set (value, " ", -1);
-		for (iter = items; iter && *iter; iter++) {
-			if (strlen (*iter)) {
-				char **keys, *key, *val;
-
-				keys = g_strsplit_set (*iter, "=", 2);
-				if (keys && *keys) {
-					key = *keys;
-					val = *(keys + 1);
-					if (val && strlen(key) && strlen(val))
-						handle_bond_option (s_bond, key, val);
-				}
-
-				g_strfreev (keys);
-			}
-		}
+		handle_ifcfg_rh_opts (NM_SETTING (s_bond), value, handle_bond_option, NULL);
 		g_free (value);
-		g_strfreev (items);
 	}
 
 	return (NMSetting *) s_bond;
@@ -4266,17 +4312,13 @@ team_connection_from_ifcfg (const char *file,
 	return connection;
 }
 
-typedef void (*BridgeOptFunc) (NMSetting *setting,
-                               gboolean stp,
-                               const char *key,
-                               const char *value);
-
 static void
 handle_bridge_option (NMSetting *setting,
-                      gboolean stp,
                       const char *key,
-                      const char *value)
+                      const char *value,
+                      gpointer data)
 {
+	gboolean stp = GPOINTER_TO_INT (data);
 	guint32 u = 0;
 
 	if (!strcmp (key, "priority")) {
@@ -4309,33 +4351,6 @@ handle_bridge_option (NMSetting *setting,
 			PLUGIN_WARN (IFCFG_PLUGIN_NAME, "    warning: unhandled bridge option '%s'", key);
 }
 
-static void
-handle_bridging_opts (NMSetting *setting,
-                      gboolean stp,
-                      const char *value,
-                      BridgeOptFunc func)
-{
-	char **items, **iter;
-
-	items = g_strsplit_set (value, " ", -1);
-	for (iter = items; iter && *iter; iter++) {
-		if (strlen (*iter)) {
-			char **keys, *key, *val;
-
-			keys = g_strsplit_set (*iter, "=", 2);
-			if (keys && *keys) {
-				key = *keys;
-				val = *(keys + 1);
-				if (val && strlen(key) && strlen(val))
-					func (setting, stp, key, val);
-			}
-
-			g_strfreev (keys);
-		}
-	}
-	g_strfreev (items);
-}
-
 static NMSetting *
 make_bridge_setting (shvarFile *ifcfg,
                      const char *file,
@@ -4391,7 +4406,8 @@ make_bridge_setting (shvarFile *ifcfg,
 
 	value = svGetValue (ifcfg, "BRIDGING_OPTS", FALSE);
 	if (value) {
-		handle_bridging_opts (NM_SETTING (s_bridge), stp, value, handle_bridge_option);
+		handle_ifcfg_rh_opts (NM_SETTING (s_bridge), value, handle_bridge_option,
+		                      GINT_TO_POINTER (stp));
 		g_free (value);
 	}
 
@@ -4430,7 +4446,7 @@ bridge_connection_from_ifcfg (const char *file,
 		g_object_unref (connection);
 		return NULL;
 	}
-	nm_connection_add_setting (connection, bridge_setting);	
+	nm_connection_add_setting (connection, bridge_setting);
 
 	if (!nm_connection_verify (connection, error)) {
 		g_object_unref (connection);
@@ -4442,9 +4458,9 @@ bridge_connection_from_ifcfg (const char *file,
 
 static void
 handle_bridge_port_option (NMSetting *setting,
-                           gboolean stp,
                            const char *key,
-                           const char *value)
+                           const char *value,
+                           gpointer data)
 {
 	guint32 u = 0;
 
@@ -4484,7 +4500,8 @@ make_bridge_port_setting (shvarFile *ifcfg)
 		s_port = nm_setting_bridge_port_new ();
 		value = svGetValue (ifcfg, "BRIDGING_OPTS", FALSE);
 		if (value)
-			handle_bridging_opts (s_port, FALSE, value, handle_bridge_port_option);
+			handle_ifcfg_rh_opts (NM_SETTING (s_port), value, handle_bridge_port_option, NULL);
+
 		g_free (value);
 	}
 
@@ -4515,7 +4532,7 @@ is_bond_device (const char *name, shvarFile *parsed)
 
 	if (svTrueValue (parsed, "BONDING_MASTER", FALSE))
 		return TRUE;
-	
+
 	/* XXX: Check for "bond[\d]+"? */
 
 	return FALSE;
diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
index 14c4fb9f72..bc8c9740b9 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -12101,9 +12101,9 @@ test_read_bond_main (void)
 	        "bond-main", "failed to verify %s: DEVICE=%s does not match bond0",
 	        TEST_IFCFG_BOND_MAIN, nm_setting_bond_get_interface_name (s_bond));
 
-	ASSERT (g_strcmp0 (nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MIIMON), "100") == 0,
-	        "bond-main", "failed to verify %s: miimon=%s does not match 100",
-	        TEST_IFCFG_BOND_MAIN, nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MIIMON));
+	ASSERT (nm_setting_bond_get_miimon (s_bond) == 100,
+	        "bond-main", "failed to verify %s: miimon=%u does not match 100",
+	        TEST_IFCFG_BOND_MAIN, nm_setting_bond_get_miimon (s_bond));
 
 	g_free (unmanaged);
 	g_free (keyfile);
diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c
index 6f302b4f7c..c83ee0a872 100644
--- a/src/settings/plugins/ifcfg-rh/writer.c
+++ b/src/settings/plugins/ifcfg-rh/writer.c
@@ -1250,7 +1250,8 @@ write_bonding_setting (NMConnection *connection, shvarFile *ifcfg, GError **erro
 {
 	NMSettingBond *s_bond;
 	const char *iface;
-	guint32 i, num_opts;
+	const char *const* kernel_names;
+	GString *str;
 
 	s_bond = nm_connection_get_setting_bond (connection);
 	if (!s_bond) {
@@ -1268,25 +1269,21 @@ write_bonding_setting (NMConnection *connection, shvarFile *ifcfg, GError **erro
 	svSetValue (ifcfg, "DEVICE", iface, FALSE);
 	svSetValue (ifcfg, "BONDING_OPTS", NULL, FALSE);
 
-	num_opts = nm_setting_bond_get_num_options (s_bond);
-	if (num_opts > 0) {
-		GString *str = g_string_sized_new (64);
-
-		for (i = 0; i < nm_setting_bond_get_num_options (s_bond); i++) {
-			const char *key, *value;
+	str = g_string_sized_new (64);
 
-			if (!nm_setting_bond_get_option (s_bond, i, &key, &value))
-				continue;
+	kernel_names = nm_setting_bond_get_kernel_names();
+	for (; *kernel_names; kernel_names++) {
+		if (nm_setting_bond_is_default (s_bond, *kernel_names, NULL)) {
+			const char *strval  = nm_setting_bond_get_string (s_bond, *kernel_names);
 
-			if (str->len)
-				g_string_append_c (str, ' ');
-
-			g_string_append_printf (str, "%s=%s", key, value);
+			/* FIXME: how to represent NULL values that are not default values? */
+			g_string_append_printf (str, "%s=%s ", *kernel_names, strval ? strval : "");
 		}
+	}
+	if (str->len > 0) {
+		g_string_truncate (str, str->len-1);
 
-		if (str->len)
-			svSetValue (ifcfg, "BONDING_OPTS", str->str, FALSE);
-
+		svSetValue (ifcfg, "BONDING_OPTS", str->str, FALSE);
 		g_string_free (str, TRUE);
 	}
 
diff --git a/src/settings/plugins/keyfile/reader.c b/src/settings/plugins/keyfile/reader.c
index 6949d1e5b1..d96a98dfbc 100644
--- a/src/settings/plugins/keyfile/reader.c
+++ b/src/settings/plugins/keyfile/reader.c
@@ -581,7 +581,7 @@ read_hash_of_string (GKeyFile *file, NMSetting *setting, const char *key)
 		}
 		if (NM_IS_SETTING_BOND (setting)) {
 			if (strcmp (*iter, NM_SETTING_BOND_INTERFACE_NAME))
-				nm_setting_bond_add_option (NM_SETTING_BOND (setting), *iter, value);
+				nm_setting_bond_set_string (NM_SETTING_BOND (setting), *iter, value);
 		}
 		g_free (value);
 	}