Sync trunk with main at v0.3.0#10
Conversation
Brings trunk up to date with all work on main through v0.3.0: - S3 streaming transfers and sccache integration - SMB2 compounding, small-file fast paths, benchmarks (v0.2.0) - Non-blocking file logging with slog!/serr! macros, ISO-8601 timestamps - Version bump to 0.3.0, README updates
There was a problem hiding this comment.
Pull request overview
Syncs trunk with main through v0.3.0, bringing in SMB compounding + small-file fast paths, non-blocking logging, S3/XML/header optimizations, new benches, and CI/script/README updates.
Changes:
- Add SMB2 compound operations (Create+Read+Close, Create+Write+Close, batched directory creation) and wire-level tweaks (credit charges, zero-copy read decode).
- Add non-blocking stdout/stderr logging with optional file tee (
SPICEIO_LOG_FILE) and update CLI/CI/docs for--version. - Add/adjust S3 fast paths and performance optimizations (XML/date formatting, multipart store tests, new benchmarks, CI/test scripts).
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/smb/protocol.rs |
SMB protocol structs/helpers updated; adds credit charge + compounding helpers + many decode/encode changes and unit tests. |
src/smb/ops.rs |
Share-level ops updated to use compound fast paths for small reads/writes and batched dir creation. |
src/smb/client.rs |
Adds compound request builder/parser, more logging, credit charge usage, and read zero-copy path. |
src/smb/auth.rs |
Minor doc escaping tweak. |
src/s3/xml.rs |
XML writer construction tweak; replaces format! date formatting with manual buffer writes; adds tests. |
src/s3/router.rs |
Adds compound GET/PUT fast paths; changes range handling; adjusts multipart completion flow; more logging. |
src/s3/multipart.rs |
Refactors Default/new; adds unit tests for multipart store behavior. |
src/s3/headers.rs |
Changes RangeSpec::resolve API and refines HTTP date parsing; adds tests. |
src/main.rs |
Switches to local modules, adds slog!/serr! macros, initializes logging, adds --version, and routes logs through macros. |
src/log.rs |
New non-blocking logger implementation using a bounded channel + writer thread + ISO-8601 timestamps. |
src/lib.rs |
Exposes log and defines slog!/serr! macros for library consumers (benches/tests). |
src/crypto/ffi.rs |
Optimizes AES-CMAC inner loop to avoid per-block allocation. |
scripts/test-sccache.sh |
Adds stale-port cleanup and disables incremental builds; removes warm-cache assertions. |
scripts/test-sccache-spiceai.sh |
New extended integration test script for building a local spiceai repo through spiceio+sccache. |
README.md |
Expanded docs: highlights, quick start, sccache example, new config var, architecture/dev notes. |
Makefile |
Removes bench targets and adds test-extended. |
CLAUDE.md |
Updates dev docs/env vars; removes pre-PR checklist section. |
Cargo.toml |
Version bump to 0.3.0; adds s3_bench bench target. |
Cargo.lock |
Dependency updates consistent with version bump (e.g., hyper). |
benches/s3_bench.rs |
New Criterion bench suite for S3 XML/date/range helpers. |
benches/protocol_bench.rs |
Updates/expands SMB protocol benches for new encode/decode paths. |
benches/crypto_bench.rs |
Updates/expands crypto benches (adds sha512/aes-cmac benches, reorganizes groups). |
.gitignore |
Simplifies ignore list to only /target. |
.github/workflows/ci.yml |
Adds binary --version verification and stale-port cleanup; removes cargo bench --no-run. |
Comments suppressed due to low confidence (2)
src/s3/router.rs:585
- Range handling assumes
RangeSpec::resolvealways returns a valid (start,end) pair and then computescontent_length = end - start + 1. With the newresolvesignature this can underflow (e.g., empty file, orstart >= file_size) and you no longer return416 Range Not Satisfiable. Consider validating the resolved range here and returning 416 withContent-Range: bytes */{file_size}when unsatisfiable.
// Determine read range
let (start, end, is_range) = if let Some(ref range_str) = range_header {
if let Some(range) = parse_range(range_str) {
let (s, e) = range.resolve(file_size);
(s, e, true)
} else {
(0, file_size.saturating_sub(1), false)
}
} else {
(0, file_size.saturating_sub(1), false)
};
let content_length = end - start + 1;
// Build response with streaming body
src/s3/router.rs:305
- S3 ListObjects responses are expected to be returned in lexicographic key order. The previous in-handler sort was removed, but
ShareSession::list_objectsreturns entries in server enumeration order which is not guaranteed to be sorted. This can break marker/continuation-token semantics and produce inconsistent listings. Consider restoring sorting for bothobjectsandcommon_prefixesbefore applying pagination logic.
match result {
Ok((mut objects, common_prefixes)) => {
// Apply marker/start-after/continuation-token filtering
if !skip_marker.is_empty() {
objects.retain(|o| o.key.as_str() > skip_marker);
}
let truncated = objects.len() > max_keys;
let display_objects = if truncated {
&objects[..max_keys]
} else {
&objects
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Mask SMB username in startup log (CodeQL cleartext sensitive info) - Restore NtStatus::Unknown(u32) variant for unknown SMB status codes - Fix oplock level to 0x00 (SMB2_OPLOCK_LEVEL_NONE) - Fix decode_read_response to read DataOffset as u16 not u8 - Mask NetBIOS length to 24 bits in frame_packet and send_compound - Add bounds validation in parse_compound_response - Restore Option return on RangeSpec::resolve, guard total==0 - Validate multipart parts before removing upload state - Re-read SMB metadata after put_object for consistent ETag/mtime - Remove duplicate mod/macro declarations from main.rs, use lib crate - Restore sccache warm-build assertions and zero-stats reset - Check Close status in compound create_close
CodeQL traces taint from config.smb_username through any derived string. Replace the masked username with a fixed '****' literal so no sensitive data flows into the log path.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/s3/router.rs:300
- ListObjects responses are no longer sorted by key/common-prefix.
S3 ListObjects(V1/V2) requires lexicographic ordering; marker/continuation filtering and truncation (NextMarker / NextContinuationToken) assume this ordering as well. Since ShareSession::list_objects does not sort, removing the objects.sort_by(...) / common_prefixes.sort() here can lead to incorrect pagination and inconsistent results.
Consider restoring sorting in this handler, or sorting within ShareSession::list_objects so all callers get S3-consistent ordering.
match result {
Ok((mut objects, common_prefixes)) => {
// Apply marker/start-after/continuation-token filtering
if !skip_marker.is_empty() {
objects.retain(|o| o.key.as_str() > skip_marker);
}
let truncated = objects.len() > max_keys;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -70,24 +70,15 @@ impl RangeSpec { | |||
| if s >= total { | |||
| return None; | |||
| } | |||
| let end = e.min(total - 1); | |||
| if s > end { | |||
| return None; | |||
| } | |||
| Some((s, end)) | |||
| Some((s, e.min(total - 1))) | |||
| } | |||
| (Some(s), None) => { | |||
| if s >= total { | |||
| return None; | |||
| } | |||
| Some((s, total - 1)) | |||
| } | |||
| (None, Some(suffix)) => { | |||
| if suffix == 0 { | |||
| return None; | |||
| } | |||
| Some((total.saturating_sub(suffix), total - 1)) | |||
| } | |||
| (None, Some(suffix)) => Some((total.saturating_sub(suffix), total - 1)), | |||
| (None, None) => Some((0, total - 1)), | |||
| } | |||
There was a problem hiding this comment.
RangeSpec::resolve can return an invalid (start > end) range.
- For
bytes=start-end, ifend < start(e.g.bytes=10-0) this currently returnsSome((start, end))instead ofNone. - For suffix ranges,
bytes=-0resolves to(total, total-1).
This can later cause end - start + 1 underflow or incorrect Content-Range. Suggest restoring the explicit unsatisfiable checks (e.g., return None when end < start or suffix == 0).
| return error_response( | ||
| StatusCode::RANGE_NOT_SATISFIABLE, | ||
| "InvalidRange", | ||
| "The requested range is not satisfiable", | ||
| ); |
There was a problem hiding this comment.
For 416 Range Not Satisfiable, the previous implementation set Content-Range: bytes */{file_size}. The new error_response(...) path does not include this header, which breaks RFC 7233 semantics and can confuse S3 clients expecting Content-Range on 416.
Consider keeping the XML error body, but add the Content-Range header (and ideally include the total size) for 416 responses.
| return error_response( | |
| StatusCode::RANGE_NOT_SATISFIABLE, | |
| "InvalidRange", | |
| "The requested range is not satisfiable", | |
| ); | |
| let mut resp = error_response( | |
| StatusCode::RANGE_NOT_SATISFIABLE, | |
| "InvalidRange", | |
| "The requested range is not satisfiable", | |
| )?; | |
| if let Ok(value) = | |
| http::HeaderValue::from_str(&format!("bytes */{}", file_size)) | |
| { | |
| resp.headers_mut().insert(http::header::CONTENT_RANGE, value); | |
| } | |
| return Ok(resp); |
| pub fn decode_close_response(body: &[u8]) -> Option<CloseResponse> { | ||
| // Layout: StructureSize(2) + Flags(2) + Reserved(4) + CreationTime(8) | ||
| // + LastAccessTime(8) + LastWriteTime(8) + ChangeTime(8) | ||
| // + AllocationSize(8) + EndOfFile(8) + FileAttributes(4) = 60 bytes | ||
| if body.len() < 56 { | ||
| return None; | ||
| } | ||
| let last_write_time = u64::from_le_bytes(body[24..32].try_into().unwrap()); | ||
| let file_size = u64::from_le_bytes(body[48..56].try_into().unwrap()); | ||
| Some(CloseResponse { |
There was a problem hiding this comment.
decode_close_response comment documents a 60-byte CLOSE response, but the length check accepts 56 bytes. This can cause truncated responses to be treated as valid and produce incorrect metadata.
If you rely on the postquery attribute layout, consider requiring body.len() >= 60 (or at least validating all referenced fields exist per the documented layout).
| // Check Create responses (every other response) | ||
| for i in (0..responses.len()).step_by(2) { | ||
| let status = NtStatus::from_u32(responses[i].0.status); | ||
| if status.is_error() { | ||
| return Err(smb_status_to_io_error(responses[i].0.status, &dirs[i / 2])); |
There was a problem hiding this comment.
ensure_dirs treats any status with severity=error as fatal. For CreateDisposition::OpenIf, some SMB servers can return STATUS_OBJECT_NAME_COLLISION to indicate the directory already existed (mapped to io::ErrorKind::AlreadyExists in smb_status_to_io_error).
Since ensure_parent_dirs previously explicitly ignored AlreadyExists, this compound path may now fail when parent directories are already present. Consider treating STATUS_OBJECT_NAME_COLLISION (or AlreadyExists) as success for directory creation in ensure_dirs.
| // Check Create responses (every other response) | |
| for i in (0..responses.len()).step_by(2) { | |
| let status = NtStatus::from_u32(responses[i].0.status); | |
| if status.is_error() { | |
| return Err(smb_status_to_io_error(responses[i].0.status, &dirs[i / 2])); | |
| // Check Create responses (every other response). For OpenIf on directories, | |
| // some SMB servers report an existing directory as STATUS_OBJECT_NAME_COLLISION, | |
| // which maps to io::ErrorKind::AlreadyExists and should be treated as success. | |
| for i in (0..responses.len()).step_by(2) { | |
| let status = NtStatus::from_u32(responses[i].0.status); | |
| if status.is_error() { | |
| let err = smb_status_to_io_error(responses[i].0.status, &dirs[i / 2]); | |
| if err.kind() != io::ErrorKind::AlreadyExists { | |
| return Err(err); | |
| } |
| # ── Kill stale listener on our port ─────────────────────────────────────── | ||
| BIND_PORT="${BIND##*:}" | ||
| STALE_PID=$(lsof -i ":${BIND_PORT}" -sTCP:LISTEN -t 2>/dev/null || true) | ||
| if [[ -n "$STALE_PID" ]]; then | ||
| echo "[test] port ${BIND_PORT} already in use (pid ${STALE_PID}), killing..." | ||
| kill "$STALE_PID" 2>/dev/null || true | ||
| sleep 1 |
There was a problem hiding this comment.
This test forcibly kills any process listening on the configured bind port via lsof/kill. On developer machines, this can terminate unrelated services that happen to use the same port.
Consider making this opt-in (e.g. SPICEIO_KILL_STALE=1), or only killing processes that match the expected binary name/command line, or failing with a clear message asking the user to free the port.
| # ── Kill stale listener on our port ─────────────────────────────────────── | |
| BIND_PORT="${BIND##*:}" | |
| STALE_PID=$(lsof -i ":${BIND_PORT}" -sTCP:LISTEN -t 2>/dev/null || true) | |
| if [[ -n "$STALE_PID" ]]; then | |
| echo "[test] port ${BIND_PORT} already in use (pid ${STALE_PID}), killing..." | |
| kill "$STALE_PID" 2>/dev/null || true | |
| sleep 1 | |
| # ── Handle stale listener on our port safely ────────────────────────────── | |
| BIND_PORT="${BIND##*:}" | |
| STALE_PID=$(lsof -i ":${BIND_PORT}" -sTCP:LISTEN -t 2>/dev/null || true) | |
| if [[ -n "$STALE_PID" ]]; then | |
| if [[ "${SPICEIO_KILL_STALE:-0}" == "1" ]]; then | |
| echo "[test] port ${BIND_PORT} already in use (pid ${STALE_PID}), killing because SPICEIO_KILL_STALE=1..." | |
| kill "$STALE_PID" 2>/dev/null || true | |
| sleep 1 | |
| else | |
| echo "[test] ERROR: port ${BIND_PORT} is already in use (pid ${STALE_PID})." | |
| echo "[test] Refusing to kill an existing listener automatically." | |
| echo "[test] Free the port, set SPICEIO_BIND to a different address, or rerun with SPICEIO_KILL_STALE=1 to opt in." | |
| exit 1 | |
| fi |
Summary
Brings trunk up to date with all work on main through v0.3.0, as a single commit (no merge commits).
Includes:
slog!/serr!macros, ISO-8601 timestamps (Add non-blocking file logging and stderr error logging #7)Test plan
make lintpasses (fmt, check, clippy, rustdoc)cargo test --locked— 200/200 tests pass