router: extend router interface to support metrics #10953
Conversation
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
WalkthroughMoves routing state out of Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as Main
participant Config as flb_config
participant Router as flb_router
participant Routes as RoutesMask
Main->>Config: flb_config_init(...)
Config->>Router: flb_router_create(config)
Router-->>Config: router*
Config->>Routes: flb_routes_mask_set_size(N, router)
Routes-->>Config: OK
Note right of Router: holds mask fields\nand cmetrics
Main->>Config: flb_config_exit(...)
Config->>Router: flb_router_destroy(router)
Router-->>Config: freed
sequenceDiagram
autonumber
participant In as Input
participant Eng as Engine
participant Task as Task
participant Router as flb_router
participant Out as Output
In->>Eng: EventChunk (type=LOGS)
Eng->>Task: schedule/process
Task->>Router: flb_routes_mask_get_bit(router)
alt Eligible
Eng->>Router: inc logs_records_total{in,out}
Eng->>Router: inc logs_bytes_total{in,out}
else Not eligible / Dropped
Eng->>Router: inc logs_drop_records_total{in,out}
Eng->>Router: inc logs_drop_bytes_total{in,out}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 (1)
🧰 Additional context used🧬 Code graph analysis (1)src/flb_routes_mask.c (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
🔇 Additional comments (8)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_routes_mask.c (1)
181-194: Correct routes mask sizing: use ceiling division to compute element count.The current formula adds the remainder instead of rounding up, which can wildly over-allocate or under-allocate (e.g., 127 slots → 64 elements instead of 2). Use ceiling division.
int flb_routes_mask_set_size(size_t mask_size, struct flb_router *router) { - if (mask_size < 1) { - mask_size = 1; - } - - mask_size = (mask_size / FLB_ROUTES_MASK_ELEMENT_BITS) + - (mask_size % FLB_ROUTES_MASK_ELEMENT_BITS); - - router->route_mask_size = mask_size; - router->route_mask_slots = mask_size * FLB_ROUTES_MASK_ELEMENT_BITS; - - return flb_routes_empty_mask_create(router); + /* mask_size is the requested number of slots (bits) */ + if (mask_size < 1) { + mask_size = 1; + } + /* number of elements needed to hold mask_size bits */ + size_t elements = (mask_size + FLB_ROUTES_MASK_ELEMENT_BITS - 1) / + FLB_ROUTES_MASK_ELEMENT_BITS; + + router->route_mask_size = elements; + router->route_mask_slots = elements * FLB_ROUTES_MASK_ELEMENT_BITS; + + return flb_routes_empty_mask_create(router); }
🧹 Nitpick comments (11)
src/flb_engine.c (3)
420-431: Router drop metrics in RETRY (max retries) path are consistent.When maximum retries are exhausted, drop metrics are correctly updated. Note that
in_nameandout_nameare reassigned (lines 421-422) but already have the correct values from line 294-295, so this is redundant but harmless.Lines 421-422 reassign
in_nameandout_namebut they already have the correct values from lines 294-295. Consider removing these redundant assignments:if (task->event_chunk->type == FLB_INPUT_LOGS) { - in_name = (char *) flb_input_name(task->i_ins); - out_name = (char *) flb_output_name(ins); - cmt_counter_add(config->router->logs_drop_records_total, ts,
246-248: Use const for names to document immutability.in_name/out_name are not modified; prefer const char *. Keep casts at call sites if needed by cmetrics API.
- char *in_name; - char *out_name; + const char *in_name; + const char *out_name; @@ - in_name = (char *) flb_input_name(task->i_ins); - out_name = (char *) flb_output_name(ins); + in_name = flb_input_name(task->i_ins); + out_name = flb_output_name(ins);Also applies to: 294-296
415-436: Remove redundant name re-assignments.in_name/out_name are already set above; reassigning adds noise.
- if (task->event_chunk->type == FLB_INPUT_LOGS) { - in_name = (char *) flb_input_name(task->i_ins); - out_name = (char *) flb_output_name(ins); - + if (task->event_chunk->type == FLB_INPUT_LOGS) { cmt_counter_add(config->router->logs_drop_records_total, ts, task->records, 2, (char *[]) {in_name, out_name});src/flb_router.c (3)
359-369: Router destruction is correct but has a minor issue.The cleanup sequence is correct (masks → cmt → router). However, line 368 sets
router = NULLafter freeing, which has no effect sincerouteris a local parameter. The caller's pointer remains unchanged.Line 368 sets
router = NULLbut this only affects the local parameter, not the caller's pointer. Consider removing this line as it has no effect:flb_free(router); - router = NULL; }Alternatively, if the intent is to null the caller's pointer, change the signature to
void flb_router_destroy(struct flb_router **router)and dereference accordingly. However, this is inconsistent with other destroy functions in the codebase (e.g.,flb_cf_destroy), so removing the line is preferred.
273-333: Metrics definitions look good; consider de-duplicating label keys.Minor DRY: reuse a single label_keys array for the four counters.
static int router_metrics_create(struct flb_router *router) { @@ - router->logs_records_total = cmt_counter_create( + static char *labels_io[] = { "input", "output" }; + router->logs_records_total = cmt_counter_create( router->cmt, "fluentbit", "routing_logs", "records_total", "Total log records routed from input to output", - 2, - (char *[]) { "input", "output" }); + 2, labels_io); @@ - router->logs_bytes_total = cmt_counter_create( + router->logs_bytes_total = cmt_counter_create( router->cmt, "fluentbit", "routing_logs", "bytes_total", "Total bytes routed from input to output (logs)", - 2, - (char *[]) { "input", "output" }); + 2, labels_io); @@ - router->logs_drop_records_total = cmt_counter_create( + router->logs_drop_records_total = cmt_counter_create( router->cmt, "fluentbit", "routing_logs", "drop_records_total", "Total log records dropped during routing", - 2, - (char *[]) { "input", "output" }); + 2, labels_io); @@ - router->logs_drop_bytes_total = cmt_counter_create( + router->logs_drop_bytes_total = cmt_counter_create( router->cmt, "fluentbit", "routing_logs", "drop_bytes_total", "Total bytes dropped during routing (logs)", - 2, - (char *[]) { "input", "output" }); + 2, labels_io);
335-357: Silence unused parameter warning.config is not used; cast to void.
struct flb_router *flb_router_create(struct flb_config *config) { struct flb_router *router; + (void) config;src/flb_routes_mask.c (4)
159-171: Add failure log and ensure mem utilities header is included.On allocation failure, emit a clear error for troubleshooting. Also ensure flb_mem.h is included directly (if not via transitive includes) so flb_calloc/flb_free are declared.
int flb_routes_empty_mask_create(struct flb_router *router) { flb_routes_empty_mask_destroy(router); router->route_empty_mask = flb_calloc(router->route_mask_size, sizeof(flb_route_mask_element)); if (router->route_empty_mask == NULL) { + flb_error("[routes_mask] cannot allocate empty mask (elements=%zu)", + router->route_mask_size); return -1; } return 0; }Please verify whether this file directly includes
<fluent-bit/flb_mem.h>. If not, add:#include <fluent-bit/flb_routes_mask.h> +#include <fluent-bit/flb_mem.h>
26-34: Optional: add defensive checks for getters.These helpers assume non-NULL router. Consider asserting or returning 0 when router is NULL to avoid UB in edge paths.
size_t flb_routes_mask_get_size(struct flb_router *router) { - return router->route_mask_size; + return router ? router->route_mask_size : 0; } size_t flb_routes_mask_get_slots(struct flb_router *router) { - return router->route_mask_slots; + return router ? router->route_mask_slots : 0; }
55-56: Consider copying the canonical empty mask instead of memset(0).If the empty mask ever changes semantics, memcpy from router->route_empty_mask keeps behavior consistent and avoids duplicating assumptions.
- size = flb_routes_mask_get_size(in->config->router); - memset(routes_mask, 0, sizeof(flb_route_mask_element) * size); + size = flb_routes_mask_get_size(in->config->router); + if (in->config->router->route_empty_mask) { + memcpy(routes_mask, + in->config->router->route_empty_mask, + sizeof(flb_route_mask_element) * size); + } + else { + memset(routes_mask, 0, sizeof(flb_route_mask_element) * size); + }
91-99: Deduplicate index validation across set/get/clear.The bounds check logic is repeated. Factor into a small static inline helper to reduce drift and keep warnings uniform.
+static inline int routes_mask_index_invalid(const struct flb_router *router, int value) +{ + if (value < 0 || (size_t) value >= router->route_mask_slots) { + return 1; + } + return 0; +} ... - if (value < 0 || value >= router->route_mask_slots) { + if (routes_mask_index_invalid(router, value)) { flb_warn("[routes_mask] Can't set bit (%d) past limits of bitfield", value); return; }(Apply similarly to clear/get.)
Also applies to: 115-124, 140-149
src/flb_config.c (1)
623-623: Guard router destroy or make flb_router_destroy NULL-safe.Defensive check prevents crashes if config->router is NULL on unusual init/fail paths. Alternatively, add a NULL guard inside flb_router_destroy (preferred; see router.c comment).
- flb_router_destroy(config->router); + if (config->router) { + flb_router_destroy(config->router); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
include/fluent-bit/flb_config.h(1 hunks)include/fluent-bit/flb_router.h(2 hunks)include/fluent-bit/flb_routes_mask.h(1 hunks)plugins/in_storage_backlog/sb.c(4 hunks)src/flb_config.c(3 hunks)src/flb_engine.c(9 hunks)src/flb_input_chunk.c(15 hunks)src/flb_metrics_exporter.c(2 hunks)src/flb_router.c(1 hunks)src/flb_routes_mask.c(8 hunks)src/flb_task.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
src/flb_task.c (1)
src/flb_routes_mask.c (1)
flb_routes_mask_get_bit(134-149)
src/flb_router.c (3)
lib/cmetrics/src/cmt_counter.c (1)
cmt_counter_create(26-81)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_routes_mask.c (1)
flb_routes_empty_mask_destroy(173-179)
src/flb_routes_mask.c (1)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)
include/fluent-bit/flb_router.h (1)
src/flb_router.c (2)
flb_router_create(335-357)flb_router_destroy(359-369)
src/flb_metrics_exporter.c (1)
lib/cmetrics/src/cmt_cat.c (1)
cmt_cat(638-649)
plugins/in_storage_backlog/sb.c (2)
src/flb_routes_mask.c (1)
flb_routes_mask_get_slots(31-34)include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)
src/flb_engine.c (4)
src/flb_input.c (1)
flb_input_name(790-797)src/flb_output.c (1)
flb_output_name(1087-1094)lib/cmetrics/src/cmt_counter.c (1)
cmt_counter_add(119-135)src/flb_routes_mask.c (1)
flb_routes_mask_set_size(181-194)
src/flb_config.c (4)
src/flb_router.c (2)
flb_router_create(335-357)flb_router_destroy(359-369)src/config_format/flb_config_format.c (1)
flb_cf_destroy(151-157)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_routes_mask.c (1)
flb_routes_mask_set_size(181-194)
src/flb_input_chunk.c (5)
src/flb_routes_mask.c (2)
flb_routes_mask_is_empty(151-157)flb_routes_mask_get_size(26-29)lib/cmetrics/src/cmt_counter.c (1)
cmt_counter_add(119-135)src/flb_input.c (1)
flb_input_name(790-797)src/flb_output.c (1)
flb_output_name(1087-1094)include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)
include/fluent-bit/flb_routes_mask.h (1)
src/flb_routes_mask.c (10)
flb_routes_mask_set_by_tag(40-76)flb_routes_mask_get_bit(134-149)flb_routes_mask_set_bit(85-100)flb_routes_mask_clear_bit(109-124)flb_routes_mask_is_empty(151-157)flb_routes_empty_mask_create(159-171)flb_routes_empty_mask_destroy(173-179)flb_routes_mask_set_size(181-194)flb_routes_mask_get_size(26-29)flb_routes_mask_get_slots(31-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (23)
src/flb_metrics_exporter.c (1)
35-35: Concatenate router CMetrics into exporter context: LGTMThe null checks and
cmt_caterror handling are consistent with existing patterns.Also applies to: 312-319
include/fluent-bit/flb_config.h (1)
294-296: Router pointer initialization and teardown confirmed —config->routeris created inflb_config_init()(flb_config.c:307) and destroyed inflb_config_exit()(flb_config.c:623).src/flb_task.c (1)
447-450: Router lifecycle validated
config->routeris instantiated in flb_config_init (src/flb_config.c:307) and cleaned up in flb_config_exit (src/flb_config.c:623), ensuring it exists before any task is created and is properly destroyed on exit.src/flb_input_chunk.c (3)
217-233: LGTM! Router metrics integration for logs is correct.The new metrics block properly gates router metrics updates with an event type check and uses the correct counter APIs with appropriate labels (input name, output name).
965-966: Same router null-check concern.Ensure
in->config->routeris valid before callingflb_routes_mask_get_size.
662-664: Resolve: router always initialized
config->routeris created at src/flb_config.c:307 and immediately checked (and aborts config creation) if null (lines 307–312), so by the timeflb_input_chunk_mapruns,in->config->routercannot be NULL. No additional null check needed.src/flb_config.c (3)
307-314: Router creation order looks correct.Router is created before inputs/outputs are initialized (which happens later in
flb_engine_start). The initial size is set to 1, then resized in the engine once the actual output count is known (line 861 in flb_engine.c). Error handling properly cleans up on failure.
623-623: LGTM! Router cleanup is correct.The router is destroyed in the cleanup path, which properly releases all router resources including metrics and masks.
47-47: Include looks correct.Header addition aligns with new router usage.
src/flb_engine.c (10)
294-295: LGTM! Input/output names extracted correctly.Using
flb_input_nameandflb_output_nameensures alias is preferred over instance name, providing consistent labeling.
314-322: Verify metrics are incremented for the OK path.Router metrics correctly increment records and bytes for the FLB_INPUT_LOGS type with input/output labels. This mirrors the output plugin metrics above.
374-382: Router drop metrics in RETRY (no retry config) path are consistent.When
retry_limit == FLB_OUT_RETRY_NONE, both drop_records and drop_bytes are correctly incremented for logs.
510-518: Router drop metrics in ERROR path are consistent.Drop metrics for errors match the pattern used in the retry paths.
861-861: LGTM! Routes mask sizing now uses router.The call to
flb_routes_mask_set_sizecorrectly passesconfig->routerinstead ofconfig, aligning with the new router-centric API.
304-322: Router metrics on OK path look correct (labels: input, output).Counters increment with total_events and size only for LOGS; matches intent.
361-364: Gauge labeling with output name is consistent.Chunk capacity gauge uses {output} uniformly across paths.
Also applies to: 384-387, 433-436, 493-496, 520-523
371-387: Drop metrics for LOGS cover both records and bytes.Updates align with the new router counters (drop_records_total / drop_bytes_total).
Also applies to: 505-523
861-866: LGTM: route mask sized using router with error handling.Dimensioning based on outputs and checking ret is the right place/time.
327-329: Histogram label schema matches observe call –cmt_latencyis created with two labels["input","output"]in the correct order, matching theobserveinvocation.src/flb_router.c (2)
273-333: LGTM! Router metrics creation is well-structured.The four counters follow a consistent naming pattern under the
fluentbit.routing_logsnamespace with appropriate descriptions. All counters use two labels (input, output) matching the usage in flb_engine.c. Error handling returns -1 on any failure.
335-357: LGTM! Router creation has proper error handling.The function correctly:
- Allocates the router structure
- Creates the cmetrics context
- Initializes metrics via
router_metrics_create- Cleans up via
flb_router_destroyon any failure- Returns NULL on error
include/fluent-bit/flb_routes_mask.h (1)
50-69: No remaining calls use the oldflb_config *config; all invocations correctly pass theflb_router *(e.g. viaconfig->router).src/flb_routes_mask.c (1)
181-194: No action required—bounds already safe
flb_routes_mask_set_size is always called with mk_list_size(&config->outputs), and output IDs are assigned sequentially from 0 up, so router->route_mask_slots (rounded-up slots × FLB_ROUTES_MASK_ELEMENT_BITS) is guaranteed to be ≥ highest output id + 1.
| int flb_router_metrics_create(struct flb_config *config, struct flb_router *router); | ||
| struct flb_router *flb_router_create(struct flb_config *config); |
There was a problem hiding this comment.
🧩 Analysis chain
API name/signature mismatch: flb_router_metrics_create vs router_metrics_create
src/flb_router.c calls router_metrics_create(router) (no config arg), while this header declares flb_router_metrics_create(struct flb_config *, struct flb_router *). This likely causes an unresolved/unused symbol scenario.
Run to confirm and decide whether to:
- remove the public
flb_router_metrics_createdeclaration, keeping a privaterouter_metrics_create, or - standardize on a single public symbol and update the call site.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'flb_router_metrics_create\s*\(' -C2 || true
rg -n 'router_metrics_create\s*\(' -C2 || true
rg -n 'flb_router_create\s*\(' -n -C3 src/flb_router.c || trueLength of output: 1338
Remove or correct flb_router_metrics_create declaration
The header declares int flb_router_metrics_create(struct flb_config *, struct flb_router *) but only a private static int router_metrics_create(struct flb_router *) exists in src/flb_router.c, leading to an unresolved symbol. Either remove the public declaration from include/fluent-bit/flb_router.h or provide a matching implementation.
🤖 Prompt for AI Agents
In include/fluent-bit/flb_router.h around lines 86-87, the header declares
flb_router_metrics_create(struct flb_config *, struct flb_router *) but only a
private static router_metrics_create(struct flb_router *) exists in
src/flb_router.c causing an unresolved symbol; either remove the public
declaration from the header or implement a public flb_router_metrics_create with
the declared signature in src/flb_router.c that adapts/forwards to the existing
router_metrics_create (accepting/using the config param or ignoring it
consistently) and ensure it is non-static and exported.
There was a problem hiding this comment.
@edsiper, thank you for confirming! I'm glad I could help identify this issue. 🐰
🌟
┏━━━━━━━━━━━━━━━━━━┓
┃ Happy to help! ┃
┗━━━━━━━━━━━━━━━━━━┛
(_/)
(='.'=)
(")(")
| slots = flb_routes_mask_get_slots(context->ins->config->router); | ||
| memset(&dummy_input_chunk, 0, sizeof(struct flb_input_chunk)); | ||
| memset(context->dummy_routes_mask, 0, slots * sizeof(flb_route_mask_element)); | ||
|
|
There was a problem hiding this comment.
Use route_mask element count, not slots (bits), for allocation/memset
You’re using flb_routes_mask_get_slots() (bits) to size arrays of flb_route_mask_element. This over-allocates by a factor of element bit-width (typically 64) and repeats during memset. Use the element count from flb_routes_mask_get_size() instead.
Apply:
- size_t slots;
+ size_t elements;
@@
- slots = flb_routes_mask_get_slots(context->ins->config->router);
+ elements = flb_routes_mask_get_size(context->ins->config->router);
memset(&dummy_input_chunk, 0, sizeof(struct flb_input_chunk));
- memset(context->dummy_routes_mask, 0, slots * sizeof(flb_route_mask_element));
+ memset(context->dummy_routes_mask, 0, elements * sizeof(flb_route_mask_element));And at init:
- size = flb_routes_mask_get_slots(config->router);
- ctx->dummy_routes_mask = flb_calloc(size, sizeof(flb_route_mask_element));
+ size = flb_routes_mask_get_size(config->router);
+ ctx->dummy_routes_mask = flb_calloc(size, sizeof(flb_route_mask_element));This prevents unnecessary memory usage and keeps the mask operations within bounds.
Also applies to: 668-671
There was a problem hiding this comment.
@edsiper, understood! I'll disregard this suggestion. Thank you for clarifying.
There was a problem hiding this comment.
@edsiper, understood! I'll defer to your judgment on this. Thank you for the clarification.
| /* Routing */ | ||
| flb_routes_mask_set_size(1, config); | ||
| config->router = flb_router_create(config); | ||
| if (!config->router) { | ||
| flb_error("[config] could not create router"); | ||
| flb_cf_destroy(cf); | ||
| flb_free(config); | ||
| return NULL; | ||
| } | ||
| flb_routes_mask_set_size(1, config->router); | ||
|
|
There was a problem hiding this comment.
Handle flb_routes_mask_set_size() failure to avoid partial-init leaks.
If mask allocation fails, we proceed with a half-initialized router. Bail out and clean up.
- flb_routes_mask_set_size(1, config->router);
+ ret = flb_routes_mask_set_size(1, config->router);
+ if (ret != 0) {
+ flb_error("[config] routing mask init failed");
+ flb_cf_destroy(cf);
+ flb_router_destroy(config->router);
+ flb_free(config);
+ return NULL;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* Routing */ | |
| flb_routes_mask_set_size(1, config); | |
| config->router = flb_router_create(config); | |
| if (!config->router) { | |
| flb_error("[config] could not create router"); | |
| flb_cf_destroy(cf); | |
| flb_free(config); | |
| return NULL; | |
| } | |
| flb_routes_mask_set_size(1, config->router); | |
| /* Routing */ | |
| config->router = flb_router_create(config); | |
| if (!config->router) { | |
| flb_error("[config] could not create router"); | |
| flb_cf_destroy(cf); | |
| flb_free(config); | |
| return NULL; | |
| } | |
| ret = flb_routes_mask_set_size(1, config->router); | |
| if (ret != 0) { | |
| flb_error("[config] routing mask init failed"); | |
| flb_cf_destroy(cf); | |
| flb_router_destroy(config->router); | |
| flb_free(config); | |
| return NULL; | |
| } |
🤖 Prompt for AI Agents
In src/flb_config.c around lines 306 to 315, flb_routes_mask_set_size(...)
return value is not checked so a mask allocation failure would leave a
half-initialized router and leak resources; update the code to check the
function's return (non-zero = failure), log an error, call flb_cf_destroy(cf),
destroy/free the router (e.g. flb_router_destroy(config->router) or the
appropriate cleanup function), free config, and return NULL to fully abort
initialization on failure.
| void flb_router_destroy(struct flb_router *router) | ||
| { | ||
| flb_routes_empty_mask_destroy(router); | ||
|
|
||
| if (router->cmt) { | ||
| cmt_destroy(router->cmt); | ||
| } | ||
|
|
||
| flb_free(router); | ||
| router = NULL; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
NULL-guard flb_router_destroy to avoid potential segfaults.
Make destroy idempotent and safe (callers like config exit paths often destroy unconditionally).
-void flb_router_destroy(struct flb_router *router)
+void flb_router_destroy(struct flb_router *router)
{
- flb_routes_empty_mask_destroy(router);
+ if (router == NULL) {
+ return;
+ }
+
+ flb_routes_empty_mask_destroy(router);
if (router->cmt) {
cmt_destroy(router->cmt);
}
flb_free(router);
- router = NULL;
+ /* note: caller holds the pointer; setting local param to NULL has no effect */
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void flb_router_destroy(struct flb_router *router) | |
| { | |
| flb_routes_empty_mask_destroy(router); | |
| if (router->cmt) { | |
| cmt_destroy(router->cmt); | |
| } | |
| flb_free(router); | |
| router = NULL; | |
| } | |
| void flb_router_destroy(struct flb_router *router) | |
| { | |
| if (router == NULL) { | |
| return; | |
| } | |
| flb_routes_empty_mask_destroy(router); | |
| if (router->cmt) { | |
| cmt_destroy(router->cmt); | |
| } | |
| flb_free(router); | |
| /* note: caller holds the pointer; setting local param to NULL has no effect */ | |
| } |
🤖 Prompt for AI Agents
In src/flb_router.c around lines 359 to 369, make flb_router_destroy NULL-safe
and idempotent by returning immediately if the passed-in router is NULL, keep
the existing cmt NULL-check and destroy call, call flb_routes_empty_mask_destroy
only when router is non-NULL, and remove the ineffective "router = NULL"
assignment after flb_free(router) (since the parameter is passed by value); this
ensures safe repeated/conditional destruction without dereferencing NULL or
leaving misleading code.
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/flb_routes_mask.c (4)
87-102: Add NULL guard to prevent crash when router is NULL.The function dereferences
router->route_mask_slotsat line 93 without validating thatrouteris non-NULL. This will cause a segmentation fault if called with NULL.Apply this diff:
void flb_routes_mask_set_bit(flb_route_mask_element *routes_mask, int value, struct flb_router *router) { int index; uint64_t bit; + if (router == NULL) { + flb_warn("[routes_mask] Can't set bit, router is NULL"); + return; + } + if (value < 0 || value >= router->route_mask_slots) {
111-126: Add NULL guard to prevent crash when router is NULL.The function dereferences
router->route_mask_slotsat line 117 without validating thatrouteris non-NULL. This will cause a segmentation fault if called with NULL.Apply this diff:
void flb_routes_mask_clear_bit(flb_route_mask_element *routes_mask, int value, struct flb_router *router) { int index; uint64_t bit; + if (router == NULL) { + flb_warn("[routes_mask] Can't clear bit, router is NULL"); + return; + } + if (value < 0 || value >= router->route_mask_slots) {
136-151: Add NULL guard to prevent crash when router is NULL.The function dereferences
router->route_mask_slotsat line 142 without validating thatrouteris non-NULL. This will cause a segmentation fault if called with NULL.Apply this diff:
int flb_routes_mask_get_bit(flb_route_mask_element *routes_mask, int value, struct flb_router *router) { int index; uint64_t bit; + if (router == NULL) { + flb_warn("[routes_mask] Can't get bit, router is NULL"); + return 0; + } + if (value < 0 || value >= router->route_mask_slots) {
192-205: Add NULL guard to prevent crash when router is NULL.The function dereferences
router->route_mask_sizeandrouter->route_mask_slotsat lines 201-202 without validating thatrouteris non-NULL. This will cause a segmentation fault if called with NULL.Apply this diff:
int flb_routes_mask_set_size(size_t mask_size, struct flb_router *router) { + if (router == NULL) { + flb_error("[routes_mask] Cannot set size, router is NULL"); + return -1; + } + if (mask_size < 1) {
♻️ Duplicate comments (1)
src/flb_routes_mask.c (1)
26-34: Add NULL guards in getter functions to prevent crashes.Both getter functions dereference
routerwithout NULL checks. Ifrouteris NULL, a segmentation fault will occur. This is inconsistent withflb_routes_mask_is_empty(lines 156-158) which properly guards against NULL.Apply this diff:
size_t flb_routes_mask_get_size(struct flb_router *router) { + if (router == NULL) { + return 0; + } return router->route_mask_size; } size_t flb_routes_mask_get_slots(struct flb_router *router) { + if (router == NULL) { + return 0; + } return router->route_mask_slots; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_routes_mask.c(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/flb_routes_mask.c (1)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
🔇 Additional comments (4)
src/flb_routes_mask.c (4)
52-54: LGTM!The NULL guards for
in->configandin->config->routerproperly prevent potential segmentation faults. The defensive check aligns with best practices.
156-161: LGTM!The NULL guards and corrected byte count in
memcmp(usingrouter->route_mask_size * sizeof(flb_route_mask_element)) properly address the issues identified in previous reviews.
166-168: LGTM!The NULL guard prevents crashes and the early return pattern is appropriate.
183-189: LGTM!The NULL guards and defensive double-check before freeing memory follow best practices for safe resource cleanup.
Signed-off-by: Eduardo Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/flb_routes_mask.c (3)
87-102: Add NULL check for consistency and safety.The function dereferences
router->route_mask_slotsat line 93 without validating thatrouteris non-NULL. This is inconsistent withflb_routes_mask_is_empty,flb_routes_empty_mask_create, andflb_routes_empty_mask_destroy, which all guard against NULL router.Apply this diff:
void flb_routes_mask_set_bit(flb_route_mask_element *routes_mask, int value, struct flb_router *router) { int index; uint64_t bit; + if (router == NULL) { + flb_warn("[routes_mask] Can't set bit with NULL router"); + return; + } + if (value < 0 || value >= router->route_mask_slots) {
111-126: Add NULL check for consistency and safety.The function dereferences
router->route_mask_slotsat line 117 without validating thatrouteris non-NULL. For consistency with other functions that guard against NULL router, add a defensive check.Apply this diff:
void flb_routes_mask_clear_bit(flb_route_mask_element *routes_mask, int value, struct flb_router *router) { int index; uint64_t bit; + if (router == NULL) { + flb_warn("[routes_mask] Can't clear bit with NULL router"); + return; + } + if (value < 0 || value >= router->route_mask_slots) {
136-151: Add NULL check for consistency and safety.The function dereferences
router->route_mask_slotsat line 142 without validating thatrouteris non-NULL. For consistency with other functions that guard against NULL router, add a defensive check.Apply this diff:
int flb_routes_mask_get_bit(flb_route_mask_element *routes_mask, int value, struct flb_router *router) { int index; uint64_t bit; + if (router == NULL) { + flb_warn("[routes_mask] Can't get bit with NULL router"); + return 0; + } + if (value < 0 || value >= router->route_mask_slots) {
♻️ Duplicate comments (1)
src/flb_routes_mask.c (1)
26-34: Add NULL guards to prevent crashes in getter functions.Both getter functions directly dereference
routerwithout NULL checks. This issue was previously flagged but remains unaddressed. For consistency withflb_routes_mask_is_empty(which guards against NULL at lines 156-158), add defensive checks here as well.Apply this diff:
size_t flb_routes_mask_get_size(struct flb_router *router) { + if (router == NULL) { + return 0; + } return router->route_mask_size; } size_t flb_routes_mask_get_slots(struct flb_router *router) { + if (router == NULL) { + return 0; + } return router->route_mask_slots; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_routes_mask.c(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/flb_routes_mask.c (1)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (4)
src/flb_routes_mask.c (4)
153-162: LGTM! NULL checks and memcmp byte count are correct.The function properly guards against NULL
routerand NULLroute_empty_maskbefore dereferencing. The memcmp now correctly usesrouter->route_mask_size * sizeof(flb_route_mask_element)to compare the full mask bytes.
164-179: LGTM! NULL check and memory handling are correct.The function properly guards against NULL
routerbefore dereferencing. The memory allocation usingflb_callocwithrouter->route_mask_sizeand proper error handling is correct.
181-190: LGTM! NULL check and cleanup pattern are correct.The function properly guards against NULL
routerbefore dereferencing. The double-check forroute_empty_maskand setting it to NULL after freeing follows best practices.
198-198: LGTM! Ceiling division formula is correct.The formula
(mask_size + FLB_ROUTES_MASK_ELEMENT_BITS - 1) / FLB_ROUTES_MASK_ELEMENT_BITScorrectly computes the minimal number of elements needed to holdmask_sizebits.
| #endif | ||
| )) { | ||
| flb_routes_mask_set_bit(routes_mask, o_ins->id, o_ins->config); | ||
| flb_routes_mask_set_bit(routes_mask, o_ins->id, o_ins->config->router); |
There was a problem hiding this comment.
Validate router before dereferencing.
While the function validates in->config->router at lines 52-54, line 72 dereferences o_ins->config->router without validation. If an output instance has a NULL config or router, this will crash.
Apply this diff:
+ if (o_ins->config == NULL || o_ins->config->router == NULL) {
+ continue;
+ }
flb_routes_mask_set_bit(routes_mask, o_ins->id, o_ins->config->router);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| flb_routes_mask_set_bit(routes_mask, o_ins->id, o_ins->config->router); | |
| if (o_ins->config == NULL || o_ins->config->router == NULL) { | |
| continue; | |
| } | |
| flb_routes_mask_set_bit(routes_mask, o_ins->id, o_ins->config->router); |
🤖 Prompt for AI Agents
In src/flb_routes_mask.c around line 72, the code dereferences
o_ins->config->router without validating o_ins or its config/router; update the
logic to first check that o_ins is non-NULL, o_ins->config is non-NULL and
o_ins->config->router is non-NULL before calling flb_routes_mask_set_bit. If any
of those are NULL, skip the call (or handle accordingly) to avoid a crash.
Signed-off-by: Eduardo Silva <[email protected]>
1 participant
This PR introduces routing metrics into the Fluent Bit router. The router API now uses dedicated contexts that extend the existing design to provide proper metric instrumentation, details:
Goals:
Here is an example of the new routing metrics:
Fluent Bit Pipeline
Metrics exposed
output:
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
Refactor