Skip to content

[0/2][CGROUP_SOCK_ADDR Logging] Refactor Log Infrastructure#485

Open
yaakov-stein wants to merge 3 commits intofacebook:mainfrom
yaakov-stein:refactor-log-infrastructure
Open

[0/2][CGROUP_SOCK_ADDR Logging] Refactor Log Infrastructure#485
yaakov-stein wants to merge 3 commits intofacebook:mainfrom
yaakov-stein:refactor-log-infrastructure

Conversation

@yaakov-stein
Copy link
Copy Markdown
Contributor

@yaakov-stein yaakov-stein commented Mar 20, 2026

Summary

Refactor the logging infrastructure to prepare for non-packet log types:

  • Rename the log ELF stub to pkt_log to distinguish it from future non-packet stubs
  • Restructure bf_log as a tagged union with pkt and sock_addr variants
  • Rename bf_pkthdr to bf_log_opt since the enum will hold non-header options

No functional change (besides the bf_log_type enum helper methods).

@meta-cla meta-cla bot added the cla signed label Mar 20, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 20, 2026

Claude review of PR #485 (fe77c33)

Must fix

  • Commit 1 incorrect component scope — The subject lib,daemon: cgen: rename log elfstub to pkt_log includes lib but all four changed files live under src/bpfilter/ (the daemon). Should be daemon: cgen: rename log elfstub to pkt_log.
  • Missed rename in benchmarkstools/benchmarks/main.cpp:244,422BF_PKTHDR_LINK still referenced; will fail to compile after this PR lands.

Suggestions

  • Doxygen references wrong union membersrc/libbpfilter/include/bpfilter/runtime.h:96 — Comment says "the sock variant" but the union member is sock_addr.
  • Silent drop of non-packet log entriessrc/bfcli/print.c:577bfc_print_log returns silently for non-packet types. Consider a bf_dbg() trace so operators know entries are being skipped.
  • Commit 3 component scopelib: rename bf_pkthdr to bf_log_opt touches src/libbpfilter/, src/bfcli/, src/bpfilter/, and tests/. Scoping as lib: is defensible since the rename originates in lib headers, but lib: runtime: rename bf_pkthdr to bf_log_opt would be more precise.
  • l3_proto/l4_proto in common headersrc/libbpfilter/include/bpfilter/runtime.h:117 — These packet-layer fields sit in the shared header but have no clear semantics for BF_LOG_TYPE_SOCK_ADDR entries. Consider moving them into the pkt variant or documenting why they are intentionally shared.
  • Add static_assert on struct bf_log sizesrc/libbpfilter/include/bpfilter/runtime.h:162 — The struct layout changed (field reordering, union wrapping). A size assertion would protect the ring buffer ABI and verify the commit claim that total size is unchanged.
  • Commit message struct size claim is incorrect — Three independent reviewers calculated that struct bf_log grew from 112 to 120 bytes (8 bytes of additional internal padding in the union's pkt variant). The commit message states "total size is unchanged for ring buffer reservations" which should be corrected.
  • Missing bf_log_type_to_str/from_strsrc/libbpfilter/include/bpfilter/runtime.h:93 — New enum bf_log_type lacks the to_str/from_str helper pair that every other public enum in the project provides. At minimum to_str would help debug logging when the sock_addr variant lands.
  • Missing unit tests for bf_log_type functionstests/unit/libbpfilter/rule.c:18bf_log_type_to_str and bf_log_type_from_str are implemented but have no unit test coverage. Add assert_enum_to_from_str for enum bf_log_type.

Nits

  • Missing assert(str) in bf_log_opt_from_strsrc/libbpfilter/rule.c:39 — Doxygen says str "Can't be NULL" and other _from_str functions assert both params. Pre-existing but good time to fix given the rename.
  • Magic numbers in sock_addr variantsrc/libbpfilter/include/bpfilter/runtime.h:154comm[16], saddr[16], daddr[16] use bare 16 instead of named constants (TASK_COMM_LEN, sizeof(struct in6_addr)).
  • req_headers naming inconsistencysrc/libbpfilter/include/bpfilter/runtime.h:132 — The pkt variant keeps the old req_headers/headers naming while sock_addr uses the new req_log_opts. Since the PR renames bf_pkthdr to bf_log_opt, aligning the pkt field names would be consistent.
  • Include ordering in runtime.hsrc/libbpfilter/include/bpfilter/runtime.h:15<linux/in6.h> and <asm/types.h> are both system headers but separated by a blank line; they should be in the same include group per the style guide.
  • Missing assert(str) in bf_log_type_from_strsrc/libbpfilter/rule.c:38 — Same pattern as the bf_log_opt_from_str nit: Doxygen documents str as "Can't be NULL" but only log_type is asserted.

Workflow run

@yaakov-stein yaakov-stein changed the title [0/2] Refactor Log Infrastructure [0/2][CGROUP_SOCK_ADDR Logging] Refactor Log Infrastructure Mar 20, 2026
Rename the packet logging ELF stub from `log` to `pkt_log` to
establish naming symmetry with the upcoming `sock_log` stub for
socket-based hooks.
@yaakov-stein yaakov-stein force-pushed the refactor-log-infrastructure branch from 2bb9a67 to c911410 Compare March 27, 2026 14:59
@yaakov-stein yaakov-stein force-pushed the refactor-log-infrastructure branch from c911410 to 7aeeab0 Compare March 27, 2026 17:06
/** Destination address (4 bytes for IPv4, 16 for IPv6). */
bf_aligned(8) __u8 daddr[sizeof(struct in6_addr)];
} sock_addr;
} payload;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd ideally have left this unnamed as there isn't much of a benefit of naming it and it now requires ->payload.pkt/sock_addr, but doxygen requires unions to be named :(

@yaakov-stein yaakov-stein marked this pull request as ready for review March 27, 2026 19:07
@yaakov-stein yaakov-stein force-pushed the refactor-log-infrastructure branch from 7aeeab0 to 4a18e6d Compare March 27, 2026 19:45
@yaakov-stein yaakov-stein force-pushed the refactor-log-infrastructure branch from 4a18e6d to 4c82f48 Compare March 27, 2026 20:26
Add a bf_log_type discriminator and restructure bf_log as a tagged
union with packet and socket variants. Packet-specific fields
(pkt_size, headers, l2hdr/l3hdr/l4hdr) move into the pkt variant.
The sock variant (pid, comm) is defined but not yet populated.

The struct sizes to the larger packet variant, so total size is
unchanged for ring buffer reservations.
@yaakov-stein yaakov-stein force-pushed the refactor-log-infrastructure branch from 4c82f48 to fe77c33 Compare March 27, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant