From 51ff2865c3237e70a51f6f3e587c46e4c0959551 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Mar 2021 08:43:22 +0100 Subject: [PATCH 1/6] refstr: drop "const" from argument of NM_IS_REF_STRING() NMRefString has only const fields itself, and all operations (except ref/unref) don't mutate the instance. As such, the type is already immutable, and using "const" is redundant and unnecessary. Drop "const" from all API of NMRefString. --- src/libnm-glib-aux/nm-ref-string.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-ref-string.h b/src/libnm-glib-aux/nm-ref-string.h index 97c263b08f..ce3e21bf92 100644 --- a/src/libnm-glib-aux/nm-ref-string.h +++ b/src/libnm-glib-aux/nm-ref-string.h @@ -58,7 +58,7 @@ nm_ref_string_equals_str(NMRefString *rstr, const char *s) } static inline gboolean -NM_IS_REF_STRING(const NMRefString *rstr) +NM_IS_REF_STRING(NMRefString *rstr) { #if NM_MORE_ASSERTS > 10 if (rstr) { From bec8928341f9588611e9ef413e7538a5e50ced28 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 17 Mar 2021 13:12:46 +0100 Subject: [PATCH 2/6] refstr: drop internal struct RefString and pack NMRefString Previously, NMRefString was the public part of the struct, while there was an internal RefString struct with private fields. That might make sense if we would need to preserve some stable ABI, but we don't because this is all internal (unstable) API. It also might make sense to hide fields, but in practice that is not necessary because the leading underscore is indicator enough that these are private fields that are not supposed to be touched (unless you really know what you do). So, drop RefString and move all fields in the public NMRefString. The advantage is that we can later inline certain trivial functions, that we otherwise couldn't. Also, drop the "str" pointer and only use the "str" array field. The pointer existed so that during nm_ref_string_new_len() we could create a lookup needle with external str pointer. That is now solved differently by using "len == G_MAXSIZE" as indicator that this is a special lookup instance. The advantage is that we save one pointer field per NMRefString, that we reduce the redundancy of the data, and that we don't need the additional indirection. --- src/libnm-glib-aux/nm-ref-string.c | 123 +++++++++++++++-------------- src/libnm-glib-aux/nm-ref-string.h | 16 +++- 2 files changed, 79 insertions(+), 60 deletions(-) diff --git a/src/libnm-glib-aux/nm-ref-string.c b/src/libnm-glib-aux/nm-ref-string.c index 0804a05782..596656e1ea 100644 --- a/src/libnm-glib-aux/nm-ref-string.c +++ b/src/libnm-glib-aux/nm-ref-string.c @@ -6,69 +6,71 @@ /*****************************************************************************/ -typedef struct { - NMRefString r; - volatile int ref_count; - char str_data[]; -} RefString; - G_LOCK_DEFINE_STATIC(gl_lock); static GHashTable *gl_hash; -/* the first field of NMRefString is a pointer to the NUL terminated string. - * This also allows to compare strings with nm_pstr_equal(), although, pointer - * equality might be better. */ -G_STATIC_ASSERT(G_STRUCT_OFFSET(NMRefString, str) == 0); -G_STATIC_ASSERT(G_STRUCT_OFFSET(RefString, r) == 0); -G_STATIC_ASSERT(G_STRUCT_OFFSET(RefString, r.str) == 0); - /*****************************************************************************/ +static void +_ref_string_get(const NMRefString *rstr, const char **out_str, gsize *out_len) +{ + if (rstr->len == G_MAXSIZE) { + *out_len = rstr->_priv_lookup.l_len; + *out_str = rstr->_priv_lookup.l_str; + } else { + *out_len = rstr->len; + *out_str = rstr->str; + } +} + static guint _ref_string_hash(gconstpointer ptr) { - const RefString *a = ptr; - NMHashState h; + const char *cstr; + gsize len; - nm_hash_init(&h, 1463435489u); - nm_hash_update(&h, a->r.str, a->r.len); - return nm_hash_complete(&h); + _ref_string_get(ptr, &cstr, &len); + return nm_hash_mem(1463435489u, cstr, len); } static gboolean -_ref_string_equal(gconstpointer pa, gconstpointer pb) +_ref_string_equal(gconstpointer ptr_a, gconstpointer ptr_b) { - const RefString *a = pa; - const RefString *b = pb; + const char *cstr_a; + const char *cstr_b; + gsize len_a; + gsize len_b; - return a->r.len == b->r.len && memcmp(a->r.str, b->r.str, a->r.len) == 0; + _ref_string_get(ptr_a, &cstr_a, &len_a); + _ref_string_get(ptr_b, &cstr_b, &len_b); + + return len_a == len_b && memcmp(cstr_a, cstr_b, len_a) == 0; } /*****************************************************************************/ static void -_ASSERT(const RefString *rstr0) +_ASSERT(const NMRefString *rstr) { int r; - nm_assert(rstr0); + nm_assert(rstr); if (NM_MORE_ASSERTS > 0) { - r = g_atomic_int_get(&rstr0->ref_count); + r = g_atomic_int_get(&rstr->_ref_count); nm_assert(r > 0); nm_assert(r < G_MAXINT); } - nm_assert(rstr0->r.str == rstr0->str_data); - nm_assert(rstr0->r.str[rstr0->r.len] == '\0'); + nm_assert(rstr->str[rstr->len] == '\0'); if (NM_MORE_ASSERTS > 10) { G_LOCK(gl_lock); - r = g_atomic_int_get(&rstr0->ref_count); + r = g_atomic_int_get(&rstr->_ref_count); nm_assert(r > 0); nm_assert(r < G_MAXINT); - nm_assert(rstr0 == g_hash_table_lookup(gl_hash, rstr0)); + nm_assert(rstr == g_hash_table_lookup(gl_hash, rstr)); G_UNLOCK(gl_lock); } } @@ -117,83 +119,88 @@ _ASSERT(const RefString *rstr0) NMRefString * nm_ref_string_new_len(const char *cstr, gsize len) { - RefString *rstr0; + NMRefString *rstr; + + /* @len cannot be close to G_MAXSIZE. For one, that would mean our call + * to malloc() below overflows. Also, we use G_MAXSIZE as special length + * to indicate using _priv_lookup. */ + nm_assert(len < G_MAXSIZE - G_STRUCT_OFFSET(NMRefString, str) - 1u); G_LOCK(gl_lock); if (G_UNLIKELY(!gl_hash)) { gl_hash = g_hash_table_new_full(_ref_string_hash, _ref_string_equal, g_free, NULL); - rstr0 = NULL; + rstr = NULL; } else { NMRefString rr_lookup = { - .len = len, - .str = cstr, + .len = G_MAXSIZE, + ._priv_lookup = + { + .l_len = len, + .l_str = cstr, + }, }; - rstr0 = g_hash_table_lookup(gl_hash, &rr_lookup); + rstr = g_hash_table_lookup(gl_hash, &rr_lookup); } - if (rstr0) { + if (rstr) { nm_assert(({ - int r = g_atomic_int_get(&rstr0->ref_count); + int r = g_atomic_int_get(&rstr->_ref_count); (r >= 0 && r < G_MAXINT); })); - g_atomic_int_inc(&rstr0->ref_count); + g_atomic_int_inc(&rstr->_ref_count); } else { - rstr0 = g_malloc(sizeof(RefString) + 1 + len); - rstr0->ref_count = 1; - *((gsize *) &rstr0->r.len) = len; - *((const char **) &rstr0->r.str) = rstr0->str_data; + rstr = g_malloc((G_STRUCT_OFFSET(NMRefString, str) + 1u) + len); if (len > 0) - memcpy(rstr0->str_data, cstr, len); - rstr0->str_data[len] = '\0'; + memcpy((char *) rstr->str, cstr, len); + ((char *) rstr->str)[len] = '\0'; + *((gsize *) &rstr->len) = len; + rstr->_ref_count = 1; - if (!g_hash_table_add(gl_hash, rstr0)) + if (!g_hash_table_add(gl_hash, rstr)) nm_assert_not_reached(); } G_UNLOCK(gl_lock); - return &rstr0->r; + return rstr; } NMRefString * nm_ref_string_ref(NMRefString *rstr) { - RefString *const rstr0 = (RefString *) rstr; - if (!rstr) return NULL; - _ASSERT(rstr0); + _ASSERT(rstr); - g_atomic_int_inc(&rstr0->ref_count); - return &rstr0->r; + g_atomic_int_inc(&rstr->_ref_count); + return rstr; } void _nm_ref_string_unref_non_null(NMRefString *rstr) { - RefString *const rstr0 = (RefString *) rstr; - int r; + int r; - _ASSERT(rstr0); + _ASSERT(rstr); /* fast-path: first try to decrement the ref-count without bringing it * to zero. */ - r = rstr0->ref_count; - if (G_LIKELY(r > 1 && g_atomic_int_compare_and_exchange(&rstr0->ref_count, r, r - 1))) + r = rstr->_ref_count; + if (G_LIKELY(r > 1 && g_atomic_int_compare_and_exchange(&rstr->_ref_count, r, r - 1))) return; /* We apparently are about to return the last reference. Take a lock. */ G_LOCK(gl_lock); - nm_assert(g_hash_table_lookup(gl_hash, rstr0) == rstr0); + nm_assert(g_hash_table_lookup(gl_hash, rstr) == rstr); - if (G_LIKELY(g_atomic_int_dec_and_test(&rstr0->ref_count))) { - if (!g_hash_table_remove(gl_hash, rstr0)) + if (G_LIKELY(g_atomic_int_dec_and_test(&rstr->_ref_count))) { + if (!g_hash_table_remove(gl_hash, rstr)) nm_assert_not_reached(); } diff --git a/src/libnm-glib-aux/nm-ref-string.h b/src/libnm-glib-aux/nm-ref-string.h index ce3e21bf92..9a3785bdf0 100644 --- a/src/libnm-glib-aux/nm-ref-string.h +++ b/src/libnm-glib-aux/nm-ref-string.h @@ -6,8 +6,20 @@ /*****************************************************************************/ typedef struct _NMRefString { - const char *const str; - const gsize len; + const gsize len; + union { + struct { + volatile int _ref_count; + const char str[]; + }; + struct { + /* This union field is only used during lookup by external string. + * In that case, len will be set to G_MAXSIZE, and the actual len/str values + * are set in _priv_lookup. */ + gsize l_len; + const char *l_str; + } _priv_lookup; + }; } NMRefString; /*****************************************************************************/ From 19d4027824882cc56b1308e3547a767cfd510b7b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 17 Mar 2021 17:34:18 +0100 Subject: [PATCH 3/6] refstr: inline nm_ref_string_{ref,unref}() In the fast path, ref/unref is just a atomic increment/decrement of an integer. Let's inline that. --- src/libnm-glib-aux/nm-ref-string.c | 30 ++------------------ src/libnm-glib-aux/nm-ref-string.h | 45 ++++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/src/libnm-glib-aux/nm-ref-string.c b/src/libnm-glib-aux/nm-ref-string.c index 596656e1ea..6fb39024e6 100644 --- a/src/libnm-glib-aux/nm-ref-string.c +++ b/src/libnm-glib-aux/nm-ref-string.c @@ -49,8 +49,8 @@ _ref_string_equal(gconstpointer ptr_a, gconstpointer ptr_b) /*****************************************************************************/ -static void -_ASSERT(const NMRefString *rstr) +void +_nm_assert_nm_ref_string(NMRefString *rstr) { int r; @@ -168,33 +168,9 @@ nm_ref_string_new_len(const char *cstr, gsize len) return rstr; } -NMRefString * -nm_ref_string_ref(NMRefString *rstr) -{ - if (!rstr) - return NULL; - - _ASSERT(rstr); - - g_atomic_int_inc(&rstr->_ref_count); - return rstr; -} - void -_nm_ref_string_unref_non_null(NMRefString *rstr) +_nm_ref_string_unref_slow_path(NMRefString *rstr) { - int r; - - _ASSERT(rstr); - - /* fast-path: first try to decrement the ref-count without bringing it - * to zero. */ - r = rstr->_ref_count; - if (G_LIKELY(r > 1 && g_atomic_int_compare_and_exchange(&rstr->_ref_count, r, r - 1))) - return; - - /* We apparently are about to return the last reference. Take a lock. */ - G_LOCK(gl_lock); nm_assert(g_hash_table_lookup(gl_hash, rstr) == rstr); diff --git a/src/libnm-glib-aux/nm-ref-string.h b/src/libnm-glib-aux/nm-ref-string.h index 9a3785bdf0..c55abdf9cf 100644 --- a/src/libnm-glib-aux/nm-ref-string.h +++ b/src/libnm-glib-aux/nm-ref-string.h @@ -24,6 +24,18 @@ typedef struct _NMRefString { /*****************************************************************************/ +void _nm_assert_nm_ref_string(NMRefString *rstr); + +static inline void +nm_assert_nm_ref_string(NMRefString *rstr) +{ +#if NM_MORE_ASSERTS + _nm_assert_nm_ref_string(rstr); +#endif +} + +/*****************************************************************************/ + NMRefString *nm_ref_string_new_len(const char *cstr, gsize len); static inline NMRefString * @@ -32,17 +44,40 @@ nm_ref_string_new(const char *cstr) return cstr ? nm_ref_string_new_len(cstr, strlen(cstr)) : NULL; } -NMRefString *nm_ref_string_ref(NMRefString *rstr); -void _nm_ref_string_unref_non_null(NMRefString *rstr); +/*****************************************************************************/ + +static inline NMRefString * +nm_ref_string_ref(NMRefString *rstr) +{ + if (rstr) { + nm_assert_nm_ref_string(rstr); + g_atomic_int_inc(&rstr->_ref_count); + } + return rstr; +} + +void _nm_ref_string_unref_slow_path(NMRefString *rstr); static inline void nm_ref_string_unref(NMRefString *rstr) { - if (rstr) - _nm_ref_string_unref_non_null(rstr); + int r; + + if (!rstr) + return; + + nm_assert_nm_ref_string(rstr); + + /* fast-path: first try to decrement the ref-count without bringing it + * to zero. */ + r = rstr->_ref_count; + if (G_LIKELY(r > 1 && g_atomic_int_compare_and_exchange(&rstr->_ref_count, r, r - 1))) + return; + + _nm_ref_string_unref_slow_path(rstr); } -NM_AUTO_DEFINE_FCN_VOID0(NMRefString *, _nm_auto_ref_string, _nm_ref_string_unref_non_null); +NM_AUTO_DEFINE_FCN_VOID(NMRefString *, _nm_auto_ref_string, nm_ref_string_unref); #define nm_auto_ref_string nm_auto(_nm_auto_ref_string) /*****************************************************************************/ From 8ba67aa705ee3516f6e6d5007b2abfb88f046f2d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Mar 2021 08:50:09 +0100 Subject: [PATCH 4/6] refstr: use nm_assert_nm_ref_string() in NM_IS_REF_STRING() --- src/libnm-glib-aux/nm-ref-string.h | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/libnm-glib-aux/nm-ref-string.h b/src/libnm-glib-aux/nm-ref-string.h index c55abdf9cf..1353b4988e 100644 --- a/src/libnm-glib-aux/nm-ref-string.h +++ b/src/libnm-glib-aux/nm-ref-string.h @@ -107,14 +107,8 @@ nm_ref_string_equals_str(NMRefString *rstr, const char *s) static inline gboolean NM_IS_REF_STRING(NMRefString *rstr) { -#if NM_MORE_ASSERTS > 10 - if (rstr) { - nm_auto_ref_string NMRefString *r2 = NULL; - - r2 = nm_ref_string_new_len(rstr->str, rstr->len); - nm_assert(rstr == r2); - } -#endif + if (rstr) + nm_assert_nm_ref_string(rstr); /* Technically, %NULL is also a valid NMRefString (according to nm_ref_string_new(), * nm_ref_string_get_str() and nm_ref_string_unref()). However, NM_IS_REF_STRING() From 4f935d1d6b573cb56884d9dae9546305889404fb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 17 Mar 2021 14:05:41 +0100 Subject: [PATCH 5/6] refstr: add NM_REF_STRING_UPCAST() helper Imaging you track a list of NMRefString instances. You could directly expose them as strv array, but then you need a way from the string back to the NMRefString instance. That's easy to do. Add NM_REF_STRING_UPCAST() for that. --- src/libnm-glib-aux/nm-ref-string.h | 13 +++++++++++++ src/libnm-glib-aux/tests/test-shared-general.c | 3 +++ 2 files changed, 16 insertions(+) diff --git a/src/libnm-glib-aux/nm-ref-string.h b/src/libnm-glib-aux/nm-ref-string.h index 1353b4988e..8ef44d43f3 100644 --- a/src/libnm-glib-aux/nm-ref-string.h +++ b/src/libnm-glib-aux/nm-ref-string.h @@ -117,4 +117,17 @@ NM_IS_REF_STRING(NMRefString *rstr) return !!rstr; } +static inline NMRefString * +NM_REF_STRING_UPCAST(const char *str) +{ + NMRefString *rstr; + + if (!str) + return NULL; + + rstr = (gpointer)(((char *) str) - G_STRUCT_OFFSET(NMRefString, str)); + nm_assert_nm_ref_string(rstr); + return rstr; +} + #endif /* __NM_REF_STRING_H__ */ diff --git a/src/libnm-glib-aux/tests/test-shared-general.c b/src/libnm-glib-aux/tests/test-shared-general.c index efb0937b54..f30240b347 100644 --- a/src/libnm-glib-aux/tests/test-shared-general.c +++ b/src/libnm-glib-aux/tests/test-shared-general.c @@ -596,10 +596,13 @@ test_nm_ref_string(void) nm_auto_ref_string NMRefString *s1 = NULL; NMRefString * s2; + g_assert(NULL == NM_REF_STRING_UPCAST(NULL)); + s1 = nm_ref_string_new("hallo"); g_assert(s1); g_assert_cmpstr(s1->str, ==, "hallo"); g_assert_cmpint(s1->len, ==, strlen("hallo")); + g_assert(s1 == NM_REF_STRING_UPCAST(s1->str)); s2 = nm_ref_string_new("hallo"); g_assert(s2 == s1); From 33a69bbde6dc128999d4ba74ee6e1fcb35b37ce5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Mar 2021 09:54:12 +0100 Subject: [PATCH 6/6] refstr: be extra careful about calling memcpy() with dangling pointer --- src/libnm-glib-aux/nm-ref-string.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-ref-string.c b/src/libnm-glib-aux/nm-ref-string.c index 6fb39024e6..93e97c7931 100644 --- a/src/libnm-glib-aux/nm-ref-string.c +++ b/src/libnm-glib-aux/nm-ref-string.c @@ -44,7 +44,11 @@ _ref_string_equal(gconstpointer ptr_a, gconstpointer ptr_b) _ref_string_get(ptr_a, &cstr_a, &len_a); _ref_string_get(ptr_b, &cstr_b, &len_b); - return len_a == len_b && memcmp(cstr_a, cstr_b, len_a) == 0; + /* memcmp() accepts "n=0" argument, but it's not clear whether in that case + * all pointers must still be valid. The input pointer might be provided by + * the user via nm_ref_string_new_len(), and for len=0 we want to allow + * also invalid pointers. Hence, this extra "len_a==0" check. */ + return len_a == len_b && (len_a == 0 || (memcmp(cstr_a, cstr_b, len_a) == 0)); } /*****************************************************************************/