Restore counter values on chain update

[yaakov-stein]

As mentioned in #389, when bf_cgen_update() replaces a chain, the new BPF program starts with a zeroed counter map. The behavior should be that when updating a specific entity within a chain (set, rule, etc.), the counters should be preserved (see @qdeslandes comment below for more details and rationale).

This PR adds that behavior specifically for chain update-set, while leaving the existing behavior for chain update.

Notes/Thoughts:

• With the current implementation, counters are copied from the old map to the new map after the new program is loaded but before the link swap. This means that it is possible for some packets to be unaccounted for in the time between the data being transferred and when the new chain begins filtering. While it is possible to keep the same maps (and thus the exact number) since we know the number of rules will be the same, doing that would require a more invasive approach (the flag would need to propagate into more methods) and will not work for add-rule or remove-rule.
• A bf_cgen_update_flags enum is added for flexibility in case we need future arguments as opposed to a boolean (which is simpler but harder to work with if we need to use or update this in the future). However, the goal either way should be for this to be temporary (in line with the consolidated behavior mentioned here).

## Design
The implementation assumes:

• Chains may have large numbers of rules, sets, and set elements, but rules won't have exceedingly large numbers of matchers
• Update overhead should remain negligible relative to program generation and loading
• Simplicity is preferred over hyper-optimizations where possible

Set mapping. Because set matchers reference sets by position and positions can change between chains, a set index mapping is built first. To efficiently compare large sets, old sets are hashed (FNV-1a with commutative element sum for order-independence), sorted by hash, and binary-searched for each new set. Hash matches are verified deterministically with bf_set_equal(), which copies each set's elements into a flat array, sorts them, and compares with memcmp.

Rule matching. Rules are hashed the same way - FNV-1a of scalar fields plus a commutative sum of per-matcher hashes. A new rules set matchers hash their mapped old index so equivalent rules produce identical hashes regardless of set position (this abstracts away the whole idea of the set so we don't need to worry about it). Old rules are sorted by hash and binary-searched for each new rule, with full order-independent matcher comparison to verify matches. Once again, hash matches are verified deterministically using _bf_rule_equal.

Counter transfer. For each matched pair, the old counter is read and written to the new program's counter map. Policy and error counters are always preserved. This happens after bf_program_load but before bf_link_update, so there is no race.

## Performance
Tested with up to 10k rules and 100 sets of 50k elements each. The mapping and transfer overhead was negligible relative to program generation and loading.

Test plan

• Added tests to chain_update.sh and chain_update_set.sh
• Manual testing
• Unit tests for bf_fnv1a and bf_set_equal
• E2E test covering rule reordering and set index shuffling

[pzmarzly]

I know it's a bit late to ask, but what if instead of identifying objects by value (rule contents, set elements, etc.), we gave every element a stable ID (e.g. random 32, 64, or 128 bits)? Then the caller could preserve IDs as its edits the chain, and the counters from old objects would be attributed to new objects if IDs are the same.

With your PR as it is, if you do bfcli chain update-set --add 1.2.3.4 to edit a set, IIUC counters for all rules that reference this set would be reset, since the new set would not be identical to old set, so hashes won't match. But maybe we do want to treat the old set and new sets as logically equivalent, and keep the counters?

That would be a much bigger change though. So another, simpler, idea I had was: what if we compared unnamed sets by-value, but named sets by their name?

@qdeslandes do you have any opinion on this?

[yaakov-stein]

I know it's a bit late to ask, but what if instead of identifying objects by value (rule contents, set elements, etc.), we gave every element a stable ID (e.g. random 32, 64, or 128 bits)? Then the caller could preserve IDs as its edits the chain, and the counters from old objects would be attributed to new objects if IDs are the same.

One of my goals for this was to be transparent to the user and for this to happen automatically behind the scenes - i.e. they shouldn't need to be aware of element id's and need to match them up (however we'd implement it, it seems like it'd add some overhead for the user).

With your PR as it is, if you do bfcli chain update-set --add 1.2.3.4 to edit a set, IIUC counters for all rules that reference this set would be reset, since the new set would not be identical to old set, so hashes won't match. But maybe we do want to treat the old set and new sets as logically equivalent, and keep the counters?

I think that behavior would be lead to confusing results. A counter would report a number that doesn't accurately reflect what has actually been matched by that rule (given that the rules behavior has changed over time).

@qdeslandes do you have any opinion on this?

I think the current approach is cleaner/more correct, but I'm happy to change things up if we'd rather go in that direction.

[qdeslandes]

Summarizing the discussion I had with Yaakov: with the daemonless changes, we will eventually simplify the commands we expose to users. E.g. chain set and chain update don't need to be separate command, we should also update atomically if possible.

Eventually, we will provide two types of commands:

• Ruleset-level commands, which manipulates chains (e.g. set a chain)
• Chain-level commands, which manipulates entities within the chains (e.g. update a set, insert a rule)

Thus, as a user, I wouldn't expect ruleset-level commands to maintain the counter values, as I'm expecting a new chain. For chain-level commands, however, we expect the chain to remain the same, so the counters should be preserved.

Hence, for now, I would only expect update-set operation to preserve the counters values.

[github-actions]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[qdeslandes]

Some test changes, otherwise LGTM!

[yaakov-stein]

Note: this does now rely on BTF

[github-actions]

Claude: Review of PR #416 — Restore counter values on chain update

The approach is sound: using a flags enum to control counter preservation in bf_cgen_update is consistent with how the codebase handles other behavioral flags (e.g. bf_chain_flags). The transfer-after-load-before-link-swap placement is the right spot, and the non-atomicity tradeoff is well-documented.

Issues found:

• Medium — Missing rule count validation at the call site. _bf_cgen_transfer_counters requires matching rule counts (documented in its docstring) but the caller doesn't enforce this. A mismatch would silently misattribute policy/error counters or fail with a confusing BPF map lookup error. See inline comment.

• Low — Asymmetric counter access. Reads go through bf_handle_get_counter, writes bypass the handle API with raw bf_bpf_map_update_elem. A bf_handle_set_counter would keep the abstraction consistent. See inline comment.

Documentation, naming conventions, tests, and commit message all follow the project conventions.

[github-actions]

Claude: Review summary

The approach is sound: transferring counters between load and link swap minimizes the packet counting gap, and the flags-based API is extensible for future update behaviors.

Two inline comments posted:

1. Low-medium: _bf_cgen_transfer_counters documents that old and new chains must have the same rule count, but there's no runtime assertion. Adding one at the call site in bf_cgen_update would prevent silent counter index misalignment if the precondition is violated by a future caller.

2. Low: Enum sentinel _BF_CGEN_UPDATE_FLAGS_MAX doesn't follow the codebase naming convention (should be _BF_CGEN_UPDATE_MAX based on the value prefix pattern used by other enums).

[github-actions]

Claude: Review of PR #416 — Restore counter values on chain update

Design is clean: the flags-based API extends bf_cgen_update without a separate code path, and the transfer-after-load-before-link-swap placement minimizes the counter gap. The enum, naming, and sentinel all follow project conventions.

One inline comment posted:

• Low-medium: The rule count validation runs after program generation and loading. Moving the precondition check to function entry would fail fast and avoid wasting BPF resources on a doomed update.

Tests cover both the reset (chain update) and preservation (update-set) cases well.

Note on previous automated reviews: the comments flagging the sentinel name _BF_CGEN_UPDATE_MAX as non-compliant were false positives — it already follows the _BF_<PREFIX>_MAX convention correctly.

[github-actions]

Claude: Review of PR #416 — Restore counter values on chain update

The design is sound: using a flags enum to control counter preservation in bf_cgen_update is clean, extensible, and keeps the existing chain update and update-set behaviors properly separated. The transfer-after-load-before-link-swap placement is the correct spot, and the non-atomicity tradeoff is well-documented.

One new inline comment posted:

• Low: bf_map_set_elem from the already-included map.h could replace the direct bf_bpf_map_update_elem call, removing the need for the new <bpfilter/bpf.h> include. Unlike the earlier suggestion to add a bf_handle_set_counter, this uses an existing API.

All three call sites (cli.c chain update, cli.c update-set, ipt.c) are correctly updated. Tests cover both counter reset (chain update) and preservation (update-set). Naming, commit message, and code style follow project conventions.

CLAUDE.md note: The Doxygen rule "first and last lines empty (unlike regular comments)" is not followed anywhere in the codebase (0 instances found across src/). CLAUDE.md should be updated to match the actual practice, which is /**\n * @brief ... without empty first/last lines.

[github-actions]

Claude: Review of PR #416 — Restore counter values on chain update

The implementation is sound. Transferring counters between program load and link swap is the correct placement, and the flags-based API for bf_cgen_update is clean and extensible. All three call sites are updated, error paths handle cleanup correctly via _free_bf_handle_ / _free_bf_program_ attributes, and naming/style follow project conventions.

Prior review feedback (sentinel naming, bf_map_set_elem usage, rule count guard, bpftool-based tests) has been addressed in the latest revision. One minor inline comment posted about loop variable type consistency.

E2E tests cover both the counter-reset (chain update) and counter-preservation (update-set) behaviors, which is the important thing for this change.