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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNormalize bracketed IPv6 listen addresses (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
cosmo0920
force-pushed
the
cosmo0920-handle-baraketed-ipv6-addresses
branch
from
51b44da to
6ca2baa
Compare
![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: 51b44da9e3
ℹ️ 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".
tests/internal/network.c
Outdated
| FLB_NETWORK_DEFAULT_BACKLOG_SIZE, | ||
| FLB_FALSE); | ||
|
|
||
| if (errno == EAFNOSUPPORT) { |
There was a problem hiding this comment.
The new test checks errno == EAFNOSUPPORT unconditionally, but errno is not reset on success, so a stale value from an earlier syscall can cause this test to return early even when flb_net_server(...) succeeded. In that case the assertion is skipped and the opened socket is leaked, which can hide regressions and make the internal network suite flaky. Check errno only when fd_server == -1.
Useful? React with 👍 / 👎.
![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 (2)
tests/internal/network.c (1)
163-181: Add a UDP bracketed IPv6 regression test as well.This PR changes both
flb_net_server()andflb_net_server_udp()insrc/flb_network.c, but only TCP has bracketed-address coverage. Add a focusedflb_net_server_udp(TEST_PORT, "[::1]", ...)test and register it inTEST_LIST.As per coding guidelines:
tests/**/*.{c,h,py}: Add or update tests for behavior changes, especially protocol parsing and encoder/decoder paths.Also applies to: 186-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/internal/network.c` around lines 163 - 181, Add a UDP counterpart to the existing TCP bracketed IPv6 test by creating a new test function (e.g., test_ipv6_bracketed_udp_listen) that calls flb_net_server_udp(TEST_PORT, TEST_HOSTv6_BRACKETED or "[::1]", FLB_NETWORK_DEFAULT_BACKLOG_SIZE, FLB_FALSE), handles EAFNOSUPPORT the same way, checks the returned fd != -1, and closes it if opened; then register this new test name in the TEST_LIST array so the UDP bracketed-address parsing change is covered. Ensure the new test mirrors the structure and error handling of test_ipv6_bracketed_listen and uses the same TEST_PORT/TEST_HOSTv6_BRACKETED symbols.src/flb_network.c (1)
1651-1669: Consider extracting duplicated IPv6 normalization into a small helper.Both
flb_net_server()andflb_net_server_udp()now carry the same normalization block; a shared helper would reduce drift risk and keep behavior aligned.♻️ Suggested refactor sketch
+static const char *flb_net_normalize_listen_addr(const char *listen_addr, + char **allocated_copy) +{ + size_t listen_addr_len; + + *allocated_copy = NULL; + if (listen_addr != NULL && listen_addr[0] == '[') { + listen_addr_len = strlen(listen_addr); + if (listen_addr_len >= 3 && listen_addr[listen_addr_len - 1] == ']') { + *allocated_copy = flb_strndup(listen_addr + 1, listen_addr_len - 2); + if (*allocated_copy != NULL) { + return *allocated_copy; + } + } + } + return listen_addr; +}Also applies to: 1725-1743
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_network.c` around lines 1651 - 1669, Extract the IPv6 bracket-stripping logic into a small helper (e.g., flb_net_normalize_listen_addr) that takes const char *listen_addr and returns a newly allocated char* containing the unbracketed address or NULL if no allocation was needed/fails; update both flb_net_server and flb_net_server_udp to call this helper, use the returned pointer as the normalized address when non-NULL, and free it after use to avoid leaks; ensure the helper encapsulates the length checks (>=3 and trailing ']') and uses flb_strndup like the original code so behavior remains identical.
🤖 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/internal/network.c`:
- Around line 171-174: The code checks errno == EAFNOSUPPORT without first
verifying the syscall failed; only check errno when the call returned -1. Locate
the conditional that compares errno to EAFNOSUPPORT (the block returning early
with TEST_MSG "This protocol is not supported in this platform") and change it
to guard on the syscall's return value (e.g., if (ret == -1 && errno ==
EAFNOSUPPORT) or if (fd < 0 && errno == EAFNOSUPPORT)) so stale errno won't
cause an incorrect early return; apply the same pattern to the other similar
check mentioned earlier.
---
Nitpick comments:
In `@src/flb_network.c`:
- Around line 1651-1669: Extract the IPv6 bracket-stripping logic into a small
helper (e.g., flb_net_normalize_listen_addr) that takes const char *listen_addr
and returns a newly allocated char* containing the unbracketed address or NULL
if no allocation was needed/fails; update both flb_net_server and
flb_net_server_udp to call this helper, use the returned pointer as the
normalized address when non-NULL, and free it after use to avoid leaks; ensure
the helper encapsulates the length checks (>=3 and trailing ']') and uses
flb_strndup like the original code so behavior remains identical.
In `@tests/internal/network.c`:
- Around line 163-181: Add a UDP counterpart to the existing TCP bracketed IPv6
test by creating a new test function (e.g., test_ipv6_bracketed_udp_listen) that
calls flb_net_server_udp(TEST_PORT, TEST_HOSTv6_BRACKETED or "[::1]",
FLB_NETWORK_DEFAULT_BACKLOG_SIZE, FLB_FALSE), handles EAFNOSUPPORT the same way,
checks the returned fd != -1, and closes it if opened; then register this new
test name in the TEST_LIST array so the UDP bracketed-address parsing change is
covered. Ensure the new test mirrors the structure and error handling of
test_ipv6_bracketed_listen and uses the same TEST_PORT/TEST_HOSTv6_BRACKETED
symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83001558-9e9b-4438-a97c-918cd1b9fe84
📒 Files selected for processing (2)
src/flb_network.ctests/internal/network.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-handle-baraketed-ipv6-addresses
branch
from
6ca2baa to
88795b1
Compare
1 participant
Closes #11668.
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:
leaks command's result on macOS:
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
Bug Fixes
Tests