lib: create data_structures directory, implement hashset data structure by pzmarzly · Pull Request #470 · facebook/bpfilter

pzmarzly · 2026-03-12T14:59:59Z

WIP draft

A simple data hashing function.

github-actions · 2026-03-12T15:11:43Z

src/libbpfilter/set.c


+static uint64_t _bf_set_elem_hash(const void *data, void *ctx)
+{
+    return bf_fnv1a(data, *(const size_t *)ctx, 0);


Claude: FNV-1a hash seeded with 0 instead of BF_FNV1A_INIT

The documentation for bf_fnv1a in helper.h explicitly states "Pass @ref BF_FNV1A_INIT as @p hash for the initial call." Using 0 instead of the FNV-1a offset basis (0xcbf29ce484222325ULL) degrades hash distribution quality, leading to more collisions in the hashset. The unit test for bf_fnv1a in tests/unit/libbpfilter/helper.c correctly uses BF_FNV1A_INIT.

Same issue in tests/unit/libbpfilter/data_structures/hashset.c line 14.

Suggested change

return bf_fnv1a(data, *(const size_t *)ctx, 0);

return bf_fnv1a(data, *(const size_t *)ctx, BF_FNV1A_INIT);

github-actions · 2026-03-12T15:11:45Z

tests/unit/libbpfilter/data_structures/hashset.c

+static uint64_t uint32_hash(const void *data, void *ctx)
+{
+    (void)ctx;
+    return bf_fnv1a(data, sizeof(uint32_t), 0);


Claude: FNV-1a hash seeded with 0 instead of BF_FNV1A_INIT

Same issue as in set.c — should use BF_FNV1A_INIT to match the documented API contract and produce correct hash distribution.

Suggested change

return bf_fnv1a(data, sizeof(uint32_t), 0);

return bf_fnv1a(data, sizeof(uint32_t), BF_FNV1A_INIT);

github-actions · 2026-03-12T15:11:46Z

src/libbpfilter/include/bpfilter/data_structures/hashset.h

+ *
+ * If the element already exists, no duplicate is added and 0 is returned.
+ * The hashset grows automatically when the load factor is exceeded.
+ * The hashset takes ownership of @p data.


Claude: bf_hashset_add ownership semantics are ambiguous for duplicates

The doc says "The hashset takes ownership of @p data" but when the element already exists, bf_hashset_add returns 0 without storing data — ownership is silently not taken. This is a memory management footgun for callers using the idiomatic TAKE_PTR pattern after a successful add.

The current caller (bf_set_add_elem) works around this by pre-checking with bf_hashset_contains, but future callers may not. Consider either:

Updating the doc to clarify: "On successful insertion, the hashset takes ownership. If the element already exists, ownership is not transferred."

Or returning -EEXIST for duplicates so callers can distinguish the two cases.

github-actions · 2026-03-12T15:11:48Z

src/libbpfilter/set.c


    bf_dump_prefix_push(prefix);


Claude: bf_set_dump lost bf_dump_prefix_last call for last element

The old bf_list_foreach loop called bf_dump_prefix_last(prefix) when iterating over the last element, producing correct tree-like connector characters in dump output. The new bf_set_foreach (backed by bf_hashset_foreach) has no way to detect the last element, so the final element's prefix will show a ├ connector instead of └. This is a consistent pattern elsewhere in the codebase (see bf_chain_dump, bf_rule_dump, bf_hook_dump).

github-actions · 2026-03-12T15:11:50Z

src/libbpfilter/include/bpfilter/data_structures/hashset.h

+ */
+
+/// Sentinel pointer value for a tombstoned (deleted) slot.
+#define _BF_HASHSET_TOMBSTONE ((void *)1)


Claude: _BF_HASHSET_TOMBSTONE is an implementation detail exposed in the public header

This sentinel value is only needed by bf_hashset_foreach's expansion. Consider moving it to the .c file and restructuring the foreach macro to call an inline helper that encapsulates the tombstone check, keeping this internal constant out of the public API surface.

github-actions · 2026-03-12T15:12:19Z

github-actions · 2026-03-13T00:22:40Z

src/libbpfilter/set.c

-    return 0;
+    return bf_hashset_is_empty(&set->elems);
 }



Claude: Memory leak in bf_set_add_elem when bf_hashset_add fails during grow

TAKE_PTR(_elem) nullifies _elem before bf_hashset_add executes. If bf_hashset_add fails (e.g., -ENOMEM from _bf_hashset_grow), the allocated copy is lost — _elem is already NULL so _cleanup_free_ won't free it, and bf_hashset_add didn't store it.

Fix by separating the ownership transfer:

Suggested change

int r = bf_hashset_add(&set->elems, _elem);

if (r)

return r;

TAKE_PTR(_elem);

return 0;

github-actions · 2026-03-13T00:22:41Z

src/libbpfilter/data_structures/vector.c

+    _free_bf_vector_ struct bf_vector *_vec = NULL;
+
+    assert(vec);
+    assert(elem_size);


Claude: assert() on non-pointer value

assert(elem_size) validates a size_t, but the style guide states: "Only use assert() for pointer values. For other validation, use if () with appropriate error logging and return codes."

All existing assert() calls in src/libbpfilter/ validate pointers. Consider:

Suggested change

assert(elem_size);

if (!elem_size)

return -EINVAL;

github-actions · 2026-03-13T00:22:42Z

src/libbpfilter/include/bpfilter/data_structures/hashset.h

+                     *(void **)bf_vector_get(&(set)->slots, _bf_hset_idx));    \
+                 _bf_hset_brk; _bf_hset_brk = 0)
+
+/**


Claude: Duplicated slot-liveness check between header and .c file

This _bf_hashset_slot_is_occupied function is functionally identical to _bf_slot_is_live in hashset.c (both check slot != NULL && slot != tombstone). The header version uses the magic (void *)1 while the .c version uses _BF_HASHSET_TOMBSTONE.

Consider consolidating into a single static inline in the header and using it from both the macro and the .c file, avoiding the duplicated magic constant.

qdeslandes

Alright, first pass of review and a few things to fix. I'll do a second, deeper pass, when those are solved. That being said, it's a very welcome addition! :D

qdeslandes · 2026-03-13T09:18:13Z

src/libbpfilter/include/bpfilter/helper.h

+/**
+ * @brief Compute a FNV-1a 64-bit hash.
+ *
+ * Pass @ref BF_FNV1A_INIT as @p hash for the initial call. To hash


@ref should be replaced with backticks.

qdeslandes · 2026-03-13T09:19:11Z

src/libbpfilter/helper.c

+    assert(data);
+
+    const uint8_t *bytes = data;


const uint8_t *bytes = data; assert(data);

qdeslandes · 2026-03-13T09:23:33Z

src/bfcli/chain.c

Not fan of bpfilter/data_structures/... path. As we're moving to daemon-less, I would suggest bpfilter/core/..., which will later on contain other core structures of the project (e.g. bf_chain).

qdeslandes · 2026-03-13T09:30:07Z

src/libbpfilter/data_structures/vector.c

Check dynbuf.c, it's the same logic but dynbuf uses elem_size = 1. I would suggest implementing bf_vector_take to take ownership of the data (so bf_vector would re-allocate memory on the next add). Then (in a subsequent commit), replace bf_dynbuf with bf_vector.

Could you also add a @todo in bf_program to replace img with a bf_vector?

qdeslandes · 2026-03-13T09:30:22Z

src/libbpfilter/data_structures/vector.c

+    _free_bf_vector_ struct bf_vector *_vec = NULL;
+
+    assert(vec);
+    assert(elem_size);


Only assert pointers for NULL. elem_size should be checked and return an error on 0.

qdeslandes · 2026-03-13T09:53:38Z

src/libbpfilter/include/bpfilter/data_structures/hashset.h

+static inline bool _bf_hashset_slot_is_occupied(void *slot)
+{
+    return slot != NULL && slot != (void *)1;
+}


No private functions in headers.

qdeslandes · 2026-03-13T09:54:35Z

src/libbpfilter/include/bpfilter/data_structures/hashset.h

+ * @param set Pointer to the hashset. Must be non-NULL.
+ * @param elem_var Name of the @c void* variable to hold each element.
+ */
+#define bf_hashset_foreach(set, elem_var)                                      \


Can't you leverage bf_vector_foreach in here to make it simpler?

qdeslandes · 2026-03-13T09:54:54Z

tests/unit/libbpfilter/data_structures/hashset.c

+
+#include "test.h"
+
+static uint64_t uint32_hash(const void *data, void *ctx)


Static symbols are prefixed with _bf_

qdeslandes · 2026-03-13T09:55:55Z

src/libbpfilter/include/bpfilter/data_structures/hashset.h

Does it need to be part of the public headers?

qdeslandes · 2026-03-13T10:03:59Z

src/libbpfilter/data_structures/hashset.c

It might be worth having a bf_hashset_take.

lib: helper: add FNV-1a hash function

db743d1

A simple data hashing function.

pzmarzly requested a review from qdeslandes as a code owner March 12, 2026 15:00

pzmarzly marked this pull request as draft March 12, 2026 15:00

meta-cla bot added the cla signed label Mar 12, 2026

github-actions bot reviewed Mar 12, 2026

View reviewed changes

pzmarzly added 5 commits March 12, 2026 23:45

lib: data_structures: move bf_list into subfolder

f91090b

lib: data_structures: add bf_vector

383a4cd

lib: set: add bf_set_foreach and bf_set_size

3741a01

lib: data_structures: add bf_hashset

8601a5c

lib: set: use bf_hashset for elems field

02938a3

pzmarzly force-pushed the push-mlqkpqspnupl branch from 314af18 to 02938a3 Compare March 12, 2026 23:56

github-actions bot reviewed Mar 13, 2026

View reviewed changes

qdeslandes requested changes Mar 13, 2026

View reviewed changes

qdeslandes mentioned this pull request Mar 13, 2026

lib: add bf_hashset #460

Draft

	return bf_fnv1a(data, (const size_t )ctx, 0);
	return bf_fnv1a(data, (const size_t )ctx, BF_FNV1A_INIT);

	return bf_fnv1a(data, sizeof(uint32_t), 0);
	return bf_fnv1a(data, sizeof(uint32_t), BF_FNV1A_INIT);

+    int r = bf_hashset_add(&set->elems, _elem);
+    if (r)
+        return r;
+    TAKE_PTR(_elem);
+    return 0;


		#include "test.h"

		static uint64_t uint32_hash(const void data, void ctx)

Conversation

pzmarzly commented Mar 12, 2026

Uh oh!

github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Must fix

Suggestions

Nits

Uh oh!

github-actions bot Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

qdeslandes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Mar 12, 2026 •

edited

Loading