Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughAdds VFS (openat) tracing: new event types and structs, eBPF programs to capture sys_enter/sys_exit_openat and emit events, and handler/encoder code to serialize and forward VFS events into Fluent Bit. Changes
sequenceDiagram
participant Kernel as Kernel
participant eBPF as "eBPF Program\n(trace_vfs)"
participant Handler as "VFS Handler\n(encode_vfs_event)"
participant FluentBit as Fluent Bit
Kernel->>eBPF: sys_enter_openat(filename, flags, mode)
eBPF->>eBPF: store args in per-tid map
Kernel->>eBPF: sys_exit_openat(ret)
eBPF->>eBPF: lookup per-tid entry
eBPF->>eBPF: build event (ts, pid, uid, gid, mntns, cmd, op, path, flags, mode, fd, error_raw)
eBPF->>Handler: submit event buffer
Handler->>Handler: validate event type/size
Handler->>Handler: encode common fields
Handler->>Handler: encode VFS fields (operation, path, flags, mode, fd, error_raw)
Handler->>FluentBit: flb_input_log_append(encoded_record)
Handler->>Handler: reset encoder
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
plugins/in_ebpf/traces/vfs/handler.c (3)
10-12: Unusedinsparameter.The
insparameter is declared but never used inencode_vfs_event. Either remove it to match the actual interface needs, or use it for debug/error logging (e.g.,flb_plg_debug(ins, ...)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/vfs/handler.c` around lines 10 - 12, The parameter `ins` in the function `encode_vfs_event` is unused; either remove `ins` from the function signature and update all declarations/call sites (prototypes and callers of `encode_vfs_event`) to match, or use it for diagnostic logging (e.g., call `flb_plg_debug(ins, ...)` inside `encode_vfs_event`) and keep the parameter; ensure the chosen approach keeps function prototypes in headers and callers consistent and rebuilds without unused-parameter warnings.
27-36: Consider encoding operation as a human-readable string.The
operationfield is encoded as anint32(the raw enum value). For consistency withevent_typewhich is encoded as a string (e.g., "vfs"), consider encoding operation as "openat" rather than0. This improves log readability without requiring downstream consumers to map enum values.♻️ Example: Add operation-to-string helper
static inline const char *vfs_op_to_string(enum vfs_op op) { switch (op) { case VFS_OP_OPENAT: return "openat"; default: return "unknown"; } }Then use
flb_log_event_encoder_append_body_cstring(log_encoder, vfs_op_to_string(ev->details.vfs.operation)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/vfs/handler.c` around lines 27 - 36, Replace the integer encoding of the VFS operation with a human-readable string: add a helper function (e.g., vfs_op_to_string(enum vfs_op op)) that maps enum values (use cases like VFS_OP_OPENAT) to strings, then call flb_log_event_encoder_append_body_cstring(log_encoder, vfs_op_to_string(ev->details.vfs.operation)) instead of flb_log_event_encoder_append_body_int32; preserve the existing error handling by checking the return for FLB_EVENT_ENCODER_SUCCESS and calling flb_log_event_encoder_rollback_record(log_encoder) and returning -1 on failure.
101-106: Type aliasing via struct field ordering is fragile.The cast of
void *ctxtostruct trace_event_context *is currently safe becauseflb_in_ebpf_contexthasinsandlog_encoderas its first two fields in matching order. However, this design implicitly relies on struct field layout rather than explicit typing. Ifflb_in_ebpf_contextfields are ever reordered, all handlers (signal, malloc, bind, vfs) will silently break.Consider a wrapper function or type-safe callback mechanism to avoid this fragility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/vfs/handler.c` around lines 101 - 106, The handler currently casts void *ctx to struct trace_event_context in trace_vfs_handler, relying on flb_in_ebpf_context and trace_event_context having matching first-field layout (ins, log_encoder) which is fragile; change the callback API or add a small, type-safe wrapper that accepts the real flb_in_ebpf_context* and extracts/forwards a properly built struct trace_event_context (or provides accessor functions for log_encoder) so handlers (trace_vfs_handler and the other handlers: signal, malloc, bind) no longer perform unsafe casts; update handler registrations to call the new wrapper/adapter so code uses explicit types instead of relying on struct field ordering.plugins/in_ebpf/traces/vfs/handler.h (1)
1-12: Header is not self-contained: missing type declarations.The header declares
encode_vfs_eventwith parameters of typestruct flb_input_instance *andstruct flb_log_event_encoder *, but neither type is forward-declared nor included. This could cause compilation errors if this header is included before the Fluent Bit headers.Consider adding forward declarations:
♻️ Proposed fix to add forward declarations
`#ifndef` VFS_HANDLER_H `#define` VFS_HANDLER_H `#include` <stddef.h> `#include` "common/events.h" +struct flb_input_instance; +struct flb_log_event_encoder; + int trace_vfs_handler(void *ctx, void *data, size_t data_sz); int encode_vfs_event(struct flb_input_instance *ins, struct flb_log_event_encoder *log_encoder, const struct event *ev); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/vfs/handler.h` around lines 1 - 12, The header declares encode_vfs_event with parameters using struct flb_input_instance and struct flb_log_event_encoder but does not forward-declare them or include their headers; add forward declarations for "struct flb_input_instance;" and "struct flb_log_event_encoder;" near the top of this header (before the prototype for encode_vfs_event) so the declarations of trace_vfs_handler and encode_vfs_event compile when this header is included independently.plugins/in_ebpf/traces/includes/common/events.h (1)
8-8: Consider path truncation implications.
VFS_PATH_MAX = 256is relatively small compared to the systemPATH_MAX(typically 4096). Long paths will be truncated duringbpf_probe_read_user_str. This is likely intentional to keep event size manageable in BPF ring buffers, but worth documenting or logging when truncation occurs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/includes/common/events.h` at line 8, VFS_PATH_MAX is set to 256 which will cause long user paths to be truncated when read with bpf_probe_read_user_str; update the implementation that reads paths (calls to bpf_probe_read_user_str) to detect truncation by checking the returned length and set a truncation indicator in the event (add or reuse a flag/field in the event struct) or increase VFS_PATH_MAX if you want to preserve full paths, and add a brief comment next to the VFS_PATH_MAX macro documenting the truncation behavior and reasoning; reference the VFS_PATH_MAX macro and the call sites that use bpf_probe_read_user_str to implement the detection and flagging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/in_ebpf/traces/vfs/bpf.c`:
- Line 1: The SPDX header and the kernel-facing LICENSE string in the BPF file
are inconsistent (SPDX says "LGPL-2.1 OR BSD-2-Clause" while the kernel-facing
string declares "Dual BSD/GPL"); decide on the intended license and make both
declarations match across this file and all BPF files: update the SPDX
identifier at the top to the chosen SPDX expression and update the kernel-facing
license string (the BPF module's LICENSE string constant, e.g., the "LICENSE"
char[] used by the BPF program) to the equivalent kernel-facing wording (e.g.,
"GPL" or "Dual BSD/GPL") so they are consistent. Ensure you apply the same
change to every BPF source file in the project.
---
Nitpick comments:
In `@plugins/in_ebpf/traces/includes/common/events.h`:
- Line 8: VFS_PATH_MAX is set to 256 which will cause long user paths to be
truncated when read with bpf_probe_read_user_str; update the implementation that
reads paths (calls to bpf_probe_read_user_str) to detect truncation by checking
the returned length and set a truncation indicator in the event (add or reuse a
flag/field in the event struct) or increase VFS_PATH_MAX if you want to preserve
full paths, and add a brief comment next to the VFS_PATH_MAX macro documenting
the truncation behavior and reasoning; reference the VFS_PATH_MAX macro and the
call sites that use bpf_probe_read_user_str to implement the detection and
flagging.
In `@plugins/in_ebpf/traces/vfs/handler.c`:
- Around line 10-12: The parameter `ins` in the function `encode_vfs_event` is
unused; either remove `ins` from the function signature and update all
declarations/call sites (prototypes and callers of `encode_vfs_event`) to match,
or use it for diagnostic logging (e.g., call `flb_plg_debug(ins, ...)` inside
`encode_vfs_event`) and keep the parameter; ensure the chosen approach keeps
function prototypes in headers and callers consistent and rebuilds without
unused-parameter warnings.
- Around line 27-36: Replace the integer encoding of the VFS operation with a
human-readable string: add a helper function (e.g., vfs_op_to_string(enum vfs_op
op)) that maps enum values (use cases like VFS_OP_OPENAT) to strings, then call
flb_log_event_encoder_append_body_cstring(log_encoder,
vfs_op_to_string(ev->details.vfs.operation)) instead of
flb_log_event_encoder_append_body_int32; preserve the existing error handling by
checking the return for FLB_EVENT_ENCODER_SUCCESS and calling
flb_log_event_encoder_rollback_record(log_encoder) and returning -1 on failure.
- Around line 101-106: The handler currently casts void *ctx to struct
trace_event_context in trace_vfs_handler, relying on flb_in_ebpf_context and
trace_event_context having matching first-field layout (ins, log_encoder) which
is fragile; change the callback API or add a small, type-safe wrapper that
accepts the real flb_in_ebpf_context* and extracts/forwards a properly built
struct trace_event_context (or provides accessor functions for log_encoder) so
handlers (trace_vfs_handler and the other handlers: signal, malloc, bind) no
longer perform unsafe casts; update handler registrations to call the new
wrapper/adapter so code uses explicit types instead of relying on struct field
ordering.
In `@plugins/in_ebpf/traces/vfs/handler.h`:
- Around line 1-12: The header declares encode_vfs_event with parameters using
struct flb_input_instance and struct flb_log_event_encoder but does not
forward-declare them or include their headers; add forward declarations for
"struct flb_input_instance;" and "struct flb_log_event_encoder;" near the top of
this header (before the prototype for encode_vfs_event) so the declarations of
trace_vfs_handler and encode_vfs_event compile when this header is included
independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c94e461-090b-4b89-af70-3048b1e612b3
📒 Files selected for processing (7)
plugins/in_ebpf/in_ebpf.cplugins/in_ebpf/traces/includes/common/encoder.hplugins/in_ebpf/traces/includes/common/events.hplugins/in_ebpf/traces/traces.hplugins/in_ebpf/traces/vfs/bpf.cplugins/in_ebpf/traces/vfs/handler.cplugins/in_ebpf/traces/vfs/handler.h
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-add-vfs-traces
branch
from
d758d42 to
858f219
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VFS also provides eBPF entrypoints so we can provide this type of traces.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Documentation