Conversation
📝 WalkthroughWalkthroughThis pull request introduces two new C API methods to the Nix store interface: Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Fix all issues with AI agents
In `@src/libstore-c/nix_api_store.cc`:
- Around line 456-470: The function nix_store_query_path_from_hash_part lacks
null checks for the store and hash params; add validation at the top (after
setting context->last_err_code = NIX_OK) to return nullptr and set
context->last_err_code = NIX_ERR_INVALID_ARG when store is null or hash is null,
mirroring the behavior in nix_store_copy_path, so you never dereference
store->ptr or use a null hash; keep the existing try/catch and
NIXC_CATCH_ERRS_NULL flow otherwise.
🧹 Nitpick comments (3)
doc/manual/rl-next/c-api-new-store-methods.md (1)
3-3: Consider including both upstream PR references.The PR description mentions cherry-picks from both upstream PRs
#14766and#14768. Thenix_store_copy_pathfunction appears to come from#14768, so consider adding it:-prs: [14766] +prs: [14766, 14768]src/libstore-c/nix_api_store.h (2)
360-369: Documentation says "empty" but function returns NULL.The doc says "or empty if no matching path is found" but the function returns
NULL(not an empty string or empty path). Consider clarifying:/** * `@brief` Query the full store path given the hash part of a valid store - * path, or empty if no matching path is found. + * path. * * `@param`[out] context Optional, stores error information * `@param`[in] store nix store reference * `@param`[in] hash Hash part of path as a string - * `@return` Store path reference, NULL if no matching path is found. + * `@return` Store path reference, or NULL if no matching path is found. Free with nix_store_path_free(). */
371-382: Add@returndocumentation for consistency.Other functions in this header document their return values. This function should specify what it returns:
* `@param`[in] path The path to copy * `@param`[in] repair Whether to repair the path * `@param`[in] checkSigs Whether to check path signatures are trusted before copying + * `@return` NIX_OK on success, or an error code on failure. */
| StorePath * nix_store_query_path_from_hash_part(nix_c_context * context, Store * store, const char * hash) | ||
| { | ||
| if (context) | ||
| context->last_err_code = NIX_OK; | ||
| try { | ||
| std::optional<nix::StorePath> s = store->ptr->queryPathFromHashPart(hash); | ||
|
|
||
| if (!s.has_value()) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| return new StorePath{std::move(s.value())}; | ||
| } | ||
| NIXC_CATCH_ERRS_NULL | ||
| } |
There was a problem hiding this comment.
Missing null validation for store and hash parameters.
Unlike nix_store_copy_path which validates its parameters, this function will crash if store is null (dereferencing store->ptr) or may have undefined behavior if hash is null. Consider adding validation for consistency:
🛡️ Proposed fix to add validation
StorePath * nix_store_query_path_from_hash_part(nix_c_context * context, Store * store, const char * hash)
{
if (context)
context->last_err_code = NIX_OK;
try {
+ if (store == nullptr) {
+ nix_set_err_msg(context, NIX_ERR_UNKNOWN, "Store is null");
+ return nullptr;
+ }
+ if (hash == nullptr) {
+ nix_set_err_msg(context, NIX_ERR_UNKNOWN, "Hash is null");
+ return nullptr;
+ }
std::optional<nix::StorePath> s = store->ptr->queryPathFromHashPart(hash);Note: The existing nix_store_copy_closure function (line 344) also lacks null validation, so if following strict consistency with existing code, this could be considered optional.
🤖 Prompt for AI Agents
In `@src/libstore-c/nix_api_store.cc` around lines 456 - 470, The function
nix_store_query_path_from_hash_part lacks null checks for the store and hash
params; add validation at the top (after setting context->last_err_code =
NIX_OK) to return nullptr and set context->last_err_code = NIX_ERR_INVALID_ARG
when store is null or hash is null, mirroring the behavior in
nix_store_copy_path, so you never dereference store->ptr or use a null hash;
keep the existing try/catch and NIXC_CATCH_ERRS_NULL flow otherwise.
This comment was marked as outdated.
This comment was marked as outdated.
Upstream C store API improvements
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
5 participants
Motivation
Merge upstream PRs NixOS#14766, NixOS#14768.
Context
Summary by CodeRabbit
New Features
Documentation