ifcfg-rh: always honor "$VLAN_ID" in ifcfg filesth/ifcfg-vlan-id-use

initscripts don't support "$VLAN_ID". They actually support "$VID",
which we don't.

"$VLAN_ID" was introduced by commit 10b32be37b52 ('ifcfg-rh: various VLAN
cleanups'). It has a misguided attempt for "backward compatibility",
where the reader would ignore "$VLAN_ID" if "$DEVICE"'s name contains
a suffix that is parsable as VLAN ID.

That is wrong. If a new feature gets introduce (like NetworkManager
supporting "$VLAN_ID"), then there is no way that an older version of the
tool which doesn't know the new feature yet (initscripts) supports it.
This is not what backward compatibility means. Backward compatibility
means that if a user has an old ifcfg-file without "$VLAN_ID", or an old
version of the tooling writes such a file, we continue to support it.

Consider, when a user (or NetworkManager) writes a configuration

  DEVICE=vlan9
  PHYSDEV=eth0
  VLAN_ID=10

then it makes no sense to ignore VLAN_ID=10 and use "9" instead.
Otherwise the user (or NetworkManager) should not have written the
file this way.

Also, NetworkManager profiles support "connection.interface-name=vlan0.3"
together with "vlan.id=10". Such a configuration is valid and must be
serializable to ifcfg-rh format. The ifcfg-rh writer code did not somehow
restrict the setting of "$VLAN_ID" to account for this odd behavior. Whenever
NetworkManager in the past wrote VLAN_ID variable to file, it really meant it.

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

6 files changed, 67 insertions, 11 deletions

diff --git a/Makefile.am b/Makefile.am
index 9e67402102..6ba369a090 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3434,6 +3434,8 @@ EXTRA_DIST += \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-reorder-hdr-1 \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-reorder-hdr-2 \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-trailing-spaces \
+	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use \
+	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use.cexpected \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-band-a \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-band-a-channel-mismatch \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-band-bg-channel-mismatch \
@@ -3463,9 +3465,9 @@ EXTRA_DIST += \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wep-eap-ttls-chap \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wep-no-keys \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wep-passphrase \
+	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-suite-b-192-tls \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-tls \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-ttls-tls \
-	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-suite-b-192-tls \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-psk \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-psk-2 \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-psk-adhoc \
diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c
index a2da910896..af15a2d357 100644
--- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c
+++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c
@@ -6071,15 +6071,14 @@ make_vlan_setting(shvarFile *ifcfg, const char *file, GError **error)
                 v = iface_name + 4;
         }
 
-        if (v) {
-            int device_vlan_id;
-
-            /* Grab VLAN ID from interface name; this takes precedence over the
-             * separate VLAN_ID property for backwards compat.
+        if (vlan_id == -1 && v) {
+            /* Grab VLAN ID from interface name; The explicit VLAN_ID option takes precedence
+             * over detecting the ID based on PHYSDEV.
+             *
+             * Note that older versions of NetworkManager had a bug and this would overwrite the
+             * VLAN_ID in this case.
              */
-            device_vlan_id = _nm_utils_ascii_str_to_int64(v, 10, 0, 4095, -1);
-            if (device_vlan_id != -1)
-                vlan_id = device_vlan_id;
+            vlan_id = _nm_utils_ascii_str_to_int64(v, 10, 0, 4095, -1);
         }
     }
 
diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use
new file mode 100644
index 0000000000..fc9c8a45ba
--- /dev/null
+++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use
@@ -0,0 +1,4 @@
+VLAN=yes
+TYPE=Vlan
+DEVICE=eth0.9
+VLAN_ID=10
diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use.cexpected b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use.cexpected
new file mode 100644
index 0000000000..a7be14ce34
--- /dev/null
+++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use.cexpected
@@ -0,0 +1,15 @@
+VLAN=yes
+TYPE=Vlan
+PHYSDEV=eth0
+VLAN_ID=10
+REORDER_HDR=yes
+GVRP=no
+MVRP=no
+HWADDR=
+PROXY_METHOD=none
+BROWSER_ONLY=no
+IPV6INIT=no
+NAME="Vlan test-vlan-vlanid-use"
+UUID=${UUID}
+DEVICE=eth0.9
+ONBOOT=yes
diff --git a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
index ad96d0c4be..af0c23f5eb 100644
--- a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -8787,6 +8787,38 @@ test_read_vlan_only_vlan_id(void)
 }
 
 static void
+test_read_vlan_vlanid_use(void)
+{
+    nmtst_auto_unlinkfile char *testfile     = NULL;
+    gs_unref_object NMConnection *connection = NULL;
+    gs_unref_object NMConnection *reread     = NULL;
+    NMSettingVlan *               s_vlan;
+
+    connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-vlan-vlanid-use",
+                                       NULL,
+                                       TYPE_ETHERNET,
+                                       NULL);
+
+    g_assert_cmpstr(nm_connection_get_interface_name(connection), ==, "eth0.9");
+
+    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), ==, 10);
+    g_assert_cmpint(nm_setting_vlan_get_flags(s_vlan), ==, NM_VLAN_FLAG_REORDER_HEADERS);
+
+    _writer_new_connec_exp(connection,
+                           TEST_SCRATCH_DIR,
+                           TEST_IFCFG_DIR "/ifcfg-test-vlan-vlanid-use.cexpected",
+                           &testfile);
+
+    reread = _connection_from_file(testfile, NULL, TYPE_ETHERNET, NULL);
+
+    nmtst_assert_connection_equals(connection, TRUE, reread, FALSE);
+}
+
+static void
 test_read_vlan_only_device(void)
 {
     NMConnection * connection;
@@ -11626,6 +11658,7 @@ main(int argc, char **argv)
     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 "vlan/read/vlanid-use", test_read_vlan_vlanid_use);
     g_test_add_func(TPATH "wired/read/read-wake-on-lan", test_read_wired_wake_on_lan);
     g_test_add_func(TPATH "wired/read/read-auto-negotiate-off", test_read_wired_auto_negotiate_off);
     g_test_add_func(TPATH "wired/read/read-auto-negotiate-on", test_read_wired_auto_negotiate_on);
diff --git a/src/libnm-core-impl/nm-setting-vlan.c b/src/libnm-core-impl/nm-setting-vlan.c
index 0d2855587b..6acf2936a1 100644
--- a/src/libnm-core-impl/nm-setting-vlan.c
+++ b/src/libnm-core-impl/nm-setting-vlan.c
@@ -862,8 +862,11 @@ nm_setting_vlan_class_init(NMSettingVlanClass *klass)
      **/
     /* ---ifcfg-rh---
      * property: id
-     * variable: VLAN_ID or DEVICE
-     * description: VLAN identifier.
+     * variable: VLAN_ID, DEVICE.
+     * description: VLAN identifier. If VLAN_ID is not set, it is attempted
+     *   to be detected from the suffix of DEVICE=.
+     *   Note that older versions of NetworkManager had a bug where they would
+     *   prefer the detected ID from the DEVICE over VLAN_ID.
      * ---end---
      */
     obj_properties[PROP_ID] =