Upstream changes to prepare for the nario command#214
Conversation
WalkthroughRefactors export/import from Store member methods to free functions with a new public header, removes corresponding Serve/Worker protocol opcodes and connection importPaths, raises the minimum protocol version, and unifies/simplifies daemon/remote-store add/set/query flows and legacy SSH/dummy-store adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as nix-store (CLI)
participant Store as Store
participant FS as Filesystem
Note over CLI,Store: Export/import are free functions taking Store&
User->>CLI: nix-store --export paths...
CLI->>Store: exportPaths(Store&, paths, sink)
Store->>FS: Read NARs / topo-sort
FS-->>Store: NAR streams
Store-->>CLI: Write export stream
User->>CLI: nix-store --import
CLI->>Store: importPaths(Store&, source, checkSigs)
Store->>FS: Add paths to store
Store-->>CLI: Return imported paths
sequenceDiagram
autonumber
participant Client as Remote client
participant Daemon as Store daemon
Note over Client,Daemon: Handshake enforces MINIMUM_PROTOCOL_VERSION
Client->>Daemon: Hello (protoVersion)
Daemon-->>Client: Hello (protoVersion)
alt GET_PROTOCOL_MAJOR(remote) != expected OR GET_PROTOCOL_MINOR(remote) < MIN
Client-->>Client: Reject connection (throw)
else
Client-->>Daemon: Proceed with ops (AddToStore, BuildPaths, etc.)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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
🧹 Nitpick comments (11)
src/libstore/daemon.cc (2)
552-565: Unify BuildMode read; drop redundant default initGood change to always read BuildMode and enforce the repair trust guard. Remove the now-redundant bmNormal init.
Apply:
- BuildMode mode = bmNormal; - mode = WorkerProto::Serialise<BuildMode>::read(*store, rconn); + const BuildMode mode = WorkerProto::Serialise<BuildMode>::read(*store, rconn);Consider the same cleanup in BuildPathsWithResults (Lines 574–576).
1033-1034: Make min client version expression self-descriptive
256 + 18encodes v1.18. Consider a helper or constant for clarity (e.g., (1u << 8) | 18u, or a MAKE_PROTOCOL_VERSION(1,18) macro if available).src/libstore/legacy-ssh-store.cc (1)
165-166: Mark connection bad on read-side protocol errorsIf the remote closes or corrupts the stream, readInt(conn->from) will throw; we should also mark the connection bad in that case.
Apply this diff:
- if (readInt(conn->from) != 1) - throw Error("failed to add path '%s' to remote host '%s'", printStorePath(info.path), config->authority.host); + try { + if (readInt(conn->from) != 1) + throw Error( + "failed to add path '%s' to remote host '%s'", + printStorePath(info.path), config->authority.host); + } catch (EndOfFile &) { + conn->good = false; + throw; + } catch (SerialisationError &) { + conn->good = false; + throw; + }Optionally, if you want to silence unused-parameter warnings since flags aren’t serialized here, add near the start of the function (outside the changed hunk):
(void) repair; (void) checkSigs;src/libstore/remote-store.cc (2)
76-79: Make minimum protocol check explicit/readable.The literal 256 + 18 is opaque. Prefer an explicit expression and a clearer error.
Apply:
- if (protoVersion < 256 + 18) - throw Error("the Nix daemon version is too old"); + if (protoVersion < ((1u << 8) | 18)) + throw Error("the Nix daemon version is too old (need >= 1.18)");
114-130: Minor: tighten iteration and avoid magic string.
- Iterate by structured binding to avoid extra map lookups/copies.
- "plugin-files" as a raw string is brittle; prefer a named constant if available.
Apply:
- for (auto & i : overrides) - conn.to << i.first << i.second.value; + for (const auto & [key, info] : overrides) + conn.to << key << info.value;And consider replacing:
overrides.erase("plugin-files");with a shared constant from the config subsystem, if one exists.
src/libstore/include/nix/store/export-import.hh (2)
10-10: Use inline constexpr for header-defined constant.Avoid multiple definition/ODR pitfalls and signal compile-time constant.
Apply:
-const uint32_t exportMagic = 0x4558494e; +[[maybe_unused]] inline constexpr uint32_t exportMagic = 0x4558494e;
22-22: Consider marking importPaths [[nodiscard]].Discarding the returned StorePaths is likely unintended.
Apply:
-StorePaths importPaths(Store & store, Source & source, CheckSigsFlag checkSigs = CheckSigs); +[[nodiscard]] StorePaths importPaths(Store & store, Source & source, CheckSigsFlag checkSigs = CheckSigs);src/libstore/serve-protocol-connection.cc (1)
18-19: Enforce min serve-protocol 2.5 in handshake: good; improve error detail.Include the remote version in the error for diagnostics.
Apply:
- if (GET_PROTOCOL_MAJOR(remoteVersion) != 0x200 || GET_PROTOCOL_MINOR(remoteVersion) < 5) - throw Error("unsupported 'nix-store --serve' protocol version on '%s'", host); + if (GET_PROTOCOL_MAJOR(remoteVersion) != 0x200 || GET_PROTOCOL_MINOR(remoteVersion) < 5) + throw Error("unsupported 'nix-store --serve' protocol version on '%s' (got 0x%x, need >= 0x205)", host, remoteVersion);src/nix/nix-store/nix-store.cc (1)
790-795: Avoid per‑line std::flush in opImport to improve performance.Flushing each line is expensive; flush once after the loop if needed.
Apply:
- for (auto & i : paths) - cout << fmt("%s\n", store->printStorePath(i)) << std::flush; + for (auto & i : paths) + cout << fmt("%s\n", store->printStorePath(i)); + cout.flush();src/perl/lib/Nix/Store.xs (1)
233-238: exportPaths: correct switch to free function; consider explicit flush.Behavior matches previous implementation. Optionally flush to ensure data is written before returning to Perl.
FdSink sink(fd); - exportPaths(*THIS->store, paths, sink); + exportPaths(*THIS->store, paths, sink); + sink.flush();src/libstore/export-import.cc (1)
50-96: importPaths: minor readability + memory usage suggestions.
- Name shadowing: local StringSource named source shadows the function parameter. Rename to avoid confusion.
- For large NARs, keeping the entire archive in memory (StringSink saved) can be costly. If feasible later, consider a streaming approach (e.g., hashing while teeing to a spool and feeding that to addToStore).
Apply shadowing fix:
- auto source = StringSource(saved.s); - store.addToStore(info, source, NoRepair, checkSigs); + auto narSource = StringSource(saved.s); + store.addToStore(info, narSource, NoRepair, checkSigs);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/libstore-tests/data/serve-protocol/handshake-to-client.binis excluded by!**/*.bin
📒 Files selected for processing (18)
src/libstore-tests/serve-protocol.cc(1 hunks)src/libstore/daemon.cc(4 hunks)src/libstore/dummy-store.cc(3 hunks)src/libstore/export-import.cc(5 hunks)src/libstore/include/nix/store/export-import.hh(1 hunks)src/libstore/include/nix/store/legacy-ssh-store.hh(0 hunks)src/libstore/include/nix/store/meson.build(1 hunks)src/libstore/include/nix/store/serve-protocol-connection.hh(0 hunks)src/libstore/include/nix/store/serve-protocol.hh(0 hunks)src/libstore/include/nix/store/store-api.hh(0 hunks)src/libstore/include/nix/store/worker-protocol-connection.hh(0 hunks)src/libstore/include/nix/store/worker-protocol.hh(0 hunks)src/libstore/legacy-ssh-store.cc(1 hunks)src/libstore/remote-store.cc(7 hunks)src/libstore/serve-protocol-connection.cc(1 hunks)src/libstore/worker-protocol-connection.cc(0 hunks)src/nix/nix-store/nix-store.cc(3 hunks)src/perl/lib/Nix/Store.xs(3 hunks)
💤 Files with no reviewable changes (7)
- src/libstore/include/nix/store/worker-protocol.hh
- src/libstore/include/nix/store/worker-protocol-connection.hh
- src/libstore/worker-protocol-connection.cc
- src/libstore/include/nix/store/serve-protocol.hh
- src/libstore/include/nix/store/store-api.hh
- src/libstore/include/nix/store/legacy-ssh-store.hh
- src/libstore/include/nix/store/serve-protocol-connection.hh
🧰 Additional context used
🧬 Code graph analysis (6)
src/nix/nix-store/nix-store.cc (2)
src/libstore/export-import.cc (4)
exportPaths(37-48)exportPaths(37-37)importPaths(50-96)importPaths(50-50)src/libstore/include/nix/store/export-import.hh (2)
exportPaths(16-16)importPaths(22-22)
src/libstore/include/nix/store/export-import.hh (2)
src/libstore/include/nix/store/store-api.hh (11)
paths(361-361)paths(367-367)paths(513-516)paths(513-513)paths(525-525)paths(640-643)paths(651-654)paths(715-715)paths(789-794)paths(814-814)source(545-545)src/nix/nix-store/nix-store.cc (4)
sink(749-749)sink(777-777)source(763-763)source(790-790)
src/libstore/legacy-ssh-store.cc (3)
src/libstore/include/nix/store/store-api.hh (3)
info(530-534)info(757-757)source(545-545)src/libstore/include/nix/store/legacy-ssh-store.hh (1)
info(81-81)src/libstore/include/nix/store/remote-store.hh (3)
info(94-94)info(101-101)source(96-96)
src/libstore/export-import.cc (1)
src/libstore/include/nix/store/store-api.hh (28)
path(313-313)path(335-335)path(341-341)path(348-348)path(352-352)path(389-389)path(395-395)path(400-400)path(410-410)path(446-446)path(456-459)path(456-456)path(469-472)path(469-469)path(477-477)path(485-485)info(530-534)info(757-757)paths(361-361)paths(367-367)paths(513-516)paths(513-513)paths(525-525)paths(640-643)paths(651-654)paths(715-715)paths(789-794)paths(814-814)
src/libstore/remote-store.cc (2)
src/libstore/include/nix/store/remote-store.hh (9)
conn(163-163)conn(167-167)paths(50-50)paths(67-67)paths(69-69)paths(107-107)info(94-94)info(101-101)source(96-96)src/libstore/include/nix/store/worker-protocol-connection.hh (1)
sink(73-73)
src/libstore/dummy-store.cc (7)
src/libstore/include/nix/store/store-api.hh (1)
requireValidPath(736-736)src/libstore/include/nix/store/legacy-ssh-store.hh (2)
requireValidPath(145-148)requireValidPath(145-145)src/libstore/include/nix/store/local-fs-store.hh (1)
requireValidPath(70-70)src/libstore/include/nix/store/uds-remote-store.hh (2)
requireValidPath(59-62)requireValidPath(59-59)src/libstore/ssh-store.cc (2)
requireValidPath(149-152)requireValidPath(149-149)src/libstore/include/nix/store/remote-store.hh (1)
requireValidPath(177-177)src/libstore/include/nix/store/binary-cache-store.hh (1)
requireValidPath(176-176)
⏰ 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). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (20)
src/libstore/dummy-store.cc (1)
68-106: Redundantvirtualremoval looks good
overridealready implies virtual dispatch, so dropping the explicitvirtualkeyword here keeps the declarations cleaner without changing behavior.src/libstore/daemon.cc (1)
853-857: Confirm queryPathInfo “not found” semanticsThis assumes queryPathInfo returns nullptr rather than throwing for missing paths. Verify all Store implementations now follow that contract; otherwise emit 0 on InvalidPath.
Option if needed:
- info = store->queryPathInfo(path); + try { + info = store->queryPathInfo(path); + } catch (InvalidPath &) { + info.reset(); + }src/libstore/legacy-ssh-store.cc (2)
158-162: Good: mark connection unusable on NAR copy failureSetting conn->good = false on streaming failure prevents reuse of a poisoned connection. LGTM.
152-157: Legacy SSH AddToStoreNar format is compatible: ServeProto v2.7 on the server (in nix-store) handles Command::AddToStoreNar by reading path, deriver, Base16 narHash, references, registrationTime, narSize, ultimate, sigs, and CA in exactly that order and then streams the NAR; repair/checkSigs flags aren’t part of the legacy SSH (ServeProto) path—they’re only added in the newer WorkerProto, so omitting them here is intentional.src/libstore-tests/serve-protocol.cc (1)
23-26: Bump to 2.5 default is consistent with serve-protocol min version. LGTM.src/libstore/remote-store.cc (5)
170-171: Delegation to connection helper looks good.
184-188: Unconditional QuerySubstitutablePaths path: OK.
197-215: Compatibility handling for < 1.22: OK.Fallback to StorePathSet for older daemons is correct; reads match write format.
563-567: Always sending buildMode simplifies the protocol surface. LGTM.
429-444: Unified AddToStoreNar write path is fine; streaming branches look correct.withFramedSink branch appropriately processes daemon stderr afterward.
src/libstore/include/nix/store/meson.build (1)
37-37: New public header inclusion looks correct.src/libstore/serve-protocol-connection.cc (1)
1-96: No remaining ServeProto::BasicClientConnection::importPaths references. Import/export now uses the freeimportPaths/exportPathsfunctions fromnix/store/export-import.hh.src/nix/nix-store/nix-store.cc (3)
17-17: Correct include for new free‑function API.Including export-import.hh here is correct for the new call sites.
777-779: Switched to free function exportPaths — good.Semantics preserved; sink is still flushed after use.
790-792: Signature-check behavior unchanged.importPathsis only invoked here withNoCheckSigs, matching priornix-store --importbehavior and no callers depend on signature validation.src/perl/lib/Nix/Store.xs (2)
14-14: Good: include new header for free‑function API.
244-249: importPaths: return value ignored — verify Perl API expectations.Free function returns StorePaths but this wrapper remains void. Confirm that Perl callers don’t expect returned paths; otherwise, push them onto the Perl stack.
If you decide to return imported paths, I can provide a patch to XPUSH each path.
src/libstore/export-import.cc (3)
1-1: Public header include is correct for new API.
12-35: exportPath free function looks correct and preserves invariants.
- Streams NAR through TeeSink and verifies narHash before metadata serialization — matches legacy behavior.
37-48: exportPaths batching protocol preserved.Toposort + per‑path marker (1) and terminating 0 are correct.
| unsigned int n = readInt(conn.from); | ||
| for (unsigned int i = 0; i < n; i++) { | ||
| auto name = readString(conn.from); | ||
| auto value = readString(conn.from); | ||
| clientSettings.overrides.emplace(name, value); | ||
| } |
There was a problem hiding this comment.
Validate override count to prevent DoS
readInt can return negative and the loop is unbounded. Add range checks and a sane cap to avoid large allocations from untrusted clients.
Apply:
- unsigned int n = readInt(conn.from);
- for (unsigned int i = 0; i < n; i++) {
+ int n = readInt(conn.from);
+ if (n < 0) throw Error("invalid overrides count");
+ constexpr int kMaxOverrides = 4096;
+ if (n > kMaxOverrides)
+ throw Error("too many overrides in SetOptions (max: %d)", kMaxOverrides);
+ for (int i = 0; i < n; i++) {
auto name = readString(conn.from);
auto value = readString(conn.from);
clientSettings.overrides.emplace(name, value);
}📝 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.
| unsigned int n = readInt(conn.from); | |
| for (unsigned int i = 0; i < n; i++) { | |
| auto name = readString(conn.from); | |
| auto value = readString(conn.from); | |
| clientSettings.overrides.emplace(name, value); | |
| } | |
| int n = readInt(conn.from); | |
| if (n < 0) | |
| throw Error("invalid overrides count"); | |
| constexpr int kMaxOverrides = 4096; | |
| if (n > kMaxOverrides) | |
| throw Error("too many overrides in SetOptions (max: %d)", kMaxOverrides); | |
| for (int i = 0; i < n; i++) { | |
| auto name = readString(conn.from); | |
| auto value = readString(conn.from); | |
| clientSettings.overrides.emplace(name, value); | |
| } |
cole-h
left a comment
cole-h
left a comment
There was a problem hiding this comment.
Got punk'd by GitHub's new review UI. Rest of my comments are nits, LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/libstore/remote-store.cc (1)
76-77: Document MINIMUM_PROTOCOL_VERSION
MINIMUM_PROTOCOL_VERSION is defined in src/libstore/include/nix/store/worker-protocol.hh; add a brief comment explaining that(1<<8 | 18)encodes protocol version 1.18 for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/libstore/daemon.cc(4 hunks)src/libstore/include/nix/store/serve-protocol.hh(1 hunks)src/libstore/include/nix/store/worker-protocol.hh(3 hunks)src/libstore/remote-store.cc(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/libstore/include/nix/store/worker-protocol.hh
- src/libstore/include/nix/store/serve-protocol.hh
🧰 Additional context used
🧬 Code graph analysis (2)
src/libstore/daemon.cc (2)
src/libstore/include/nix/store/worker-protocol-connection.hh (6)
store(102-102)store(113-113)store(115-120)store(126-126)store(128-132)store(161-161)src/libstore/include/nix/store/store-api.hh (18)
info(530-534)info(757-757)path(313-313)path(335-335)path(341-341)path(348-348)path(352-352)path(389-389)path(395-395)path(400-400)path(410-410)path(446-446)path(456-459)path(456-456)path(469-472)path(469-469)path(477-477)path(485-485)
src/libstore/remote-store.cc (4)
src/libstore/include/nix/store/remote-store.hh (9)
conn(163-163)conn(167-167)paths(50-50)paths(67-67)paths(69-69)paths(107-107)info(94-94)info(101-101)source(96-96)src/libstore/include/nix/store/worker-protocol.hh (4)
write(131-134)write(131-131)sink(221-224)sink(221-221)src/libstore/store-dir-config.cc (4)
parseStorePath(9-26)parseStorePath(9-9)printStorePath(50-53)printStorePath(50-50)src/libstore/include/nix/store/worker-protocol-connection.hh (1)
sink(73-73)
⏰ 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). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (10)
src/libstore/remote-store.cc (6)
170-170: LGTM!The simplification to always delegate
queryValidPathsto the server is clean and removes unnecessary protocol-specific branching.
184-187: LGTM!Good simplification - removing the protocol-minor gated branches for
querySubstitutablePathsmakes the code more maintainable.
197-215: LGTM!The unified approach for
querySubstitutablePathInfosmaintains backward compatibility while simplifying the code flow. The protocol version check at line 198 appropriately handles legacy clients.
429-443: LGTM!Clean simplification of
addToStorethat removes legacy protocol branches while maintaining the necessary protocol version checks for framing support.
566-566: LGTM!Simplified
buildPathsby removing protocol assertions. The unconditional sending ofbuildModestreamlines the protocol flow.
114-129: Consider bounds checking for overrides to prevent potential DoS.The code now unconditionally reads overrides without checking the count. A malicious client could potentially send a very large number of overrides causing excessive memory allocation.
Consider adding a reasonable upper limit check:
+ constexpr size_t kMaxOverrides = 4096; + size_t overrideCount = overrides.size(); + if (overrideCount > kMaxOverrides) { + throw Error("too many overrides in SetOptions (max: %zu)", kMaxOverrides); + } conn.to << overrides.size();src/libstore/daemon.cc (4)
552-564: LGTM!The refactored BuildPaths handling is cleaner with unified BuildMode reading and appropriate trust validation for repair mode.
853-860: Clean simplification of QueryPathInfo handling.Removing the try/catch for InvalidPath and using a consistent null-check pattern improves code clarity.
1033-1033: LGTM!Using
MINIMUM_PROTOCOL_VERSIONconstant instead of magic number0x10aimproves maintainability.
784-789: Validate override count to prevent DoS.The code reads an arbitrary number of overrides without bounds checking. A malicious client could cause excessive memory allocation.
Apply this fix to add validation:
- unsigned int n = readInt(conn.from); - for (unsigned int i = 0; i < n; i++) { + int n = readInt(conn.from); + if (n < 0) throw Error("invalid overrides count"); + constexpr int kMaxOverrides = 4096; + if (n > kMaxOverrides) + throw Error("too many overrides in SetOptions (max: %d)", kMaxOverrides); + for (int i = 0; i < n; i++) {
2 participants
Motivation
This applies merged upstream PRs NixOS#13959, NixOS#13957, NixOS#13956, NixOS#13951 which are prerequisites for the
nix nariocommand.Context
Summary by CodeRabbit
Refactor
Tests
Chores