From 5d6532f2d7c203c2d70c0e328806dcb0b0ea53bc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 25 Mar 2021 11:55:22 +0100 Subject: [PATCH] ifcfg-rh: always honor "$VLAN_ID" in ifcfg files initscripts don't support "$VLAN_ID". They actually support "$VID", which NetworkManager doesn't. "$VLAN_ID" was introduced by commit 10b32be37b52 ('ifcfg-rh: various VLAN cleanups'). It has a comment about "backward compatibility" for the case 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", then we continue parsing it as before. 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=vlan9" together with "vlan.id=10". Such a configuration is valid and must be expressible in 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 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/794 --- Makefile.am | 4 ++- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 15 ++++----- .../ifcfg-test-vlan-vlanid-use | 4 +++ .../ifcfg-test-vlan-vlanid-use.cexpected | 15 +++++++++ .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 33 +++++++++++++++++++ src/libnm-core-impl/nm-setting-vlan.c | 7 ++-- 6 files changed, 67 insertions(+), 11 deletions(-) create mode 100644 src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use create mode 100644 src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-vlanid-use.cexpected 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 @@ -8786,6 +8786,38 @@ test_read_vlan_only_vlan_id(void) g_object_unref(connection); } +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) { @@ -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] =