From 64da830b07d57f1c8ec5d72e990302d647ac3036 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 Mar 2020 12:41:13 +0100 Subject: [PATCH] libnm: allow setting empty vpn.data item Until now, nm_setting_vpn_add_data_item() would reject empty data values. This leads for example to an assertion failure, if you write a keyfile that assigns an empty value to a key. Keyfile reader would not check that the value is non-empty before calling nm_setting_vpn_add_data_item(). Anyway, I think we should not require having non-empty data elements. It's an unnecessary and sometimes harmful restriction. NetworkManager doesn't understand not care about the content of the vpn data. That is up the VPN plugins. Sometimes and empty value may be desirable. Also, the NM_SETTING_VPN_DATA property setter wouldn't filter out empty values either. So it was always possible to use some libnm API to set data with empty values. The restriction in nm_setting_vpn_add_data_item() was inconsistent. --- libnm-core/nm-setting-vpn.c | 12 ++++++++++-- libnm-core/tests/test-general.c | 10 ++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/libnm-core/nm-setting-vpn.c b/libnm-core/nm-setting-vpn.c index 30d67b1ea4..cf9cbd1d91 100644 --- a/libnm-core/nm-setting-vpn.c +++ b/libnm-core/nm-setting-vpn.c @@ -163,20 +163,28 @@ nm_setting_vpn_get_num_data_items (NMSettingVpn *setting) * nm_setting_vpn_add_data_item: * @setting: the #NMSettingVpn * @key: a name that uniquely identifies the given value @item - * @item: the value to be referenced by @key + * @item: (allow-none): the value to be referenced by @key * * Establishes a relationship between @key and @item internally in the * setting which may be retrieved later. Should not be used to store passwords * or other secrets, which is what nm_setting_vpn_add_secret() is for. + * + * Before 1.24, @item must not be %NULL and not an empty string. Since 1.24, + * @item can be set to an empty string. It can also be set to %NULL to unset + * the key. In that case, the behavior is as if calling nm_setting_vpn_remove_data_item(). **/ void nm_setting_vpn_add_data_item (NMSettingVpn *setting, const char *key, const char *item) { + if (!item) { + nm_setting_vpn_remove_data_item (setting, key); + return; + } + g_return_if_fail (NM_IS_SETTING_VPN (setting)); g_return_if_fail (key && key[0]); - g_return_if_fail (item && item[0]); g_hash_table_insert (_ensure_strdict (&NM_SETTING_VPN_GET_PRIVATE (setting)->data, FALSE), g_strdup (key), diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index e0d0d55fa2..5d3494d869 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -1172,13 +1172,11 @@ test_setting_vpn_items (void) nm_setting_vpn_add_data_item (s_vpn, "", ""); g_test_assert_expected_messages (); - NMTST_EXPECT_LIBNM_CRITICAL (NMTST_G_RETURN_MSG (item && item[0])); - nm_setting_vpn_add_data_item (s_vpn, "foobar1", NULL); - g_test_assert_expected_messages (); - - NMTST_EXPECT_LIBNM_CRITICAL (NMTST_G_RETURN_MSG (item && item[0])); nm_setting_vpn_add_data_item (s_vpn, "foobar1", ""); - g_test_assert_expected_messages (); + g_assert_cmpstr (nm_setting_vpn_get_data_item (s_vpn, "foobar1"), ==, ""); + + nm_setting_vpn_add_data_item (s_vpn, "foobar1", NULL); + g_assert_cmpstr (nm_setting_vpn_get_data_item (s_vpn, "foobar1"), ==, NULL); NMTST_EXPECT_LIBNM_CRITICAL (NMTST_G_RETURN_MSG (key && key[0])); nm_setting_vpn_add_data_item (s_vpn, NULL, "blahblah1");