tests: core_routes: Handle limit of rlimit's soft limit#11592
tests: core_routes: Handle limit of rlimit's soft limit#11592
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe test Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/runtime/core_routes.c (1)
53-53: Extract257into a named constant to keep bounds synchronized.
257is repeated acrossoutput_instances,olimit, loop bounds, and final assertion. A single constant reduces drift risk in future edits.Proposed refactor
+ `#define` CORE_ROUTES_MAX_OUTPUTS 257 + void flb_test_basic_functionality_test(void) { - int output_instances[257]; + int output_instances[CORE_ROUTES_MAX_OUTPUTS]; @@ - size_t olimit = 257; + size_t olimit = CORE_ROUTES_MAX_OUTPUTS; @@ - if (rlim.rlim_cur < 257) { + if (rlim.rlim_cur < CORE_ROUTES_MAX_OUTPUTS) { olimit = rlim.rlim_cur/2.5; }Also applies to: 77-77, 101-101, 108-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/core_routes.c` at line 53, Extract the repeated literal 257 into a named constant and replace all occurrences to keep bounds synchronized: declare a constant (e.g., const size_t OUTPUT_LIMIT = 257) near the top of the test and use OUTPUT_LIMIT in place of olimit's initialization, the output_instances array size, any loop bounds that iterate to 257, and the final assertion; update the variable olimit to be initialized from this constant or remove olimit and use the constant directly so all references (output_instances, olimit, the for/while loop conditions, and the assertion) consistently rely on the single named symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/runtime/core_routes.c`:
- Around line 68-72: Guard the getrlimit call and ensure olimit is at least 1:
check the return value of getrlimit(RLIMIT_NOFILE, &rlim) and if it fails,
record the failure and return/abort the test early instead of proceeding to read
uninitialized rlim; when computing olimit from rlim.rlim_cur (used later with
delivery_counter) ensure you convert to an integer and enforce a minimum of 1
(e.g., olimit = max(1, (int)(rlim.rlim_cur / 2.5))) so the loop runs at least
once; update the code around getrlimit, rlim, olimit, and the test exit path to
avoid undefined reads and vacuous pass of TEST_CHECK(delivery_counter ==
olimit).
---
Nitpick comments:
In `@tests/runtime/core_routes.c`:
- Line 53: Extract the repeated literal 257 into a named constant and replace
all occurrences to keep bounds synchronized: declare a constant (e.g., const
size_t OUTPUT_LIMIT = 257) near the top of the test and use OUTPUT_LIMIT in
place of olimit's initialization, the output_instances array size, any loop
bounds that iterate to 257, and the final assertion; update the variable olimit
to be initialized from this constant or remove olimit and use the constant
directly so all references (output_instances, olimit, the for/while loop
conditions, and the assertion) consistently rely on the single named symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a786e7e5-3dca-4fbf-94e4-f75e5765d491
📒 Files selected for processing (1)
tests/runtime/core_routes.c
![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.
Reviewed commit: c6dcd3a53a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cosmo0920
force-pushed
the
cosmo0920-handle-small-rlim_cur
branch
from
c6dcd3a to
d411694
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d411694b37
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/runtime/core_routes.c (1)
53-53: Optional: extract257into a named constant to prevent drift.
257is now used in multiple places for the same conceptual limit. A single named constant would make future edits safer and clearer.♻️ Suggested refactor
+ const size_t max_outputs = 257; - size_t olimit = 257; + size_t olimit = max_outputs; ... - if (rlimit_rc == 0 && rlim.rlim_cur < 257) { + if (rlimit_rc == 0 && rlim.rlim_cur < max_outputs) {Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/core_routes.c` at line 53, Extract the literal 257 into a single named constant (e.g., MAX_PATH_LEN or CORE_ROUTES_OLIMIT) and replace the hard-coded 257 in the declaration of olimit and the other occurrences referenced (the two uses around the later block) with that constant; update any related comments to reference the constant name so all uses share the same semantic limit and future edits only change the constant definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/runtime/core_routes.c`:
- Line 53: Extract the literal 257 into a single named constant (e.g.,
MAX_PATH_LEN or CORE_ROUTES_OLIMIT) and replace the hard-coded 257 in the
declaration of olimit and the other occurrences referenced (the two uses around
the later block) with that constant; update any related comments to reference
the constant name so all uses share the same semantic limit and future edits
only change the constant definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b516962-7122-47d3-a815-49b1ff26620e
📒 Files selected for processing (1)
tests/runtime/core_routes.c
By default, macOS's soft limit of rlimit is up to 256. SO, just eating up 256 of output instances could cause too many open errors. Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-handle-small-rlim_cur
branch
from
d411694 to
710b042
Compare
1 participant
By default, macOS's soft limit of rlimit is up to 256. SO, just eating up 256 of output instances could cause too many open errors.
This is because the default value of masOS's
ulimit -nis: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