kv: add flb_kv_get_all_key_values function for mk_list#11777
kv: add flb_kv_get_all_key_values function for mk_list#11777iypetrov wants to merge 4 commits intofluent:masterfrom
flb_kv_get_all_key_values function for mk_list#11777Conversation
|
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:
📝 WalkthroughWalkthroughAdded a public bulk-retrieval API ChangesKV Bulk Retrieval
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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: 15a81e8953
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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)
include/fluent-bit/flb_kv.h (1)
49-49: ⚡ Quick winClarify memory ownership/lifetime for
flb_kv_get_all_key_values().The declaration does not define who owns the returned array/elements or whether entries remain valid after
flb_kv_release()/list mutation. Please document the contract (and preferably add a matching destroy helper) to prevent leaks/UAF in callers.🤖 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 `@include/fluent-bit/flb_kv.h` at line 49, Document the ownership/lifetime contract for flb_kv_get_all_key_values: specify whether the returned struct flb_kv_pair ** array and its flb_kv_pair elements are heap-allocated and owned by the caller, whether they remain valid after calling flb_kv_release() or mutating the source mk_list, and whether the array is NULL-terminated or accompanied by a count; then add a matching destroy helper (e.g., flb_kv_free_key_values or flb_kv_destroy_key_values) declaration and describe its required use so callers know how to free the array and avoid leaks/UAFs — reference the existing flb_kv_get_all_key_values and flb_kv_release symbols so reviewers can locate changes.
🤖 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 `@src/flb_kv.c`:
- Around line 181-191: The array arr is allocated as pointers to struct
flb_kv_pair but the code writes to arr[i]->key/val without allocating each
pointed-to struct; update the mk_list_foreach loop (and related logic that
returns arr) to allocate each element before use (e.g., arr[i] = flb_calloc(1,
sizeof(struct flb_kv_pair))) and then set arr[i]->key and arr[i]->val from the
mk_list_foreach entry (struct flb_kv *kv). On allocation failure, free any
previously allocated arr[j] and arr itself, call flb_errno(), and return NULL to
avoid leaks or crashes. Ensure i increments and the final returned array length
semantics remain unchanged.
---
Nitpick comments:
In `@include/fluent-bit/flb_kv.h`:
- Line 49: Document the ownership/lifetime contract for
flb_kv_get_all_key_values: specify whether the returned struct flb_kv_pair **
array and its flb_kv_pair elements are heap-allocated and owned by the caller,
whether they remain valid after calling flb_kv_release() or mutating the source
mk_list, and whether the array is NULL-terminated or accompanied by a count;
then add a matching destroy helper (e.g., flb_kv_free_key_values or
flb_kv_destroy_key_values) declaration and describe its required use so callers
know how to free the array and avoid leaks/UAFs — reference the existing
flb_kv_get_all_key_values and flb_kv_release symbols so reviewers can locate
changes.
🪄 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: 96fbec84-c045-48d9-a91b-cd0344e1cc7b
📒 Files selected for processing (3)
include/fluent-bit/flb_kv.hsrc/flb_kv.ctests/internal/kv.c
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/flb_kv.c (1)
164-194: ⚡ Quick winNo count out-parameter or NULL sentinel leaves callers unable to bound the returned array.
The function returns a heap-allocated
struct flb_kv **whose length is not communicated to callers. They must independently callmk_list_size(list)again — a second traversal — just to iterate the result safely. The idiomatic fix in C is either:
- Option A (preferred): Add a
NULLsentinel by allocatingcount + 1slots and leaving the last entryNULL(costs one extra pointer).- Option B: Add a
size_t *out_countout-parameter.Additionally, since
arr[i]entries are direct pointers into list-ownedflb_kvnodes, callers mustflb_free(arr)only — freeing individual elements would corrupt the list. This contract should either be enforced structurally (e.g., a const-qualified element type) or documented in a comment.✏️ Option A: NULL-terminate the array
- arr = flb_calloc(count, sizeof(struct flb_kv *)); + arr = flb_calloc(count + 1, sizeof(struct flb_kv *)); if (!arr) { flb_errno(); return NULL; } mk_list_foreach(head, list) { kv = mk_list_entry(head, struct flb_kv, _head); arr[i] = kv; i++; } + arr[count] = NULL; /* sentinel */ return arr;🤖 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/flb_kv.c` around lines 164 - 194, flb_kv_get_all_key_values currently returns an unbounded array, forcing callers to re-check mk_list_size; change the allocation to reserve one extra slot (use flb_calloc(count + 1, sizeof(struct flb_kv *))) and set arr[count] = NULL so the returned array is NULL-terminated (Option A), update the mk_list_foreach loop to fill arr[0..count-1] as before, and add a brief comment above the function stating that the array elements are direct pointers into the list and callers must only flb_free(arr) (not free individual entries).
🤖 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 `@src/flb_kv.c`:
- Line 181: The allocation uses the wrong type: change the flb_calloc call that
assigns to arr from using sizeof(struct flb_kv_pair *) to sizeof(struct flb_kv
*) so the allocated array element size matches the actual pointer type stored
(refer to the variable arr and the struct flb_kv type in this file); update the
sizeof expression accordingly wherever flb_kv_pair is still referenced.
---
Nitpick comments:
In `@src/flb_kv.c`:
- Around line 164-194: flb_kv_get_all_key_values currently returns an unbounded
array, forcing callers to re-check mk_list_size; change the allocation to
reserve one extra slot (use flb_calloc(count + 1, sizeof(struct flb_kv *))) and
set arr[count] = NULL so the returned array is NULL-terminated (Option A),
update the mk_list_foreach loop to fill arr[0..count-1] as before, and add a
brief comment above the function stating that the array elements are direct
pointers into the list and callers must only flb_free(arr) (not free individual
entries).
🪄 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: 1aad2dd7-d4a9-4911-a92d-5f1f0ee4b084
📒 Files selected for processing (3)
include/fluent-bit/flb_kv.hsrc/flb_kv.ctests/internal/kv.c
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/internal/kv.c
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/flb_kv.c (1)
164-194: ⚡ Quick winAdd a NULL sentinel to make the returned array self-describing.
The function returns an array of
countelements, but the signature (struct flb_kv **) gives callers no way to determine its length. Callers must independently callmk_list_size(list)before they can safely iterate the array, which is fragile and non-obvious. A NULL sentinel is the idiomatic fix in C for this pattern.♻️ Proposed fix — allocate count+1 and NULL-terminate
- arr = flb_calloc(count, sizeof(struct flb_kv *)); + arr = flb_calloc(count + 1, sizeof(struct flb_kv *)); if (!arr) { flb_errno(); return NULL; } mk_list_foreach(head, list) { kv = mk_list_entry(head, struct flb_kv, _head); arr[i] = kv; i++; } + arr[count] = NULL; return arr;With this change callers can iterate until
arr[n] == NULLwithout needing a separatemk_list_size()call, and the declaration inflb_kv.hneed not change.🤖 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/flb_kv.c` around lines 164 - 194, The returned array from flb_kv_get_all_key_values is not NULL-terminated, forcing callers to separately call mk_list_size; fix this by allocating one extra slot and NULL-terminating the array: use flb_calloc(count + 1, sizeof(struct flb_kv *)) instead of flb_calloc(count, ...), remove the early return that yields NULL for count==0 (or return an allocated array with arr[0]=NULL), populate arr[0..count-1] as now and set arr[count] = NULL before returning; update uses of arr accordingly but do not change the function signature or flb_kv.h declaration.
🤖 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/flb_kv.c`:
- Around line 164-194: The returned array from flb_kv_get_all_key_values is not
NULL-terminated, forcing callers to separately call mk_list_size; fix this by
allocating one extra slot and NULL-terminating the array: use flb_calloc(count +
1, sizeof(struct flb_kv *)) instead of flb_calloc(count, ...), remove the early
return that yields NULL for count==0 (or return an allocated array with
arr[0]=NULL), populate arr[0..count-1] as now and set arr[count] = NULL before
returning; update uses of arr accordingly but do not change the function
signature or flb_kv.h declaration.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 290a52d251
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
iypetrov
changed the title
flb_kv_get_all_key_values function for mk_listflb_kv_get_all_key_values function for mk_list
|
Firstly, we need to address the following commit linter errors: ref: https://github.com/fluent/fluent-bit/actions/runs/25450708887/job/74789123392?pr=11777#step:6:13 |
…mk_list) Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
iypetrov
force-pushed
the
add-option-to-get-all-items-from-flb-kv-list
branch
from
e87d8a0 to
8d4fe4a
Compare
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
iypetrov
force-pushed
the
add-option-to-get-all-items-from-flb-kv-list
branch
from
8d4fe4a to
e1603f8
Compare
Thanks for the remark, commit linter errors are already fixed: ❯ python .github/scripts/commit_prefix_check.py
✅ Commit prefix validation passed. |
2 participants
Add
flb_kv_get_all_key_valuesfunction formk_listAddresses #11776 (1/3 sub-task)
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.