From 208381f78b5b2af7bd546cd4954fb9bedc0b261e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 25 Oct 2023 20:27:10 +0200 Subject: [PATCH 1/3] glib-aux/trivial: improve code comment for nm_g_array_index_p() Explain why it exists. --- src/libnm-glib-aux/nm-shared-utils.h | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 941b7fd986..fb88578b28 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1941,7 +1941,27 @@ nm_g_array_unref(GArray *arr) * When accessing index zero, then this returns NULL if-and-only-if * "arr" is NULL or "arr->data" is NULL. In all other cases, this * returns the pointer &((Type*) arr->data)[idx]. Note that the pointer - * may not be followed, if "idx" is equal to "arr->len". */ + * may not be followed, if "idx" is equal to "arr->len". + * + * The reason to allow access one element past the length is the + * following usage: + * + * ptr = nm_g_array_index_p(arr, Type, 0); + * end = nm_g_array_index_p(arr, Type, length); + * for (; ptr < end; ptr++) { ... } + * + * Another usage is to get a buffer, if the length might be zero. If + * length is zero, you cannot dereference the pointer, but it can be convenient + * to not require special casing: + * + * // length might be zero. + * nm_memdup(nm_g_array_index_p(arr, Type, length), sizeof(Type) * length); + * + * Note that in C, it's valid point one past the end of an array. So getting + * a pointer at index "length" is valid, and what nm_g_array_index_p() allows. + * If you don't need that, nm_g_array_index() is usually preferable, + * because it asserts against access at index "length". + */ #define nm_g_array_index_p(arr, Type, idx) \ ({ \ const GArray *const _arr_55 = (arr); \ From 0554ef380863618e02f08fa614fde50f49eeb2ad Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 25 Oct 2023 20:33:26 +0200 Subject: [PATCH 2/3] all: use nm_g_array_index() instead of g_array_index() nm_g_array_index() performs additional nm_assert() checks for correctness. In this case, it's pretty clear that the assertion will hold and that the code is correct. Note that most of the time when having assertions, we expect that they hold. Since nm_assert() is disabled in release build, arguing that an assertion holds is not a strong argument against having the assert (they are always supposed to hold, quite obviously so!). The reason to change is that we should use the wrappers that perform additional checks. Especially when the additional checks are nm_assert() or static-asserts, as they are not present in release builds. To find how well we are doing in this regard we can check `git grep -w g_array_index`. If that gives many uses of the unchecked function, then we cannot manually check them all to be really obviously correct. Instead, we should not use g_array_index() and trivially see that all array accesses are guarded by assertions. "checkpatch.pl" also recommends against g_array_index(). --- src/libnm-glib-aux/nm-enum-utils.c | 7 ++++--- src/nmcli/gen-metadata-nm-settings-nmcli.c | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libnm-glib-aux/nm-enum-utils.c b/src/libnm-glib-aux/nm-enum-utils.c index 0d63278045..212c0021e0 100644 --- a/src/libnm-glib-aux/nm-enum-utils.c +++ b/src/libnm-glib-aux/nm-enum-utils.c @@ -325,7 +325,7 @@ _nm_utils_enum_get_values(GType type, int from, int to) GPtrArray *values = g_ptr_array_sized_new(values_full->len + 1); for (i = 0; i < values_full->len; i++) { - NMUtilsEnumValueInfoFull *v = &g_array_index(values_full, NMUtilsEnumValueInfoFull, i); + NMUtilsEnumValueInfoFull *v = &nm_g_array_index(values_full, NMUtilsEnumValueInfoFull, i); g_ptr_array_add(values, (gpointer) v->nick); } @@ -418,8 +418,9 @@ _nm_utils_enum_get_values_full(GType type, g_array_set_clear_func(array, (GDestroyNotify) _free_value_info_full); for (i = 0; i < array->len; i++) { - NMUtilsEnumValueInfoFull *vi_full = &g_array_index(array, NMUtilsEnumValueInfoFull, i); - GPtrArray *aliases = g_ptr_array_new(); + NMUtilsEnumValueInfoFull *vi_full = + &nm_g_array_index(array, NMUtilsEnumValueInfoFull, i); + GPtrArray *aliases = g_ptr_array_new(); const NMUtilsEnumValueInfo *vi; for (vi = value_infos; vi && vi->nick; vi++) { diff --git a/src/nmcli/gen-metadata-nm-settings-nmcli.c b/src/nmcli/gen-metadata-nm-settings-nmcli.c index f8dc303628..2def909565 100644 --- a/src/nmcli/gen-metadata-nm-settings-nmcli.c +++ b/src/nmcli/gen-metadata-nm-settings-nmcli.c @@ -266,7 +266,7 @@ _append_enum_valid_values(GType g_type, GArray *values = _nm_utils_enum_get_values_full(g_type, min, max, value_infos); for (i = 0; i < values->len; i++) { - NMUtilsEnumValueInfoFull *val = &g_array_index(values, NMUtilsEnumValueInfoFull, i); + NMUtilsEnumValueInfoFull *val = &nm_g_array_index(values, NMUtilsEnumValueInfoFull, i); g_string_assign(names, val->nick); From 563a18513514f8eeb2d83849bbcbd110e4888b8c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 30 Oct 2023 17:21:58 +0100 Subject: [PATCH 3/3] all: use nm_g_array_first_p() instead of nm_g_array_index_p(,,0) where applicable --- src/libnm-core-impl/nm-setting.c | 4 ++-- src/nmtui/nmt-8021x-fields.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index 276dfd3475..efbdc3f5da 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -355,7 +355,7 @@ _nm_setting_class_commit(NMSettingClass *setting_class, guint k; nm_assert(!_nm_sett_info_property_find_in_array( - nm_g_array_index_p(properties_override, NMSettInfoProperty, 0), + nm_g_array_first_p(properties_override, NMSettInfoProperty), i, p->name)); for (k = 0; k < n_property_specs; k++) { @@ -374,7 +374,7 @@ _nm_setting_class_commit(NMSettingClass *setting_class, NMSettInfoProperty *p; if (_nm_sett_info_property_find_in_array( - nm_g_array_index_p(properties_override, NMSettInfoProperty, 0), + nm_g_array_first_p(properties_override, NMSettInfoProperty), override_len, name)) continue; diff --git a/src/nmtui/nmt-8021x-fields.c b/src/nmtui/nmt-8021x-fields.c index 3a2185cfbb..a323be62d7 100644 --- a/src/nmtui/nmt-8021x-fields.c +++ b/src/nmtui/nmt-8021x-fields.c @@ -552,7 +552,7 @@ nmt_8021x_fields_constructed(GObject *object) entry.id = (char *) eap_method_descs[i].id; g_array_append_val(entries, entry); } - priv->authentication = nmt_newt_popup_new(nm_g_array_index_p(entries, NmtNewtPopupEntry, 0)); + priv->authentication = nmt_newt_popup_new(nm_g_array_first_p(entries, NmtNewtPopupEntry)); nmt_editor_grid_append(grid, "Authentication", NMT_NEWT_WIDGET(priv->authentication), NULL); widget = nmt_newt_stack_new();