- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.8k
 
blob: Plug a sqldb encapsulation leak #10784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
blob: Plug a sqldb encapsulation leak #10784
Conversation
Signed-off-by: Hiroshi Hatake <[email protected]>
          
Walkthroughflb_blob_db_file_part_insert now retrieves the inserted row ID using flb_sqldb_last_id(context->db) instead of sqlite3_last_insert_rowid(context->db). Error handling, cleanup, and output initialization remain unchanged. Changes
 Sequence Diagram(s)sequenceDiagram
  autonumber
  participant Caller
  participant BlobDB as flb_blob_db_file_part_insert
  participant SQL as SQLite DB
  Caller->>BlobDB: insert(file_part)
  BlobDB->>SQL: EXECUTE INSERT
  alt success
    BlobDB->>SQL: flb_sqldb_last_id(db)
    SQL-->>BlobDB: last_id
    BlobDB-->>Caller: out_id = last_id
  else error
    BlobDB-->>Caller: out_id = -1 (error)
  end
    Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
 Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
 🧪 Generate unit tests
 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit: 
 SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type  Other keywords and placeholders
 CodeRabbit Configuration File (
 | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/flb_blob_db.c (1)
881-921: Minor: consider documenting/defending out_id contractCurrent code assigns -1 on error and a valid rowid on success. Since you’re touching this function, consider briefly documenting that out_id must be non-NULL, or defensively initializing it to -1 at function entry when non-NULL. Not urgent; just helps avoid stale values if future early-returns are introduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
 - Jira integration is disabled by default for public repositories
 - Linear integration is disabled by default for public repositories
 
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/flb_blob_db.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/flb_blob_db.c (1)
src/flb_sqldb.c (1)
flb_sqldb_last_id(168-171)
⏰ 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). (28)
- 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, gcc, g++)
 - GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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_SIMD=Off, 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 (-DFLB_SIMD=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_SANITIZE_THREAD=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_JEMALLOC=On, 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, 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, clang, clang++)
 - GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=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, gcc, g++)
 - GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 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-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, 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)
 
🔇 Additional comments (1)
src/flb_blob_db.c (1)
912-912: No stray sqlite3_last_insert_rowid() calls found – change approvedThe use of flb_sqldb_last_id(context->db) properly replaces direct sqlite3_last_insert_rowid() calls and aligns with the existing pattern (e.g., in flb_blob_db_file_insert). The wrapper is defined only once in src/flb_sqldb.c and no other direct calls remain in the project code. Locking is correctly held around the insert/last-id pair.
– Change is good to merge.
| 
           flb_blob_db.c is not included in 4.0. So, we needn't backport to 4.0.  | 
    
This should be needed for compilation with the recent version of gcc.
gcc-14 or later checks that kind of glitches and make an error.
This is also occurred on my dev box's archlinux WSL2 environment.
This issue was initially reported in #10728 (comment).
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit