Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
📝 WalkthroughWalkthroughAdds a cross-platform cfl_atomic API and multiple platform backends, introduces depth-limited container containment and ownership mechanics to prevent cycles, applies widespread NULL and integer-overflow hardening across headers and sources, updates CMake/CI/docs, and expands unit tests (including multithreaded atomic tests and many defensive-case tests). ChangesCFL Core Library Enhancement
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: c80159a0c0
ℹ️ 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".
| elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "GNU") | ||
| set(PLATFORM_SPECIFIC_ATOMIC_MODULE cfl_atomic_gcc.c) |
There was a problem hiding this comment.
When CFL is built with GCC/Clang on targets that cannot inline 64-bit __atomic_* operations (for example several 32-bit embedded architectures), this branch selects cfl_atomic_gcc.c/cfl_atomic_clang.c but never adds -latomic; the new implementation uses 64-bit __atomic_compare_exchange, __atomic_store_n, and __atomic_load_n, so executables that reference the new cfl_atomic_* API can fail at link time with unresolved __atomic_*_8 symbols. Please either probe/link atomic for these compiler branches or fall back to the mutex implementation when the builtins are not linkable.
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 (1)
lib/cfl/AGENTS.md (1)
1-77: ⚡ Quick winKeep vendor bump PRs free of local process docs
Please avoid introducing
lib/cfl/AGENTS.mdin this dependency-upgrade PR. For Line 1 onward, this adds repo-local workflow policy inside vendored code and will likely increase drift/merge friction on future upstream CFL syncs. Consider moving this guidance to a top-level maintainer doc (or a separate PR) and keeping this PR limited to thev0.7.0vendor update.Based on learnings: "Prefer minimal patches that avoid unrelated formatting or refactoring churn" and "Do not mix unrelated code and documentation updates in one commit unless explicitly requested."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cfl/AGENTS.md` around lines 1 - 77, Remove lib/cfl/AGENTS.md from this vendor bump PR and keep the change focused on the v0.7.0 dependency update; either relocate the guidance into a top-level maintainer document (e.g., MAINTAINERS.md) or open a separate PR for workflow/process docs, and ensure the commit/PR only contains the vendor upgrade files referenced in this diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/cfl/src/cfl_sds.c`:
- Around line 227-229: The bounds check in cfl_sds.c for self-append slice
validation is off-by-one: replace the conditional that currently uses "if
(append_len - 1 > head->alloc - source_offset)" with a comparison using ">=" so
that when the source slice ends exactly at head->alloc it is rejected; update
the check around the variables append_len, source_offset and head->alloc in the
same block (the self-append slice validation) to use ">=" to prevent reading
s[head->alloc].
---
Nitpick comments:
In `@lib/cfl/AGENTS.md`:
- Around line 1-77: Remove lib/cfl/AGENTS.md from this vendor bump PR and keep
the change focused on the v0.7.0 dependency update; either relocate the guidance
into a top-level maintainer document (e.g., MAINTAINERS.md) or open a separate
PR for workflow/process docs, and ensure the commit/PR only contains the vendor
upgrade files referenced in this diff.
🪄 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: f9000ea8-4e50-4221-a359-e81fc01b0ac0
📒 Files selected for processing (45)
lib/cfl/.github/workflows/build.yamllib/cfl/.github/workflows/lint.yamllib/cfl/.github/workflows/packages.yamllib/cfl/AGENTS.mdlib/cfl/CMakeLists.txtlib/cfl/README.mdlib/cfl/include/cfl/cfl.hlib/cfl/include/cfl/cfl_array.hlib/cfl/include/cfl/cfl_atomic.hlib/cfl/include/cfl/cfl_checksum.hlib/cfl/include/cfl/cfl_container.hlib/cfl/include/cfl/cfl_kv.hlib/cfl/include/cfl/cfl_kvlist.hlib/cfl/include/cfl/cfl_list.hlib/cfl/include/cfl/cfl_object.hlib/cfl/include/cfl/cfl_sds.hlib/cfl/include/cfl/cfl_time.hlib/cfl/include/cfl/cfl_utils.hlib/cfl/include/cfl/cfl_variant.hlib/cfl/src/CMakeLists.txtlib/cfl/src/cfl.clib/cfl/src/cfl_array.clib/cfl/src/cfl_atomic_clang.clib/cfl/src/cfl_atomic_gcc.clib/cfl/src/cfl_atomic_generic.clib/cfl/src/cfl_atomic_msvc.clib/cfl/src/cfl_checksum.clib/cfl/src/cfl_container.clib/cfl/src/cfl_kv.clib/cfl/src/cfl_kvlist.clib/cfl/src/cfl_object.clib/cfl/src/cfl_sds.clib/cfl/src/cfl_utils.clib/cfl/src/cfl_variant.clib/cfl/tests/CMakeLists.txtlib/cfl/tests/array.clib/cfl/tests/atomic_operations.clib/cfl/tests/checksum.clib/cfl/tests/headers.clib/cfl/tests/kv.clib/cfl/tests/kvlist.clib/cfl/tests/object.clib/cfl/tests/sds.clib/cfl/tests/utils.clib/cfl/tests/variant.c
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/cfl/CMakeLists.txt`:
- Around line 6-10: The CMake version variables currently set
(CFL_VERSION_MAJOR, CFL_VERSION_MINOR, CFL_VERSION_PATCH) compute
CFL_VERSION_STR as "0.7.1" which mismatches the PR text; either set
CFL_VERSION_PATCH to 0 so CFL_VERSION_STR becomes "0.7.0" or update the PR
title/description to "v0.7.1" for consistency—locate and edit the entries
CFL_VERSION_PATCH and CFL_VERSION_STR in lib/cfl/CMakeLists.txt (or update PR
metadata) so package naming and release artifacts match the declared version.
In `@lib/cfl/src/cfl_container.c`:
- Around line 475-519: The function cfl_container_move_variant_to_array must
reject variants already owned by another container: check variant->owned (and
return -1) early—before calling claim_variant_container() and before modifying
any parent pointers; do the same change in the sibling helper (the
move-to-kvlist function, e.g. cfl_container_move_variant_to_kvlist) so both
helpers fail fast on variant->owned to prevent double-ownership/double-free/UAF.
In `@lib/cfl/src/CMakeLists.txt`:
- Around line 65-71: The Windows atomic fallback currently forces
PLATFORM_SPECIFIC_ATOMIC_MODULE to cfl_atomic_msvc.c on CFL_SYSTEM_WINDOWS which
uses MSVC-only intrinsics (e.g., _InterlockedCompareExchange64) and fails for
MinGW/Clang/GCC; update the CMake logic to only select cfl_atomic_msvc.c when
the compiler is MSVC (e.g., CMAKE_C_COMPILER_ID MATCHES "MSVC") and otherwise
fall back to cfl_atomic_generic.c or set CFL_ATOMIC_NEEDS_THREADS On so non‑MSVC
Windows builds use the compiler-agnostic implementation; modify the branch
around CFL_SYSTEM_WINDOWS/CFL_ATOMIC_BUILTINS_LINK_WITH_LIBATOMIC to gate
PLATFORM_SPECIFIC_ATOMIC_MODULE selection by the compiler ID.
In `@lib/cfl/tests/kvlist.c`:
- Around line 1254-1267: The test must return immediately on setup failures
instead of proceeding because TEST_CHECK doesn't abort; after creating the list
(cfl_kvlist_create), after inserting the string (cfl_kvlist_insert_string),
after creating the tmp file (tmpfile) and after printing (cfl_kvlist_print)
check the return values and if any indicate failure, free any allocated
resources (e.g., destroy the list) and return from the test function so
subsequent calls (like compare which uses fp) cannot dereference NULL or invalid
handles; specifically add early returns when list == NULL, ret != 0 for
cfl_kvlist_insert_string, fp == NULL after tmpfile, and ret <= 0 for
cfl_kvlist_print, cleaning up as needed before returning.
🪄 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: e2aef14a-3075-4d16-8e7a-64b6e00395d9
📒 Files selected for processing (45)
lib/cfl/.github/workflows/build.yamllib/cfl/.github/workflows/lint.yamllib/cfl/.github/workflows/packages.yamllib/cfl/AGENTS.mdlib/cfl/CMakeLists.txtlib/cfl/README.mdlib/cfl/include/cfl/cfl.hlib/cfl/include/cfl/cfl_array.hlib/cfl/include/cfl/cfl_atomic.hlib/cfl/include/cfl/cfl_checksum.hlib/cfl/include/cfl/cfl_container.hlib/cfl/include/cfl/cfl_kv.hlib/cfl/include/cfl/cfl_kvlist.hlib/cfl/include/cfl/cfl_list.hlib/cfl/include/cfl/cfl_object.hlib/cfl/include/cfl/cfl_sds.hlib/cfl/include/cfl/cfl_time.hlib/cfl/include/cfl/cfl_utils.hlib/cfl/include/cfl/cfl_variant.hlib/cfl/src/CMakeLists.txtlib/cfl/src/cfl.clib/cfl/src/cfl_array.clib/cfl/src/cfl_atomic_clang.clib/cfl/src/cfl_atomic_gcc.clib/cfl/src/cfl_atomic_generic.clib/cfl/src/cfl_atomic_msvc.clib/cfl/src/cfl_checksum.clib/cfl/src/cfl_container.clib/cfl/src/cfl_kv.clib/cfl/src/cfl_kvlist.clib/cfl/src/cfl_object.clib/cfl/src/cfl_sds.clib/cfl/src/cfl_utils.clib/cfl/src/cfl_variant.clib/cfl/tests/CMakeLists.txtlib/cfl/tests/array.clib/cfl/tests/atomic_operations.clib/cfl/tests/checksum.clib/cfl/tests/headers.clib/cfl/tests/kv.clib/cfl/tests/kvlist.clib/cfl/tests/object.clib/cfl/tests/sds.clib/cfl/tests/utils.clib/cfl/tests/variant.c
✅ Files skipped from review due to trivial changes (10)
- lib/cfl/.github/workflows/lint.yaml
- lib/cfl/include/cfl/cfl_utils.h
- lib/cfl/include/cfl/cfl_object.h
- lib/cfl/include/cfl/cfl_time.h
- lib/cfl/include/cfl/cfl_kv.h
- lib/cfl/include/cfl/cfl_checksum.h
- lib/cfl/include/cfl/cfl_atomic.h
- lib/cfl/AGENTS.md
- lib/cfl/README.md
- lib/cfl/include/cfl/cfl.h
🚧 Files skipped from review as they are similar to previous changes (20)
- lib/cfl/tests/headers.c
- lib/cfl/src/cfl_atomic_gcc.c
- lib/cfl/src/cfl_checksum.c
- lib/cfl/tests/kv.c
- lib/cfl/tests/checksum.c
- lib/cfl/include/cfl/cfl_sds.h
- lib/cfl/src/cfl.c
- lib/cfl/src/cfl_atomic_clang.c
- lib/cfl/include/cfl/cfl_list.h
- lib/cfl/tests/array.c
- lib/cfl/.github/workflows/packages.yaml
- lib/cfl/tests/object.c
- lib/cfl/src/cfl_atomic_msvc.c
- lib/cfl/src/cfl_atomic_generic.c
- lib/cfl/src/cfl_sds.c
- lib/cfl/src/cfl_kv.c
- lib/cfl/tests/CMakeLists.txt
- lib/cfl/.github/workflows/build.yaml
- lib/cfl/tests/atomic_operations.c
- lib/cfl/src/cfl_kvlist.c
| # C Floppy Version | ||
| set(CFL_VERSION_MAJOR 0) | ||
| set(CFL_VERSION_MINOR 6) | ||
| set(CFL_VERSION_MINOR 7) | ||
| set(CFL_VERSION_PATCH 1) | ||
| set(CFL_VERSION_STR "${CFL_VERSION_MAJOR}.${CFL_VERSION_MINOR}.${CFL_VERSION_PATCH}") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of the cfl library (C Floppy library)?
💡 Result:
The “C Floppy library” (cfl) appears to refer to the fluent/cfl C-flavored library. Its latest released version is v0.6.1 (marked “Latest” and dated May 22, 2025 on the repository page). [1] Note: there are other unrelated software projects abbreviated “cfl” (e.g., “Causal Feature Learning”, “Compressed File Library”, etc.), so the name is ambiguous; the fluent/cfl repository is the only one in the sources explicitly describing “c:\ floppy” as “CFL”. [2]
Citations:
Update PR title and description or adjust version numbers for consistency.
The version configured in the code (0.7.1) does not match the PR title and description, which state "upgrade to v0.7.0." The computed version string will be "0.7.1", which flows through to package naming and release artifacts. Either update CFL_VERSION_PATCH to 0 (to match the stated v0.7.0), or update the PR title/description to "v0.7.1" for consistency.
Note: The upstream cfl library's latest released version is v0.6.1; the 0.7.x version does not yet have an official release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cfl/CMakeLists.txt` around lines 6 - 10, The CMake version variables
currently set (CFL_VERSION_MAJOR, CFL_VERSION_MINOR, CFL_VERSION_PATCH) compute
CFL_VERSION_STR as "0.7.1" which mismatches the PR text; either set
CFL_VERSION_PATCH to 0 so CFL_VERSION_STR becomes "0.7.0" or update the PR
title/description to "v0.7.1" for consistency—locate and edit the entries
CFL_VERSION_PATCH and CFL_VERSION_STR in lib/cfl/CMakeLists.txt (or update PR
metadata) so package naming and release artifacts match the declared version.
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/cfl/tests/kvlist.c (1)
1326-1350: 💤 Low valueConsider surfacing a
TEST_MSGwhen/dev/fullcannot be opened.If
fopen("/dev/full", ...)returnsNULL(e.g., restricted container), the test silently passes with no signal that the write-error path was never exercised. A briefTEST_MSGon the skip path would make this observable without changing the test outcome.♻️ Suggested tweak
fp = fopen("/dev/full", "w"); if (fp == NULL) { + TEST_MSG("skipping: /dev/full unavailable"); cfl_kvlist_destroy(list); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cfl/tests/kvlist.c` around lines 1326 - 1350, In print_write_error(), the test silently returns when fopen("/dev/full", "w") yields NULL; update the fp == NULL branch to call TEST_MSG with a short explanation (e.g., "skipping write-error test: /dev/full not available") before cleaning up (cfl_kvlist_destroy(list)) and returning so the skipped-path is visible in test output; keep all other behavior (setvbuf, cfl_kvlist_print, TEST_CHECK) unchanged.lib/cfl/tests/variant.c (1)
663-663: 💤 Low valueConsider clarifying intent with a comment or TEST_CHECK.
Line 663 calls
cfl_variant_size_set(NULL, 1)without any assertion. While this likely verifies that the function doesn't crash on NULL input, adding a comment or wrapping inTEST_CHECKwould make the intent clearer for maintainability.💡 Suggested clarification
Option 1: Add a comment
+ /* Verify size_set doesn't crash on NULL */ cfl_variant_size_set(NULL, 1);Option 2: Make it explicit (if size_set returns a value indicating success/failure)
- cfl_variant_size_set(NULL, 1); + /* Should handle NULL gracefully without crashing */ + cfl_variant_size_set(NULL, 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cfl/tests/variant.c` at line 663, The call to cfl_variant_size_set(NULL, 1) in variant.c should be made explicit: either add a one-line comment explaining that this invocation asserts the function is NULL-safe (e.g., "ensure no crash on NULL input"), or wrap the call in a TEST_CHECK (or equivalent test-assert) if cfl_variant_size_set returns a status so the test records pass/fail; update the line containing cfl_variant_size_set(NULL, 1) to include the chosen clarification so future readers understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/cfl/tests/variant.c`:
- Around line 532-537: The test leaks the open file pointer fp when
cfl_variant_create_from_string_s(...) returns NULL; before the early return
inside the failure branch, call fclose(fp) (or ensure fp is closed) so the file
descriptor is released; update the block that checks val from
cfl_variant_create_from_string_s to fclose(fp) prior to TEST_MSG and return (or
refactor to a single cleanup path that always closes fp).
- Around line 617-618: The function definition for
test_variant_print_referenced_bytes currently has its opening brace on the same
line; move the brace to the next line so the function follows the project's
style guide (i.e., change "static void test_variant_print_referenced_bytes() {"
to have the "{" on the following line) and ensure other nearby function
definitions follow the same brace style for consistency.
---
Nitpick comments:
In `@lib/cfl/tests/kvlist.c`:
- Around line 1326-1350: In print_write_error(), the test silently returns when
fopen("/dev/full", "w") yields NULL; update the fp == NULL branch to call
TEST_MSG with a short explanation (e.g., "skipping write-error test: /dev/full
not available") before cleaning up (cfl_kvlist_destroy(list)) and returning so
the skipped-path is visible in test output; keep all other behavior (setvbuf,
cfl_kvlist_print, TEST_CHECK) unchanged.
In `@lib/cfl/tests/variant.c`:
- Line 663: The call to cfl_variant_size_set(NULL, 1) in variant.c should be
made explicit: either add a one-line comment explaining that this invocation
asserts the function is NULL-safe (e.g., "ensure no crash on NULL input"), or
wrap the call in a TEST_CHECK (or equivalent test-assert) if
cfl_variant_size_set returns a status so the test records pass/fail; update the
line containing cfl_variant_size_set(NULL, 1) to include the chosen
clarification so future readers understand the intent.
🪄 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: c22c37bc-6ca1-4847-be91-41f9ffea5547
📒 Files selected for processing (45)
lib/cfl/.github/workflows/build.yamllib/cfl/.github/workflows/lint.yamllib/cfl/.github/workflows/packages.yamllib/cfl/AGENTS.mdlib/cfl/CMakeLists.txtlib/cfl/README.mdlib/cfl/include/cfl/cfl.hlib/cfl/include/cfl/cfl_array.hlib/cfl/include/cfl/cfl_atomic.hlib/cfl/include/cfl/cfl_checksum.hlib/cfl/include/cfl/cfl_container.hlib/cfl/include/cfl/cfl_kv.hlib/cfl/include/cfl/cfl_kvlist.hlib/cfl/include/cfl/cfl_list.hlib/cfl/include/cfl/cfl_object.hlib/cfl/include/cfl/cfl_sds.hlib/cfl/include/cfl/cfl_time.hlib/cfl/include/cfl/cfl_utils.hlib/cfl/include/cfl/cfl_variant.hlib/cfl/src/CMakeLists.txtlib/cfl/src/cfl.clib/cfl/src/cfl_array.clib/cfl/src/cfl_atomic_clang.clib/cfl/src/cfl_atomic_gcc.clib/cfl/src/cfl_atomic_generic.clib/cfl/src/cfl_atomic_msvc.clib/cfl/src/cfl_checksum.clib/cfl/src/cfl_container.clib/cfl/src/cfl_kv.clib/cfl/src/cfl_kvlist.clib/cfl/src/cfl_object.clib/cfl/src/cfl_sds.clib/cfl/src/cfl_utils.clib/cfl/src/cfl_variant.clib/cfl/tests/CMakeLists.txtlib/cfl/tests/array.clib/cfl/tests/atomic_operations.clib/cfl/tests/checksum.clib/cfl/tests/headers.clib/cfl/tests/kv.clib/cfl/tests/kvlist.clib/cfl/tests/object.clib/cfl/tests/sds.clib/cfl/tests/utils.clib/cfl/tests/variant.c
✅ Files skipped from review due to trivial changes (9)
- lib/cfl/AGENTS.md
- lib/cfl/include/cfl/cfl_checksum.h
- lib/cfl/include/cfl/cfl_kv.h
- lib/cfl/include/cfl/cfl_time.h
- lib/cfl/include/cfl/cfl_utils.h
- lib/cfl/.github/workflows/lint.yaml
- lib/cfl/include/cfl/cfl_atomic.h
- lib/cfl/include/cfl/cfl_object.h
- lib/cfl/include/cfl/cfl.h
🚧 Files skipped from review as they are similar to previous changes (32)
- lib/cfl/include/cfl/cfl_variant.h
- lib/cfl/tests/headers.c
- lib/cfl/tests/utils.c
- lib/cfl/.github/workflows/packages.yaml
- lib/cfl/CMakeLists.txt
- lib/cfl/src/cfl_atomic_gcc.c
- lib/cfl/src/cfl.c
- lib/cfl/src/cfl_atomic_clang.c
- lib/cfl/tests/sds.c
- lib/cfl/tests/checksum.c
- lib/cfl/tests/object.c
- lib/cfl/tests/kv.c
- lib/cfl/tests/CMakeLists.txt
- lib/cfl/src/cfl_object.c
- lib/cfl/tests/array.c
- lib/cfl/src/cfl_atomic_generic.c
- lib/cfl/include/cfl/cfl_container.h
- lib/cfl/include/cfl/cfl_sds.h
- lib/cfl/src/cfl_checksum.c
- lib/cfl/src/cfl_atomic_msvc.c
- lib/cfl/tests/atomic_operations.c
- lib/cfl/include/cfl/cfl_list.h
- lib/cfl/.github/workflows/build.yaml
- lib/cfl/include/cfl/cfl_array.h
- lib/cfl/src/CMakeLists.txt
- lib/cfl/src/cfl_sds.c
- lib/cfl/src/cfl_array.c
- lib/cfl/src/cfl_utils.c
- lib/cfl/src/cfl_kv.c
- lib/cfl/src/cfl_variant.c
- lib/cfl/src/cfl_kvlist.c
- lib/cfl/src/cfl_container.c
| val = cfl_variant_create_from_string_s(input, 2, CFL_TRUE); | ||
| if (!TEST_CHECK(val != NULL)) { | ||
| TEST_MSG("cfl_variant_create_from_string_s failed"); | ||
| fclose(fp); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Resource leak: close file pointer before early return.
If cfl_variant_create_from_string_s fails, the function returns without closing fp, leaking the file descriptor.
🔧 Proposed fix
val = cfl_variant_create_from_string_s(input, 2, CFL_TRUE);
if (!TEST_CHECK(val != NULL)) {
TEST_MSG("cfl_variant_create_from_string_s failed");
+ fclose(fp);
return;
}📝 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.
| val = cfl_variant_create_from_string_s(input, 2, CFL_TRUE); | |
| if (!TEST_CHECK(val != NULL)) { | |
| TEST_MSG("cfl_variant_create_from_string_s failed"); | |
| fclose(fp); | |
| return; | |
| } | |
| val = cfl_variant_create_from_string_s(input, 2, CFL_TRUE); | |
| if (!TEST_CHECK(val != NULL)) { | |
| TEST_MSG("cfl_variant_create_from_string_s failed"); | |
| fclose(fp); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cfl/tests/variant.c` around lines 532 - 537, The test leaks the open file
pointer fp when cfl_variant_create_from_string_s(...) returns NULL; before the
early return inside the failure branch, call fclose(fp) (or ensure fp is closed)
so the file descriptor is released; update the block that checks val from
cfl_variant_create_from_string_s to fclose(fp) prior to TEST_MSG and return (or
refactor to a single cleanup path that always closes fp).
| static void test_variant_print_referenced_bytes() | ||
| { |
There was a problem hiding this comment.
Function opening brace should be on the next line.
As per coding guidelines, function opening braces should be placed on the next line.
📐 Proposed fix
static void test_variant_print_referenced_bytes()
-{
+{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cfl/tests/variant.c` around lines 617 - 618, The function definition for
test_variant_print_referenced_bytes currently has its opening brace on the same
line; move the brace to the next line so the function follows the project's
style guide (i.e., change "static void test_variant_print_referenced_bytes() {"
to have the "{" on the following line) and ensure other nearby function
definitions follow the same brace style for consistency.
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/config_format/flb_cf_yaml.c (1)
773-873: 💤 Low valueInconsistent kvpair cleanup between the two transfer sites.
In
state_move_into_config_group()(line 762), aftercfl_kvpair_take_value()succeeds the code explicitly callscfl_kvpair_destroy(kvp)so the now-empty kvp is removed fromstate->keyvals. Instate_copy_into_properties()the corresponding CFL_VARIANT_ARRAY (mixed, line 831) and CFL_VARIANT_KVLIST (line 850) branches do not — the emptied kvp is left instate->keyvals->listand is only torn down later when the parser state is popped.If
cfl_kvlist_destroy()in cfl 0.7.1 is NULL-val safe this is functionally fine, but the asymmetry is easy to misread andprint_current_properties()(or anything else that walksstate->keyvalsafterwards) would dereferenceprop->val->typeon a NULL pointer. Worth either:
- mirroring the
state_move_into_config_grouppattern (callcfl_kvpair_destroy(kvp)after a successful take in both branches, and switch the outer loop tocfl_list_foreach_safe), or- adding a brief comment that the take leaves a dangling-empty kvp on purpose and that downstream cleanup is required to be NULL-val safe.
♻️ Sketch: align with the move-into-config-group cleanup pattern
- cfl_list_foreach(head, &state->keyvals->list) { + struct cfl_list *tmp; + + cfl_list_foreach_safe(head, tmp, &state->keyvals->list) { kvp = cfl_list_entry(head, struct cfl_kvpair, _head); switch (kvp->val->type) { @@ else { value = cfl_kvpair_take_value(kvp); @@ if (flb_cf_section_property_add_variant(conf, properties, kvp->key, cfl_sds_len(kvp->key), value) == NULL) { flb_error("unable to add variant property"); cfl_variant_destroy(value); return YAML_FAILURE; } + cfl_kvpair_destroy(kvp); } break; case CFL_VARIANT_KVLIST: value = cfl_kvpair_take_value(kvp); @@ if (flb_cf_section_property_add_variant(conf, properties, kvp->key, cfl_sds_len(kvp->key), value) == NULL) { flb_error("unable to add variant property"); cfl_variant_destroy(value); return YAML_FAILURE; } + cfl_kvpair_destroy(kvp); break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config_format/flb_cf_yaml.c` around lines 773 - 873, The loop in state_copy_into_properties leaves kvpairs emptied by cfl_kvpair_take_value() in state->keyvals->list, causing asymmetry with state_move_into_config_group; change the outer iteration to use cfl_list_foreach_safe and mirror the other function by calling cfl_kvpair_destroy(kvp) after any successful cfl_kvpair_take_value() in the CFL_VARIANT_ARRAY (mixed-array -> value path) and CFL_VARIANT_KVLIST branches, ensuring you destroy the now-empty kvp immediately after adding the variant (and keep existing cfl_variant_destroy(value) on error paths).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/config_format/flb_cf_yaml.c`:
- Around line 773-873: The loop in state_copy_into_properties leaves kvpairs
emptied by cfl_kvpair_take_value() in state->keyvals->list, causing asymmetry
with state_move_into_config_group; change the outer iteration to use
cfl_list_foreach_safe and mirror the other function by calling
cfl_kvpair_destroy(kvp) after any successful cfl_kvpair_take_value() in the
CFL_VARIANT_ARRAY (mixed-array -> value path) and CFL_VARIANT_KVLIST branches,
ensuring you destroy the now-empty kvp immediately after adding the variant (and
keep existing cfl_variant_destroy(value) on error paths).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df1cd016-917f-4191-b126-5e602adca90a
📒 Files selected for processing (1)
src/config_format/flb_cf_yaml.c
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
1 participant
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
Bug Fixes
Documentation
Version
Tests
Chores