out_logrotate: add new plugin (#2) #10824
Conversation
SagiROosto
requested review from
celalettin1286,
edsiper,
fujimotos,
koleini,
niedbalski and
patrick-stephens
as code owners
WalkthroughAdds a new out_logrotate output plugin (implementation, header, CMake build, example config, and runtime tests), plus CMake options/flags to enable it under FLB_ALL and Windows defaults and unconditional plugin registration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Config/Admin
participant FB as Fluent Bit
participant PL as out_logrotate
participant FS as Filesystem
Admin->>FB: Load config (input + out_logrotate)
FB->>PL: cb_init(config)
Note over PL: configure path,file,format,max_size,max_files,gzip,mkdir
loop On flush
FB->>PL: cb_flush(events)
PL->>PL: Resolve target filename (path/file/tag)
alt current_file_size >= max_size
PL->>FS: Close & rename active -> file.timestamp
opt gzip enabled
PL->>FS: Stream-compress -> file.timestamp.gz
end
PL->>FS: Remove oldest rotated files if count > max_files
PL->>PL: Reset current_file_size
end
PL->>FS: Ensure directory (mkdir) and open append
alt msgpack payload
PL->>FS: Write raw msgpack
else metrics payload
PL->>FS: Write rendered metrics text
else
PL->>PL: Render records (json/csv/ltsv/plain/template)
PL->>FS: Append rendered lines
end
PL->>PL: Update current_file_size
PL-->>FB: cb_flush return status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-31T12:46:11.940ZApplied to files:
🔇 Additional comments (1)
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: 15
🧹 Nitpick comments (7)
plugins/out_logrotate/logrotate.h (1)
23-30: Optional: C++ friendliness and API commentIf tests or embedders include this from C++, consider extern "C" guards; also add a brief comment mapping enum values to the config's Format strings.
Example:
+#ifdef __cplusplus +extern "C" { +#endif enum { @@ }; +#ifdef __cplusplus +} +#endifconf/fluent-bit-logrotate.conf (1)
12-21: Clarify size units and option names to match plugin config map“Max_Size 10” is ambiguous (bytes? KB?). Prefer explicit units if supported (e.g., 10M), and ensure option keys exactly match the plugin’s config map.
- Max_Size 10 + Max_Size 10M # 10 megabytesPlease confirm the accepted keys/values (Format, Max_Size, Max_Files, Gzip, Mkdir) align with logrotate.c’s flb_config_map; adjust example accordingly.
tests/runtime/out_logrotate.c (2)
322-325: Assertion doesn’t prove rotation happened.Comparing sizes without ensuring a rotation occurred can pass spuriously. Instead, verify a rotated file exists and the active file size reset.
I can rewrite this test to:
- set max_size to a small value (e.g., 64K),
- push until rotation triggers,
- assert count_files_in_directory(".", "flb_test_logrotate.log.") >= 1,
- and the current file size < 64K.
498-500: Time-based performance assertions are brittle in CI.CPU and I/O variance makes this flaky. Either increase the threshold generously, skip under CI, or convert to a basic smoke test.
plugins/out_logrotate/logrotate.c (3)
511-516: Comment mismatch: size is in bytes, not MB.Avoid confusion with FLB_CONFIG_MAP_SIZE which accepts human-readable units but stores bytes.
-/* Function to check if file size exceeds max size in MB */ +/* Function to check if file size exceeds max size (bytes) */ static int should_rotate_file(struct flb_logrotate_conf *ctx)
165-176: Delimiter handling is restrictive vs. docs.Docs say “Set a custom delimiter,” but check_delimiter only accepts a few keywords/escapes. Consider passing through arbitrary delimiters when provided.
Minimal change inside init:
tmp = flb_output_get_property("delimiter", ins); ret_str = check_delimiter(tmp); - if (ret_str != NULL) { - ctx->delimiter = ret_str; - } + if (ret_str != NULL) { + ctx->delimiter = ret_str; + } else if (tmp && *tmp) { + ctx->delimiter = tmp; /* accept arbitrary delimiter string */ + }Same for label_delimiter if desired.
180-185: Log human-friendly sizes to improve operability.Printing raw bytes is harder to read. Optional: format as KiB/MiB when logging.
I can provide a small helper to pretty-print sizes if you want it included.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
cmake/plugins_options.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
🧬 Code graph analysis (2)
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(1250-1291)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(99-116)flb_log_event_decoder_next(310-406)flb_log_event_decoder_destroy(147-179)
tests/runtime/out_logrotate.c (2)
src/flb_lib.c (10)
flb_create(138-220)flb_service_set(647-673)flb_input(261-271)flb_input_set(300-330)flb_output(274-284)flb_output_set(515-546)flb_start(914-925)flb_lib_push(774-801)flb_stop(942-985)flb_destroy(223-258)src/flb_sds.c (4)
flb_sds_create(78-90)flb_sds_create_size(92-95)flb_sds_printf(336-387)flb_sds_destroy(389-399)
🔇 Additional comments (4)
plugins/CMakeLists.txt (1)
367-369: LGTM: conditional registration is correct and placed logically after out_fileRegistration behind FLB_OUT_LOGROTATE matches the new option; ordering is consistent with related outputs.
cmake/plugins_options.cmake (1)
120-120: Option wiring looks correct; defaults align with existing pattern.DEFINE_OPTION for FLB_OUT_LOGROTATE mirrors other outputs and respects FLB_MINIMAL. No blockers.
tests/runtime/out_logrotate.c (2)
263-266: File count assertion may be racy; wait until rotation settles.After heavy writes, let the engine flush again before counting files, or poll until count stabilizes within TEST_TIMEOUT.
I can add a small polling loop to wait until count_files_in_directory(".", "flb_test_logrotate.log.") <= 3 or timeout. Want me to patch it?
380-382: Header check aligns once csv_column_names=true.After enabling column names, "timestamp" should be present. No further action.
* add logrotate out plugin * fix gzip * fix test * update max_size * update gzip * add logs Signed-off-by: SagiROosto <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
plugins/out_logrotate/logrotate.c (4)
31-39: Add missing headers for PATH_MAX, PRIu64, and dirname().
Compilation will fail or be non-portable without these includes.Apply:
#include <miniz/miniz.h> +#include <limits.h> /* PATH_MAX */ +#include <inttypes.h> /* PRIu64 */ +#ifndef FLB_SYSTEM_WINDOWS +#include <libgen.h> /* dirname */ +#endif
869-901: Use project allocators and guard against uninitialized directory on strdup failure.
Replace strdup/free with flb_strdup/flb_free and skip cleanup if directory couldn’t be resolved.Apply:
- out_file_copy = strdup(out_file); - if (out_file_copy) { + out_file_copy = flb_strdup(out_file); + if (out_file_copy != NULL) { #ifdef FLB_SYSTEM_WINDOWS PathRemoveFileSpecA(out_file_copy); strncpy(directory, out_file_copy, PATH_MAX - 1); directory[PATH_MAX - 1] = '\0'; #else strncpy(directory, dirname(out_file_copy), PATH_MAX - 1); directory[PATH_MAX - 1] = '\0'; #endif - free(out_file_copy); + flb_free(out_file_copy); } @@ - /* Rotate the file */ + /* Rotate the file */ if (rotate_file(ctx, out_file) == 0) { /* Reset file size counter after rotation */ ctx->current_file_size = 0; - /* Clean up old rotated files */ - cleanup_old_files(ctx, directory, base_filename); + /* Clean up old rotated files (only if directory was resolved) */ + if (directory[0] != '\0') { + cleanup_old_files(ctx, directory, base_filename); + } }Also initialize directory to an empty string before use:
- char directory[PATH_MAX]; + char directory[PATH_MAX] = {0};
903-919: Use project allocators in mkdir branch and avoid passing possibly static buffer from dirname directly.Apply:
- out_file_copy = strdup(out_file); - if (out_file_copy) { + out_file_copy = flb_strdup(out_file); + if (out_file_copy != NULL) { #ifdef FLB_SYSTEM_WINDOWS PathRemoveFileSpecA(out_file_copy); ret = mkpath(ctx->ins, out_file_copy); #else - ret = mkpath(ctx->ins, dirname(out_file_copy)); + { + char tmpdir[PATH_MAX]; + const char *dd = dirname(out_file_copy); + strncpy(tmpdir, dd, sizeof(tmpdir) - 1); + tmpdir[sizeof(tmpdir) - 1] = '\0'; + ret = mkpath(ctx->ins, tmpdir); + } #endif - free(out_file_copy); + flb_free(out_file_copy); if (ret == 0) { fp = fopen(out_file, "ab+"); } }
937-944: Update size counter after writing metrics; otherwise rotation may never trigger.Apply:
if (event_chunk->type == FLB_INPUT_METRICS) { print_metrics_text(ctx->ins, fp, event_chunk->data, event_chunk->size); + update_file_size_counter(ctx, fp); fclose(fp); FLB_OUTPUT_RETURN(FLB_OK); }
🧹 Nitpick comments (4)
plugins/out_logrotate/logrotate.c (4)
81-99: Make check_delimiter const-correct and align call sites.
Avoid discarding const and storing string literals in non-const pointers.Apply:
-static char *check_delimiter(const char *str) +static const char *check_delimiter(const char *str) @@ - char *ret_str; + const char *ret_str; @@ - ret_str = check_delimiter(tmp); + ret_str = check_delimiter(tmp);(No functional change; improves type safety.)
Also applies to: 165-176
511-516: Comment says “in MB” but value is bytes.
Update comment to avoid confusion.Apply:
-/* Function to check if file size exceeds max size in MB */ +/* Check if current file size in bytes exceeds max_size (bytes) */
1110-1114: Align default value with documentation and allow size suffix parsing.
Use “100M” so FLB_CONFIG_MAP_SIZE parses the suffix and matches the doc string.Apply:
- FLB_CONFIG_MAP_SIZE, "max_size", "100000000", + FLB_CONFIG_MAP_SIZE, "max_size", "100M",
715-731: Optional: reuse flb_gzip helpers instead of hand-rolled gzip wrapper.
Reduces maintenance and ensures parity with other plugins. If feasible, replace write_gzip_header/write_gzip_footer/deflate logic with flb_gzip APIs.I can draft a patch that streams file->gzip via flb_gzip_* and handles CRC/ISIZE internally; let me know.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
cmake/plugins_options.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 (6)
- cmake/plugins_options.cmake
- plugins/out_logrotate/CMakeLists.txt
- conf/fluent-bit-logrotate.conf
- tests/runtime/out_logrotate.c
- plugins/out_logrotate/logrotate.h
- tests/runtime/CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (1)
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(1250-1291)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(99-116)flb_log_event_decoder_next(310-406)flb_log_event_decoder_destroy(147-179)
🔇 Additional comments (1)
plugins/CMakeLists.txt (1)
367-369: LGTM: conditional registration for out_logrotate is correct and in the right section.
Build-time gating via FLB_OUT_LOGROTATE matches the project’s plugin pattern.
Signed-off-by: SagiROosto <[email protected]>
There was a problem hiding this comment.
We should provide a YAML version too.
There was a problem hiding this comment.
Sure, I didn't see other configs. let me know where can I put it. anyway in fluent-bit-docs I'll list both options
| DEFINE_OPTION(FLB_OUT_ES "Enable Elasticsearch output plugin" ON) | ||
| DEFINE_OPTION(FLB_OUT_EXIT "Enable Exit output plugin" ON) | ||
| DEFINE_OPTION(FLB_OUT_FILE "Enable file output plugin" ON) | ||
| DEFINE_OPTION(FLB_OUT_LOGROTATE "Enable logrotate output plugin" ON) |
There was a problem hiding this comment.
Will this build for all targets including macOS and Windows? They may need configuring via their specific cmake/ config files.
There was a problem hiding this comment.
the logrotate plugin is actually a fork of the out_file with gzip and couple of move/copy files.
I've matched all cmakes to be the same as the out_file
Signed-off-by: SagiROosto <[email protected]>
| # These plugins work only on Linux | ||
| if(NOT FLB_SYSTEM_WINDOWS) | ||
| FLB_RT_TEST(FLB_OUT_FILE "out_file.c") | ||
| FLB_RT_TEST(FLB_OUT_LOGROTATE "out_logrotate.c") | ||
| endif() |
There was a problem hiding this comment.
I think this is outdated right as we seem to enable them on windows @cosmo0920 ?
SagiROosto
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugins/out_logrotate/logrotate.c (1)
954-960: File size counter not updated after writing metrics.After
print_metrics_textwrites metrics data to the file, thecurrent_file_sizecounter is not updated. This means subsequent flushes won't trigger rotation even if the metrics data causes the file to exceedmax_size.Apply this diff:
/* Check if the event type is metrics, handle the payload differently */ if (event_chunk->type == FLB_INPUT_METRICS) { print_metrics_text(ctx->ins, fp, event_chunk->data, event_chunk->size); + update_file_size_counter(ctx, fp); fclose(fp); FLB_OUTPUT_RETURN(FLB_OK); }
🧹 Nitpick comments (1)
plugins/out_logrotate/logrotate.c (1)
530-536: Consider logging when file size tracking fails.If
fstatfails,current_file_sizeis not updated, which could prevent rotation from triggering correctly. While this is a rare scenario, logging a debug or warning message would aid troubleshooting.Apply this diff:
static void update_file_size_counter(struct flb_logrotate_conf *ctx, FILE *fp) { struct stat st; if (fstat(fileno(fp), &st) == 0 && st.st_size >= 0) { ctx->current_file_size = (size_t) st.st_size; + } else { + flb_plg_debug(ctx->ins, "failed to update file size counter (fstat error)"); } }
📜 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/runtime/out_logrotate.c
🧰 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 (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(1250-1291)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(99-116)flb_log_event_decoder_next(310-406)flb_log_event_decoder_destroy(147-179)
🔇 Additional comments (13)
plugins/out_logrotate/logrotate.c (13)
31-68: Includes and constants look good.The required headers for
PATH_MAX,PRIu64, anddirname()are now properly included with platform-specific guards. The constants for compression and path handling are well-defined.
70-84: Configuration struct is well-designed.The structure appropriately tracks both configuration settings and runtime state (current file size). All fields are clearly named and typed.
86-103: Delimiter mapping is straightforward and correct.The function correctly maps common delimiter names to their character representations using case-insensitive comparison.
105-192: Initialization logic is robust.The initialization function properly allocates memory, applies configuration mappings, validates the format setting, and logs the resolved configuration. Error paths correctly clean up allocated resources.
194-392: Output formatting functions are well-implemented.All format handlers (CSV, LTSV, plain, template, metrics) correctly process their respective formats. Template parsing handles edge cases like nested and unclosed braces. Memory management in
plain_outputproperly frees the allocated JSON string buffer.
394-514: Directory creation logic is platform-aware and robust.The
mkpathfunction correctly implements recursive directory creation for Windows, macOS, and other POSIX systems. Platform-specific path handling is appropriate, and error checking is thorough. Memory allocation uses project allocators consistently.
516-550: Rotation helper functions are correct and thread-safe.The rotation check, file size tracking (using
fstat), and timestamp generation (using thread-safelocaltime_r/localtime_s) are all properly implemented per previous review feedback.
708-751: File rotation logic is sound with appropriate error handling.The function correctly renames the file, compresses it if enabled, and handles failures by cleaning up partial artifacts. Keeping the uncompressed rotated file when compression fails is a reasonable fallback.
886-918: Rotation error handling is appropriate.When rotation fails, the code correctly keeps the existing file and retries on the next flush. Errors are logged by
rotate_fileitself. The cleanup call at line 916 is best-effort; its return value need not be checked since cleanup failure doesn't affect correctness.
920-946: File opening and initialization logic is robust.The code correctly handles directory creation when
mkdiris enabled, retries file opening after successfulmkpath, and initializes the file size counter for new or existing files.
966-1057: Event processing and file size tracking are correct for log events.The msgpack and log event paths properly decode data, format output according to the selected format, and update the file size counter before closing. The log event decoder is correctly initialized and destroyed.
1062-1072: Exit handler correctly releases resources.The cleanup function properly checks for
NULLcontext and frees the allocated configuration structure.
1074-1159: Configuration and plugin definition are comprehensive.The configuration map provides sensible defaults (100MB max size, 7 rotated files, gzip enabled) and covers all necessary parameters. The plugin structure correctly declares support for both logs and metrics, uses a single worker for sequential file writes, and registers all required callbacks.
| /* Function to compress a file using streaming gzip (memory-safe for large files) */ | ||
| static int gzip_compress_file(const char *input_filename, const char *output_filename, struct flb_output_instance *ins) | ||
| { | ||
| FILE *src_fp, *dst_fp; | ||
| char *input_buffer, *output_buffer; | ||
| size_t bytes_read, output_buffer_size; | ||
| size_t total_input_size = 0; | ||
| mz_ulong crc = MZ_CRC32_INIT; | ||
| z_stream strm; | ||
| int ret = 0, flush, status; | ||
|
|
||
| /* Open source file */ | ||
| src_fp = fopen(input_filename, "rb"); | ||
| if (!src_fp) { | ||
| flb_plg_error(ins, "failed to open source file for gzip: %s", input_filename); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Open destination file */ | ||
| dst_fp = fopen(output_filename, "wb"); | ||
| if (!dst_fp) { | ||
| flb_plg_error(ins, "failed to create gzip file: %s", output_filename); | ||
| fclose(src_fp); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Allocate input and output buffers */ | ||
| input_buffer = flb_malloc(GZIP_CHUNK_SIZE); | ||
| output_buffer_size = compressBound(GZIP_CHUNK_SIZE); | ||
| output_buffer = flb_malloc(output_buffer_size); | ||
|
|
||
| if (!input_buffer || !output_buffer) { | ||
| flb_plg_error(ins, "failed to allocate compression buffers"); | ||
| ret = -1; | ||
| goto cleanup; | ||
| } | ||
|
|
||
| /* Write gzip header */ | ||
| write_gzip_header(dst_fp); | ||
|
|
||
| /* Initialize deflate stream (raw deflate without gzip wrapper) */ | ||
| memset(&strm, 0, sizeof(strm)); | ||
| strm.zalloc = Z_NULL; | ||
| strm.zfree = Z_NULL; | ||
| strm.opaque = Z_NULL; | ||
|
|
||
| status = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED, | ||
| -Z_DEFAULT_WINDOW_BITS, 9, Z_DEFAULT_STRATEGY); | ||
| if (status != Z_OK) { | ||
| flb_plg_error(ins, "failed to initialize deflate stream"); | ||
| ret = -1; | ||
| goto cleanup; | ||
| } | ||
|
|
||
| /* Process file in chunks (ensure Z_FINISH is always issued) */ | ||
| do { | ||
| bytes_read = fread(input_buffer, 1, GZIP_CHUNK_SIZE, src_fp); | ||
| if (bytes_read > 0) { | ||
| /* Update CRC and total size */ | ||
| crc = mz_crc32(crc, (const unsigned char *)input_buffer, bytes_read); | ||
| total_input_size += bytes_read; | ||
|
|
||
| /* Set up deflate input */ | ||
| strm.next_in = (Bytef *)input_buffer; | ||
| strm.avail_in = bytes_read; | ||
|
|
||
| /* Determine flush mode based on EOF after this read */ | ||
| flush = feof(src_fp) ? Z_FINISH : Z_NO_FLUSH; | ||
|
|
||
| /* Compress chunk */ | ||
| do { | ||
| strm.next_out = (Bytef *)output_buffer; | ||
| strm.avail_out = output_buffer_size; | ||
|
|
||
| status = deflate(&strm, flush); | ||
| if (status == Z_STREAM_ERROR) { | ||
| flb_plg_error(ins, "deflate stream error during compression"); | ||
| ret = -1; | ||
| goto deflate_cleanup; | ||
| } | ||
|
|
||
| /* Write compressed data */ | ||
| size_t compressed_bytes = output_buffer_size - strm.avail_out; | ||
| if (compressed_bytes > 0) { | ||
| if (fwrite(output_buffer, 1, compressed_bytes, dst_fp) != compressed_bytes) { | ||
| flb_plg_error(ins, "failed to write compressed data"); | ||
| ret = -1; | ||
| goto deflate_cleanup; | ||
| } | ||
| } | ||
| } while (strm.avail_out == 0); | ||
|
|
||
| /* Verify all input was consumed */ | ||
| if (strm.avail_in != 0) { | ||
| flb_plg_error(ins, "deflate did not consume all input data"); | ||
| ret = -1; | ||
| goto deflate_cleanup; | ||
| } | ||
| } | ||
| } while (bytes_read > 0 && status != Z_STREAM_END); | ||
|
|
||
| /* Verify compression completed successfully */ | ||
| if (status != Z_STREAM_END) { | ||
| flb_plg_error(ins, "compression did not complete properly"); | ||
| ret = -1; | ||
| } else { | ||
| /* Write gzip footer (CRC32 + original size) */ | ||
| write_gzip_footer(dst_fp, crc, total_input_size); | ||
| } | ||
|
|
||
| deflate_cleanup: | ||
| deflateEnd(&strm); | ||
|
|
||
| cleanup: | ||
| if (input_buffer) flb_free(input_buffer); | ||
| if (output_buffer) flb_free(output_buffer); | ||
| fclose(src_fp); | ||
| fclose(dst_fp); | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
Critical: Gzip stream not finalized for files that are multiples of 64KB.
When a file's size is an exact multiple of GZIP_CHUNK_SIZE (64KB), the final fread fills the buffer completely. At this point, feof(src_fp) returns false because the EOF indicator is only set when a read attempts to go past EOF. Consequently, flush is set to Z_NO_FLUSH instead of Z_FINISH. On the next iteration, bytes_read is 0, the loop exits, and status != Z_STREAM_END triggers the error at line 687, or worse, produces a truncated gzip file.
Apply this fix to ensure Z_FINISH is always issued:
- /* Process file in chunks (ensure Z_FINISH is always issued) */
- do {
+ /* Process file in chunks */
+ for (;;) {
bytes_read = fread(input_buffer, 1, GZIP_CHUNK_SIZE, src_fp);
if (bytes_read > 0) {
/* Update CRC and total size */
crc = mz_crc32(crc, (const unsigned char *)input_buffer, bytes_read);
total_input_size += bytes_read;
/* Set up deflate input */
strm.next_in = (Bytef *)input_buffer;
strm.avail_in = bytes_read;
- /* Determine flush mode based on EOF after this read */
- flush = feof(src_fp) ? Z_FINISH : Z_NO_FLUSH;
+ /* Check if this is the last chunk by attempting to peek ahead */
+ int next_byte = fgetc(src_fp);
+ if (next_byte == EOF) {
+ flush = Z_FINISH;
+ } else {
+ ungetc(next_byte, src_fp);
+ flush = Z_NO_FLUSH;
+ }
/* Compress chunk */
do {
strm.next_out = (Bytef *)output_buffer;
strm.avail_out = output_buffer_size;
status = deflate(&strm, flush);
if (status == Z_STREAM_ERROR) {
flb_plg_error(ins, "deflate stream error during compression");
ret = -1;
goto deflate_cleanup;
}
/* Write compressed data */
size_t compressed_bytes = output_buffer_size - strm.avail_out;
if (compressed_bytes > 0) {
if (fwrite(output_buffer, 1, compressed_bytes, dst_fp) != compressed_bytes) {
flb_plg_error(ins, "failed to write compressed data");
ret = -1;
goto deflate_cleanup;
}
}
} while (strm.avail_out == 0);
/* Verify all input was consumed */
if (strm.avail_in != 0) {
flb_plg_error(ins, "deflate did not consume all input data");
ret = -1;
goto deflate_cleanup;
}
+
+ if (flush == Z_FINISH) {
+ break;
+ }
+ } else {
+ /* No more data read, break out */
+ break;
}
- } while (bytes_read > 0 && status != Z_STREAM_END);
+ }
/* Verify compression completed successfully */
if (status != Z_STREAM_END) {
flb_plg_error(ins, "compression did not complete properly");
ret = -1;
} else {
/* Write gzip footer (CRC32 + original size) */
write_gzip_footer(dst_fp, crc, total_input_size);
}| static int cleanup_old_files(struct flb_logrotate_conf *ctx, const char *directory, const char *base_filename) | ||
| { | ||
| DIR *dir; | ||
| struct dirent *entry; | ||
| char pattern[PATH_MAX]; | ||
| char full_path[PATH_MAX]; | ||
| char **files = NULL; | ||
| int file_count = 0; | ||
| int max_files = ctx->max_files; | ||
| int i, j; | ||
|
|
||
| /* Create pattern to match rotated files */ | ||
| snprintf(pattern, PATH_MAX - 1, "%s.", base_filename); | ||
|
|
||
| dir = opendir(directory); | ||
| if (!dir) { | ||
| return 0; /* Directory doesn't exist or can't be opened */ | ||
| } | ||
|
|
||
| /* Count matching files */ | ||
| while ((entry = readdir(dir)) != NULL) { | ||
| if (strncmp(entry->d_name, pattern, strlen(pattern)) == 0) { | ||
| file_count++; | ||
| } | ||
| } | ||
|
|
||
| if (file_count <= max_files) { | ||
| closedir(dir); | ||
| return 0; | ||
| } | ||
|
|
||
| /* Allocate array for file names */ | ||
| files = flb_calloc(file_count, sizeof(char *)); | ||
| if (!files) { | ||
| closedir(dir); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Collect file names */ | ||
| rewinddir(dir); | ||
| i = 0; | ||
| while ((entry = readdir(dir)) != NULL && i < file_count) { | ||
| if (strncmp(entry->d_name, pattern, strlen(pattern)) == 0) { | ||
| snprintf(full_path, PATH_MAX - 1, "%s" FLB_PATH_SEPARATOR "%s", | ||
| directory, entry->d_name); | ||
| files[i] = flb_strdup(full_path); | ||
| i++; | ||
| } | ||
| } | ||
| closedir(dir); |
There was a problem hiding this comment.
Pattern matching may delete unrelated files; check allocation return values.
The cleanup pattern "base_filename." matches any file starting with that prefix, which could inadvertently include non-rotated files like "base_filename.config" or "base_filename.backup". Additionally, flb_strdup at line 799 can return NULL on allocation failure, but this isn't checked before stat is called on the pointer (line 809), which would cause a crash.
Issue 1: Tighten the pattern match to avoid false positives.
Consider matching the timestamp format explicitly (e.g., \d{8}_\d{6} optionally followed by .gz) or at minimum verify that matched filenames follow the expected rotation naming scheme before including them in cleanup.
Issue 2: Check flb_strdup return value.
Apply this diff:
while ((entry = readdir(dir)) != NULL && i < file_count) {
if (strncmp(entry->d_name, pattern, strlen(pattern)) == 0) {
snprintf(full_path, PATH_MAX - 1, "%s" FLB_PATH_SEPARATOR "%s",
directory, entry->d_name);
files[i] = flb_strdup(full_path);
+ if (!files[i]) {
+ flb_plg_error(ctx->ins, "failed to allocate memory for file path");
+ /* Clean up already allocated entries */
+ for (int k = 0; k < i; k++) {
+ flb_free(files[k]);
+ }
+ flb_free(files);
+ closedir(dir);
+ return -1;
+ }
i++;
}
}Signed-off-by: SagiROosto <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
plugins/out_logrotate/logrotate.c (3)
955-958: Metrics writes never bumpcurrent_file_size.After writing metrics we close the file without updating the size counter, so rotations never trigger for metrics-only streams and the file grows unbounded.
if (event_chunk->type == FLB_INPUT_METRICS) { print_metrics_text(ctx->ins, fp, event_chunk->data, event_chunk->size); + update_file_size_counter(ctx, fp); fclose(fp); FLB_OUTPUT_RETURN(FLB_OK); }
793-833: Checkflb_strdupresults before using them.When
flb_strdup(full_path)fails,files[i]is NULL but later passed tostat()/unlink(), leading to crashes. We need to validate the allocation, free any previous entries, and abort the cleanup gracefully on failure.files[i] = flb_strdup(full_path); - i++; + if (!files[i]) { + flb_plg_error(ctx->ins, "failed to duplicate path for rotated file"); + while (i-- > 0) { + flb_free(files[i]); + } + flb_free(files); + closedir(dir); + return -1; + } + i++;
639-694: Always finish the gzip stream when EOF aligns with the chunk boundary.When the source file size is an exact multiple of
GZIP_CHUNK_SIZE,feof(src_fp)is still false after the final full read. The loop exits withstatus == Z_OK, the plugin logs "compression did not complete properly", and rotation fails even though the file was fully read. Issue #7572 still reproduces this way. Make sure we always emit aZ_FINISHpass (withstrm.avail_in == 0) after the read loop sodeflatereachesZ_STREAM_ENDand the footer is written.- /* Process file in chunks (ensure Z_FINISH is always issued) */ - do { + /* Process file in chunks */ + do { bytes_read = fread(input_buffer, 1, GZIP_CHUNK_SIZE, src_fp); if (bytes_read > 0) { @@ - } while (bytes_read > 0 && status != Z_STREAM_END); - - /* Verify compression completed successfully */ - if (status != Z_STREAM_END) { - flb_plg_error(ins, "compression did not complete properly"); - ret = -1; - } else { - /* Write gzip footer (CRC32 + original size) */ - write_gzip_footer(dst_fp, crc, total_input_size); - } + } while (bytes_read > 0 && status != Z_STREAM_END); + + if (status != Z_STREAM_END) { + flush = Z_FINISH; + strm.next_in = Z_NULL; + strm.avail_in = 0; + + do { + strm.next_out = (Bytef *)output_buffer; + strm.avail_out = output_buffer_size; + + status = deflate(&strm, flush); + if (status != Z_STREAM_END && status != Z_OK) { + flb_plg_error(ins, "deflate finish error during compression"); + ret = -1; + goto deflate_cleanup; + } + + size_t compressed_bytes = output_buffer_size - strm.avail_out; + if (compressed_bytes > 0) { + if (fwrite(output_buffer, 1, compressed_bytes, dst_fp) != compressed_bytes) { + flb_plg_error(ins, "failed to write compressed data"); + ret = -1; + goto deflate_cleanup; + } + } + } while (status != Z_STREAM_END); + } + + if (status == Z_STREAM_END) { + /* Write gzip footer (CRC32 + original size) */ + write_gzip_footer(dst_fp, crc, total_input_size); + } + else { + flb_plg_error(ins, "compression did not complete properly"); + ret = -1; + }
📜 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 (2)
📚 Learning: 2025-09-04T12:32:46.030Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10825
File: plugins/out_s3/s3.c:0-0
Timestamp: 2025-09-04T12:32:46.030Z
Learning: In Fluent Bit plugins, avoid creating plugin-local config_map entries that duplicate core functionality. The core already provides Retry_Limit parsed into ins->retry_limit, so plugins should use that directly rather than defining their own retry_limit configuration option.
Applied to files:
tests/runtime/out_logrotate.c
📚 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)
tests/runtime/out_logrotate.c (1)
src/flb_lib.c (10)
flb_create(138-220)flb_service_set(647-673)flb_input(261-271)flb_input_set(300-330)flb_output(274-284)flb_output_set(515-546)flb_start(914-925)flb_lib_push(774-801)flb_stop(942-985)flb_destroy(223-258)
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(1250-1291)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(99-116)flb_log_event_decoder_next(310-406)flb_log_event_decoder_destroy(147-179)
| /* Create pattern to match rotated files */ | ||
| snprintf(pattern, PATH_MAX - 1, "%s.", base_filename); | ||
|
|
||
| dir = opendir(directory); | ||
| if (!dir) { | ||
| return 0; /* Directory doesn't exist or can't be opened */ | ||
| } | ||
|
|
||
| /* Count matching files */ | ||
| while ((entry = readdir(dir)) != NULL) { | ||
| if (strncmp(entry->d_name, pattern, strlen(pattern)) == 0) { | ||
| file_count++; | ||
| } | ||
| } | ||
|
|
||
| if (file_count <= max_files) { | ||
| closedir(dir); | ||
| return 0; | ||
| } | ||
|
|
||
| /* Allocate array for file names */ | ||
| files = flb_calloc(file_count, sizeof(char *)); | ||
| if (!files) { | ||
| closedir(dir); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Collect file names */ | ||
| rewinddir(dir); | ||
| i = 0; | ||
| while ((entry = readdir(dir)) != NULL && i < file_count) { | ||
| if (strncmp(entry->d_name, pattern, strlen(pattern)) == 0) { | ||
| snprintf(full_path, PATH_MAX - 1, "%s" FLB_PATH_SEPARATOR "%s", | ||
| directory, entry->d_name); | ||
| files[i] = flb_strdup(full_path); | ||
| i++; | ||
| } | ||
| } | ||
| closedir(dir); | ||
|
|
||
| /* Sort files by modification time (oldest first) */ | ||
| for (i = 0; i < file_count - 1; i++) { | ||
| for (j = i + 1; j < file_count; j++) { | ||
| struct stat st1, st2; | ||
| if (stat(files[i], &st1) == 0 && stat(files[j], &st2) == 0) { | ||
| if (st1.st_mtime > st2.st_mtime) { | ||
| char *temp = files[i]; | ||
| files[i] = files[j]; | ||
| files[j] = temp; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /* Remove oldest files */ | ||
| if (file_count > max_files) { | ||
| flb_plg_info(ctx->ins, "cleaning up old rotated files: removing %d files (keeping %d)", | ||
| file_count - max_files, max_files); | ||
| } | ||
| for (i = 0; i < file_count - max_files; i++) { | ||
| if (unlink(files[i]) == 0) { | ||
| flb_plg_debug(ctx->ins, "removed old rotated file: %s", files[i]); |
There was a problem hiding this comment.
Tighten the cleanup filename pattern.
Using strncmp(entry->d_name, pattern, strlen(pattern)) == 0 will happily delete anything starting with "base_filename."—for example base_filename.config or base_filename.backup. That’s data loss. Please ensure the suffix matches the rotation format (YYYYMMDD_HHMMSS optionally followed by .gz) before counting or unlinking entries.
A simple guard is to validate that the substring after pattern is 15 digits with an underscore at position 8, or 18 characters ending in .gz, and bail out otherwise.
🤖 Prompt for AI Agents
In plugins/out_logrotate/logrotate.c around lines 765-826, the code currently
treats any filename starting with "base_filename." as a rotated file; tighten
the matching so only files that follow the rotation timestamp format are
counted/removed: when scanning entries, after matching the prefix (pattern)
validate the remainder exactly matches either "YYYYMMDD_HHMMSS" (15 chars: 8
digits, '_' at pos 8, 6 digits) or that same 15-char timestamp followed by ".gz"
(18 chars total with trailing ".gz"); use isdigit checks for the digit positions
and explicit length checks, and only increment file_count or add to files[] when
the validation passes; apply the same validation before unlinking so only
strictly matching files are deleted, and ensure you continue to close the
directory and free allocated memory on error/exit.
| /* Extract directory and base filename for cleanup */ | ||
| out_file_copy = flb_strdup(out_file); | ||
| if (out_file_copy) { | ||
| #ifdef FLB_SYSTEM_WINDOWS | ||
| PathRemoveFileSpecA(out_file_copy); | ||
| strncpy(directory, out_file_copy, PATH_MAX - 1); | ||
| directory[PATH_MAX - 1] = '\0'; | ||
| #else | ||
| strncpy(directory, dirname(out_file_copy), PATH_MAX - 1); | ||
| directory[PATH_MAX - 1] = '\0'; | ||
| #endif | ||
| flb_free(out_file_copy); | ||
| } | ||
|
|
||
| /* Get base filename for cleanup */ | ||
| char *last_sep = strrchr(out_file, FLB_PATH_SEPARATOR[0]); | ||
| if (last_sep) { | ||
| strncpy(base_filename, last_sep + 1, PATH_MAX - 1); | ||
| } else { | ||
| strncpy(base_filename, out_file, PATH_MAX - 1); | ||
| } | ||
| base_filename[PATH_MAX - 1] = '\0'; | ||
|
|
||
| /* Rotate the file */ | ||
| if (rotate_file(ctx, out_file) == 0) { | ||
| /* Reset file size counter after rotation */ | ||
| ctx->current_file_size = 0; | ||
| /* Clean up old rotated files */ | ||
| cleanup_old_files(ctx, directory, base_filename); | ||
| } |
There was a problem hiding this comment.
Guard cleanup_old_files when directory extraction fails.
If flb_strdup(out_file) returns NULL we leave directory uninitialized, yet still pass it into cleanup_old_files, yielding undefined behavior on low-memory paths. Either bail out of rotation cleanup or set a deterministic fallback (e.g., ".") before invoking cleanup.
- if (should_rotate_file(ctx)) {
+ if (should_rotate_file(ctx)) {
+ bool have_directory = false;
+ directory[0] = '\0';
/* Extract directory and base filename for cleanup */
out_file_copy = flb_strdup(out_file);
if (out_file_copy) {
@@
- flb_free(out_file_copy);
+ flb_free(out_file_copy);
+ have_directory = true;
}
@@
- if (rotate_file(ctx, out_file) == 0) {
+ if (rotate_file(ctx, out_file) == 0) {
/* Reset file size counter after rotation */
ctx->current_file_size = 0;
/* Clean up old rotated files */
- cleanup_old_files(ctx, directory, base_filename);
+ if (have_directory) {
+ cleanup_old_files(ctx, directory, base_filename);
+ }
}
}🤖 Prompt for AI Agents
In plugins/out_logrotate/logrotate.c around lines 888 to 917, the code can call
cleanup_old_files with an uninitialized directory when flb_strdup(out_file)
returns NULL; ensure you guard that case by either skipping the cleanup step
when directory extraction failed or by assigning a deterministic fallback (e.g.,
directory = "." and ensure it is NUL-terminated) before calling
cleanup_old_files; also make sure base_filename remains valid and NUL-terminated
even when out_file_copy is NULL to avoid passing invalid pointers to
cleanup_old_files.
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).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
Configuration
Build
Tests