out_logrotate: add new out plugin #11110
Conversation
SagiROosto
requested review from
celalettin1286,
cosmo0920,
edsiper,
fujimotos,
niedbalski and
patrick-stephens
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new out_logrotate output plugin: build options and defaults, CMake registration and target, full C implementation and header, an example configuration, and extensive runtime tests for size-based rotation, optional gzip compression, retention, multiple formats, and mkdir handling. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(248,250,255)
participant Input as Input Plugin
participant LR as out_logrotate
participant FS as File System
participant GZ as Gzip (optional)
end
Input->>LR: submit event chunk (flush)
LR->>LR: lock per-file state (mutex)
LR->>FS: ensure directory & open current file
LR->>LR: check current size vs Max_Size
alt size >= Max_Size
LR->>FS: rename current -> rotated.<ts>
alt Gzip enabled
LR->>GZ: stream-compress rotated file
GZ-->>FS: write rotated file.gz
FS->>FS: remove uncompressed rotated file
end
LR->>FS: prune rotated files (Max_Files)
LR->>FS: create new current file
end
LR->>LR: format records (JSON/CSV/LTSV/PLAIN/MSGPACK/TEMPLATE)
LR->>FS: append formatted records to current file
LR-->>Input: acknowledge flush
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-08-31T12:46:11.940ZApplied to files:
📚 Learning: 2025-08-31T12:46:11.940ZApplied to files:
📚 Learning: 2025-09-08T11:21:33.975ZApplied to files:
📚 Learning: 2025-10-23T07:43:16.216ZApplied to files:
🧬 Code graph analysis (1)tests/runtime/out_logrotate.c (2)
🪛 ast-grep (0.40.0)tests/runtime/out_logrotate.c[warning] 585-585: A check is done with (file-stat-before-action-c) 🔇 Additional comments (9)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txtcmake/windows-setup.cmakeplugins/out_logrotate/CMakeLists.txtCMakeLists.txt
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
CMakeLists.txt
🧬 Code graph analysis (2)
tests/runtime/out_logrotate.c (1)
src/flb_lib.c (9)
flb_create(143-225)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
plugins/out_logrotate/logrotate.c (6)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (8)
conf/fluent-bit-logrotate.conf (1)
1-21: LGTM! Configuration demonstrates plugin features well.The example configuration effectively showcases the logrotate plugin's key features: size-based rotation (Max_Size 10M), file retention (Max_Files 5), gzip compression, and automatic directory creation.
Note: The path
/tmp/logsis Linux-specific. While this is acceptable for an example, consider documenting that users on Windows would need to adjust the path (e.g.,C:\logs).plugins/out_logrotate/CMakeLists.txt (1)
1-5: LGTM! Build configuration is correct.The CMake configuration properly declares the ZLIB dependency (required for gzip compression) and follows the standard plugin registration pattern used throughout Fluent Bit.
plugins/CMakeLists.txt (1)
366-367: LGTM! Plugin registration is correct.The
out_logrotateplugin is properly registered following the same pattern as other output plugins.cmake/plugins_options.cmake (1)
120-120: LGTM! Build option properly defined.The plugin option is correctly defined with an appropriate description and default value (ON), consistent with other output plugins in the codebase.
CMakeLists.txt (1)
301-301: LGTM! Plugin enabled in all-features build.The
FLB_OUT_LOGROTATEflag is correctly set within theFLB_ALLblock, ensuring the plugin is included when building with all features enabled.cmake/windows-setup.cmake (1)
103-103: LGTM! Windows support enabled.The plugin is enabled for Windows builds following the standard pattern used for other output plugins in this file.
Note: Runtime tests for this plugin are currently only registered for non-Windows systems (see
tests/runtime/CMakeLists.txtline 237). If Windows support is intended, consider adding Windows-specific tests or verifying the existing tests work on Windows.plugins/out_logrotate/logrotate.h (1)
1-32: LGTM! Header structure is clean and correct.The header properly defines output format constants for the logrotate plugin with appropriate include guards and licensing. The enum provides clear format options (JSON, CSV, LTSV, PLAIN, MSGPACK, TEMPLATE) that align with the PR objectives for multi-format support.
tests/runtime/CMakeLists.txt (1)
235-238: Discrepancy confirmed: out_logrotate is enabled on Windows in cmake/windows-setup.cmake but excluded from runtime tests.Verification shows the review comment's concern is valid. The out_logrotate plugin:
- Is set to enabled (
Yes) on Windows in cmake/windows-setup.cmake line 103- Contains extensive Windows-specific code: Windows headers (Shlobj.h, Shlwapi.h), Windows API calls (strncpy_s, localtime_s, PathRemoveFileSpecA), and platform-specific macros
- Has no Windows-specific build exclusions in its own CMakeLists.txt
Yet the runtime test is excluded on Windows (tests/runtime/CMakeLists.txt line 236-238) under a comment stating "These plugins work only on Linux"—contradicting the Windows code present in the source.
The same pattern applies to out_file, which also has Windows-specific code but excluded tests.
Action needed: Manually verify whether Windows support is production-ready and either extend tests to cover Windows or disable the plugin on Windows if support is incomplete.
|
Once the coderabbit comments are addressed we should trigger a full package build but @SagiROosto can you confirm it builds for all targets? |
|
@patrick-stephens UT failed for typo issue I Fixed it along with coderabbit suggestions. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/runtime/out_logrotate.c (1)
294-354: Address the past review concern and clarify expected file count.The addition of
TEST_ASSERT(file_count >= 0)at lines 340 and 349 correctly addresses the past review concern about directory listing failures being silently ignored. Good fix!However, there's an inconsistency in the comment at line 350: it states "Current file + 2 rotated files" but checks
file_count <= 4. Withmax_files=3configured at line 324, the expected count should be 1 current file + 3 rotated files = 4 total, not "2 rotated files" as the comment suggests.Consider updating the comment to match the actual logic:
- TEST_CHECK(file_count <= 4); /* Current file + 2 rotated files */ + TEST_CHECK(file_count <= 4); /* Current file + 3 rotated files (max_files=3) */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🔇 Additional comments (11)
plugins/CMakeLists.txt (1)
366-366: LGTM!The plugin registration follows the established pattern and is correctly positioned alphabetically between
out_fileandout_forward.tests/runtime/out_logrotate.c (3)
36-154: LGTM!The helper functions are well-structured with appropriate error handling and resource cleanup. The recursive directory deletion, file counting, and pattern matching logic are all correct.
156-229: LGTM!The basic rotation test correctly configures the plugin, triggers rotation by writing sufficient data, and validates that both the original file and rotated files exist.
231-292: LGTM!The gzip compression test properly validates that rotated files are compressed by waiting for
.gzfiles to appear, accounting for potential async compression.plugins/out_logrotate/logrotate.c (7)
106-206: LGTM!The initialization function correctly sets up the plugin context, creates the hash table for per-file size tracking, parses configuration options, and includes proper error handling with resource cleanup.
531-576: LGTM!The file size tracking correctly uses a hash table to maintain per-file size state, addressing the previous concern about a shared global counter. The
should_rotate_filefunction retrieves size by filename, andupdate_file_size_counterusesfstatto read actual file size before updating the hash table.
625-745: LGTM!The streaming gzip compression implementation is memory-efficient and correct:
- Processes files in 64KB chunks to avoid loading entire files into memory
- Properly initializes deflate with raw deflate mode (negative window bits)
- Writes RFC 1952 compliant gzip header and footer with CRC32
- Verifies all input is consumed and compression completes with
Z_STREAM_END- Includes proper error handling and resource cleanup
860-944: LGTM!The cleanup logic correctly:
- Counts rotated files matching the timestamp pattern
- Returns early if within the
max_fileslimit- Sorts files by modification time (oldest first) using a simple bubble sort
- Removes excess files to maintain the
max_fileslimitThe approach ensures that rotated files are cleaned up while preserving the current active file.
1067-1081: LGTM!The metrics handling path now correctly calls
update_file_size_counter(ctx, out_file, fp)at line 1078 before closing the file, which addresses the previous review concern about stale file size counters preventing rotation for metrics-only streams.
946-1182: LGTM!The flush callback implements the complete workflow correctly:
- Checks rotation condition using per-file size from hash table
- Performs rotation and cleanup when needed
- Removes rotated file's entry from hash table to reset tracking
- Initializes file size from disk for new files using
fstat- Updates file size counter after all write operations (logs, metrics, msgpack)
The per-file size tracking ensures correct behavior when multiple files are managed by a single plugin instance.
1184-1274: LGTM!The exit function properly cleans up resources by destroying the hash table and freeing the context. The configuration map is comprehensive with sensible defaults: 100MB max size, 7 max files retained, and gzip compression enabled by default.
|
You'll have to fix the commits that don't match the guidelines btw, i.e. you need a plugin/module prefix like |
SagiROosto
force-pushed
the
out_logrotate
branch
2 times, most recently
from
0dd42f9 to
99917b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
plugins/out_logrotate/logrotate.c (8)
129-134: Consider a larger hash table size for multi-file scenarios.The hash table is created with only 64 buckets. If the plugin writes to many different files (when
fileis not set and tags are used as filenames), this could lead to excessive hash collisions and degraded performance. Consider using a larger size (e.g., 256 or 512) or making it configurable for high-throughput, multi-file deployments.
605-623: Document the 4GB limitation for gzip compression.The gzip footer stores the original file size in 32 bits (4 bytes), which limits support to files under 4GB. While this is a standard gzip limitation, files larger than 4GB will have their size truncated modulo 2^32, which could cause decompression tools to report incorrect sizes or fail validation. Consider logging a warning when compressing files approaching or exceeding this limit, or documenting this limitation in the plugin configuration.
783-795: Check unlink() return values for error logging.Both
unlink()calls (lines 789 and 793) don't check return values. If deletion fails (e.g., permission denied), the files remain on disk but the function returns success or failure based solely on compression status. While the gzip failure path returns -1, the success path (line 790) doesn't verify that cleanup succeeded. Consider logging warnings ifunlink()fails.if (ret == 0) { /* Remove the uncompressed file */ - unlink(rotated_filename); + if (unlink(rotated_filename) != 0) { + flb_plg_warn(ctx->ins, "failed to remove uncompressed file %s", rotated_filename); + } flb_plg_debug(ctx->ins, "rotated and compressed file: %s", gzip_filename); } else { /* Remove the failed gzip file */ - unlink(gzip_filename); + if (unlink(gzip_filename) != 0) { + flb_plg_warn(ctx->ins, "failed to remove incomplete gzip file %s", gzip_filename); + } return -1; }
911-923: Bubble sort may be inefficient for many rotated files.The bubble sort has O(n²) complexity. If
max_filesis high and many files accumulate (e.g., hundreds of rotated logs), this could introduce noticeable latency during cleanup. For production environments with large retention counts, consider usingqsort()with a comparison function for O(n log n) performance.
216-242: No validation for non-map objects in CSV output.The function checks
obj->type == MSGPACK_OBJECT_MAP && obj->via.map.size > 0(line 216) but doesn't handle the case where the object is not a map or is empty. If a non-map object is passed, the function returns 0 (success) without writing anything. While this may be intentional, it could lead to silent data loss. Consider logging a warning when non-map objects are encountered.static int csv_output(FILE *fp, int column_names, struct flb_time *tm, msgpack_object *obj, struct flb_logrotate_conf *ctx) { int i; int map_size; msgpack_object_kv *kv = NULL; - if (obj->type == MSGPACK_OBJECT_MAP && obj->via.map.size > 0) { + if (obj->type != MSGPACK_OBJECT_MAP) { + flb_plg_warn(ctx->ins, "csv_output: expected map, got type %d", obj->type); + return -1; + } + + if (obj->via.map.size > 0) { kv = obj->via.map.ptr; map_size = obj->via.map.size; ... + } else { + flb_plg_debug(ctx->ins, "csv_output: empty map, skipping"); } return 0; }
252-272: No validation for non-map objects in LTSV output.Similar to
csv_output, this function silently returns success if the object is not a map or is empty (line 252). Consider adding validation and logging for consistency with error handling elsewhere in the plugin.
370-381: Silent failure in plain_output when JSON conversion fails.If
flb_msgpack_to_json_str()returns NULL, the function silently skips writing and returns 0 (success). This could lead to data loss without any indication to the caller. Consider returning -1 or logging a warning when conversion fails.static int plain_output(FILE *fp, msgpack_object *obj, size_t alloc_size, int escape_unicode) { char *buf; buf = flb_msgpack_to_json_str(alloc_size, obj, escape_unicode); if (buf) { fprintf(fp, "%s" NEWLINE, buf); flb_free(buf); + } else { + return -1; } return 0; }
383-407: print_metrics_text doesn't return error status.The function is
voidand doesn't return error status to the caller. Ifcmt_decode_msgpack_create()fails (line 394), an error is logged but the caller (line 1075) has no way to know the write failed. Consider changing the return type tointand propagating the error so the flush operation can retry or handle the failure appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/runtime/out_logrotate.c
- plugins/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (1)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_hash_table.c (5)
flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)flb_hash_table_add(401-494)flb_hash_table_del(605-674)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (1)
plugins/out_logrotate/logrotate.c (1)
1073-1081: Metrics file size update now included - excellent!The past review noted that
update_file_size_counter()was missing after writing metrics, causing rotation to never trigger for metrics-only streams. This has been correctly addressed at line 1078.
SagiROosto
force-pushed
the
out_logrotate
branch
2 times, most recently
from
2a77707 to
329cba4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- cmake/plugins_options.cmake
- tests/runtime/out_logrotate.c
- conf/fluent-bit-logrotate.conf
- plugins/out_logrotate/CMakeLists.txt
- plugins/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
cmake/windows-setup.cmakeCMakeLists.txttests/runtime/CMakeLists.txt
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
cmake/windows-setup.cmakeCMakeLists.txt
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
SagiROosto
force-pushed
the
out_logrotate
branch
from
329cba4 to
9d1e142
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/out_logrotate/logrotate.c (2)
578-582: Optional: Remove redundant hash table delete.Lines 579–581 delete the existing key before re-adding it, but
flb_hash_table_add()already handles replacement internally (seesrc/flb_hash_table.clines 430–442). The delete-then-add pattern adds unnecessary overhead—though it's safe, it doubles the hash lookups.Apply this diff to streamline the update:
- /* Delete existing entry if it exists to ensure we can update the value */ - flb_hash_table_del(ctx->file_sizes, filename); /* Store or update file size in hash table */ ret = flb_hash_table_add(ctx->file_sizes, filename, strlen(filename), &file_size, sizeof(size_t));The
flb_hash_table_addfunction already performs an in-place update for existing keys, so the explicit delete is not required.
783-795: Optional: Add path-length validation before constructing rotated filenames.Lines 784 and 795 use
snprintfwithPATH_MAX - 1, which will silently truncate iffilenameis too long. WhilePATH_MAXis typically large (4096 on Linux), a truncated filename would cause rotated files to be unreachable byis_valid_rotation_filename()andcleanup_old_files(), breaking rotation cleanup.Add a length check before constructing the rotated filename:
+ /* Validate that we have room for timestamp suffix */ + if (strlen(filename) + 16 >= PATH_MAX) { /* ".YYYYMMDD_HHMMSS" = 16 chars */ + flb_plg_error(ctx->ins, "filename too long for rotation: %s", filename); + return -1; + } + /* Create rotated filename with timestamp */ snprintf(rotated_filename, PATH_MAX - 1, "%s.%s", filename, timestamp);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_logrotate/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (2)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_hash_table.c (5)
flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)flb_hash_table_del(605-674)flb_hash_table_add(401-494)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
tests/runtime/out_logrotate.c (1)
src/flb_lib.c (10)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🔇 Additional comments (4)
plugins/CMakeLists.txt (1)
366-366: LGTM: Plugin registration is correct.The
out_logrotateplugin is registered following the same pattern as other output plugins, and the placement maintains alphabetical order.tests/runtime/out_logrotate.c (3)
157-235: LGTM: Basic rotation test is well-structured.The test properly:
- Sets up and tears down the test directory
- Configures rotation with
max_size=5Kandgzip=false- Writes enough data (~16KB) to trigger rotation
- Verifies both the current file and at least one rotated file exist
The sleep intervals (lines 210, 218) are necessary to allow async flush operations to complete.
237-300: LGTM: Gzip compression test validates the compression feature.The test correctly:
- Enables gzip compression with
gzip=true- Triggers rotation by writing sufficient data
- Uses
wait_for_file_pattern()to poll for rotated files with.gzsuffix- Verifies that gzip compression was applied to rotated files
302-366: LGTM: Max files cleanup test validates the retention policy.The test properly:
- Configures
max_files=3to limit retained rotated files- Triggers multiple rotations by writing data in 5 iterations
- Validates that file count never exceeds 4 (current file + 3 rotated files)
- Uses
TEST_ASSERT(file_count >= 0)to catch directory listing failures (addressing the past review comment)- Verifies the final count after all rotations complete
The iterative checking (lines 349–351) ensures cleanup happens progressively during rotation, not just at the end.
SagiROosto
force-pushed
the
out_logrotate
branch
from
9d1e142 to
277aea3
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
plugins/out_logrotate/logrotate.c (1)
118-154: Validatemax_sizeduring init to avoid pathological rotation on misconfig
max_filesis validated (ctx->max_files <= 0fails init), butmax_sizeis not. If a user configuresmax_size 0,file_size >= ctx->max_sizewill always be true andcb_logrotate_flush()will attempt rotation on every flush, leading to repeated failedrename()s before the file even exists.Consider rejecting or clamping
max_size == 0incb_logrotate_init()alongside themax_filescheck, e.g.:if (ctx->max_files <= 0) { flb_plg_error(ctx->ins, "invalid max_files=%d; must be >= 1", ctx->max_files); flb_free(ctx); return -1; } + + /* Protect against misconfigured zero-size rotation */ + if (ctx->max_size == 0) { + flb_plg_error(ctx->ins, "invalid max_size=0; must be > 0"); + flb_free(ctx); + return -1; + }This keeps behavior well-defined for simple config typos.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (1)
tests/runtime/out_logrotate.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (9)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🪛 ast-grep (0.40.0)
tests/runtime/out_logrotate.c
[warning] 514-514: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🪛 Cppcheck (2.18.0)
tests/runtime/out_logrotate.c
[information] 3-3: Include file
(missingInclude)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 37-37: Include file
(missingIncludeSystem)
[information] 134-134: Include file
(missingIncludeSystem)
[information] 135-135: Include file
(missingIncludeSystem)
[information] 136-136: Include file
(missingIncludeSystem)
[information] 137-137: Include file
(missingIncludeSystem)
[information] 138-138: Include file
(missingIncludeSystem)
[information] 139-139: Include file
(missingIncludeSystem)
[information] 140-140: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 39-39: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 37-37: Include file
(missingIncludeSystem)
[information] 39-39: Include file
(missingIncludeSystem)
[information] 40-40: Include file
(missingIncludeSystem)
[information] 41-41: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 38-38: Include file
(missingIncludeSystem)
[information] 39-39: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 36-36: Include file
(missingIncludeSystem)
[information] 38-38: Include file
(missingIncludeSystem)
[information] 39-39: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 53-53: Include file
(missingIncludeSystem)
[information] 54-54: Include file
(missingIncludeSystem)
[information] 55-55: Include file
(missingIncludeSystem)
[information] 57-57: Include file
(missingIncludeSystem)
[information] 58-58: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 52-52: Include file
(missingIncludeSystem)
[information] 53-53: Include file
(missingIncludeSystem)
[information] 54-54: Include file
(missingIncludeSystem)
[information] 55-55: Include file
(missingIncludeSystem)
[information] 56-56: Include file
(missingIncludeSystem)
[information] 57-57: Include file
(missingIncludeSystem)
[information] 58-58: Include file
(missingIncludeSystem)
[information] 60-60: Include file
(missingIncludeSystem)
[information] 61-61: Include file
(missingIncludeSystem)
[information] 62-62: Include file
(missingIncludeSystem)
[information] 63-63: Include file
(missingIncludeSystem)
[information] 65-65: Include file
(missingIncludeSystem)
[information] 66-66: Include file
(missingIncludeSystem)
[information] 67-67: Include file
(missingIncludeSystem)
[information] 68-68: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 8-8: Include file
(missingIncludeSystem)
[information] 9-9: Include file
(missingIncludeSystem)
[information] 10-10: Include file
(missingIncludeSystem)
[information] 11-11: Include file
(missingIncludeSystem)
[information] 12-12: Include file
(missingIncludeSystem)
[information] 13-13: Include file
(missingIncludeSystem)
[information] 15-15: Include file
(missingIncludeSystem)
[information] 16-16: Include file
(missingIncludeSystem)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] 31-31: Include file
(missingIncludeSystem)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
plugins/out_logrotate/logrotate.c
[information] 34-34: Include file
(missingIncludeSystem)
[information] 37-37: Include file
(missingIncludeSystem)
[information] 134-134: Include file
(missingIncludeSystem)
[information] 135-135: Include file
(missingIncludeSystem)
[information] 136-136: Include file
(missingIncludeSystem)
[information] 137-137: Include file
(missingIncludeSystem)
[information] 138-138: Include file
(missingIncludeSystem)
[information] 139-139: Include file
(missingIncludeSystem)
[information] 140-140: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 37-37: Include file
(missingIncludeSystem)
[information] 39-39: Include file
(missingIncludeSystem)
[information] 40-40: Include file
(missingIncludeSystem)
[information] 41-41: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 38-38: Include file
(missingIncludeSystem)
[information] 39-39: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 36-36: Include file
(missingIncludeSystem)
[information] 38-38: Include file
(missingIncludeSystem)
[information] 39-39: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 53-53: Include file
(missingIncludeSystem)
[information] 54-54: Include file
(missingIncludeSystem)
[information] 55-55: Include file
(missingIncludeSystem)
[information] 57-57: Include file
(missingIncludeSystem)
[information] 58-58: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 52-52: Include file
(missingIncludeSystem)
[information] 53-53: Include file
(missingIncludeSystem)
[information] 54-54: Include file
(missingIncludeSystem)
[information] 55-55: Include file
(missingIncludeSystem)
[information] 56-56: Include file
(missingIncludeSystem)
[information] 57-57: Include file
(missingIncludeSystem)
[information] 58-58: Include file
(missingIncludeSystem)
[information] 60-60: Include file
(missingIncludeSystem)
[information] 61-61: Include file
(missingIncludeSystem)
[information] 62-62: Include file
(missingIncludeSystem)
[information] 63-63: Include file
(missingIncludeSystem)
[information] 65-65: Include file
(missingIncludeSystem)
[information] 66-66: Include file
(missingIncludeSystem)
[information] 67-67: Include file
(missingIncludeSystem)
[information] 68-68: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 36-36: Include file
(missingIncludeSystem)
[information] 37-37: Include file
(missingIncludeSystem)
[information] 38-38: Include file
(missingIncludeSystem)
[information] 39-39: Include file
(missingIncludeSystem)
[information] 41-41: Include file
(missingIncludeSystem)
[information] 42-42: Include file
(missingIncludeSystem)
[information] 43-43: Include file
(missingIncludeSystem)
[information] 45-45: Include file
(missingIncludeSystem)
[information] 46-46: Include file
(missingIncludeSystem)
[information] 115-115: Include file
(missingInclude)
[information] 165-165: Include file
(missingIncludeSystem)
[information] 168-168: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingInclude)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 48-48: Include file
(missingIncludeSystem)
[information] 49-49: Include file
(missingIncludeSystem)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] 31-31: Include file
(missingIncludeSystem)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
SagiROosto
force-pushed
the
out_logrotate
branch
from
370817c to
776dd3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugins/out_logrotate/logrotate.c (1)
148-154: Consider validatingmax_sizesimilarly tomax_filesYou already reject invalid
max_filesvalues incb_logrotate_init. Amax_sizeof0currently causes an attempted rotation on every flush (including before the file exists), which is surprising for a simple typo.Not mandatory, but worth considering:
- Either reject
max_size <= 0during init (log error and return-1), or- Treat
max_size == 0as “no rotation” and bypass the rotation check incb_logrotate_flush.This would make misconfigurations fail fast instead of leading to noisy logs and odd rotation behavior.
tests/runtime/out_logrotate.c (1)
1396-1452: UseTEST_ASSERTfor critical allocations to avoid NULL deref in tests
flb_test_logrotate_gzip_compression_exact_chunk()usesTEST_CHECKafterflb_malloc()but immediately dereferences the pointers (memset,snprintf) regardless of the check. On OOM, the test will crash instead of failing cleanly.Switch these to hard assertions so the test aborts before dereferencing a NULL pointer:
- large_message = flb_malloc(msg_size + 1); - TEST_CHECK(large_message != NULL); + large_message = flb_malloc(msg_size + 1); + TEST_ASSERT(large_message != NULL); @@ - json_payload = flb_malloc(json_size); - TEST_CHECK(json_payload != NULL); + json_payload = flb_malloc(json_size); + TEST_ASSERT(json_payload != NULL);This matches the pattern used elsewhere in the file for critical allocations.
🧹 Nitpick comments (2)
plugins/out_logrotate/logrotate.h (1)
20-30: Avoid using feature macroFLB_OUT_LOGROTATEas the include guardThe guard name collides with the build/test feature macro
FLB_OUT_LOGROTATE(used in CMake/runtime tests). If that macro is ever defined when including this header, the enum body will be skipped entirely. Rename the guard to a header-specific symbol to avoid future surprises.-#ifndef FLB_OUT_LOGROTATE -#define FLB_OUT_LOGROTATE +#ifndef FLB_OUT_LOGROTATE_H +#define FLB_OUT_LOGROTATE_H @@ -#endif +#endif /* FLB_OUT_LOGROTATE_H */plugins/out_logrotate/logrotate.c (1)
1360-1372: Remove redundantupdate_file_size_countercall while holdingentry->lockIn the “new file” branch you call
update_file_size_counter(ctx, out_file, fp)while already holdingentry->lock, then immediately recompute the size withfstat()and overwriteentry->size.update_file_size_counteritself acquireslist_lockand thenentry->lock, so this call will always fail to acquire the per-file lock and is effectively a no-op plus extra locking churn.You can simplify and avoid nested locking attempts by relying solely on the direct
fstat():- if (entry->size == 0) { - update_file_size_counter(ctx, out_file, fp); - /* update_file_size_counter cycles locks internally which is bad since we - * hold one. */ - /* Actually, since we already have the entry and the lock, we should just - * update size directly! */ - /* But wait, we opened the file. We should just check fstat. */ - if (fstat(fileno(fp), &st) == 0 && st.st_size >= 0) { - entry->size = (size_t)st.st_size; - } - } + if (entry->size == 0) { + if (fstat(fileno(fp), &st) == 0 && st.st_size >= 0) { + entry->size = (size_t)st.st_size; + } + }Optionally, if nothing else uses
update_file_size_counter, you can drop that helper entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/CMakeLists.txt
- cmake/plugins_options.cmake
- CMakeLists.txt
- cmake/windows-setup.cmake
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txt
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
tests/runtime/out_logrotate.c
🧬 Code graph analysis (2)
plugins/out_logrotate/logrotate.c (4)
src/flb_lock.c (3)
flb_lock_init(6-17)flb_lock_acquire(32-65)flb_lock_release(67-100)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1303-1326)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)src/flb_log_event_decoder.c (2)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)
tests/runtime/out_logrotate.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (9)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)
🪛 ast-grep (0.40.0)
tests/runtime/out_logrotate.c
[warning] 585-585: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🔇 Additional comments (4)
plugins/out_logrotate/CMakeLists.txt (1)
1-8: CMake wiring forout_logrotatelooks consistentSource list and
FLB_PLUGINinvocation (withShlwapionly on MSVC) match existing patterns for plugins using Windows path helpers. No issues from a build/config perspective.tests/runtime/CMakeLists.txt (1)
235-238: Runtime test registration forFLB_OUT_LOGROTATEis well-placedRegistering
out_logrotate.cviaFLB_RT_TEST(FLB_OUT_LOGROTATE ...)inside the non-Windows block mirrorsout_fileand ensures the test only builds where the POSIX-heavy test code is supported.conf/fluent-bit-logrotate.conf (1)
1-21: Sample configuration covers core logrotate featuresThis example exercises path/file, JSON format, size-based rotation,
Max_Files,Gzip, andMkdirwith a simple dummy input, which aligns well with the plugin’s intended usage.plugins/out_logrotate/logrotate.c (1)
689-863: Gzip streaming fix now correctly issues a finalZ_FINISHThe updated
gzip_compress_file()logic that runs a dedicated finaldeflate(..., Z_FINISH)loop whenstatus != Z_STREAM_ENDensures a proper end-of-stream marker even when file size is an exact multiple ofGZIP_CHUNK_SIZE. This removes the earlier corruption risk for “exact chunk multiple” files.
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
SagiROosto
force-pushed
the
out_logrotate
branch
from
776dd3b to
89c58ca
Compare
|
I fixed the last coderabbit comment. A small fix so tests should still pass. Now ready to review |
|
windows failure seems like unrelated flaky issue. Can you re-run it? (also happened in #11006 -> https://github.com/fluent/fluent-bit/actions/runs/19861920849/job/56914033108) |
2 participants
This PR present a new out plugin
out_logrotate. This Will answer #7572 question and solve #7541 suggestion.This feature is highly required in bare-metal or physical machines where you have finite and limited storage.
The plugin is basically a copy of
out_filewith a rotation feature. The reason to do that in additional plugin and not on theout_fileitself is to avoid accidentally turning this feature on and having redundant info/allocs/processing where it is not needed (so normal users can still use the regular file plugin without issue).(this replaces closed #10824 PR)
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.