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

diff --git a/libnm-core/nm-setting-vlan.c b/libnm-core/nm-setting-vlan.c
index 90798c4..39587d5 100644 (file)
--- 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 c8871a5..ca512fc 100644 (file)
--- 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 ab85797..36e52af 100644 (file)
--- 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 3b536a6..2c01be7 100644 (file)
--- 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 ccd75d7..d8d9193 100644 (file)
--- 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 ca38f83..0dc5397 100644 (file)
--- 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 (file)
index 0000000..d98a9d3
--- /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 9f663c9..8ca5c8b 100644 (file)
--- 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 53e2f56..e7d6437 100644 (file)
--- 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);