glob: fix glob function and extract as win32 utility file#11165
glob: fix glob function and extract as win32 utility file#11165zshuang0316 wants to merge 3 commits intofluent:masterfrom
Conversation
📝 WalkthroughWalkthroughSwitches Windows glob from an unconditional source-include to a guarded header/API ( Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin (plugins/in_blob/blob.c)
participant GlobAPI as Glob API (flb_glob_win32.h / src/win32/flb_glob.c)
participant FS as Filesystem
Plugin->>GlobAPI: glob(pattern, flags, NULL, &ctx)
GlobAPI->>FS: enumerate matching paths / stat entries
FS-->>GlobAPI: entries & metadata
GlobAPI-->>Plugin: return results (ctx->gl_pathv, ctx->gl_pathc) / status
Plugin->>GlobAPI: globfree(&ctx)
GlobAPI-->>Plugin: free resources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/in_blob/win32_glob.c (1)
107-117: Consider defensive cleanup in globfree.For consistency and defensive programming, consider:
- Setting
context->gl_pathc = 0after freeinggl_pathv- Removing the unused
indexvariable declared on line 109These changes would make the cleanup more complete and remove dead code.
static void globfree(glob_t *context) { - size_t index; - if (context->gl_pathv != NULL) { flb_free(context->gl_pathv); context->gl_pathv = NULL; + context->gl_pathc = 0; } flb_file_glob_clean(&context->inner_context); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_blob/win32_glob.c(2 hunks)
⏰ 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). (31)
- 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 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: PR - fuzzing test
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- 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 (-DSANITIZE_UNDEFINED=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 (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 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_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_JEMALLOC=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_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- 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-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- 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-centos-7
🔇 Additional comments (2)
plugins/in_blob/win32_glob.c (2)
153-153: Essential fix: properly exposes the match count.This correctly sets
gl_pathcto the number of matched paths after populatinggl_pathv. Without this assignment, callers would have no way to determine how many entries are in the path array, breaking the glob functionality on Windows.
608-608: Good practice: file ends with newline.This formatting improvement aligns with standard coding conventions.
|
@coderabbitai help could you guide me how to re-run the failed checks? I think they should not relate to my change. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22891 🏁 Script executed: Length of output: 6507 🏁 Script executed: Length of output: 2445 @zshuang0316, you're correct that the failed check is unrelated to your Windows-specific changes. Let me explain what happened and how to re-run the checks. What FailedThe only failing check is:
This is a Linux unit test with the undefined behavior sanitizer enabled. Your changes are in Good NewsAll Windows-related checks passed successfully:
How to Re-run Failed ChecksYou have a few options: Option 1: Ask a MaintainerSince you've already tagged maintainers as reviewers (@edsiper, @cosmo0920), you can ask them to re-run the failed checks. Maintainers can click the "Re-run failed jobs" button in the Actions tab. Option 2: Push an Empty Commit (if you have write access)git commit --allow-empty -m "chore: trigger CI re-run"
git pushOption 3: Wait for Maintainer ReviewOften maintainers will recognize flaky tests and either re-run them or merge the PR knowing the failure is unrelated. RecommendationI suggest commenting on your PR mentioning the maintainers and noting that the failing test is a Linux sanitizer test unrelated to your Windows-specific fix, and all Windows checks passed. They can then re-run the failed job or proceed with the merge. Would you like me to help you draft a comment for the maintainers? |
|
Hello @cosmo0920, could you please help on the failed linux check which should not related to the code change? Thanks! |
This is not related to your changes. But this type of errors are already eliminated in the current master. So, could you rebase off master? Then, this type of error will be gone. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/in_blob/win32_glob.c (1)
107-117: Consider resettinggl_pathcinglobfreefor consistency.While not strictly required (POSIX allows glob_t contents to be undefined after
globfree), resettinggl_pathc = 0alongsidegl_pathv = NULLwould be more defensive and consistent.Apply this diff:
static void globfree(glob_t *context) { size_t index; if (context->gl_pathv != NULL) { flb_free(context->gl_pathv); context->gl_pathv = NULL; } + + context->gl_pathc = 0; flb_file_glob_clean(&context->inner_context); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_blob/win32_glob.c(1 hunks)
⏰ 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). (31)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-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 32bit, x86, x86-windows-static, 3.31.6)
- 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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- 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_COMPILER_STRICT_POINTER_TYPES=On, 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_THREAD=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 (-DSANITIZE_UNDEFINED=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_SANITIZE_MEMORY=On, 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=Off, 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 (-DFLB_SMALL=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 (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=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_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- 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, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- 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-centos-7
- GitHub Check: PR - fuzzing test
🔇 Additional comments (1)
plugins/in_blob/win32_glob.c (1)
153-153: Essential fix: correctly exposes the match count.This assignment is critical for the glob API to function correctly. Without it, callers have no way to determine how many paths are in
gl_pathv, rendering the implementation unusable. The placement after successful population of the array is correct, and the value accurately reflects the number of matches found.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/internal/win32_glob.c (1)
16-19: Test proceeds even if file creation fails.The test creates files but only checks if
fpis non-NULL before closing. If file creation fails (e.g., permissions), the test will still runglob()expecting 2 matches, leading to a false test failure that doesn't indicate an actual glob bug.Consider adding explicit checks:
/* Create some dummy files */ fp = fopen("test_glob_1.txt", "w"); - if (fp) fclose(fp); + TEST_CHECK(fp != NULL); + if (fp) fclose(fp); fp = fopen("test_glob_2.txt", "w"); - if (fp) fclose(fp); + TEST_CHECK(fp != NULL); + if (fp) fclose(fp);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
include/fluent-bit/flb_glob.h(1 hunks)plugins/in_blob/blob.c(1 hunks)src/CMakeLists.txt(1 hunks)src/win32/flb_glob.c(9 hunks)tests/internal/CMakeLists.txt(1 hunks)tests/internal/win32_glob.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/fluent-bit/flb_glob.h
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/CMakeLists.txtsrc/win32/flb_glob.c
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-11-21T06:23:29.770Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11171
File: include/fluent-bit/flb_lib.h:52-53
Timestamp: 2025-11-21T06:23:29.770Z
Learning: In Fluent Bit core (fluent/fluent-bit repository), function descriptions/documentation are not required for newly added functions in header files.
Applied to files:
src/win32/flb_glob.c
🧬 Code graph analysis (2)
tests/internal/win32_glob.c (1)
src/win32/flb_glob.c (2)
glob(60-98)globfree(48-58)
src/win32/flb_glob.c (1)
plugins/in_blob/blob.c (1)
is_directory(140-143)
⏰ 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). (31)
- 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: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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 (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=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_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=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 (-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=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=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-24.04, clang-14)
- 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-without-cxx (3.31.6)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (9)
src/win32/flb_glob.c (5)
48-58: Potential memory leak: individual path strings not freed.The
globfreefunction freesgl_pathvarray but doesn't free the individual path strings stored in it. Looking atflb_file_glob_fetch, the paths stored ingl_pathvare pointers directly frominner_context->current_entry->path. Whenflb_file_glob_cleanruns, those entries (and their paths) are freed, leaving thegl_pathvpointers dangling before they're accessed by callers.However, since
flb_file_glob_cleanis called at the end ofglobfree, and callers should processgl_pathvbefore callingglobfree, this should be safe if documented. The ownership model is: paths belong to inner_context, gl_pathv just holds references.Please verify this ownership model is intentional and that callers don't access
gl_pathvafterglobfree. The standard POSIXglobfreetypically frees the path strings, so this behavior differs from the standard API contract.
60-98: LGTM - glob function correctly sets gl_pathc.The fix at line 94 (
context->gl_pathc = entries) ensures the glob context's path count is set correctly after populating the path array. This aligns with the PR objective of fixing the glob path size.The function properly:
- Initializes the inner context
- Allocates the path array based on actual entry count
- Populates paths via
flb_file_glob_fetch- Sets
gl_pathcto the correct count
100-103: Consistent with non-Windows implementation.The
is_directoryfunction now uses explicit bitmask comparison((fs_entry_metadata->st_mode & S_IFDIR) != 0)instead of theS_ISDIRmacro which may not be available on Windows. This is functionally equivalent to the non-Windows version inplugins/in_blob/blob.c(line 139-143) which usesS_ISDIR.
430-471: LGTM - flb_file_glob_start properly initializes context.The function correctly:
- Validates the context pointer
- Allocates and initializes the inner context
- Copies flags appropriately
- Duplicates the path string
- Invokes
limited_win32_globfor actual globbing
473-507: LGTM - flb_file_glob_clean properly frees resources.The cleanup function correctly iterates through all entries, frees each path string, removes from list, and frees the entry structure. The final
memsetensures the context is zeroed.tests/internal/CMakeLists.txt (1)
84-89: LGTM - Windows-specific test properly integrated.The conditional block correctly adds
win32_glob.cto the test sources only on Windows builds, following the existing pattern used for other platform-specific tests (e.g., lines 91-98 for non-Windows tests).src/CMakeLists.txt (1)
130-137: LGTM - Windows glob implementation added to build.The
win32/flb_glob.csource file is correctly added to the Windows build alongside the existingflb_dlfcn_win32.c. This follows the established pattern for Windows-specific source files.plugins/in_blob/blob.c (1)
46-48: Good refactor: header inclusion replaces source inclusion.The change from including a
.csource file directly to using the proper header<fluent-bit/flb_glob.h>is a significant improvement. This:
- Follows proper C conventions for code organization
- Allows the glob implementation to be compiled once and linked
- Enables better symbol visibility control
tests/internal/win32_glob.c (1)
64-73: LGTM - Test list with proper fallback.The TEST_LIST properly registers the three test cases for Windows builds and provides an empty fallback for non-Windows builds (where this test file is still compiled but has no tests to run).
| void test_glob_basic() | ||
| { | ||
| glob_t glob_data; | ||
| int ret; | ||
| FILE *fp; | ||
|
|
||
| /* Create some dummy files */ | ||
| fp = fopen("test_glob_1.txt", "w"); | ||
| if (fp) fclose(fp); | ||
| fp = fopen("test_glob_2.txt", "w"); | ||
| if (fp) fclose(fp); | ||
|
|
||
| ret = glob("test_glob_*.txt", 0, NULL, &glob_data); | ||
| TEST_CHECK(ret == 0); | ||
| TEST_CHECK(glob_data.gl_pathc == 2); | ||
|
|
||
| globfree(&glob_data); | ||
|
|
||
| /* Cleanup */ | ||
| unlink("test_glob_1.txt"); | ||
| unlink("test_glob_2.txt"); | ||
| } |
There was a problem hiding this comment.
Missing initialization for glob_data struct.
In test_glob_basic, the glob_data variable is declared without initialization (line 11), unlike test_glob_nomatch which initializes with {0} (line 34). This could lead to undefined behavior if the glob implementation doesn't fully initialize all struct members.
Apply this diff to initialize the struct:
void test_glob_basic()
{
- glob_t glob_data;
+ glob_t glob_data = {0};
int ret;
FILE *fp;📝 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 test_glob_basic() | |
| { | |
| glob_t glob_data; | |
| int ret; | |
| FILE *fp; | |
| /* Create some dummy files */ | |
| fp = fopen("test_glob_1.txt", "w"); | |
| if (fp) fclose(fp); | |
| fp = fopen("test_glob_2.txt", "w"); | |
| if (fp) fclose(fp); | |
| ret = glob("test_glob_*.txt", 0, NULL, &glob_data); | |
| TEST_CHECK(ret == 0); | |
| TEST_CHECK(glob_data.gl_pathc == 2); | |
| globfree(&glob_data); | |
| /* Cleanup */ | |
| unlink("test_glob_1.txt"); | |
| unlink("test_glob_2.txt"); | |
| } | |
| void test_glob_basic() | |
| { | |
| glob_t glob_data = {0}; | |
| int ret; | |
| FILE *fp; | |
| /* Create some dummy files */ | |
| fp = fopen("test_glob_1.txt", "w"); | |
| if (fp) fclose(fp); | |
| fp = fopen("test_glob_2.txt", "w"); | |
| if (fp) fclose(fp); | |
| ret = glob("test_glob_*.txt", 0, NULL, &glob_data); | |
| TEST_CHECK(ret == 0); | |
| TEST_CHECK(glob_data.gl_pathc == 2); | |
| globfree(&glob_data); | |
| /* Cleanup */ | |
| unlink("test_glob_1.txt"); | |
| unlink("test_glob_2.txt"); | |
| } |
🤖 Prompt for AI Agents
In tests/internal/win32_glob.c around lines 9 to 30, the local variable
glob_data is declared uninitialized which can cause undefined behavior; change
its declaration to zero-initialize the struct (e.g., declare glob_t glob_data =
{0}; or otherwise zero it before use) so all members are defined prior to
calling glob(), keeping the rest of the test (glob call, checks, globfree and
cleanup) unchanged.
| void test_glob_wildcard() | ||
| { | ||
| glob_t glob_data; | ||
| int ret; | ||
| FILE *fp; | ||
|
|
||
| /* Create dummy file */ | ||
| fp = fopen("test_wildcard.txt", "w"); | ||
| if (fp) fclose(fp); | ||
|
|
||
| ret = glob("test_wild*.txt", 0, NULL, &glob_data); | ||
| TEST_CHECK(ret == 0); | ||
| TEST_CHECK(glob_data.gl_pathc == 1); | ||
| if (glob_data.gl_pathc > 0) { | ||
| TEST_CHECK(strstr(glob_data.gl_pathv[0], "test_wildcard.txt") != NULL); | ||
| } | ||
|
|
||
| globfree(&glob_data); | ||
| unlink("test_wildcard.txt"); | ||
| } |
There was a problem hiding this comment.
Same initialization concern in test_glob_wildcard.
Similar to test_glob_basic, the glob_data variable is not initialized.
Apply this diff:
void test_glob_wildcard()
{
- glob_t glob_data;
+ glob_t glob_data = {0};
int ret;
FILE *fp;📝 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 test_glob_wildcard() | |
| { | |
| glob_t glob_data; | |
| int ret; | |
| FILE *fp; | |
| /* Create dummy file */ | |
| fp = fopen("test_wildcard.txt", "w"); | |
| if (fp) fclose(fp); | |
| ret = glob("test_wild*.txt", 0, NULL, &glob_data); | |
| TEST_CHECK(ret == 0); | |
| TEST_CHECK(glob_data.gl_pathc == 1); | |
| if (glob_data.gl_pathc > 0) { | |
| TEST_CHECK(strstr(glob_data.gl_pathv[0], "test_wildcard.txt") != NULL); | |
| } | |
| globfree(&glob_data); | |
| unlink("test_wildcard.txt"); | |
| } | |
| void test_glob_wildcard() | |
| { | |
| glob_t glob_data = {0}; | |
| int ret; | |
| FILE *fp; | |
| /* Create dummy file */ | |
| fp = fopen("test_wildcard.txt", "w"); | |
| if (fp) fclose(fp); | |
| ret = glob("test_wild*.txt", 0, NULL, &glob_data); | |
| TEST_CHECK(ret == 0); | |
| TEST_CHECK(glob_data.gl_pathc == 1); | |
| if (glob_data.gl_pathc > 0) { | |
| TEST_CHECK(strstr(glob_data.gl_pathv[0], "test_wildcard.txt") != NULL); | |
| } | |
| globfree(&glob_data); | |
| unlink("test_wildcard.txt"); | |
| } |
🤖 Prompt for AI Agents
In tests/internal/win32_glob.c around lines 43 to 62, the local variable
glob_data is used without being initialized; initialize it before calling glob
(either declare as "glob_t glob_data = {0};" or call memset(&glob_data, 0,
sizeof glob_data);) so globfree can safely be called and gl_pathc/gl_pathv have
defined initial values.
@cosmo0920 I have the code updated, could you please take a look to see if there is any misunderstanding? |
There was a problem hiding this comment.
We're going to a right direction but I found some of the glitches for naming conventions.
We need to indicate that the newly added files should be Windows specific.
So, it would be great to use _win32 suffixes in file names.
src/win32/flb_glob.c
Outdated
| #include <fluent-bit/flb_str.h> | ||
| #include <cfl/cfl.h> | ||
| #include <cfl/cfl_list.h> | ||
| #include <fluent-bit/flb_glob.h> |
There was a problem hiding this comment.
It would be good to use flb_glob_win32.h to indicate Windows specific header.
include/fluent-bit/flb_glob_win32.h
Outdated
| #ifndef FLB_GLOB_H | ||
| #define FLB_GLOB_H |
There was a problem hiding this comment.
We need to use _win32 suffix in this file name.
|
Oh, our commit linter complains prefix issue in your commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/internal/win32_glob.c (2)
9-30: Initializeglob_datato prevent undefined behavior.The
glob_datavariable at line 11 is declared without initialization, which could lead to undefined behavior ifglob()doesn't fully initialize all struct members. Initialize it as done intest_glob_nomatchat line 34.Apply this diff:
void test_glob_basic() { - glob_t glob_data; + glob_t glob_data = {0}; int ret; FILE *fp;
43-62: Initializeglob_datato prevent undefined behavior.Same issue as
test_glob_basic: theglob_datavariable at line 45 should be initialized.Apply this diff:
void test_glob_wildcard() { - glob_t glob_data; + glob_t glob_data = {0}; int ret; FILE *fp;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
include/fluent-bit/flb_glob_win32.h(1 hunks)plugins/in_blob/blob.c(1 hunks)src/CMakeLists.txt(1 hunks)src/win32/flb_glob.c(9 hunks)tests/internal/CMakeLists.txt(1 hunks)tests/internal/win32_glob.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
plugins/in_blob/blob.csrc/win32/flb_glob.cinclude/fluent-bit/flb_glob_win32.h
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-11-21T06:23:29.770Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11171
File: include/fluent-bit/flb_lib.h:52-53
Timestamp: 2025-11-21T06:23:29.770Z
Learning: In Fluent Bit core (fluent/fluent-bit repository), function descriptions/documentation are not required for newly added functions in header files.
Applied to files:
src/win32/flb_glob.c
🧬 Code graph analysis (3)
tests/internal/win32_glob.c (1)
src/win32/flb_glob.c (2)
glob(60-98)globfree(48-58)
src/win32/flb_glob.c (1)
plugins/in_blob/blob.c (1)
is_directory(140-143)
include/fluent-bit/flb_glob_win32.h (2)
src/win32/flb_glob.c (3)
glob(60-98)globfree(48-58)is_directory(100-103)plugins/in_blob/blob.c (1)
is_directory(140-143)
🔇 Additional comments (6)
tests/internal/CMakeLists.txt (1)
84-89: LGTM!The Windows-specific test file is correctly added to the unit test suite following the existing pattern for platform-conditional test registration.
plugins/in_blob/blob.c (1)
46-48: LGTM!The transition from direct source file inclusion to header-based API is a clean improvement that better separates interface from implementation.
include/fluent-bit/flb_glob_win32.h (1)
89-91: LGTM!The public API declarations are clean and consistent with the implementation. The previously flagged issues regarding missing includes for
struct statandPATH_MAXhave been properly addressed with the additions at lines 24-25.src/win32/flb_glob.c (3)
21-31: LGTM!The conditional compilation guard and header inclusion properly establish the Windows-specific glob implementation.
48-58: LGTM!The public API functions (
globfree,glob, andis_directory) are correctly exposed with external linkage, aligning with the header declarations. The operator precedence fix inis_directoryat line 102 correctly wraps the bitwise AND operation in parentheses, ensuring proper directory detection.Also applies to: 60-98, 100-103
430-471: LGTM!The internal helper functions (
flb_file_glob_start,flb_file_glob_clean, andflb_file_glob_fetch) remain properly encapsulated as static functions, maintaining good separation between the public API and internal implementation details.Also applies to: 473-507, 509-547
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/win32/flb_glob.c (1)
100-103: Operator precedence fix is correct; consider marking unused parameter.The explicit parentheses
((fs_entry_metadata->st_mode & S_IFDIR) != 0)correctly fix the operator precedence bug. However, thepathparameter is unused in the implementation.Apply this diff to mark the unused parameter:
int is_directory(char *path, struct stat *fs_entry_metadata) { + (void) path; return ((fs_entry_metadata->st_mode & S_IFDIR) != 0); }Alternatively, consider using the
S_ISDIRmacro if available on Windows for consistency with the non-Windows implementation inplugins/in_blob/blob.c:139-142.include/fluent-bit/flb_glob_win32.h (1)
20-30: LGTM! Required includes addressed.The header guard
FLB_GLOB_WIN32_Hcorrectly matches the filename, and the required includes (<sys/stat.h>and<limits.h>) have been added as requested in past review comments.For strictness, consider moving lines 23-25 inside the
#ifdef FLB_SYSTEM_WINDOWSblock at line 27 to ensure these headers are only processed when building for Windows:#ifndef FLB_GLOB_WIN32_H #define FLB_GLOB_WIN32_H +#ifdef FLB_SYSTEM_WINDOWS + #include <fluent-bit/flb_info.h> #include <sys/stat.h> #include <limits.h> -#ifdef FLB_SYSTEM_WINDOWS - #include <cfl/cfl_list.h>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
include/fluent-bit/flb_glob_win32.h(1 hunks)plugins/in_blob/blob.c(1 hunks)src/CMakeLists.txt(1 hunks)src/win32/flb_glob.c(9 hunks)tests/internal/CMakeLists.txt(1 hunks)tests/internal/win32_glob.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/internal/CMakeLists.txt
- tests/internal/win32_glob.c
- plugins/in_blob/blob.c
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/CMakeLists.txtsrc/win32/flb_glob.c
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
include/fluent-bit/flb_glob_win32.hsrc/win32/flb_glob.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-11-21T06:23:29.770Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11171
File: include/fluent-bit/flb_lib.h:52-53
Timestamp: 2025-11-21T06:23:29.770Z
Learning: In Fluent Bit core (fluent/fluent-bit repository), function descriptions/documentation are not required for newly added functions in header files.
Applied to files:
src/win32/flb_glob.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
src/win32/flb_glob.c
🧬 Code graph analysis (2)
include/fluent-bit/flb_glob_win32.h (2)
src/win32/flb_glob.c (3)
glob(60-98)globfree(48-58)is_directory(100-103)plugins/in_blob/blob.c (1)
is_directory(140-143)
src/win32/flb_glob.c (1)
plugins/in_blob/blob.c (1)
is_directory(140-143)
⏰ 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). (31)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- 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: run-ubuntu-unit-tests (-DFLB_SIMD=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 (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=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_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=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_SANITIZE_MEMORY=On, 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_SIMD=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: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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 (-DSANITIZE_ADDRESS=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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- 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-without-cxx (3.31.6)
- 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, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-centos-7
- GitHub Check: PR - fuzzing test
🔇 Additional comments (7)
src/win32/flb_glob.c (3)
21-31: LGTM! Proper Windows-specific header inclusion.The Windows compilation guard and the new
flb_glob_win32.hheader inclusion are correct and align with the refactoring to extract glob as a Win32 utility.
48-58: LGTM! Correctly exposed as public API.The
globfreefunction signature matches the header declaration and properly cleans up resources.
60-98: LGTM! Critical fix correctly setsgl_pathc.The glob function now properly sets
context->gl_pathc = entrieson line 94, which is the key fix mentioned in the PR objectives. The function correctly populates the result array and returns appropriate error codes.include/fluent-bit/flb_glob_win32.h (3)
31-63: LGTM! Macro definitions are well-structured.The glob flags, error codes, and conditional GLOB_* macro definitions are properly structured. The use of
PATH_MAXat line 62 is now correct since<limits.h>is included.
64-87: LGTM! Structure definitions are correct.The structure definitions correctly expose the Windows glob implementation's internal scaffolding and provide a POSIX-like
glob_ttypedef withgl_pathvandgl_pathcfields.
89-91: LGTM! Function declarations match implementations.The function declarations correctly match the implementations in
src/win32/flb_glob.cand provide a clean POSIX-like glob API for Windows.src/CMakeLists.txt (1)
130-137: LGTM! Build configuration correctly updated.The addition of
win32/flb_glob.cto the Windows-specific sources list correctly integrates the extracted glob implementation into the build system.
@cosmo0920 The file name is updated. I updated the commit message with "glob:" but the check still failed, any suggestions? |
|
@cosmo0920 Do I have to split the commit to meet the commit id check? |
|
@cosmo0920 Do I have to split the commit to meet the commit id check? |
You can check with as follows: $ python .github/scripts/commit_prefix_check.pySee: https://github.com/fluent/fluent-bit/blob/797031cc30d2845b4dcb36cece6d6f2dd363d430/COMMIT_LINT.md |
|
@cosmo0920 Thanks for your guidance! Finally the check is passed and please review, thanks! |
|
@edsiper can I get your push here? Do you have any comments for me? Thanks! |
|
@edsiper do you have any comments to this PR? Could it be merged? |
|
This commit fails the automated prefix validation check due to a limitation in the validation script when handling cross-component refactorings. Why the validation fails:
Since this refactoring necessarily spans multiple directories to extract shared Windows functionality, neither condition can be satisfied. I chose "win32:" as the prefix since it best represents the primary purpose of the change. @cosmo0920 I mistakenly rebase master and the check cannot pass. |
Your commit includes lots of components. So, our linter almost always complains about this. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/win32/flb_glob.c`:
- Around line 100-103: The is_directory function currently tests file type
incorrectly by using (fs_entry_metadata->st_mode & S_IFDIR) != 0; update
is_directory to mask the type field first and compare to S_IFDIR (e.g.
(fs_entry_metadata->st_mode & S_IFMT) == S_IFDIR) or use the existing
FLB_FILE_ISTYPE(fs_entry_metadata->st_mode, S_IFDIR) macro defined earlier in
the file; modify the return expression in is_directory to use one of these
proper checks so directory detection is correct on Windows/MSVC.
🧹 Nitpick comments (1)
src/win32/flb_glob.c (1)
60-95: Consider NULL-terminatinggl_pathvfor safer API semantics.
gl_pathvis allocated with exactlyentrieselements without a trailing sentinel. All current callers correctly usegl_pathcas the iteration limit, so this is safe today. However, if the API is intended to follow POSIX conventions, allocatingentries + 1and leaving the final elementNULL(viacalloc) would prevent out-of-bounds reads if future callers assume sentinel termination.Suggested improvement
- context->gl_pathv = flb_calloc(entries, sizeof(char *)); + context->gl_pathv = flb_calloc(entries + 1, sizeof(char *));
| int is_directory(char *path, struct stat *fs_entry_metadata) | ||
| { | ||
| return (fs_entry_metadata->st_mode & S_IFDIR != 0); | ||
| return ((fs_entry_metadata->st_mode & S_IFDIR) != 0); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "flb_glob.c" -type fRepository: fluent/fluent-bit
Length of output: 83
🌐 Web query:
MSVC Windows stat.h S_IFDIR S_ISDIR macro definition st_mode directory check
💡 Result:
On MSVC/Windows, <sys/stat.h> exposes the file-type bits mainly as underscore-prefixed constants (for _stat::st_mode), e.g. _S_IFMT (type mask) and _S_IFDIR (directory). The usual directory test is: (st.st_mode & _S_IFMT) == _S_IFDIR. [1]
MSVC typically does not provide POSIX S_ISDIR() (nor S_ISREG()), so you either:
- use the mask test directly, or
- define the macro yourself (POSIX-style):
#define S_ISDIR(m) (((m) & S_IFMT) == S_IFDIR)[2][3]
If you want the non-underscore names S_IFMT, S_IFDIR, etc. from MSVC’s headers, define this before including <sys/stat.h>:
`#define` _CRT_INTERNAL_NONSTDC_NAMES 1
`#include` <sys/stat.h>Then you can use the POSIX-style macro definitions above. [2]
Minimal MSVC-compatible directory check:
`#include` <sys/stat.h>
int is_dir(const struct _stat* st) {
return (st->st_mode & _S_IFMT) == _S_IFDIR;
}
``` [1]
References:
[1] Microsoft `_stat` `st_mode` constants (`_S_IFMT`, `_S_IFDIR`, etc.)
[2] MSVC note/workaround: `_CRT_INTERNAL_NONSTDC_NAMES` and defining `S_ISDIR`
[3] POSIX definition/intent of `S_ISDIR(m)` checking `st_mode`
---
🏁 Script executed:
```shell
cat -n ./src/win32/flb_glob.c | head -120 | tail -50Repository: fluent/fluent-bit
Length of output: 1640
🏁 Script executed:
cat -n ./src/win32/flb_glob.c | head -20Repository: fluent/fluent-bit
Length of output: 979
🏁 Script executed:
cat -n ./src/win32/flb_glob.c | head -50Repository: fluent/fluent-bit
Length of output: 2031
🏁 Script executed:
rg "S_ISDIR|_CRT_INTERNAL_NONSTDC_NAMES" src/win32/Repository: fluent/fluent-bit
Length of output: 43
🏁 Script executed:
rg "FLB_FILE_ISTYPE" src/win32/flb_glob.c -A 2 -B 2Repository: fluent/fluent-bit
Length of output: 380
Use proper file type masking for directory checks: (st_mode & S_IFMT) == S_IFDIR or the existing FLB_FILE_ISTYPE macro.
The current check (st_mode & S_IFDIR) != 0 is unsafe—it tests a single bit without masking the type field first, which can incorrectly match other file types if their bits overlap with S_IFDIR. The correct approach is to mask with S_IFMT (file type field) before comparing. Since this is Windows-specific code, S_ISDIR() is not available in MSVC's standard library; use (st_mode & S_IFMT) == S_IFDIR instead, or leverage the FLB_FILE_ISTYPE(st_mode, S_IFDIR) macro already defined in this file (line 37).
🛠️ Suggested fix
- return ((fs_entry_metadata->st_mode & S_IFDIR) != 0);
+ return FLB_FILE_ISTYPE(fs_entry_metadata->st_mode, S_IFDIR);📝 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.
| int is_directory(char *path, struct stat *fs_entry_metadata) | |
| { | |
| return (fs_entry_metadata->st_mode & S_IFDIR != 0); | |
| return ((fs_entry_metadata->st_mode & S_IFDIR) != 0); | |
| } | |
| int is_directory(char *path, struct stat *fs_entry_metadata) | |
| { | |
| return FLB_FILE_ISTYPE(fs_entry_metadata->st_mode, S_IFDIR); | |
| } |
🤖 Prompt for AI Agents
In `@src/win32/flb_glob.c` around lines 100 - 103, The is_directory function
currently tests file type incorrectly by using (fs_entry_metadata->st_mode &
S_IFDIR) != 0; update is_directory to mask the type field first and compare to
S_IFDIR (e.g. (fs_entry_metadata->st_mode & S_IFMT) == S_IFDIR) or use the
existing FLB_FILE_ISTYPE(fs_entry_metadata->st_mode, S_IFDIR) macro defined
earlier in the file; modify the return expression in is_directory to use one of
these proper checks so directory detection is correct on Windows/MSVC.
Extract glob implementation into src/win32/flb_glob.c as a shared utility that can be used across different components on Windows. Signed-off-by: zshuang0316 <zshuang0316@163.com>
Replace in_blob's local glob implementation with the shared win32 glob utility. Signed-off-by: zshuang0316 <zshuang0316@163.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/internal/win32_glob.c`:
- Around line 16-19: The test currently calls fopen for "test_glob_1.txt" and
"test_glob_2.txt" into variable fp but does not handle fopen returning NULL;
update the test to check fp after each fopen, and if fp == NULL emit a clear
error (e.g., perror or fprintf to stderr with the filename) and fail the test
early (exit/return non-zero or call the test framework's fail helper), and only
call fclose when fp is non-NULL; ensure both file creations are checked
separately so a single failure stops the test instead of producing misleading
results.
- Around line 50-51: The fopen call that creates "test_wildcard.txt" uses fp
without checking for failure (fp = fopen(...); if (fp) fclose(fp);), which can
yield misleading test outcomes; update tests/internal/win32_glob.c to check for
fp == NULL after fopen and handle the error (e.g., call perror or the test
harness failure/ASSERT/return) instead of silently continuing, mirroring the fix
used in test_glob_basic so the test aborts or records a clear failure if the
file cannot be created.
| fp = fopen("test_glob_1.txt", "w"); | ||
| if (fp) fclose(fp); | ||
| fp = fopen("test_glob_2.txt", "w"); | ||
| if (fp) fclose(fp); |
There was a problem hiding this comment.
Add error handling for file creation.
If fopen fails, the test proceeds and may not find the expected 2 files, causing a misleading test failure. Consider failing the test early if file creation fails.
Proposed fix
/* Create some dummy files */
fp = fopen("test_glob_1.txt", "w");
- if (fp) fclose(fp);
+ TEST_CHECK(fp != NULL);
+ if (fp) fclose(fp);
fp = fopen("test_glob_2.txt", "w");
- if (fp) fclose(fp);
+ TEST_CHECK(fp != NULL);
+ if (fp) fclose(fp);📝 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.
| fp = fopen("test_glob_1.txt", "w"); | |
| if (fp) fclose(fp); | |
| fp = fopen("test_glob_2.txt", "w"); | |
| if (fp) fclose(fp); | |
| fp = fopen("test_glob_1.txt", "w"); | |
| TEST_CHECK(fp != NULL); | |
| if (fp) fclose(fp); | |
| fp = fopen("test_glob_2.txt", "w"); | |
| TEST_CHECK(fp != NULL); | |
| if (fp) fclose(fp); |
🤖 Prompt for AI Agents
In `@tests/internal/win32_glob.c` around lines 16 - 19, The test currently calls
fopen for "test_glob_1.txt" and "test_glob_2.txt" into variable fp but does not
handle fopen returning NULL; update the test to check fp after each fopen, and
if fp == NULL emit a clear error (e.g., perror or fprintf to stderr with the
filename) and fail the test early (exit/return non-zero or call the test
framework's fail helper), and only call fclose when fp is non-NULL; ensure both
file creations are checked separately so a single failure stops the test instead
of producing misleading results.
| fp = fopen("test_wildcard.txt", "w"); | ||
| if (fp) fclose(fp); |
There was a problem hiding this comment.
Add error handling for file creation.
Same issue as test_glob_basic - if fopen fails, the test will produce misleading results.
Proposed fix
/* Create dummy file */
fp = fopen("test_wildcard.txt", "w");
+ TEST_CHECK(fp != NULL);
if (fp) fclose(fp);📝 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.
| fp = fopen("test_wildcard.txt", "w"); | |
| if (fp) fclose(fp); | |
| fp = fopen("test_wildcard.txt", "w"); | |
| TEST_CHECK(fp != NULL); | |
| if (fp) fclose(fp); |
🤖 Prompt for AI Agents
In `@tests/internal/win32_glob.c` around lines 50 - 51, The fopen call that
creates "test_wildcard.txt" uses fp without checking for failure (fp =
fopen(...); if (fp) fclose(fp);), which can yield misleading test outcomes;
update tests/internal/win32_glob.c to check for fp == NULL after fopen and
handle the error (e.g., call perror or the test harness failure/ASSERT/return)
instead of silently continuing, mirroring the fix used in test_glob_basic so the
test aborts or records a clear failure if the file cannot be created.
@cosmo0920 Thanks for your advice! I split it to three commits, now it looks good. |
Signed-off-by: zshuang0316 <zshuang0316@163.com>
Set context glob path size correctly.
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:
[2025/11/15 17:48:33] [debug] [input:blob:blob.0] scanning path C:\data\Logs\local*.log
[2025/11/15 17:48:33] [ info] [input:blob:blob.0] DEBUG: recursive_file_search called with path='NULL' pattern='C:\data\Logs\local*.log'
[2025/11/15 17:48:33] [ info] [input:blob:blob.0] DEBUG: calling glob with local_pattern='C:\data\Logs\local*.log'
[2025/11/15 17:48:33] [ info] [input:blob:blob.0] DEBUG: glob returned 0, found 14 matches
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
New Features
Refactor
Bug Fixes
Tests
Chores