From f6345180b16f7cb1b991cfa173b6d2aa6526143c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 8 Sep 2022 16:56:16 +0200 Subject: [PATCH 1/3] core/config: fix duplicate entires in `NetworkManager --print-config` output _nm_config_data_log_sort() is used for sorting the groups in the keyfile during nm_config_data_log(). The idea is to present the keyfile in a defined, but useful order. However, it is not a total order. That is, it will return c=0 (equal) for certain groups, if the pre-existing order in the GKeyFile should be honored. For example, we want to sort all [device*] sections close to each other, but we want to preserve their relative order. In that case, the function would return 0 although the group names differed. Also, _nm_config_data_log_sort() does not expect to receive duplicate names. It would return c!=0 for comparing "device" and "device". This means, _nm_config_data_log_sort() is fine for sorting the input as we have it. However, it cannot be used to binary search the groups. This caused that some sections might be duplicated in the `NetworkManager --print-config` output. Otherwise, it had no bad effects. Fixes(no-backport): 78d34d7c2e00 ('config: fix printing default values for missing sections') --- src/core/nm-config-data.c | 43 ++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/src/core/nm-config-data.c b/src/core/nm-config-data.c index fd98797186..1c573549b5 100644 --- a/src/core/nm-config-data.c +++ b/src/core/nm-config-data.c @@ -634,6 +634,8 @@ _nm_config_data_log_sort(const char **pa, const char **pb, gpointer dummy) const char *a = *pa; const char *b = *pb; + nm_assert(a && b && !nm_streq(a, b)); + /* we sort intern groups to the end. */ a_is_intern = g_str_has_prefix(a, NM_CONFIG_KEYFILE_GROUPPREFIX_INTERN); b_is_intern = g_str_has_prefix(b, NM_CONFIG_KEYFILE_GROUPPREFIX_INTERN); @@ -770,27 +772,48 @@ nm_config_data_log(const NMConfigData *self, groups_full = g_ptr_array_sized_new(ngroups + 5); if (ngroups) { + /* g_key_file_get_groups() can return duplicates ( :( ), but the + * keyfile that we constructed should not have any. Assert for that. */ + nm_assert(!nm_strv_has_duplicate((const char *const *) groups, ngroups, FALSE)); + g_ptr_array_set_size(groups_full, ngroups); memcpy(groups_full->pdata, groups, sizeof(groups[0]) * ngroups); - g_ptr_array_sort_with_data(groups_full, (GCompareDataFunc) _nm_config_data_log_sort, NULL); } if (print_default) { for (g = 0; g < G_N_ELEMENTS(default_values); g++) { const char *group = default_values[g].group; - gssize idx; + guint g2; - idx = nm_utils_array_find_binary_search((gconstpointer *) groups_full->pdata, - sizeof(char *), - groups_full->len, - &group, - (GCompareDataFunc) _nm_config_data_log_sort, - NULL); - if (idx < 0) - g_ptr_array_insert(groups_full, (~idx), (gpointer) group); + if (g > 0) { + if (nm_streq(group, default_values[g - 1].group)) { + /* Repeated values. We already added this one. Skip */ + continue; + } + if (NM_MORE_ASSERT_ONCE(20)) { + /* We require that the default values are grouped by their "group". + * That is, all default values for a certain "group" are close to + * each other in the list. Assert for that. */ + for (g2 = g + 1; g2 < groups_full->len; g2++) { + nm_assert(!nm_streq(default_values[g - 1].group, default_values[g2].group)); + } + } + } + + for (g2 = 0; g2 < groups_full->len; g2++) { + if (nm_streq(group, groups_full->pdata[g2])) + goto next; + } + + g_ptr_array_add(groups_full, (gpointer) group); + +next: + (void) 0; } } + g_ptr_array_sort_with_data(groups_full, (GCompareDataFunc) _nm_config_data_log_sort, NULL); + if (!stream) _LOG(stream, prefix, "config-data[%p]: %u groups", self, groups_full->len); From f1c1d93bc52903a388c3c6e3841e6eeb43229bde Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 8 Sep 2022 16:45:04 +0200 Subject: [PATCH 2/3] core/config: use NM_STR_HAS_PREFIX() instead of g_str_has_prefix() With C literals as prefix, this macro is more efficient as it just expands to a strncmp(). Also, it accepts NULL string. --- src/core/nm-config-data.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/nm-config-data.c b/src/core/nm-config-data.c index 1c573549b5..61cf111894 100644 --- a/src/core/nm-config-data.c +++ b/src/core/nm-config-data.c @@ -585,7 +585,7 @@ _merge_keyfiles(GKeyFile *keyfile_user, GKeyFile *keyfile_intern) if (!keys) continue; - is_intern = g_str_has_prefix(group, NM_CONFIG_KEYFILE_GROUPPREFIX_INTERN); + is_intern = NM_STR_HAS_PREFIX(group, NM_CONFIG_KEYFILE_GROUPPREFIX_INTERN); if (!is_intern && g_key_file_has_key(keyfile_intern, group, @@ -637,8 +637,8 @@ _nm_config_data_log_sort(const char **pa, const char **pb, gpointer dummy) nm_assert(a && b && !nm_streq(a, b)); /* we sort intern groups to the end. */ - a_is_intern = g_str_has_prefix(a, NM_CONFIG_KEYFILE_GROUPPREFIX_INTERN); - b_is_intern = g_str_has_prefix(b, NM_CONFIG_KEYFILE_GROUPPREFIX_INTERN); + a_is_intern = NM_STR_HAS_PREFIX(a, NM_CONFIG_KEYFILE_GROUPPREFIX_INTERN); + b_is_intern = NM_STR_HAS_PREFIX(b, NM_CONFIG_KEYFILE_GROUPPREFIX_INTERN); if (a_is_intern && b_is_intern) return 0; @@ -648,8 +648,8 @@ _nm_config_data_log_sort(const char **pa, const char **pb, gpointer dummy) return -1; /* we sort connection groups before intern groups (to the end). */ - a_is_connection = a && g_str_has_prefix(a, NM_CONFIG_KEYFILE_GROUPPREFIX_CONNECTION); - b_is_connection = b && g_str_has_prefix(b, NM_CONFIG_KEYFILE_GROUPPREFIX_CONNECTION); + a_is_connection = NM_STR_HAS_PREFIX(a, NM_CONFIG_KEYFILE_GROUPPREFIX_CONNECTION); + b_is_connection = NM_STR_HAS_PREFIX(b, NM_CONFIG_KEYFILE_GROUPPREFIX_CONNECTION); if (a_is_connection && b_is_connection) { /* if both are connection groups, we want the explicit [connection] group first. */ @@ -670,8 +670,8 @@ _nm_config_data_log_sort(const char **pa, const char **pb, gpointer dummy) return -1; /* we sort device groups before connection groups (to the end). */ - a_is_device = a && g_str_has_prefix(a, NM_CONFIG_KEYFILE_GROUPPREFIX_DEVICE); - b_is_device = b && g_str_has_prefix(b, NM_CONFIG_KEYFILE_GROUPPREFIX_DEVICE); + a_is_device = NM_STR_HAS_PREFIX(a, NM_CONFIG_KEYFILE_GROUPPREFIX_DEVICE); + b_is_device = NM_STR_HAS_PREFIX(b, NM_CONFIG_KEYFILE_GROUPPREFIX_DEVICE); if (a_is_device && b_is_device) { /* if both are device groups, we want the explicit [device] group first. */ From 527061ed4866eaaf122ca92a213f626a8d43af00 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 8 Sep 2022 17:05:53 +0200 Subject: [PATCH 3/3] glib-aux/trivial: fix typo in code comment --- src/libnm-glib-aux/nm-keyfile-aux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-keyfile-aux.c b/src/libnm-glib-aux/nm-keyfile-aux.c index 1cae28b49a..20a4690f2f 100644 --- a/src/libnm-glib-aux/nm-keyfile-aux.c +++ b/src/libnm-glib-aux/nm-keyfile-aux.c @@ -442,7 +442,7 @@ nm_key_file_db_prune(NMKeyFileDB *self, _LOGD("prune keyfile of old entries: \"%s\"", self->filename); if (!self->groups_pruned) { - /* When we prune the first time, we swap the GKeyfile instance. + /* When we prune the first time, we swap the GKeyFile instance. * The instance loaded from disk might have unrelated groups and * comments. Let's get rid of them by creating a new instance. *