libnm: add API to NMLldpNeighbor to access by indexth/lldp-neighbor-cleanup

NMLldpNeighbor tries to abstract the way how tracking of the attributes
is implemented. For example, it does not offer an API to lookup by index
and it (previously) did not guarantee any sort order of the attribute
names.

Let's be more explicit about what and how the attributes are tracked. NMLldpNeighbor
is a list of name-gvariant pairs. Let's commit to some things:

 - the instance shall be immutable and there is no API for changing them
   (or even for creating them, aside nm_lldp_neighbor_new(), which is pointless).

 - when we expose the list of names, the names shall be sorted.

 - as we sort them, each attribute has a fixed index.

Add API to access the attributes by index. Also add API to access the
list of names without requiring to copy the list.

While at it, also rework the way we track the data. NMLldpNeighbor is
simple, the attributes don't change. We don't need a GHashTable for
that, instead track the attributes in a way that is more suited to the
way we use it. Since we have a sorted list of names, we still can do
fast binary search to find an attribute by name.

3 files changed, 183 insertions, 35 deletions

diff --git a/src/libnm-client-impl/libnm.ver b/src/libnm-client-impl/libnm.ver
index 693591efe6..7ac44f61a7 100644
--- a/src/libnm-client-impl/libnm.ver
+++ b/src/libnm-client-impl/libnm.ver
@@ -1787,5 +1787,7 @@ global:
 
 libnm_1_32_0 {
 global:
+	nm_lldp_neighbor_get_attr_at_index;
+	nm_lldp_neighbor_get_attr_names2;
 	nm_setting_match_new;
 } libnm_1_30_0;
diff --git a/src/libnm-client-impl/nm-device.c b/src/libnm-client-impl/nm-device.c
index dd5f7d4889..5f02cd9f60 100644
--- a/src/libnm-client-impl/nm-device.c
+++ b/src/libnm-client-impl/nm-device.c
@@ -130,12 +130,15 @@ G_DEFINE_ABSTRACT_TYPE(NMDevice, nm_device, NM_TYPE_OBJECT);
 
 static gboolean connection_compatible(NMDevice *device, NMConnection *connection, GError **error);
 static NMLldpNeighbor *_nm_lldp_neighbor_dup(NMLldpNeighbor *neighbor);
+static NMLldpNeighbor *_nm_lldp_neighbor_new(guint n_attrs);
 
 /*****************************************************************************/
 
 struct _NMLldpNeighbor {
-    int         refcount;
-    GHashTable *attrs;
+    int        refcount;
+    guint      n_attrs;
+    GVariant **attr_vals;
+    char *     attr_names[];
 };
 
 G_DEFINE_BOXED_TYPE(NMLldpNeighbor, nm_lldp_neighbor, _nm_lldp_neighbor_dup, nm_lldp_neighbor_unref)
@@ -238,24 +241,77 @@ _notify_update_prop_lldp_neighbors(NMClient *              client,
     new = g_ptr_array_new_with_free_func((GDestroyNotify) nm_lldp_neighbor_unref);
 
     if (value) {
+        gs_unref_array GArray *tmp_array = NULL;
+
         g_variant_iter_init(&iter, value);
         while (g_variant_iter_next(&iter, "a{sv}", &attrs_iter)) {
-            GVariant *      attr_variant;
-            const char *    attr_name;
-            NMLldpNeighbor *neigh;
+            NMLldpNeighbor *   neighbor;
+            GVariant *         attr_variant;
+            const char *       attr_name;
+            NMUtilsNamedValue *named_val;
+            guint              i;
+            guint              j;
+
+            if (!tmp_array)
+                tmp_array = g_array_new(FALSE, FALSE, sizeof(NMUtilsNamedValue));
+            else
+                g_array_set_size(tmp_array, 0);
 
-            /* Note that there is no public API to mutate a NMLldpNeighbor instance.
-             * This is the only place where we actually mutate it. */
-            neigh = nm_lldp_neighbor_new();
             while (g_variant_iter_next(attrs_iter, "{&sv}", &attr_name, &attr_variant)) {
                 if (attr_name[0] == '\0') {
                     g_variant_unref(attr_variant);
                     continue;
                 }
-                g_hash_table_insert(neigh->attrs, g_strdup(attr_name), attr_variant);
+                named_val  = nm_g_array_append_new(tmp_array, NMUtilsNamedValue);
+                *named_val = (NMUtilsNamedValue){
+                    .name      = g_strdup(attr_name),
+                    .value_ptr = attr_variant,
+                };
+            }
+
+            if (tmp_array->len > 1u) {
+                g_array_sort(tmp_array, nm_strcmp_p);
+
+                /* Remove duplicate entries. We don't actually expect them as NetworkManager
+                 * should never expose them. Still, make sure! */
+                named_val = (NMUtilsNamedValue *) (gpointer) tmp_array->data;
+                for (i = 1, j = 1; i < tmp_array->len; i++) {
+                    if (G_UNLIKELY(nm_streq(named_val[j - 1].name, named_val[i].name))) {
+                        /* duplicate. We keep the latter. */
+                        j--;
+                        g_free((char *) named_val[j].name);
+                        g_variant_unref(named_val[j].value_ptr);
+                    }
+
+                    if (j != i) {
+                        named_val[j].name      = named_val[i].name;
+                        named_val[j].value_ptr = named_val[i].value_ptr;
+                    }
+                    j++;
+                }
+                if (j != tmp_array->len)
+                    g_array_set_size(tmp_array, j);
             }
 
-            g_ptr_array_add(new, neigh);
+            /* Note that there is no public API to mutate a NMLldpNeighbor instance.
+             * This is the only place where we actually create (mutate) it. */
+
+            neighbor = _nm_lldp_neighbor_new(tmp_array->len);
+
+            named_val = (NMUtilsNamedValue *) (gpointer) tmp_array->data;
+            for (i = 0; i < tmp_array->len; i++) {
+                /* we transfer ownership here! */
+                neighbor->attr_names[i] = (char *) named_val[i].name;
+                neighbor->attr_vals[i]  = named_val[i].value_ptr;
+            }
+            neighbor->attr_names[i] = NULL;
+            neighbor->attr_vals[i]  = NULL;
+
+            nm_assert(neighbor->n_attrs == i);
+            nm_assert(NM_PTRARRAY_LEN(neighbor->attr_names) == neighbor->n_attrs);
+            nm_assert(NM_PTRARRAY_LEN(neighbor->attr_vals) == neighbor->n_attrs);
+
+            g_ptr_array_add(new, neighbor);
 
             g_variant_iter_free(attrs_iter);
         }
@@ -2869,10 +2925,26 @@ nm_device_get_setting_type(NMDevice *device)
 /*****************************************************************************/
 
 static gboolean
-NM_IS_LLDP_NEIGHBOR(const NMLldpNeighbor *self)
+NM_IS_LLDP_NEIGHBOR(const NMLldpNeighbor *neighbor)
 {
-    nm_assert(!self || (self->refcount > 0 && self->attrs));
-    return self && self->refcount > 0;
+    nm_assert(!neighbor || neighbor->refcount > 0);
+    if (NM_MORE_ASSERTS > 10)
+        nm_assert(!neighbor || NM_PTRARRAY_LEN(neighbor->attr_names) == neighbor->n_attrs);
+    return neighbor && neighbor->refcount > 0;
+}
+
+static NMLldpNeighbor *
+_nm_lldp_neighbor_new(guint n_attrs)
+{
+    NMLldpNeighbor *neighbor;
+
+    neighbor = g_malloc(G_STRUCT_OFFSET(NMLldpNeighbor, attr_names)
+                        + (((gsize) n_attrs) + 1u) * 2u * sizeof(gpointer));
+
+    neighbor->refcount  = 1;
+    neighbor->n_attrs   = n_attrs;
+    neighbor->attr_vals = (GVariant **) &neighbor->attr_names[(((gsize) n_attrs) + 1u)];
+    return neighbor;
 }
 
 /**
@@ -2895,18 +2967,12 @@ NM_IS_LLDP_NEIGHBOR(const NMLldpNeighbor *self)
 NMLldpNeighbor *
 nm_lldp_neighbor_new(void)
 {
-    NMLldpNeighbor *neigh;
-
-    neigh  = g_slice_new(NMLldpNeighbor);
-    *neigh = (NMLldpNeighbor){
-        .refcount = 1,
-        .attrs    = g_hash_table_new_full(nm_str_hash,
-                                       g_str_equal,
-                                       g_free,
-                                       (GDestroyNotify) g_variant_unref),
-    };
+    NMLldpNeighbor *neighbor;
 
-    return neigh;
+    neighbor                = _nm_lldp_neighbor_new(0);
+    neighbor->attr_vals[0]  = NULL;
+    neighbor->attr_names[0] = NULL;
+    return neighbor;
 }
 
 static NMLldpNeighbor *
@@ -2953,12 +3019,79 @@ nm_lldp_neighbor_ref(NMLldpNeighbor *neighbor)
 void
 nm_lldp_neighbor_unref(NMLldpNeighbor *neighbor)
 {
+    guint i;
+
     g_return_if_fail(NM_IS_LLDP_NEIGHBOR(neighbor));
 
-    if (g_atomic_int_dec_and_test(&neighbor->refcount)) {
-        g_hash_table_unref(neighbor->attrs);
-        nm_g_slice_free(neighbor);
+    if (!g_atomic_int_dec_and_test(&neighbor->refcount))
+        return;
+
+    for (i = 0; i < neighbor->n_attrs; i++) {
+        g_free(neighbor->attr_names[i]);
+        g_variant_unref(neighbor->attr_vals[i]);
+    }
+    g_free(neighbor);
+}
+
+/**
+ * nm_lldp_neighbor_get_attr_names2:
+ * @neighbor: the #NMLldpNeighbor
+ * @out_len: (out) (allow-none): the number of attributes.
+ *
+ * Gets an array of attribute names available for @neighbor.
+ *
+ * This is like nm_lldp_neighbor_get_attr_names() but it does not copy
+ * the names nor the strv array. Note that #NMLldpNeighbor is immutable,
+ * thus the values stay valid as long as @neighbor is.
+ *
+ * The names are sorted asciibetically.
+ *
+ * Returns: (array length=out_len) (transfer none): a %NULL-terminated array of attribute names.
+ *
+ * Since: 1.32
+ **/
+const char *const *
+nm_lldp_neighbor_get_attr_names2(NMLldpNeighbor *neighbor, gsize *out_len)
+{
+    g_return_val_if_fail(NM_IS_LLDP_NEIGHBOR(neighbor), NULL);
+
+    NM_SET_OUT(out_len, neighbor->n_attrs);
+    return (const char *const *) neighbor->attr_names;
+}
+
+/**
+ * nm_lldp_neighbor_get_attr_at_index:
+ * @neighbor: the #NMLldpNeighbor
+ * @idx: the index of the attribute that is requested.
+ * @name: (out) (allow-none): the corresponding name of the attribute.
+ *
+ * Note that nm_lldp_neighbor_get_attr_names2() gives the number of attributes
+ * that are available. Also, nm_lldp_neighbor_get_attr_names2() gives the attribute
+ * names with the same order as the numbering via @idx.
+ *
+ * Note that it is permissible to pass @idx out of range. In that case, @name
+ * will be set to %NULL and %NULL will be returned.
+ *
+ * Returns: (transfer none): the #GVariant attribute at the given @idx
+ *   or %NULL if the index is out of range.
+ *
+ * Since: 1.32
+ */
+GVariant *
+nm_lldp_neighbor_get_attr_at_index(NMLldpNeighbor *neighbor, gsize idx, const char **name)
+{
+    g_return_val_if_fail(NM_IS_LLDP_NEIGHBOR(neighbor), NULL);
+
+    if (idx >= neighbor->n_attrs) {
+        /* we accept out of range access and return %NULL. It's even documented behavior,
+         * so the caller may also start accessing the attributes without knowing how
+         * many there are (and stop when getting the first %NULL). */
+        NM_SET_OUT(name, NULL);
+        return NULL;
     }
+
+    NM_SET_OUT(name, neighbor->attr_names[idx]);
+    return neighbor->attr_vals[idx];
 }
 
 /**
@@ -2976,10 +3109,22 @@ nm_lldp_neighbor_unref(NMLldpNeighbor *neighbor)
 GVariant *
 nm_lldp_neighbor_get_attr_value(NMLldpNeighbor *neighbor, const char *name)
 {
+    gssize idx;
+
     g_return_val_if_fail(NM_IS_LLDP_NEIGHBOR(neighbor), FALSE);
     g_return_val_if_fail(name && name[0], FALSE);
 
-    return g_hash_table_lookup(neighbor->attrs, name);
+    idx = nm_utils_ptrarray_find_binary_search((gconstpointer *) neighbor->attr_names,
+                                               neighbor->n_attrs,
+                                               name,
+                                               nm_strcmp_with_data,
+                                               NULL,
+                                               NULL,
+                                               NULL);
+    if (idx < 0)
+        return NULL;
+
+    return neighbor->attr_vals[idx];
 }
 
 /**
@@ -2995,13 +3140,9 @@ nm_lldp_neighbor_get_attr_value(NMLldpNeighbor *neighbor, const char *name)
 char **
 nm_lldp_neighbor_get_attr_names(NMLldpNeighbor *neighbor)
 {
-    const char **keys;
-
     g_return_val_if_fail(NM_IS_LLDP_NEIGHBOR(neighbor), NULL);
 
-    keys = nm_utils_strdict_get_keys(neighbor->attrs, TRUE, NULL);
-
-    return nm_utils_strv_make_deep_copied_nonnull(keys);
+    return nm_utils_strv_dup(neighbor->attr_names, neighbor->n_attrs, TRUE) ?: g_new0(char *, 1);
 }
 
 /**
diff --git a/src/libnm-client-public/nm-device.h b/src/libnm-client-public/nm-device.h
index 410f083f76..840b9c762f 100644
--- a/src/libnm-client-public/nm-device.h
+++ b/src/libnm-client-public/nm-device.h
@@ -191,14 +191,19 @@ NM_AVAILABLE_IN_1_2
 void nm_lldp_neighbor_ref(NMLldpNeighbor *neighbor);
 NM_AVAILABLE_IN_1_2
 void nm_lldp_neighbor_unref(NMLldpNeighbor *neighbor);
-NM_AVAILABLE_IN_1_2
-char **nm_lldp_neighbor_get_attr_names(NMLldpNeighbor *neighbor);
+NM_AVAILABLE_IN_1_32
+const char *const *nm_lldp_neighbor_get_attr_names2(NMLldpNeighbor *neighbor, gsize *out_len);
+NM_AVAILABLE_IN_1_32
+GVariant *
+nm_lldp_neighbor_get_attr_at_index(NMLldpNeighbor *neighbor, gsize idx, const char **name);
 NM_AVAILABLE_IN_1_18
 GVariant *nm_lldp_neighbor_get_attr_value(NMLldpNeighbor *neighbor, const char *name);
 
 NM_AVAILABLE_IN_1_2
 NMLldpNeighbor *nm_lldp_neighbor_new(void);
 NM_AVAILABLE_IN_1_2
+char **nm_lldp_neighbor_get_attr_names(NMLldpNeighbor *neighbor);
+NM_AVAILABLE_IN_1_2
 gboolean nm_lldp_neighbor_get_attr_string_value(NMLldpNeighbor *neighbor,
                                                 const char *    name,
                                                 const char **   out_value);