From 33f2f82a0951c79a5e0363413c63f9067fd85340 Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Fri, 2 Sep 2022 13:44:30 -0400 Subject: [PATCH 1/3] infiniband: avoid normalizing the p-key when reading from ifcfg When writing the p-key setting to the ifcfg file and reading the setting back, the value has to be consistent. This is not limited to p-key only, any setting value during the ifcfg write and read also has to be consistent. This was probably added in commit cb5606cf1c7a ('ifcfg-rh: add support for Infiniband partitions') as this is also what ifup-ib does ([1]). For NetworkManager profiles however, the p-key is also valid without the high bit set, so the ifcfg-rh reader must honor that. [1] https://github.com/alaahl/rdma/blob/0c9fb6ca7bcb4f24a6134e68338a88a84c4ab56c/rdma.ifup-ib#L75 (cherry picked from commit a4fe16a426097eee263cb3ef831dcea468b1ca26) --- src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 1 - 1 file changed, 1 deletion(-) 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 6452d72a48..f948eaa959 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 @@ -5383,7 +5383,6 @@ parse_infiniband_p_key(shvarFile *ifcfg, int *out_p_key, char **out_parent, GErr PARSE_WARNING("invalid InfiniBand PKEY_ID '%s'", pkey_id); goto done; } - id = (id | 0x8000); ifname = g_strdup_printf("%s.%04x", physdev, (unsigned) id); if (strcmp(device, ifname) != 0) { From fe21880c46ed45ea6d33332f2eb767644cf83fee Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Tue, 30 Aug 2022 13:34:04 -0400 Subject: [PATCH 2/3] ipoib: skip validating the DEVICE when reading the ifcfg file For the ipoib connection, it is still considered as valid if the profile does not set the device name. Also, the ifcfg reader should not duplicate the checks that `nm_connection_verify()` performs (especially not wrongly). Therefore, NM should skip validating the DEVICE when reading the ifcfg file for the ipoib connection. https://bugzilla.redhat.com/show_bug.cgi?id=2122703 (cherry picked from commit 4c32dd9d252959b9bab5de6277418939b64d1bb1) --- Makefile.am | 1 + .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 17 +---- .../tests/network-scripts/ifcfg-test-ipoib | 9 +++ .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 74 ++++++++++++++++--- 4 files changed, 73 insertions(+), 28 deletions(-) create mode 100644 src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-ipoib diff --git a/Makefile.am b/Makefile.am index 109326c309..e3b96895a1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3433,6 +3433,7 @@ EXTRA_DIST += \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-ibft \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-infiniband \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-ip6-disabled.cexpected \ + src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-ipoib \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-link_local \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-minimal \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-misc-variables \ 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 f948eaa959..e261069c0e 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 @@ -5355,17 +5355,10 @@ wired_connection_from_ifcfg(const char *file, shvarFile *ifcfg, GError **error) static gboolean parse_infiniband_p_key(shvarFile *ifcfg, int *out_p_key, char **out_parent, GError **error) { - char *device = NULL, *physdev = NULL, *pkey_id = NULL; - char *ifname = NULL; + char *physdev = NULL, *pkey_id = NULL; int id; gboolean ret = FALSE; - device = svGetValueStr_cp(ifcfg, "DEVICE"); - if (!device) { - PARSE_WARNING("InfiniBand connection specified PKEY but not DEVICE"); - goto done; - } - physdev = svGetValueStr_cp(ifcfg, "PHYSDEV"); if (!physdev) { PARSE_WARNING("InfiniBand connection specified PKEY but not PHYSDEV"); @@ -5384,21 +5377,13 @@ parse_infiniband_p_key(shvarFile *ifcfg, int *out_p_key, char **out_parent, GErr goto done; } - ifname = g_strdup_printf("%s.%04x", physdev, (unsigned) id); - if (strcmp(device, ifname) != 0) { - PARSE_WARNING("InfiniBand DEVICE (%s) does not match PHYSDEV+PKEY_ID (%s)", device, ifname); - goto done; - } - *out_p_key = id; *out_parent = g_strdup(physdev); ret = TRUE; done: - g_free(device); g_free(physdev); g_free(pkey_id); - g_free(ifname); if (!ret) { g_set_error(error, diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-ipoib b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-ipoib new file mode 100644 index 0000000000..b45da5f7f7 --- /dev/null +++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-ipoib @@ -0,0 +1,9 @@ +TYPE=InfiniBand +PKEY=yes +PKEY_ID=12 +PHYSDEV=ib0 +CONNECTED_MODE=yes +IPADDR=192.168.2.2 +NETMASK=255.255.255.0 +GATEWAY=192.168.2.1 +NAME=ib012 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 9fdae4bd25..f9fd7fd58b 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 @@ -8211,8 +8211,35 @@ test_read_infiniband(void) } static void -test_write_infiniband(void) +test_read_ipoib(void) { + gs_unref_object NMConnection *connection = NULL; + NMSettingInfiniband *s_infiniband; + char *unmanaged = NULL; + const char *transport_mode; + int pkey; + + connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-ipoib", + NULL, + TYPE_INFINIBAND, + &unmanaged); + g_assert(!unmanaged); + + s_infiniband = nmtst_connection_assert_setting(connection, NM_TYPE_SETTING_INFINIBAND); + + pkey = nm_setting_infiniband_get_p_key(s_infiniband); + g_assert(pkey); + g_assert_cmpint(pkey, ==, 12); + + transport_mode = nm_setting_infiniband_get_transport_mode(s_infiniband); + g_assert(transport_mode); + g_assert_cmpstr(transport_mode, ==, "connected"); +} + +static void +test_write_infiniband(gconstpointer test_data) +{ + const int TEST_IDX = GPOINTER_TO_INT(test_data); nmtst_auto_unlinkfile char *testfile = NULL; gs_unref_object NMConnection *connection = NULL; gs_unref_object NMConnection *reread = NULL; @@ -8223,14 +8250,15 @@ test_write_infiniband(void) const char *mac = "80:00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00:11:22"; guint32 mtu = 65520; NMIPAddress *addr; - GError *error = NULL; + GError *error = NULL; + const char *interface_name = NULL; connection = nm_simple_connection_new(); s_con = _nm_connection_new_setting(connection, NM_TYPE_SETTING_CONNECTION); g_object_set(s_con, NM_SETTING_CONNECTION_ID, - "Test Write InfiniBand", + "Test Write Infiniband", NM_SETTING_CONNECTION_UUID, nm_uuid_generate_random_str_a(), NM_SETTING_CONNECTION_AUTOCONNECT, @@ -8239,15 +8267,28 @@ test_write_infiniband(void) NM_SETTING_INFINIBAND_SETTING_NAME, NULL); + if (NM_IN_SET(TEST_IDX, 1, 3)) + interface_name = "ib0.000c"; + + g_object_set(s_con, NM_SETTING_CONNECTION_INTERFACE_NAME, interface_name, NULL); + s_infiniband = _nm_connection_new_setting(connection, NM_TYPE_SETTING_INFINIBAND); - g_object_set(s_infiniband, - NM_SETTING_INFINIBAND_MAC_ADDRESS, - mac, - NM_SETTING_INFINIBAND_MTU, - mtu, - NM_SETTING_INFINIBAND_TRANSPORT_MODE, - "connected", - NULL); + g_object_set(s_infiniband, NM_SETTING_INFINIBAND_TRANSPORT_MODE, "connected", NULL); + if (NM_IN_SET(TEST_IDX, 1, 2)) { + g_object_set(s_infiniband, + NM_SETTING_INFINIBAND_MAC_ADDRESS, + mac, + NM_SETTING_INFINIBAND_MTU, + mtu, + NULL); + } else { + g_object_set(s_infiniband, + NM_SETTING_INFINIBAND_P_KEY, + 12, + NM_SETTING_INFINIBAND_PARENT, + "ib0", + NULL); + } s_ip4 = _nm_connection_new_setting(connection, NM_TYPE_SETTING_IP4_CONFIG); g_object_set(s_ip4, @@ -8267,6 +8308,9 @@ test_write_infiniband(void) s_ip6 = _nm_connection_new_setting(connection, NM_TYPE_SETTING_IP6_CONFIG); g_object_set(s_ip6, NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NULL); + if (nmtst_get_rand_bool()) + nmtst_connection_normalize(connection); + nmtst_assert_connection_verifies(connection); _writer_new_connection(connection, TEST_SCRATCH_DIR, &testfile); @@ -8274,6 +8318,8 @@ test_write_infiniband(void) reread = _connection_from_file(testfile, NULL, TYPE_INFINIBAND, NULL); nmtst_assert_connection_equals(connection, TRUE, reread, FALSE); + + g_assert_cmpstr(interface_name, ==, nm_connection_get_interface_name(reread)); } static void @@ -10446,6 +10492,7 @@ main(int argc, char **argv) g_test_add_func(TPATH "wifi/read/wep-no-keys", test_read_wifi_wep_no_keys); g_test_add_func(TPATH "wifi/read/wep-agent-keys", test_read_wifi_wep_agent_keys); g_test_add_func(TPATH "infiniband/read", test_read_infiniband); + g_test_add_func(TPATH "ipoib/read", test_read_ipoib); g_test_add_func(TPATH "vlan/read", test_read_vlan_interface); g_test_add_func(TPATH "vlan/read-flags-1", test_read_vlan_flags_1); g_test_add_func(TPATH "vlan/read-flags-2", test_read_vlan_flags_2); @@ -10582,7 +10629,10 @@ main(int argc, char **argv) g_test_add_func(TPATH "permissions/read", test_read_permissions); g_test_add_func(TPATH "permissions/write", test_write_permissions); g_test_add_func(TPATH "wifi/write-wep-agent-keys", test_write_wifi_wep_agent_keys); - g_test_add_func(TPATH "infiniband/write", test_write_infiniband); + g_test_add_data_func(TPATH "infiniband/write/1", GINT_TO_POINTER(1), test_write_infiniband); + g_test_add_data_func(TPATH "infiniband/write/2", GINT_TO_POINTER(2), test_write_infiniband); + g_test_add_data_func(TPATH "infiniband/write/3", GINT_TO_POINTER(3), test_write_infiniband); + g_test_add_data_func(TPATH "infiniband/write/4", GINT_TO_POINTER(4), test_write_infiniband); g_test_add_func(TPATH "vlan/write", test_write_vlan); g_test_add_func(TPATH "vlan/write-flags", test_write_vlan_flags); g_test_add_func(TPATH "vlan/write-only-vlanid", test_write_vlan_only_vlanid); From 533b30e5a2bc1e6c086626f34f6b2198627f3c60 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Sep 2022 21:15:57 +0200 Subject: [PATCH 3/3] ifcfg-rh: rework error handling in parse_infiniband_p_key() (cherry picked from commit 61d74b0c152874eda03bc67bbb5feb85e0879e97) --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) 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 e261069c0e..4d8e7bd69b 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 @@ -5355,43 +5355,41 @@ wired_connection_from_ifcfg(const char *file, shvarFile *ifcfg, GError **error) static gboolean parse_infiniband_p_key(shvarFile *ifcfg, int *out_p_key, char **out_parent, GError **error) { - char *physdev = NULL, *pkey_id = NULL; - int id; - gboolean ret = FALSE; + gs_free char *physdev = NULL; + gs_free char *pkey_id = NULL; + int id; physdev = svGetValueStr_cp(ifcfg, "PHYSDEV"); if (!physdev) { - PARSE_WARNING("InfiniBand connection specified PKEY but not PHYSDEV"); - goto done; + g_set_error(error, + NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_INVALID_CONNECTION, + "infiniband connection specified PKEY but not PHYSDEV"); + return FALSE; } pkey_id = svGetValueStr_cp(ifcfg, "PKEY_ID"); if (!pkey_id) { - PARSE_WARNING("InfiniBand connection specified PKEY but not PKEY_ID"); - goto done; + g_set_error(error, + NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_INVALID_CONNECTION, + "infiniband connection specified PKEY but not PKEY_ID"); + return FALSE; } id = _nm_utils_ascii_str_to_int64(pkey_id, 0, 0, 0xFFFF, -1); if (id == -1) { - PARSE_WARNING("invalid InfiniBand PKEY_ID '%s'", pkey_id); - goto done; - } - - *out_p_key = id; - *out_parent = g_strdup(physdev); - ret = TRUE; - -done: - g_free(physdev); - g_free(pkey_id); - - if (!ret) { g_set_error(error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, - "Failed to create InfiniBand setting"); + "invalid infiniband PKEY_ID '%s'", + pkey_id); + return FALSE; } - return ret; + + *out_p_key = id; + *out_parent = g_steal_pointer(&physdev); + return TRUE; } static NMSetting *