bond: small cleanupsac/bond_mode_fix

* Use an enum instead of a string, is faster for comparisons.
* Add debug assertions
* Have NMBondMode enum correspond to Kernel numbering

3 files changed, 42 insertions, 29 deletions

diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h
index babf7dfd40..ef9e982bf6 100644
--- a/libnm-core/nm-core-internal.h
+++ b/libnm-core/nm-core-internal.h
@@ -521,14 +521,17 @@ NMConnectionMultiConnect _nm_connection_get_multi_connect (NMConnection *connect
 /*****************************************************************************/
 
 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,
+	NM_BOND_MODE_UNKNOWN      = -1,
+
+	/* The numeric values correspond to kernel's numbering of the modes. */
+	NM_BOND_MODE_ROUNDROBIN   = 0,
+	NM_BOND_MODE_ACTIVEBACKUP = 1,
+	NM_BOND_MODE_XOR          = 2,
+	NM_BOND_MODE_BROADCAST    = 3,
+	NM_BOND_MODE_8023AD       = 4,
+	NM_BOND_MODE_TLB          = 5,
+	NM_BOND_MODE_ALB          = 6,
+	_NM_BOND_MODE_NUM,
 } NMBondMode;
 
 NMBondMode _nm_setting_bond_mode_from_string (const char *str);
diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c
index 3a6fddf355..8986e434e5 100644
--- a/libnm-core/nm-setting-bond.c
+++ b/libnm-core/nm-setting-bond.c
@@ -45,6 +45,7 @@ G_DEFINE_TYPE (NMSettingBond, nm_setting_bond, NM_TYPE_SETTING)
 /*****************************************************************************/
 
 static const char *const valid_options_lst[] = {
+	/* mode must be the first element. nm-device-bond.c relies on that. */
 	NM_SETTING_BOND_OPTION_MODE,
 	NM_SETTING_BOND_OPTION_MIIMON,
 	NM_SETTING_BOND_OPTION_DOWNDELAY,
@@ -145,6 +146,7 @@ NM_UTILS_STRING_TABLE_LOOKUP_STRUCT_DEFINE (
 			nm_assert (G_N_ELEMENTS (LIST) == NM_PTRARRAY_LEN (valid_options_lst));
 			for (i = 0; i < G_N_ELEMENTS (LIST); i++)
 				_nm_assert_bond_meta (&LIST[i].value);
+			nm_assert (nm_streq (valid_options_lst[0], NM_SETTING_BOND_OPTION_MODE));
 		}
 	},
 	{ return NULL; },
@@ -204,6 +206,7 @@ gboolean
 _nm_setting_bond_option_supported (const char *option, NMBondMode mode)
 {
 	nm_assert (option);
+	nm_assert (mode != NM_BOND_MODE_UNKNOWN);
 	nm_assert (_NM_INT_NOT_NEGATIVE (mode) && mode < 32);
 
 	return !NM_FLAGS_ANY (_bond_option_unsupp_mode (option), BIT (mode));
@@ -728,13 +731,11 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 {
 	NMSettingBond *self = NM_SETTING_BOND (setting);
 	NMSettingBondPrivate *priv = NM_SETTING_BOND_GET_PRIVATE (setting);
-	int mode;
 	int miimon;
 	int arp_interval;
 	int num_grat_arp;
 	int num_unsol_na;
-	const char *mode_orig;
-	const char *mode_new;
+	const char *mode_str;
 	const char *arp_ip_target = NULL;
 	const char *lacp_rate;
 	const char *primary;
@@ -780,8 +781,8 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 	 */
 
 	/* Verify bond mode */
-	mode_orig = _bond_get_option (self, NM_SETTING_BOND_OPTION_MODE);
-	if (!mode_orig) {
+	mode_str = _bond_get_option (self, NM_SETTING_BOND_OPTION_MODE);
+	if (!mode_str) {
 		g_set_error (error,
 		             NM_CONNECTION_ERROR,
 		             NM_CONNECTION_ERROR_INVALID_PROPERTY,
@@ -790,13 +791,13 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 		g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS);
 		return FALSE;
 	}
-	mode = nm_utils_bond_mode_string_to_int (mode_orig);
-	if (mode == -1) {
+	bond_mode = _nm_setting_bond_mode_from_string (mode_str);
+	if (bond_mode == NM_BOND_MODE_UNKNOWN) {
 		g_set_error (error,
 		             NM_CONNECTION_ERROR,
 		             NM_CONNECTION_ERROR_INVALID_PROPERTY,
 		             _("'%s' is not a valid value for '%s'"),
-		             mode_orig,
+		             mode_str,
 		             NM_SETTING_BOND_OPTION_MODE);
 		g_prefix_error (error,
 		                "%s.%s: ",
@@ -804,18 +805,17 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 		                NM_SETTING_BOND_OPTIONS);
 		return FALSE;
 	}
-	mode_new = nm_utils_bond_mode_int_to_string (mode);
 
 	/* Make sure mode is compatible with other settings */
-	if (NM_IN_STRSET (mode_new, "balance-alb",
-	                            "balance-tlb")) {
+	if (NM_IN_SET (bond_mode, NM_BOND_MODE_TLB,
+	                          NM_BOND_MODE_ALB)) {
 		if (arp_interval > 0) {
 			g_set_error (error,
 			             NM_CONNECTION_ERROR,
 			             NM_CONNECTION_ERROR_INVALID_PROPERTY,
 			             _("'%s=%s' is incompatible with '%s > 0'"),
 			             NM_SETTING_BOND_OPTION_MODE,
-			             mode_new,
+			             mode_str,
 			             NM_SETTING_BOND_OPTION_ARP_INTERVAL);
 			g_prefix_error (error,
 			                "%s.%s: ",
@@ -826,7 +826,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 	}
 
 	primary = _bond_get_option (self, NM_SETTING_BOND_OPTION_PRIMARY);
-	if (NM_IN_STRSET (mode_new, "active-backup")) {
+	if (bond_mode == NM_BOND_MODE_ACTIVEBACKUP) {
 		GError *tmp_error = NULL;
 
 		if (primary && !nm_utils_ifname_valid_kernel (primary, &tmp_error)) {
@@ -855,12 +855,12 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 
 	if (   connection
 	    && nm_connection_get_setting_infiniband (connection)) {
-		if (!NM_IN_STRSET (mode_new, "active-backup")) {
+		if (bond_mode != NM_BOND_MODE_ACTIVEBACKUP) {
 			g_set_error (error,
 			             NM_CONNECTION_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);
+			             NM_SETTING_BOND_OPTION_MODE, mode_str, NM_SETTING_INFINIBAND_SETTING_NAME);
 			g_prefix_error (error,
 			                "%s.%s: ",
 			                NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS);
@@ -967,7 +967,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 
 	lacp_rate = _bond_get_option (self, NM_SETTING_BOND_OPTION_LACP_RATE);
 	if (   lacp_rate
-	    && !nm_streq0 (mode_new, "802.3ad")
+	    && bond_mode != NM_BOND_MODE_8023AD
 	    && !NM_IN_STRSET (lacp_rate, "0", "slow")) {
 		g_set_error (error,
 		             NM_CONNECTION_ERROR,
@@ -996,7 +996,14 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 
 	/* *** errors above here should be always fatal, below NORMALIZABLE_ERROR *** */
 
-	if (!nm_streq0 (mode_orig, mode_new)) {
+	if (!NM_IN_STRSET (mode_str,
+	                   "802.3ad",
+	                   "active-backup",
+	                   "balance-rr",
+	                   "balance-alb",
+	                   "balance-tlb",
+	                   "balance-xor",
+	                   "broadcast")) {
 		g_set_error (error,
 		             NM_CONNECTION_ERROR,
 		             NM_CONNECTION_ERROR_INVALID_PROPERTY,
@@ -1007,7 +1014,6 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 	}
 
 	/* normalize unsupported options for the current mode */
-	bond_mode = _nm_setting_bond_mode_from_string (mode_new);
 	for (i = 0; priv->options_idx_cache[i].name; i++) {
 		n = &priv->options_idx_cache[i];
 		if (!_nm_setting_bond_option_supported (n->name, bond_mode)) {
@@ -1015,7 +1021,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 			             NM_CONNECTION_ERROR,
 			             NM_CONNECTION_ERROR_INVALID_PROPERTY,
 			             _("'%s' option is not valid with mode '%s'"),
-			             n->name, mode_new);
+			             n->name, mode_str);
 			g_prefix_error (error,
 			                "%s.%s: ",
 			                NM_SETTING_BOND_SETTING_NAME,
diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c
index a038a6ffc6..c15605ce16 100644
--- a/src/devices/nm-device-bond.c
+++ b/src/devices/nm-device-bond.c
@@ -131,8 +131,12 @@ update_connection (NMDevice *device, NMConnection *connection)
 				*p = '\0';
 		}
 
-		if (value && nm_streq (*options, NM_SETTING_BOND_OPTION_MODE))
-			mode = _nm_setting_bond_mode_from_string (value);
+		if (mode == NM_BOND_MODE_UNKNOWN) {
+			if (value && nm_streq (*options, NM_SETTING_BOND_OPTION_MODE))
+				mode = _nm_setting_bond_mode_from_string (value);
+			if (mode == NM_BOND_MODE_UNKNOWN)
+				continue;
+		}
 
 		if (!_nm_setting_bond_option_supported (*options, mode))
 			continue;