libnm-core: remove unsupported bond options during normalization

In an ideal world, we should not validate connections containing
options not valid for the current bond mode. However adding such
restriction now means that during an upgrade to the new NM version
some connections that were valid before become invalid, possibly
disrupting connectivity.

Instead, consider invalid options as a normalizable error and remove
them during normalization.

Converting the setting to a "canonical" form without invalid options
is important for the connection matching logic, where such invalid
options can cause false mismatches.

3 files changed, 98 insertions, 0 deletions

diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c
index 2b9a05ca19..309df782a9 100644
--- a/libnm-core/nm-connection.c
+++ b/libnm-core/nm-connection.c
@@ -922,6 +922,36 @@ _normalize_bond_mode (NMConnection *self, GHashTable *parameters)
 }
 
 static gboolean
+_normalize_bond_options (NMConnection *self, GHashTable *parameters)
+{
+	NMSettingBond *s_bond = nm_connection_get_setting_bond (self);
+	gboolean changed = FALSE;
+	const char *name, *mode_str;
+	NMBondMode mode;
+	guint32 num, i;
+
+	/* Strip away unsupported options for current mode */
+	if (s_bond) {
+		mode_str = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MODE);
+		mode = _nm_setting_bond_mode_from_string (mode_str);
+		if (mode == NM_BOND_MODE_UNKNOWN)
+			return FALSE;
+again:
+		num = nm_setting_bond_get_num_options (s_bond);
+		for (i = 0; i < num; i++) {
+			if (   nm_setting_bond_get_option (s_bond, i, &name, NULL)
+			    && !_nm_setting_bond_option_supported (name, mode)) {
+				nm_setting_bond_remove_option (s_bond, name);
+				changed = TRUE;
+				goto again;
+			}
+		}
+	}
+
+	return changed;
+}
+
+static gboolean
 _normalize_wireless_mac_address_randomization (NMConnection *self, GHashTable *parameters)
 {
 	NMSettingWireless *s_wifi = nm_connection_get_setting_wireless (self);
@@ -1296,6 +1326,7 @@ nm_connection_normalize (NMConnection *connection,
 	was_modified |= _normalize_ethernet_link_neg (connection);
 	was_modified |= _normalize_infiniband_mtu (connection, parameters);
 	was_modified |= _normalize_bond_mode (connection, parameters);
+	was_modified |= _normalize_bond_options (connection, parameters);
 	was_modified |= _normalize_wireless_mac_address_randomization (connection, parameters);
 	was_modified |= _normalize_team_config (connection, parameters);
 	was_modified |= _normalize_team_port_config (connection, parameters);
diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c
index 50540d72e8..5c58697100 100644
--- a/libnm-core/nm-setting-bond.c
+++ b/libnm-core/nm-setting-bond.c
@@ -557,6 +557,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 	const char *arp_ip_target = NULL;
 	const char *lacp_rate;
 	const char *primary;
+	NMBondMode bond_mode;
 
 	g_hash_table_iter_init (&iter, priv->options);
 	while (g_hash_table_iter_next (&iter, (gpointer) &key, (gpointer) &value)) {
@@ -791,6 +792,23 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 		return NM_SETTING_VERIFY_NORMALIZABLE;
 	}
 
+	/* normalize unsupported options for the current mode */
+	bond_mode = _nm_setting_bond_mode_from_string (mode_new);
+	g_hash_table_iter_init (&iter, priv->options);
+	while (g_hash_table_iter_next (&iter, (gpointer) &key, NULL)) {
+		if (nm_streq (key, "mode"))
+			continue;
+		if (!_nm_setting_bond_option_supported (key, bond_mode)) {
+			g_set_error (error,
+			             NM_CONNECTION_ERROR,
+			             NM_CONNECTION_ERROR_INVALID_PROPERTY,
+			             _("'%s' option is not valid with mode '%s'"),
+			             key, mode_new);
+			g_prefix_error (error, "%s.%s: ", NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS);
+			return NM_SETTING_VERIFY_NORMALIZABLE;
+		}
+	}
+
 	return TRUE;
 }
 
diff --git a/libnm-core/tests/test-setting-bond.c b/libnm-core/tests/test-setting-bond.c
index 91a8199719..e6a65bba64 100644
--- a/libnm-core/tests/test-setting-bond.c
+++ b/libnm-core/tests/test-setting-bond.c
@@ -182,6 +182,54 @@ test_compare (void)
 	                      ((const char *[]){ "num_unsol_na", "4", "num_grat_arp", "4", NULL }));
 }
 
+static void
+test_normalize_options (const char **opts1, const char **opts2)
+{
+	gs_unref_object NMConnection *con = NULL;
+	NMSettingBond *s_bond;
+	GError *error = NULL;
+	gboolean success;
+	const char **p;
+	int num = 0;
+
+	create_bond_connection (&con, &s_bond);
+
+	for (p = opts1; p[0] && p[1]; p += 2)
+		g_assert (nm_setting_bond_add_option (s_bond, p[0], p[1]));
+
+	nmtst_assert_connection_verifies_and_normalizable (con);
+	nmtst_connection_normalize (con);
+	success = nm_setting_verify ((NMSetting *) s_bond, con, &error);
+	nmtst_assert_success (success, error);
+
+	for (p = opts2; p[0] && p[1]; p += 2) {
+		g_assert_cmpstr (nm_setting_bond_get_option_by_name (s_bond, p[0]), ==, p[1]);
+		num++;
+	}
+
+	g_assert_cmpint (num, ==, nm_setting_bond_get_num_options (s_bond));
+}
+
+static void
+test_normalize (void)
+{
+	test_normalize_options (
+		((const char *[]){ "mode", "802.3ad", "ad_actor_system", "00:02:03:04:05:06", NULL }),
+		((const char *[]){ "mode", "802.3ad", "ad_actor_system", "00:02:03:04:05:06", NULL }));
+	test_normalize_options (
+		((const char *[]){ "mode", "1", "miimon", "1", NULL }),
+		((const char *[]){ "mode", "active-backup", "miimon", "1", NULL }));
+	test_normalize_options (
+		((const char *[]){ "mode", "balance-alb", "tlb_dynamic_lb", "1", NULL }),
+		((const char *[]){ "mode", "balance-alb", NULL }));
+	test_normalize_options (
+		((const char *[]){ "mode", "balance-tlb", "tlb_dynamic_lb", "1", NULL }),
+		((const char *[]){ "mode", "balance-tlb", "tlb_dynamic_lb", "1", NULL }));
+	test_normalize_options (
+		((const char *[]){ "mode", "balance-rr", "ad_actor_sys_prio", "4", "packets_per_slave", "3", NULL }),
+		((const char *[]){ "mode", "balance-rr", "packets_per_slave", "3", NULL }));
+}
+
 #define TPATH "/libnm/settings/bond/"
 
 NMTST_DEFINE ();
@@ -193,6 +241,7 @@ main (int argc, char **argv)
 
 	g_test_add_func (TPATH "verify", test_verify);
 	g_test_add_func (TPATH "compare", test_compare);
+	g_test_add_func (TPATH "normalize", test_normalize);
 
 	return g_test_run ();
 }