ifcfg-rh: change the handling of REORDER_HDR flag

On NM 1.0 connections were created by default without the REORDER_HDR
flag, but then due to a bug in platform code (fixed in [1]), the
kernel interface always had the flag set.

Now that the setting is honored, users upgrading to the new version of
NM will see a change from the previous behavior, since interfaces will
not have REORDER_HDR and this will certainly break functionality.

The only solution here seems to be to ignore the REORDER_HDR variable
in ifcfg files (since it never had any effect) and introduce a new
NO_REORDER_HDR option for the VLAN_FLAGS variable which allows to turn
the flag off. The consequence is that the flag will be set for all old
connections.

This change introduces an incompatibility with initscripts, however is
necessary to avoid breaking user functionality upon upgrade.
Connections created through NetworkManager will still be parsed
correctly by initscripts (since we always write the REORDER_HDR
variable).

[1] db62fc9d72fa ("platform: fix adding VLAN flags")

https://bugzilla.gnome.org/show_bug.cgi?id=762626

9 files changed, 79 insertions, 27 deletions

diff --git a/libnm-core/nm-setting-vlan.c b/libnm-core/nm-setting-vlan.c
index 90798c40d3..39587d59f9 100644
--- a/libnm-core/nm-setting-vlan.c
+++ b/libnm-core/nm-setting-vlan.c
@@ -893,8 +893,8 @@ nm_setting_vlan_class_init (NMSettingVlanClass *setting_class)
 	 **/
 	/* ---ifcfg-rh---
 	 * property: flags
-	 * variable: REORDER_HDR, GVRP, MVRP, VLAN_FLAGS
-	 * values: "yes or "no" for REORDER_HDR, GVRP and MVRP; "LOOSE_BINDING" for VLAN_FLAGS
+	 * variable: GVRP, MVRP, VLAN_FLAGS
+	 * values: "yes or "no" for GVRP and MVRP; "LOOSE_BINDING" and "NO_REORDER_HDR" for VLAN_FLAGS
 	 * description: VLAN flags.
 	 * ---end---
 	 */
diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c
index c8871a55a8..ca512fc6c2 100644
--- a/src/settings/plugins/ifcfg-rh/reader.c
+++ b/src/settings/plugins/ifcfg-rh/reader.c
@@ -4647,7 +4647,7 @@ make_vlan_setting (shvarFile *ifcfg,
 	char *end = NULL;
 	gint vlan_id = -1;
 	guint32 vlan_flags = 0;
-	gint gvrp;
+	gint gvrp, reorder_hdr;
 
 	value = svGetValue (ifcfg, "VLAN_ID", FALSE);
 	if (value) {
@@ -4722,8 +4722,7 @@ make_vlan_setting (shvarFile *ifcfg,
 	g_object_set (s_vlan, NM_SETTING_VLAN_PARENT, parent, NULL);
 	g_clear_pointer (&parent, g_free);
 
-	if (svGetValueBoolean (ifcfg, "REORDER_HDR", FALSE))
-		vlan_flags |= NM_VLAN_FLAG_REORDER_HEADERS;
+	vlan_flags |= NM_VLAN_FLAG_REORDER_HEADERS;
 
 	gvrp = svGetValueBoolean (ifcfg, "GVRP", -1);
 	if (gvrp > 0)
@@ -4731,13 +4730,26 @@ make_vlan_setting (shvarFile *ifcfg,
 
 	value = svGetValue (ifcfg, "VLAN_FLAGS", FALSE);
 	if (value) {
-		/* Prefer GVRP variable; only take VLAN_FLAG=GVRP when GVRP is not specified */
-		if (g_strstr_len (value, -1, "GVRP") && gvrp == -1)
-			vlan_flags |= NM_VLAN_FLAG_GVRP;
-		if (g_strstr_len (value, -1, "LOOSE_BINDING"))
-			vlan_flags |= NM_VLAN_FLAG_LOOSE_BINDING;
+		gs_strfreev char **strv = NULL;
+		char **ptr;
+
+		strv = g_strsplit_set (value, ", ", 0);
+
+		for (ptr = strv; ptr && *ptr; ptr++) {
+			if (nm_streq (*ptr, "GVRP") && gvrp == -1)
+				vlan_flags |= NM_VLAN_FLAG_GVRP;
+			if (nm_streq (*ptr, "LOOSE_BINDING"))
+				vlan_flags |=  NM_VLAN_FLAG_LOOSE_BINDING;
+			if (nm_streq (*ptr, "NO_REORDER_HDR"))
+				vlan_flags &= ~NM_VLAN_FLAG_REORDER_HEADERS;
+		}
 	}
 
+	reorder_hdr = svGetValueBoolean (ifcfg, "REORDER_HDR", -1);
+	if (   reorder_hdr != -1
+	    && reorder_hdr != NM_FLAGS_HAS (vlan_flags, NM_VLAN_FLAG_REORDER_HEADERS))
+		PARSE_WARNING ("REORDER_HDR key is deprecated, use VLAN_FLAGS");
+
 	if (svGetValueBoolean (ifcfg, "MVRP", FALSE))
 		vlan_flags |= NM_VLAN_FLAG_MVRP;
 
diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/Makefile.am b/src/settings/plugins/ifcfg-rh/tests/network-scripts/Makefile.am
index ab8579762a..36e52af4d1 100644
--- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/Makefile.am
+++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/Makefile.am
@@ -107,6 +107,7 @@ EXTRA_DIST = \
 	ifcfg-test-vlan-only-device \
 	ifcfg-test-vlan-physdev \
 	ifcfg-test-vlan-reorder-hdr-1 \
+	ifcfg-test-vlan-reorder-hdr-2 \
 	ifcfg-test-vlan-flags-1 \
 	ifcfg-test-vlan-flags-2 \
 	ifcfg-test-wifi-wep-no-keys \
diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-flags-2 b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-flags-2
index 3b536a6628..2c01be71fc 100644
--- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-flags-2
+++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-flags-2
@@ -3,7 +3,6 @@ TYPE=Vlan
 DEVICE=super-vlan
 VLAN_ID=44
 PHYSDEV=eth9
-REORDER_HDR=no
 VLAN_FLAGS="GVRP LOOSE_BINDING"
 ONBOOT=yes
 BOOTPROTO=static
diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-interface b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-interface
index ccd75d7ffb..d8d9193fe4 100644
--- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-interface
+++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-interface
@@ -2,7 +2,6 @@ VLAN=yes
 TYPE=Vlan
 DEVICE=vlan43
 PHYSDEV=eth9
-REORDER_HDR=0
 VLAN_FLAGS=GVRP,LOOSE_BINDING
 VLAN_INGRESS_PRIORITY_MAP=0:1,2:5
 VLAN_EGRESS_PRIORITY_MAP=12:3,14:7,3:1
diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-reorder-hdr-1 b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-reorder-hdr-1
index ca38f839b1..0dc539712b 100644
--- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-reorder-hdr-1
+++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-reorder-hdr-1
@@ -3,5 +3,4 @@ TYPE=Vlan
 DEVICE=vlan0.3
 PHYSDEV=eth0
 VLAN_ID=3
-REORDER_HDR=1
-
+REORDER_HDR=0
diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-reorder-hdr-2 b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-reorder-hdr-2
new file mode 100644
index 0000000000..d98a9d364d
--- /dev/null
+++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-reorder-hdr-2
@@ -0,0 +1,6 @@
+VLAN=yes
+TYPE=Vlan
+DEVICE=vlan0.3
+PHYSDEV=eth0
+VLAN_ID=3
+VLAN_FLAGS="LOOSE_BINDING,NO_REORDER_HDR"
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 9f663c9c87..8ca5c8bbe0 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -7230,7 +7230,7 @@ test_read_vlan_interface (void)
 	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth9");
 	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 43);
 	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==,
-	                 NM_VLAN_FLAG_GVRP | NM_VLAN_FLAG_LOOSE_BINDING);
+	                 NM_VLAN_FLAG_GVRP | NM_VLAN_FLAG_LOOSE_BINDING | NM_VLAN_FLAG_REORDER_HEADERS);
 
 	/* Ingress map */
 	g_assert_cmpint (nm_setting_vlan_get_num_priorities (s_vlan, NM_VLAN_INGRESS_MAP), ==, 2);
@@ -7278,8 +7278,7 @@ test_read_vlan_only_vlan_id (void)
 
 	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth9");
 	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 43);
-	/* Ensure that flags are 0 if both REORDER_HDR and VLAN_FLAGS are missing */
-	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, 0);
+	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, NM_VLAN_FLAG_REORDER_HEADERS);
 
 	g_object_unref (connection);
 }
@@ -7330,8 +7329,33 @@ test_read_vlan_reorder_hdr_1 (void)
 	NMConnection *connection;
 	NMSettingVlan *s_vlan;
 
+	g_test_expect_message ("NetworkManager", G_LOG_LEVEL_WARNING,
+	                       "*REORDER_HDR key is deprecated, use VLAN_FLAGS*");
 	connection = _connection_from_file (TEST_IFCFG_DIR"/network-scripts/ifcfg-test-vlan-reorder-hdr-1",
 	                                        NULL, TYPE_ETHERNET, NULL);
+	g_test_assert_expected_messages ();
+
+	g_assert_cmpstr (nm_connection_get_interface_name (connection), ==, "vlan0.3");
+
+	s_vlan = nm_connection_get_setting_vlan (connection);
+	g_assert (s_vlan);
+
+	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth0");
+	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 3);
+	/* Check that REORDER_HDR=0 is ignored */
+	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, NM_VLAN_FLAG_REORDER_HEADERS);
+
+	g_object_unref (connection);
+}
+
+static void
+test_read_vlan_reorder_hdr_2 (void)
+{
+	NMConnection *connection;
+	NMSettingVlan *s_vlan;
+
+	connection = _connection_from_file (TEST_IFCFG_DIR"/network-scripts/ifcfg-test-vlan-reorder-hdr-2",
+	                                    NULL, TYPE_ETHERNET, NULL);
 
 	g_assert_cmpstr (nm_connection_get_interface_name (connection), ==, "vlan0.3");
 
@@ -7340,8 +7364,8 @@ test_read_vlan_reorder_hdr_1 (void)
 
 	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth0");
 	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 3);
-	/* Check correct read of REORDER_HDR=1 */
-	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, 1);
+	/* Check that VLAN_FLAGS=NO_REORDER_HDR works */
+	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, NM_VLAN_FLAG_LOOSE_BINDING);
 
 	g_object_unref (connection);
 }
@@ -7362,8 +7386,9 @@ test_read_vlan_flags_1 (void)
 
 	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth9");
 	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 44);
-	/* reorder_hdr and loose_binding */
-	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, 5);
+	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==,
+	                                            NM_VLAN_FLAG_LOOSE_BINDING |
+	                                            NM_VLAN_FLAG_REORDER_HEADERS);
 
 	g_object_unref (connection);
 }
@@ -7384,8 +7409,10 @@ test_read_vlan_flags_2 (void)
 
 	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth9");
 	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 44);
-	/* gvrp and loose_binding */
-	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, 6);
+	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==,
+	                                            NM_VLAN_FLAG_GVRP |
+	                                            NM_VLAN_FLAG_LOOSE_BINDING |
+	                                            NM_VLAN_FLAG_REORDER_HEADERS);
 
 	g_object_unref (connection);
 }
@@ -8706,7 +8733,7 @@ test_read_vlan_trailing_spaces (void)
 	g_assert_cmpstr (nm_connection_get_interface_name (connection), ==, "vlan201");
 	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "enccw0.0.fb00");
 	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 201);
-	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, 0);
+	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, NM_VLAN_FLAG_REORDER_HEADERS);
 
 	g_object_unref (connection);
 }
@@ -8836,6 +8863,7 @@ int main (int argc, char **argv)
 	g_test_add_func (TPATH "vlan/read/only-device", test_read_vlan_only_device);
 	g_test_add_func (TPATH "vlan/read/physdev", test_read_vlan_physdev);
 	g_test_add_func (TPATH "vlan/read/reorder-hdr-1", test_read_vlan_reorder_hdr_1);
+	g_test_add_func (TPATH "vlan/read/reorder-hdr-2", test_read_vlan_reorder_hdr_2);
 	g_test_add_func (TPATH "wired/read/read-wake-on-lan", test_read_wired_wake_on_lan);
 
 	g_test_add_func (TPATH "wired/write/static", test_write_wired_static);
diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c
index 53e2f56b22..e7d64379da 100644
--- a/src/settings/plugins/ifcfg-rh/writer.c
+++ b/src/settings/plugins/ifcfg-rh/writer.c
@@ -45,6 +45,7 @@
 #include "nm-core-internal.h"
 #include "nm-utils.h"
 #include "nm-core-internal.h"
+#include "NetworkManagerUtils.h"
 
 #include "common.h"
 #include "shvar.h"
@@ -1260,6 +1261,8 @@ write_vlan_setting (NMConnection *connection, shvarFile *ifcfg, gboolean *wired,
 	NMSettingConnection *s_con;
 	char *tmp;
 	guint32 vlan_flags = 0;
+	gsize s_buf_len;
+	char s_buf[50], *s_buf_ptr;
 
 	s_con = nm_connection_get_setting_connection (connection);
 	if (!s_con) {
@@ -1292,9 +1295,14 @@ write_vlan_setting (NMConnection *connection, shvarFile *ifcfg, gboolean *wired,
 
 	svSetValue (ifcfg, "GVRP", vlan_flags & NM_VLAN_FLAG_GVRP ? "yes" : "no", FALSE);
 
-	svSetValue (ifcfg, "VLAN_FLAGS", NULL, FALSE);
-	if (vlan_flags & NM_VLAN_FLAG_LOOSE_BINDING)
-		svSetValue (ifcfg, "VLAN_FLAGS", "LOOSE_BINDING", FALSE);
+	nm_utils_strbuf_init (s_buf, &s_buf_ptr, &s_buf_len);
+
+	if (NM_FLAGS_HAS (vlan_flags, NM_VLAN_FLAG_LOOSE_BINDING))
+		nm_utils_strbuf_append_str (&s_buf_ptr, &s_buf_len, "LOOSE_BINDING");
+	if (!NM_FLAGS_HAS (vlan_flags, NM_VLAN_FLAG_REORDER_HEADERS))
+		nm_utils_strbuf_append (&s_buf_ptr, &s_buf_len, "%sNO_REORDER_HDR", s_buf[0] ? "," : "");
+
+	svSetValue (ifcfg, "VLAN_FLAGS", s_buf, FALSE);
 
 	svSetValue (ifcfg, "MVRP", vlan_flags & NM_VLAN_FLAG_MVRP ? "yes" : "no", FALSE);