diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index 9b032c4cd8..d2360f8c2a 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -1369,6 +1369,9 @@ nm_keyfile_detect_unqualified_path_scheme (const char *base_dir, */ path = get_cert_path (base_dir, (const guint8 *) data, data_len); + + /* FIXME(keyfile-parse-in-memory): it is wrong that keyfile reader makes decisions based on + * the file systems content. The serialization/parsing should be entirely in-memory. */ if ( !memchr (data, '/', data_len) && !has_cert_ext (path)) { if (!consider_exists) @@ -1449,6 +1452,10 @@ cert_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key) path2 = path2_free; } + /* FIXME(keyfile-parse-in-memory): keyfile reader must not access the file system and + * (in a first step) only operate in memory-only. If the presence of files should be checked, + * then by invoking a callback (and possibly keyfile settings plugin would + * collect the file names to be checked and check them later). */ if (!g_file_test (path2, G_FILE_TEST_EXISTS)) { handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_INFO_MISSING_FILE, _("certificate or key file '%s' does not exist"), @@ -2906,41 +2913,48 @@ static const ParseInfoSetting *const parse_infos[_NM_META_SETTING_TYPE_NUM] = { ), }; -static const ParseInfoProperty * +static void _parse_info_find (NMSetting *setting, const char *property_name, - const NMMetaSettingInfo **out_setting_info) + const NMMetaSettingInfo **out_setting_info, + const ParseInfoSetting **out_parse_info_setting, + const ParseInfoProperty **out_parse_info_property) { const NMMetaSettingInfo *setting_info; const ParseInfoSetting *pis; - gssize idx; + const ParseInfoProperty *pip; #if NM_MORE_ASSERTS > 10 { guint i, j; + static int asserted = FALSE; - for (i = 0; i < G_N_ELEMENTS (parse_infos); i++) { - pis = parse_infos[i]; + if (!asserted) { + for (i = 0; i < G_N_ELEMENTS (parse_infos); i++) { + pis = parse_infos[i]; - if (!pis) - continue; + if (!pis) + continue; + if (!pis->properties) + continue; - g_assert (pis->properties); - g_assert (pis->properties[0]); - for (j = 0; pis->properties[j]; j++) { - const ParseInfoProperty *pip0; - const ParseInfoProperty *pip = pis->properties[j]; + g_assert (pis->properties[0]); + for (j = 0; pis->properties[j]; j++) { + const ParseInfoProperty *pip0; + const ParseInfoProperty *pipj = pis->properties[j]; - g_assert (pip->property_name); - if ( j > 0 - && (pip0 = pis->properties[j - 1]) - && strcmp (pip0->property_name, pip->property_name) >= 0) { - g_error ("Wrong order at index #%d.%d: \"%s.%s\" before \"%s.%s\"", - i, j - 1, - nm_meta_setting_infos[i].setting_name, pip0->property_name, - nm_meta_setting_infos[i].setting_name, pip->property_name); + g_assert (pipj->property_name); + if ( j > 0 + && (pip0 = pis->properties[j - 1]) + && strcmp (pip0->property_name, pipj->property_name) >= 0) { + g_error ("Wrong order at index #%d.%d: \"%s.%s\" before \"%s.%s\"", + i, j - 1, + nm_meta_setting_infos[i].setting_name, pip0->property_name, + nm_meta_setting_infos[i].setting_name, pipj->property_name); + } } } + asserted = TRUE; } } #endif @@ -2948,16 +2962,25 @@ _parse_info_find (NMSetting *setting, if ( !NM_IS_SETTING (setting) || !(setting_info = NM_SETTING_GET_CLASS (setting)->setting_info)) { /* handle invalid setting objects gracefully. */ - *out_setting_info = NULL; - return NULL; + NM_SET_OUT (out_setting_info, NULL); + NM_SET_OUT (out_parse_info_setting, NULL); + NM_SET_OUT (out_parse_info_property, NULL); + return; } nm_assert (setting_info->setting_name); + nm_assert (_NM_INT_NOT_NEGATIVE (setting_info->meta_type)); + nm_assert (setting_info->meta_type < G_N_ELEMENTS (parse_infos)); - *out_setting_info = setting_info; + pis = parse_infos[setting_info->meta_type]; + + pip = NULL; + if ( pis + && property_name) { + gssize idx; - if ((pis = parse_infos[setting_info->meta_type])) { G_STATIC_ASSERT_EXPR (G_STRUCT_OFFSET (ParseInfoProperty, property_name) == 0); + idx = nm_utils_ptrarray_find_binary_search ((gconstpointer *) pis->properties, NM_PTRARRAY_LEN (pis->properties), &property_name, @@ -2966,10 +2989,12 @@ _parse_info_find (NMSetting *setting, NULL, NULL); if (idx >= 0) - return pis->properties[idx]; + pip = pis->properties[idx]; } - return NULL; + NM_SET_OUT (out_setting_info, setting_info); + NM_SET_OUT (out_parse_info_setting, pis); + NM_SET_OUT (out_parse_info_property, pip); } /*****************************************************************************/ @@ -2995,7 +3020,7 @@ read_one_setting_value (KeyfileReaderInfo *info, key = property_info->name; - pip = _parse_info_find (setting, key, &setting_info); + _parse_info_find (setting, key, &setting_info, NULL, &pip); nm_assert (setting_info); @@ -3613,7 +3638,7 @@ write_setting_value (KeyfileWriterInfo *info, key = property_info->name; - pip = _parse_info_find (setting, key, &setting_info); + _parse_info_find (setting, key, &setting_info, NULL, &pip); if (!pip) { if (!setting_info) { @@ -3850,9 +3875,13 @@ nm_keyfile_write (NMConnection *connection, for (i = 0; i < n_settings; i++) { const NMSettInfoSetting *sett_info; NMSetting *setting = settings[i]; + const char *setting_name; + const char *setting_alias; sett_info = _nm_setting_class_get_sett_info (NM_SETTING_GET_CLASS (setting)); + setting_name = sett_info->setting_class->setting_info->setting_name; + if (sett_info->detail.gendata_info) { guint k, n_keys; const char *const*keys; @@ -3862,7 +3891,6 @@ nm_keyfile_write (NMConnection *connection, n_keys = _nm_setting_gendata_get_all (setting, &keys, NULL); if (n_keys > 0) { - const char *setting_name = sett_info->setting_class->setting_info->setting_name; GHashTable *h = _nm_setting_gendata_hash (setting, FALSE); for (k = 0; k < n_keys; k++) { @@ -3902,6 +3930,18 @@ nm_keyfile_write (NMConnection *connection, goto out_with_info_error; } + setting_alias = nm_keyfile_plugin_get_alias_for_setting_name (setting_name); + if ( ( setting_alias + && g_key_file_has_group (info.keyfile, setting_alias)) + || g_key_file_has_group (info.keyfile, setting_name)) { + /* we have a section for the setting. Nothing to do. */ + } else { + /* ensure the group is present. There is no API for that, so add and remove + * a dummy key. */ + g_key_file_set_value (info.keyfile, setting_alias ?: setting_name, ".X", "1"); + g_key_file_remove_key (info.keyfile, setting_alias ?: setting_name, ".X", NULL); + } + nm_assert (!info.error); } diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index 3438aea99d..b24bb1e37f 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -2581,7 +2581,7 @@ static void test_roundtrip_conversion (gconstpointer test_data) { const int MODE = GPOINTER_TO_INT (test_data); - const char *ID= nm_sprintf_bufa (100, "roundtip-conversion-%d", MODE); + const char *ID= nm_sprintf_bufa (100, "roundtrip-conversion-%d", MODE); const char *UUID= "63376701-b61e-4318-bf7e-664a1c1eeaab"; const char *INTERFACE_NAME = nm_sprintf_bufa (100, "ifname%d", MODE); guint32 ETH_MTU = nmtst_rand_select ((guint32) 0u, @@ -2663,6 +2663,8 @@ test_roundtrip_conversion (gconstpointer test_data) "addr-gen-mode=stable-privacy\n" "dns-search=\n" "method=auto\n" + "\n" + "[proxy]\n" "", ID, UUID, @@ -2722,6 +2724,8 @@ test_roundtrip_conversion (gconstpointer test_data) "interface-name=%s\n" "permissions=\n" "\n" + "[wireguard]\n" + "\n" "[ipv4]\n" "dns-search=\n" "method=disabled\n" @@ -2730,6 +2734,8 @@ test_roundtrip_conversion (gconstpointer test_data) "addr-gen-mode=stable-privacy\n" "dns-search=\n" "method=ignore\n" + "\n" + "[proxy]\n" "", ID, UUID, @@ -2770,7 +2776,8 @@ test_roundtrip_conversion (gconstpointer test_data) "type=wireguard\n" "interface-name=%s\n" "permissions=\n" - "%s" /* [wireguard] */ + "\n" + "[wireguard]\n" "%s" /* fwmark */ "%s" /* listen-port */ "%s" /* private-key-flags */ @@ -2785,17 +2792,12 @@ test_roundtrip_conversion (gconstpointer test_data) "addr-gen-mode=stable-privacy\n" "dns-search=\n" "method=ignore\n" + "\n" + "[proxy]\n" "", ID, UUID, INTERFACE_NAME, - ( ( (WG_FWMARK != 0) - || (WG_LISTEN_PORT != 0) - || (WG_PRIVATE_KEY_FLAGS != NM_SETTING_SECRET_FLAG_NONE) - || ( WG_PRIVATE_KEY - && WG_PRIVATE_KEY_FLAGS == NM_SETTING_SECRET_FLAG_NONE)) - ? "\n[wireguard]\n" - : ""), ( (WG_FWMARK != 0) ? nm_sprintf_bufa (100, "fwmark=%u\n", WG_FWMARK) : ""), @@ -2887,6 +2889,8 @@ test_roundtrip_conversion (gconstpointer test_data) "routing-rule1=priority 1 from ::/0 table 1000\n" "routing-rule2=priority 2 from 1:2:3:b::/65 table 1001\n" "routing-rule3=priority 3 from 1:2:3:c::/66 table 1002\n" + "\n" + "[proxy]\n" "", ID, UUID, @@ -3281,6 +3285,51 @@ test_parse_tc_handle (void) /*****************************************************************************/ +static void +test_empty_setting (void) +{ + gs_unref_object NMConnection *con = NULL; + gs_unref_object NMConnection *con2 = NULL; + NMSettingBluetooth *s_bt; + NMSettingGsm *s_gsm; + gs_unref_keyfile GKeyFile *kf = NULL; + gs_free_error GError *error = NULL; + + con = nmtst_create_minimal_connection ("bt-empty-gsm", "dca3192a-f2dc-48eb-b806-d0ff788f122c", NM_SETTING_BLUETOOTH_SETTING_NAME, NULL); + + s_bt = _nm_connection_get_setting (con, NM_TYPE_SETTING_BLUETOOTH); + g_object_set (s_bt, + NM_SETTING_BLUETOOTH_TYPE, "dun", + NM_SETTING_BLUETOOTH_BDADDR, "aa:bb:cc:dd:ee:ff", + NULL); + + s_gsm = NM_SETTING_GSM (nm_setting_gsm_new ()); + nm_connection_add_setting (con, NM_SETTING (s_gsm)); + + nmtst_connection_normalize (con); + + nmtst_assert_connection_verifies_without_normalization (con); + + kf = nm_keyfile_write (con, NULL, NULL, &error); + nmtst_assert_success (kf, error); + + g_assert (g_key_file_has_group (kf, "gsm")); + g_assert_cmpint (nmtst_keyfile_get_num_keys (kf, "gsm"), ==, 0); + + con2 = nm_keyfile_read (kf, + "/ignored/current/working/directory/for/loading/relative/paths", + NULL, + NULL, + &error); + nmtst_assert_success (con2, error); + + g_assert (nm_connection_get_setting (con2, NM_TYPE_SETTING_GSM)); + + nmtst_assert_connection_verifies_without_normalization (con2); +} + +/*****************************************************************************/ + NMTST_DEFINE (); int @@ -3368,5 +3417,7 @@ main (int argc, char **argv) g_test_add_func ("/libnm/test_team_setting", test_team_setting); + g_test_add_func ("/libnm/test_empty_setting", test_empty_setting); + return g_test_run (); } diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index b2d103f18a..d7a8788dfb 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -2272,6 +2272,29 @@ nmtst_keyfile_assert_data (GKeyFile *kf, const char *data, gssize data_len) g_assert_cmpmem (d2, d2_len, d1, d1_len); } +static inline gssize +nmtst_keyfile_get_num_keys (GKeyFile *keyfile, + const char *group_name) +{ + gs_strfreev char **keys = NULL; + gs_free_error GError *error = NULL; + gsize l; + + g_assert (keyfile); + g_assert (group_name); + + if (!g_key_file_has_group (keyfile, group_name)) + return -1; + + keys = g_key_file_get_keys (keyfile, group_name, &l, &error); + + nmtst_assert_success (keys, error); + + g_assert_cmpint (NM_PTRARRAY_LEN (keys), ==, l); + + return l; +} + /*****************************************************************************/ #endif /* __NM_TEST_UTILS_H__ */ diff --git a/src/settings/plugins/keyfile/nms-keyfile-writer.c b/src/settings/plugins/keyfile/nms-keyfile-writer.c index ef42928fb1..515376f8df 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-writer.c +++ b/src/settings/plugins/keyfile/nms-keyfile-writer.c @@ -126,6 +126,10 @@ cert_writer (NMConnection *connection, new_path = g_strdup_printf ("%s/%s-%s.%s", info->keyfile_dir, nm_connection_get_uuid (connection), cert_data->vtable->file_suffix, ext); + /* FIXME(keyfile-parse-in-memory): writer must not access/write to the file system before + * being sure that the entire profile can be written and all circumstances are good to + * proceed. That means, while writing we must only collect the blogs in-memory, and write + * them all in the end together (or not at all). */ success = nm_utils_file_set_contents (new_path, (const char *) blob_data, blob_len, @@ -197,10 +201,12 @@ _internal_write_connection (NMConnection *connection, gs_free char *path = NULL; const char *id; WriteInfo info = { 0 }; - GError *local_err = NULL; + gs_free_error GError *local_err = NULL; int errsv; gboolean rename; int i_path; + gs_unref_object NMConnection *reread = NULL; + gboolean reread_same = FALSE; g_return_val_if_fail (!out_path || !*out_path, FALSE); g_return_val_if_fail (keyfile_dir && keyfile_dir[0] == '/', FALSE); @@ -313,6 +319,35 @@ _internal_write_connection (NMConnection *connection, return FALSE; } + if ( out_reread + || out_reread_same) { + gs_free_error GError *reread_error = NULL; + + reread = nms_keyfile_reader_from_keyfile (kf_file, path, NULL, profile_dir, FALSE, &reread_error); + + if ( !reread + || !nm_connection_normalize (reread, NULL, NULL, &reread_error)) { + nm_log_err (LOGD_SETTINGS, "BUG: the profile cannot be stored in keyfile format without becoming unusable: %s", reread_error->message); + g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, + "keyfile writer produces an invalid connection: %s", + reread_error->message); + nm_assert_not_reached (); + return FALSE; + } + + if (out_reread_same) { + reread_same = !!nm_connection_compare (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT); + + nm_assert (reread_same == nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT)); + nm_assert (reread_same == ({ + gs_unref_hashtable GHashTable *_settings = NULL; + + ( nm_connection_diff (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT, &_settings) + && !_settings); + })); + } + } + nm_utils_file_set_contents (path, kf_content_buf, kf_content_len, @@ -323,7 +358,6 @@ _internal_write_connection (NMConnection *connection, g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, "error writing to file '%s': %s", path, local_err->message); - g_error_free (local_err); return FALSE; } @@ -344,36 +378,8 @@ _internal_write_connection (NMConnection *connection, && !nm_streq (path, existing_path)) unlink (existing_path); - if (out_reread || out_reread_same) { - gs_unref_object NMConnection *reread = NULL; - gboolean reread_same = FALSE; - - reread = nms_keyfile_reader_from_keyfile (kf_file, path, NULL, profile_dir, FALSE, NULL); - - nm_assert (NM_IS_CONNECTION (reread)); - - if ( reread - && !nm_connection_normalize (reread, NULL, NULL, NULL)) { - nm_assert_not_reached (); - g_clear_object (&reread); - } - - if (reread && out_reread_same) { - reread_same = !!nm_connection_compare (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT); - - nm_assert (reread_same == nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT)); - nm_assert (reread_same == ({ - gs_unref_hashtable GHashTable *_settings = NULL; - - ( nm_connection_diff (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT, &_settings) - && !_settings); - })); - } - - NM_SET_OUT (out_reread, g_steal_pointer (&reread)); - NM_SET_OUT (out_reread_same, reread_same); - } - + NM_SET_OUT (out_reread, g_steal_pointer (&reread)); + NM_SET_OUT (out_reread_same, reread_same); NM_SET_OUT (out_path, g_steal_pointer (&path)); return TRUE; @@ -448,4 +454,3 @@ nms_keyfile_writer_test_connection (NMConnection *connection, out_reread_same, error); } -