110 add config for default sizes of query directory and similar ops #141
Conversation
- `ReceiveOptions`: add `async_cancel` & `async_msg_ids` - Migrate CLI watch to use new APIs - Test new and old APIs, Async+Mtd
- add integration tests for query file info/fs info - verify file_quota_information basic size input valid before sending request - add UT for fs info (fscc) - add smb-tests crates for common test logic - add missing Status options
WalkthroughThis PR adds extensive SMB protocol support features including new FSCC information classes and directory structures, introduces async message cancellation infrastructure, expands resource query APIs with optional buffer sizing, replaces the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Worker as Worker (receive_next_cancellable)
participant Cancel as Cancellation Signal
participant RecvLoop as Receive Loop
App->>Worker: receive_next_cancellable(options)
Note over Worker: Set async_msg_ids from options
alt feature = "async"
Worker->>Worker: tokio::select!
par
Worker->>RecvLoop: receive_next()
Note over RecvLoop: Wait for message
RecvLoop-->>Worker: IncomingMessage or Error
and
Worker->>Cancel: Wait on cancellation
Note over Cancel: Monitor CancellationToken
Cancel-->>Worker: Cancelled signal
end
alt Cancellation fires first
Worker-->>App: Err(Cancelled("receive_next"))
else Message received first
Worker-->>App: Ok(IncomingMessage) or Err(...)
end
else feature != "async"
Worker->>Worker: Check async_cancel flag
alt Atomic flag is set
Worker-->>App: Err(Cancelled("receive_next"))
else Flag clear
Worker->>RecvLoop: receive_next()
RecvLoop-->>Worker: Result
Worker-->>App: Result
end
end
sequenceDiagram
participant Dir as Directory
participant App as Application
participant QueryMgr as Query Manager
participant SMB as SMB Connection
App->>Dir: query_fs_info_with_options(Some(4096))
Note over Dir: Provide optional output_buffer_length
Dir->>QueryMgr: query_common(output_buffer_length)
Note over QueryMgr: Calculate transaction size<br/>respecting max negotiated size
QueryMgr->>SMB: Send QueryInfo request
alt Response: Success
SMB-->>QueryMgr: FileSystemInfo data
QueryMgr-->>Dir: Parsed result
Dir-->>App: Ok(T: QueryFileSystemInfoValue)
else Response: BufferOverflow
SMB-->>QueryMgr: Status with required_size
QueryMgr->>QueryMgr: Emit BufferTooSmall error<br/>with type & required size info
QueryMgr-->>Dir: Err(BufferTooSmall {...})
Dir-->>App: Err(BufferTooSmall {...})
else Response: Other error
SMB-->>QueryMgr: Status error
QueryMgr-->>Dir: Err(status)
Dir-->>App: Err(status)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: This PR introduces substantial new FSCC information structures (20+ new structs/enums across filesystem, directory, and query domains), async cancellation infrastructure with feature-gated implementations, a new test utilities crate with public macros, expansive resource API modifications with optional buffer sizing, and heterogeneous changes including macro path replacements, error handling refactors, and CLI enhancements. While some changes are repetitive (macro path swaps), the density of new public APIs, the complexity of cancellation handling across async/non-async builds, and the breadth of FSCC modeling demand careful review of type correctness, API usability, and control-flow interactions. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/smb/src/client/smb_client.rs (1)
774-793: Resilience: also fall back to next DFS referral on file creation failureCurrently, you only continue to the next referral when
_share_connectfails. If_create_filefails (e.g., path not covered on that target, transient error), the?returns immediately, skipping remaining referrals.Consider continuing on failure for selected errors (or all, to be simple), and only after exhausting all refs, return
Error::DfsError.Example change:
- let resource = self - .client - ._create_file(ref_unc_path, args) - .await - .map_err(|e| { - log::error!("Failed to create file on DFS referral: {e}",); - e - })?; - log::info!("Successfully created file on DFS referral: {ref_unc_path}",); - return Ok(resource); + match self.client._create_file(ref_unc_path, args).await { + Ok(resource) => { + log::info!("Successfully created file on DFS referral: {ref_unc_path}"); + return Ok(resource); + } + Err(e) => { + log::error!("Failed to create file on DFS referral {ref_unc_path}: {e}"); + // Optional: filter which errors should continue vs return early. + continue; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.github/workflows/build.yml(1 hunks)Cargo.toml(1 hunks)crates/smb-cli/Cargo.toml(1 hunks)crates/smb-cli/src/watch.rs(3 hunks)crates/smb-dtyp/src/guid.rs(1 hunks)crates/smb-dtyp/src/security/sid.rs(1 hunks)crates/smb-fscc/Cargo.toml(1 hunks)crates/smb-fscc/src/filesystem_info.rs(5 hunks)crates/smb-fscc/src/quota.rs(1 hunks)crates/smb-msg/src/error.rs(1 hunks)crates/smb-msg/src/header.rs(4 hunks)crates/smb-msg/src/lib.rs(1 hunks)crates/smb-msg/src/plain.rs(2 hunks)crates/smb-tests/Cargo.toml(1 hunks)crates/smb-tests/README.md(1 hunks)crates/smb-tests/src/binrw.rs(1 hunks)crates/smb-tests/src/lib.rs(1 hunks)crates/smb/Cargo.toml(2 hunks)crates/smb/src/client/smb_client.rs(1 hunks)crates/smb/src/connection.rs(1 hunks)crates/smb/src/connection/config.rs(3 hunks)crates/smb/src/connection/worker/parallel/threading_backend.rs(1 hunks)crates/smb/src/connection/worker/worker_trait.rs(2 hunks)crates/smb/src/error.rs(4 hunks)crates/smb/src/lib.rs(1 hunks)crates/smb/src/msg_handler.rs(6 hunks)crates/smb/src/resource.rs(10 hunks)crates/smb/src/resource/directory.rs(7 hunks)crates/smb/src/tree.rs(1 hunks)crates/smb/tests/common.rs(1 hunks)crates/smb/tests/info_query.rs(1 hunks)crates/smb/tests/notify.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
crates/smb/src/lib.rs (1)
crates/smb/src/resource/pipe.rs (1)
PipeRpcConnection(25-25)
crates/smb-msg/src/plain.rs (1)
crates/smb/src/resource/directory.rs (1)
cancel(861-879)
crates/smb-cli/src/watch.rs (2)
crates/smb/src/resource/directory.rs (9)
new(26-33)new(656-683)new(777-790)new(845-853)new(923-996)watch_stream_cancellable(278-402)watch_stream(269-275)watch_stream(405-414)watch_stream(417-427)crates/smb-msg/src/notify.rs (1)
all(61-75)
crates/smb/src/msg_handler.rs (1)
crates/smb/src/resource/directory.rs (6)
std(555-555)new(26-33)new(656-683)new(777-790)new(845-853)new(923-996)
crates/smb/tests/info_query.rs (2)
crates/smb/src/resource.rs (1)
make_create_new(54-61)crates/smb/tests/common.rs (1)
make_server_connection(36-48)
crates/smb/src/resource.rs (1)
crates/smb/src/msg_handler.rs (7)
new(33-43)new(75-77)new(93-100)new(201-203)new(386-390)default(135-140)default(249-260)
crates/smb/tests/notify.rs (4)
crates/smb/src/resource.rs (5)
std(483-483)std(521-521)std(713-713)make_create_new(54-61)new(872-876)crates/smb/src/resource/directory.rs (9)
std(555-555)new(26-33)new(656-683)new(777-790)new(845-853)new(923-996)watch_stream(269-275)watch_stream(405-414)watch_stream(417-427)crates/smb/tests/common.rs (1)
make_server_connection(36-48)crates/smb-msg/src/notify.rs (1)
all(61-75)
crates/smb/src/resource/directory.rs (2)
crates/smb/src/resource.rs (6)
new(872-876)send_cancel(842-851)std(483-483)std(521-521)std(713-713)file_id(306-314)crates/smb/src/msg_handler.rs (8)
new(33-43)new(75-77)new(93-100)new(201-203)new(386-390)default(135-140)default(249-260)send(299-301)
crates/smb-fscc/src/quota.rs (2)
crates/smb-dtyp/src/guid.rs (1)
size_of(16-16)crates/smb-dtyp/src/binrw_util/helpers.rs (1)
size_of(92-92)
crates/smb/src/error.rs (1)
crates/smb/src/resource/directory.rs (1)
from(601-613)
⏰ 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: test (async)
- GitHub Check: test (single_threaded)
🔇 Additional comments (33)
crates/smb-msg/src/plain.rs (2)
174-180: LGTM!The
From<cancel::CancelRequest>implementation correctly follows the established pattern for other request types and enables ergonomic conversion using.into(). SinceCancelis a special case (not macro-generated), this manual implementation is appropriate and consistent with the codebase.
279-279: LGTM!The comment accurately clarifies the default initialization semantics: the header defaults to a synchronous command state (non-async flag, tree_id placeholder). This is helpful for understanding the constructor's behavior.
crates/smb-dtyp/src/security/sid.rs (1)
39-42: LGTM! Correct minimum size calculation.The MIN_SIZE constant correctly calculates the minimum serialized size of a SID structure (8 bytes total: 1 byte Revision + 1 byte SubAuthorityCount + 6 bytes IdentifierAuthority). This represents a SID with zero sub-authorities.
crates/smb-fscc/src/quota.rs (1)
30-38: LGTM! Correct minimum size calculation.The MIN_SIZE constant correctly calculates the minimum serialized size of FileQuotaInformation by summing the sizes of all fixed fields plus SID::MIN_SIZE. The calculation accurately reflects the struct's binary layout.
crates/smb/src/connection/config.rs (3)
155-160: LGTM!The documentation clearly lists all affected operations. The field type
Option<u32>is appropriate for buffer sizes in bytes.
188-191: LGTM!The documentation update improves clarity by explicitly referencing the
timeoutfield.
193-194: Increase default transaction buffer size or document rationale
- 4 KB (0x1024) default for
default_transaction_sizecan fragment large directory listings and security descriptor queries; consider raising it to 32 KB (0x8000) or 64 KB (0x10000).- If 4 KB is an intentional constraint, add a comment explaining the trade-off.
Cargo.toml (1)
36-37: LGTM! Workspace dependencies added to support async utilities.The addition of
tokio-streamandfuturesas workspace dependencies aligns with the async streaming and cancellation features introduced in this PR. The version specifications (0.1 and 0.3) appropriately allow compatible updates within the minor version range.Based on learnings: tokio-stream 0.1.17 and futures 0.3.31 are the current maintenance releases in their respective series.
crates/smb/src/connection/worker/parallel/threading_backend.rs (1)
54-59: LGTM! Enhanced cleanup properly notifies awaiting tasks.The improved cleanup logic now drains pending tasks from
state.awaitingand notifies each one withError::ConnectionStopped, ensuring graceful termination. This complements the cancellation-aware flows introduced elsewhere in the PR.crates/smb-tests/src/binrw.rs (1)
14-37: LGTM! Well-designed test macro for BinWrite.The
test_binrw_writemacro provides a clean DSL for generating write round-trip tests. The use ofpaste!for generating snake_case test names is appropriate.crates/smb-fscc/src/filesystem_info.rs (5)
31-31: LGTM! Automatic length calculation for serialization.The
#[bw(calc = file_system_name.len() as u32)]attribute ensures the length field is automatically computed during serialization, eliminating potential inconsistencies.
214-214: LGTM! Automatic length calculation for serialization.Similar to the file system name, this
calcattribute ensuresvolume_label_lengthis automatically derived during serialization.
102-114: LGTM! Enhanced documentation improves clarity.The added documentation for
FsDeviceCharacteristicsfields provides valuable context about removable media, remote file systems, and virtual volumes.
155-166: LGTM! Well-documented new struct.
FileFsFullSizeInformationis properly documented with an MS-FSCC reference and includes all necessary fields for querying sector size information.
224-305: LGTM! Comprehensive test coverage for filesystem info structs.The new test module leverages the
smb-testsutilities to provide binary round-trip testing for multipleFileFs*structures. The tests cover key scenarios including volume information, size queries, device info, and attributes.crates/smb-tests/src/lib.rs (1)
1-1: LGTM! Appropriate module exposure.Declaring
pub mod binrw;correctly exposes the test utilities for use by other crates in the workspace.crates/smb/Cargo.toml (1)
25-33: Retain thefuturesmeta-crate dependency
Thefuturesmeta-crate is used in crates/smb-cli/src/watch.rs (e.g.,use futures::StreamExt,futures::future::ready); remove it only if you replace these calls withfutures-core/futures-utilequivalents.Likely an incorrect or invalid review comment.
crates/smb-cli/Cargo.toml (1)
16-16: Enable ctrlc termination feature
- In crates/smb-cli/Cargo.toml, update the
ctrlcdependency tofeatures = ["termination"]for SIGTERM/SIGHUP handling.Likely an incorrect or invalid review comment.
crates/smb/src/client/smb_client.rs (1)
792-792: DFS error variant rename is consistent
Renaming toError::DfsErroris applied across the error definition and call sites; no references to the old variant remain.crates/smb/src/msg_handler.rs (3)
154-158: ReceiveOptions: builder ergonomics and docs LGTMClone derive and docs clarifying allow_async and async paths look good.
Also applies to: 168-176
180-189: Cancellation field shape is appropriate per cfgUsing CancellationToken for async and Arc for non-async keeps APIs clean. No issues.
190-194: async_msg_ids wiring is soundBuilder and Default initialization are correct. This will enable safe cross-thread cancel/send-cancel flows.
Also applies to: 225-241, 248-258
crates/smb-msg/src/header.rs (2)
120-165: Status enum updates are correctNew variants and the SharingViolation rename look right. Hex values match NTSTATUS codes.
If you want, I can scan repo/tests to update any remaining usages of the old SharingViloation spelling.
210-217: Header::to_async maintains header invariantsSetting async_command, clearing tree_id, and setting async_id is the correct transition.
crates/smb/src/connection/worker/worker_trait.rs (3)
45-64: Async cancellation path is correct and non-leakytokio::select! cancellation branch is biased and drops the in-flight future; Tokio IO is drop-cancellable, so this is fine.
66-82: Non-async path: best-effort pre-check is reasonableGiven lack of async cancellation, upfront flag check is acceptable. Documented behavior is clear.
127-133: async_msg_ids set before loop; receive uses cancellable versionOrder is correct and prevents races for send_cancel lookups. Good change.
crates/smb/src/error.rs (2)
31-33: Cancelled as &'static str aligns with call sitesThis reduces allocations and matches literals used across the codebase.
41-47: New variants improve error fidelityNegotiationError, SspiError, BufferTooSmall, and DfsError additions are appropriate and already exercised by resource flows.
Also applies to: 60-68, 69-75, 107-109
crates/smb-cli/src/watch.rs (4)
16-19: New --number option is a nice UX additionLimiting events via take(number) is straightforward and efficient.
41-45: Resource conversion and cleanup are correcttry_into() check for Directory type and explicit close of dir and client look good.
Also applies to: 47-58
60-94: Async watch flow: cancellation and streaming are well-structuredCancellationToken + ctrlc handler, use of take(number), and per-item logging are solid.
96-126: Non-async watch flow is consistentIterator-based stream with a cancellable handle and ctrlc handler is implemented cleanly.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
crates/smb-fscc/src/set_file_info.rs (1)
139-150: Expose a way to constructFileShortNameInformation.With
innerkept private, callers outside this module cannot build aFileShortNameInformationvalue, so the new info-class is unusable for write operations. Either make the field public or add a public constructor/newtype wrapper that lets users supply aFileNameInformation.crates/smb-msg/src/header.rs (1)
120-165: Add deprecated alias forSharingViloation
The rename fromSharingViloationtoSharingViolationis a public breaking change—add this to the existingimpl Status:impl Status { #[deprecated(note = "Renamed to `SharingViolation`")] pub const SharingViloation: Status = Status::SharingViolation; }crates/smb/src/resource.rs (1)
587-596: Fix docs: parameter list out of dateDocs mention
max_input_responsewhich no longer exists.- /// * `max_input_response` - The maximum input response size. - /// * `max_output_response` - The maximum output response size. + /// * `max_output_response` - The maximum output response size.crates/smb/src/connection/worker/worker_trait.rs (1)
93-99: Consider making the initial receive cancellable as wellreceive() calls receive_next for the first response, ignoring cancellation until after Pending. Using receive_next_cancellable here would allow cancelling earlier.
Apply:
- let curr = self.receive_next(options).await?; + let curr = self.receive_next_cancellable(options).await?;Also applies to: 127-133
crates/smb-fscc/src/common_info.rs (1)
41-57: EA name parsing is incorrect (not null-terminated per spec)
ea_nameis declaredNullString, but MS-FSCC states the EA name is not null-terminated and its length isea_name_length. This will misparse EA names that are not NUL-terminated.Read exactly
ea_name_lengthbytes instead.pub struct FileFullEaInformationInner { @@ - #[bw(try_calc = ea_name.len().try_into())] + #[bw(try_calc = ea_name.len().try_into())] ea_name_length: u8, @@ - /// The name of the extended attribute. This field is not null-terminated. - #[br(assert(ea_name.len() == ea_name_length as usize))] - pub ea_name: NullString, + /// The name of the extended attribute. This field is not null-terminated. + #[br(count = ea_name_length as usize)] + pub ea_name: Vec<u8>,Based on learnings
crates/smb/src/resource/directory.rs (1)
670-691: Race: fetch loop starts before acquiringquery_lock
QueryDirectoryStream::newspawnsfetch_loopbefore takingquery_lock, allowing a brief overlap with other queries. Acquire the lock first, then spawn.pub async fn new( directory: &'a Arc<Directory>, pattern: String, buffer_size: u32, ) -> crate::Result<Self> { let (sender, receiver) = tokio::sync::mpsc::channel(1024); let notify_fetch_next = Arc::new(tokio::sync::Notify::new()); - { - let notify_fetch_next = notify_fetch_next.clone(); - let directory = directory.clone(); - tokio::spawn(async move { - Self::fetch_loop( - directory, - pattern, - buffer_size, - sender, - notify_fetch_next.clone(), - ) - .await; - }); - } - let guard = directory.query_lock.lock().await?; + let guard = directory.query_lock.lock().await?; + { + let notify_fetch_next = notify_fetch_next.clone(); + let directory = directory.clone(); + tokio::spawn(async move { + Self::fetch_loop( + directory, + pattern, + buffer_size, + sender, + notify_fetch_next.clone(), + ) + .await; + }); + } Ok(Self { receiver, notify_fetch_next, _lock_guard: guard, }) }
♻️ Duplicate comments (8)
crates/smb-tests/README.md (1)
3-3: Fix typo: "testsing" → "testing".The word "testsing" should be "testing".
Apply this diff:
-This crate contains utilities for testsing the `smb-rs` crate. +This crate contains utilities for testing the `smb-rs` crate..github/workflows/build.yml (1)
85-87: Typo in comment has been flagged.The typo "prvent" → "prevent" was already identified in a previous review.
crates/smb-fscc/Cargo.toml (1)
23-24: Path dev-dependency concern already flagged.The previous review already identified that the path-based dev-dependency on
smb-testswill block publishing. Sincesmb-testsis being published (see.github/workflows/build.yml:123), this should use a version constraint instead of a path.crates/smb/tests/notify.rs (1)
11-14: Fix sleep import/await mismatch across features (use a helper)Current dual imports plus unconditional
.awaitcan fail under some feature combos and in sync builds. Replace with amaybe_sleephelper to unify async/sync.Apply:
-#[cfg(feature = "multi_threaded")] -use std::thread::sleep; -#[cfg(feature = "async")] -use tokio::time::sleep; +// use a helper instead of importing conflicting sleep symbols- sleep(std::time::Duration::from_secs(2)).await; + maybe_sleep(std::time::Duration::from_secs(2)).await;Add helpers (outside shown ranges, e.g., near other helpers):
#[maybe_async::async_impl] async fn maybe_sleep(d: std::time::Duration) { tokio::time::sleep(d).await } #[maybe_async::sync_impl] fn maybe_sleep(d: std::time::Duration) { std::thread::sleep(d) }Also applies to: 94-94
crates/smb/src/tree.rs (1)
314-319: Critical: Async CANCEL requests require TreeId and encryption.The conditional guard
if !msg.message.header.flags.async_command()prevents TreeId assignment and encryption flag setting for async commands. However,SMB2_FLAGS_ASYNC_COMMANDis set on client CANCEL requests, which still require:
- The original TreeId (per MS-SMB2 specification)
- Encryption when the share mandates it (MS-SMB2 3.3.4.4 & 3.3.5.11)
The current implementation will send unencrypted CANCEL requests without TreeId, causing servers that mandate encryption to drop them.
Apply this diff to ensure CANCEL/async requests maintain TreeId and encryption:
- if !msg.message.header.flags.async_command() { - msg.message.header.tree_id = self.tree_id.load(Ordering::SeqCst).into(); - if self.info.share_flags.encrypt_data() { - msg.encrypt = true; - } - } + msg.message.header.tree_id = self.tree_id.load(Ordering::SeqCst).into(); + if self.info.share_flags.encrypt_data() { + msg.encrypt = true; + }crates/smb-cli/src/watch.rs (1)
4-5: Compile blockers: missing imports (Arc, CancellationToken)Arc and CancellationToken are used unqualified later.
Apply:
use maybe_async::*; use smb::{Client, DirAccessMask, NotifyFilter, UncPath, resource::*, sync_helpers::*}; use std::error::Error; +use std::sync::Arc; +#[cfg(feature = "async")] +use tokio_util::sync::CancellationToken;crates/smb/src/resource/directory.rs (2)
303-325: Reset async message IDs between watch attempts (avoid stale IDs)The async receive loop reuses
receive_optionsacross iterations without resettingasync_msg_ids, risking stale ID reuse. Reset after each_watch_optionscompletes (mirrors the multi_threaded path).result = directory ._watch_options(filter, recursive, receive_options.clone()) => { let should_stop = matches!(result, DirectoryWatchResult::Cancelled | DirectoryWatchResult::Cleanup); + // Reset async msg ids before next watch attempt + if let Some(ids) = receive_options.async_msg_ids.as_ref() { + ids.reset(); + } if watch_tx.send(result).await.is_err() { break; // Receiver dropped } if should_stop { break; } }
371-379: Surface Cleanup as an error on the stream (consistency with From)On
Cleanup, the stream silently stops. Convert it into a cancellation error so downstream sees the terminal condition (matches the multi_threaded path andFrom<DirectoryWatchResult>mapping).- Some(DirectoryWatchResult::Cleanup) => { - // Server cleaned up the watch, exit the loop. - log::debug!("Watch cleaned up by server. Stopping stream."); - break; - } + Some(DirectoryWatchResult::Cleanup) => { + log::debug!("Watch cleaned up by server. Stopping stream with error."); + let _ = sender + .send(Err(Error::Cancelled("watch cleaned up by server"))) + .await; + break; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
.github/workflows/build.yml(2 hunks)Cargo.toml(2 hunks)crates/smb-cli/src/watch.rs(3 hunks)crates/smb-dtyp/Cargo.toml(1 hunks)crates/smb-dtyp/src/guid.rs(3 hunks)crates/smb-dtyp/src/security/ace.rs(3 hunks)crates/smb-fscc/Cargo.toml(1 hunks)crates/smb-fscc/src/acces_masks.rs(1 hunks)crates/smb-fscc/src/chained_list.rs(1 hunks)crates/smb-fscc/src/common_info.rs(2 hunks)crates/smb-fscc/src/directory_info.rs(2 hunks)crates/smb-fscc/src/error.rs(1 hunks)crates/smb-fscc/src/file_attributes.rs(1 hunks)crates/smb-fscc/src/filesystem_info.rs(7 hunks)crates/smb-fscc/src/info_classes.rs(1 hunks)crates/smb-fscc/src/lib.rs(1 hunks)crates/smb-fscc/src/notify.rs(1 hunks)crates/smb-fscc/src/query_file_info.rs(4 hunks)crates/smb-fscc/src/set_file_info.rs(4 hunks)crates/smb-msg/Cargo.toml(1 hunks)crates/smb-msg/src/create.rs(2 hunks)crates/smb-msg/src/dfsc.rs(1 hunks)crates/smb-msg/src/error.rs(1 hunks)crates/smb-msg/src/header.rs(5 hunks)crates/smb-msg/src/info/common.rs(1 hunks)crates/smb-msg/src/info/query.rs(1 hunks)crates/smb-msg/src/info/set.rs(1 hunks)crates/smb-msg/src/ioctl/msg.rs(1 hunks)crates/smb-msg/src/plain.rs(5 hunks)crates/smb-msg/src/tree_connect.rs(1 hunks)crates/smb-rpc/Cargo.toml(1 hunks)crates/smb-rpc/src/interface/srvsvc.rs(2 hunks)crates/smb-rpc/src/ndr64/consts.rs(1 hunks)crates/smb-rpc/src/pdu.rs(5 hunks)crates/smb-tests/README.md(1 hunks)crates/smb-tests/src/binrw.rs(1 hunks)crates/smb-transport/Cargo.toml(1 hunks)crates/smb/Cargo.toml(2 hunks)crates/smb/src/client/smb_client.rs(2 hunks)crates/smb/src/connection/config.rs(3 hunks)crates/smb/src/connection/worker/worker_trait.rs(2 hunks)crates/smb/src/resource.rs(10 hunks)crates/smb/src/resource/directory.rs(7 hunks)crates/smb/src/tree.rs(3 hunks)crates/smb/tests/basic.rs(1 hunks)crates/smb/tests/dialects.rs(2 hunks)crates/smb/tests/long_dir.rs(1 hunks)crates/smb/tests/notify.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
crates/smb-cli/src/watch.rs (1)
crates/smb/src/resource/directory.rs (9)
new(26-33)new(670-697)new(788-801)new(863-871)new(941-1014)watch_stream_cancellable(278-402)watch_stream(269-275)watch_stream(407-416)watch_stream(420-441)
crates/smb/tests/notify.rs (3)
crates/smb/src/resource/directory.rs (9)
std(569-569)new(26-33)new(670-697)new(788-801)new(863-871)new(941-1014)watch_stream(269-275)watch_stream(407-416)watch_stream(420-441)crates/smb/tests/common.rs (1)
make_server_connection(36-48)crates/smb-msg/src/notify.rs (1)
all(61-75)
crates/smb/src/resource/directory.rs (2)
crates/smb/src/resource.rs (6)
new(872-876)send_cancel(842-851)std(483-483)std(521-521)std(713-713)file_id(306-314)crates/smb/src/msg_handler.rs (8)
new(33-43)new(75-77)new(93-100)new(201-203)new(386-390)default(135-140)default(249-260)send(299-301)
crates/smb/tests/dialects.rs (2)
crates/smb/tests/long_dir.rs (1)
Directory(88-88)crates/smb-cli/src/info.rs (2)
Directory(200-200)Directory(216-216)
crates/smb-fscc/src/directory_info.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (1)
size(17-19)
crates/smb-fscc/src/query_file_info.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (1)
size(17-19)
crates/smb-fscc/src/common_info.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (1)
size(17-19)
crates/smb-fscc/src/set_file_info.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (1)
size(17-19)
⏰ 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). (3)
- GitHub Check: test (single_threaded)
- GitHub Check: test (multi_threaded)
- GitHub Check: test (async)
🔇 Additional comments (52)
crates/smb/src/client/smb_client.rs (1)
790-790: Error variant rename complete – no remaining references
Search forDfsReferralConnectionFailreturned zero matches; the rename is fully applied.crates/smb-msg/src/header.rs (1)
86-93: Finalize paste → pastey migrationNo residual
paste::macro usages orpastedependency in Cargo.toml.crates/smb-tests/src/binrw.rs (2)
3-37: LGTM! Past recommendation already implemented.The
test_binrw_writemacro is well-designed for testing BinWrite implementations. The past review comment recommended migrating frompastetopastey, and this has already been implemented—the macro now usespastey::paste!(line 22).
39-73: LGTM! Well-designed test macro for BinRead.The
test_binrw_readmacro complements the write macro, enabling comprehensive binary round-trip testing with minimal boilerplate.crates/smb-dtyp/Cargo.toml (1)
18-18: LGTM! Dependency migration aligned with workspace-wide changes.The replacement of
pastewithpasteyis part of the workspace-wide migration to a maintained fork. Based on learnings,paste(v1.0.15) is archived/unmaintained, andpasteyis an actively maintained drop-in replacement.crates/smb-fscc/src/chained_list.rs (1)
6-6: LGTM! Cleanup of unnecessary blank doc comment.Removing the standalone
//!doc comment line is a minor cosmetic improvement with no functional impact.crates/smb-rpc/Cargo.toml (1)
20-20: LGTM! Dependency migration consistent with workspace changes.The replacement of
pastewithpasteyaligns with the workspace-wide migration to a maintained alternative. Based on learnings,pasteyis a drop-in successor to the archivedpastecrate.crates/smb-transport/Cargo.toml (1)
43-43: LGTM! Dependency migration aligned with workspace.The replacement of
pastewithpasteyis consistent with the workspace-wide migration. Based on learnings,pasteyis an actively maintained fork of the archivedpastecrate.crates/smb-msg/src/info/common.rs (1)
49-49: LGTM! Macro invocation updated to use pastey.The change from
paste::paste!topastey::paste!correctly reflects the dependency migration in Cargo.toml. Based on learnings,pasteymaintains API compatibility withpaste, making this a straightforward drop-in replacement.crates/smb-msg/Cargo.toml (1)
21-21: LGTM! Dependency migration supports macro usage.The replacement of
pastewithpasteyis consistent with the workspace-wide migration and enables the corresponding macro invocation change insrc/info/common.rs. Based on learnings,pasteyis a maintained drop-in replacement forpaste.Cargo.toml (2)
25-25: LGTM: Migration to maintained fork.The switch from
pastetopasteyaligns with best practices, aspasteis archived and has a security advisory (RUSTSEC-2024-0436). Thepasteyfork is actively maintained.Based on learnings.
36-37: LGTM: New async dependencies.Adding
tokio-streamandfuturesas workspace dependencies is appropriate for the expanded async/streaming functionality mentioned in the PR objectives..github/workflows/build.yml (1)
88-98: LGTM: Build step simplified.Removing
Cargo.lockbefore builds ensures dependency resolution uses the latest compatible versions, which is appropriate for library crates. The formatting alignment of cargo build commands improves readability.crates/smb-dtyp/src/guid.rs (2)
84-104: LGTM: Well-documented alias addition.The
make_guid!alias is properly documented with clear guidance on when to use it overguid!. The documentation explains its purpose for SMB filesystem contexts, addressing the previous review concern.
187-191: LGTM: Tests updated to use new alias.The tests consistently use
make_guid!instead ofguid!, demonstrating the preferred usage pattern documented above.crates/smb-msg/src/info/query.rs (1)
321-321: LGTM: Consistent with new GUID alias.The test correctly uses
make_guid!following the established pattern fromcrates/smb-dtyp/src/guid.rs.crates/smb-msg/src/info/set.rs (1)
118-118: LGTM: Consistent with new GUID alias.The test correctly uses
make_guid!in alignment with the codebase-wide convention.crates/smb-fscc/Cargo.toml (1)
20-20: LGTM: Dependency updated to maintained fork.The change from
pastetopasteyis consistent with the workspace-wide migration.crates/smb-msg/src/ioctl/msg.rs (1)
58-58: LGTM: Macro path updated for new dependency.The change from
paste::paste!topastey::paste!is consistent with the workspace-wide dependency migration. The macro functionality remains identical.crates/smb/Cargo.toml (3)
27-31: LGTM: New optional async dependencies.The
futuresandtokio-streamdependencies are correctly marked as optional and will be activated by theasyncfeature flag.
39-39: LGTM: Dependency updated to maintained fork.The change from
pastetopasteyis consistent with the workspace-wide migration to the maintained fork.
75-83: LGTM: Feature flag properly wired.The
asyncfeature correctly includes the new dependencies using thedep:syntax. The feature composition is clean and follows Cargo best practices.crates/smb-fscc/src/acces_masks.rs (2)
6-21: Access mask layouts look correct; bit‑parity dir↔file is preservedDefinitions map SMB2 bits 0–8 between file/dir forms; 7-bit reserved gap is fine given standard/generic rights live elsewhere via the macro. Consider adding a small round‑trip test to assert
DirAccessMask::from(FileAccessMask::from_bytes(x)).into_bytes()==xacross randomized masks.Also applies to: 23-38
40-52: Byte-wise conversions are appropriateUsing into_bytes/from_bytes leverages identical bit layouts; simple and correct.
crates/smb/tests/notify.rs (1)
141-145: Good: JoinHandle results are checkedAwaiting
join_alland asserting each handle prevents swallowed panics. LGTM.crates/smb/src/resource.rs (2)
316-335: calc_transact_size: sensible clamping and defaultsClamps to negotiated max and falls back to configured default; warning message is clear. LGTM.
840-851: send_cancel: correct message-id/async-id population and fire-and-forget sendApproach matches SMB CANCEL semantics. LGTM.
crates/smb-rpc/src/ndr64/consts.rs (1)
1-7: LGTM: guid macro swapmake_guid! use is consistent with updated import; no behavior change.
crates/smb-fscc/src/notify.rs (1)
1-3: LGTM!The module-level documentation clearly describes Directory Change Notifications and provides a useful reference to the MS-FSCC specification.
crates/smb-rpc/src/interface/srvsvc.rs (2)
4-4: LGTM!The import path change from
guidtomake_guidaligns with the workspace-wide macro namespace updates mentioned in the AI summary.
231-231: LGTM!The macro call update from
guid!tomake_guid!is consistent with the import change and maintains the same UUID value.crates/smb/tests/long_dir.rs (1)
4-4: LGTM!The import path change from
resource::Directoryto top-levelDirectorysimplifies the import and aligns with the public API reorganization mentioned in the AI summary.crates/smb/tests/basic.rs (1)
161-161: LGTM!The migration from
paste::paste!topastey::paste!aligns with the workspace-wide dependency change. According to the learnings, pastey is a maintained fork of paste that provides drop-in compatibility with additional features.Based on learnings
crates/smb/tests/dialects.rs (2)
8-8: LGTM!The import path change from
resource::Directoryto top-levelDirectoryis consistent with the API reorganization seen in other test files.
18-18: LGTM!The migration from
paste::paste!topastey::paste!is consistent with the workspace-wide macro dependency change.Based on learnings
crates/smb-fscc/src/error.rs (2)
3-3: LGTM!The module-level documentation improvement provides better clarity by explicitly stating these are errors specific to the smb-fscc crate.
6-6: LGTM!The added documentation for
UnexpectedInformationTypeclearly explains when this error is returned and what it means in the context of type conversions.crates/smb-rpc/src/pdu.rs (5)
9-9: LGTM!The import path change from
guidtomake_guidaligns with the workspace-wide macro namespace updates.
13-14: LGTM!The macro call update from
guid!tomake_guid!is consistent with the import change and maintains the same UUID value forBIND_TIME_NEGOTIATION.
24-24: LGTM!The migration from
paste::paste!topastey::paste!is consistent with the workspace-wide macro dependency change. This affects the code generation for RPC packet types.Based on learnings
366-396: LGTM!The test code consistently uses
make_guid!instead ofguid!for UUID construction, maintaining consistency with the updated macro namespace throughout the file.
462-462: LGTM!The test assertion correctly uses
make_guid!for UUID construction, consistent with the updated macro namespace.crates/smb/src/tree.rs (2)
7-7: LGTM!The import addition of
FileCreateArgsis appropriate for the module's functionality.
17-17: LGTM!The import addition of
Resourceis appropriate for the module's functionality.crates/smb-dtyp/src/security/ace.rs (3)
28-31: Bitfield↔bytes mapping: verify on‑wire endiannessUsing modular_bitfield with BinRead/BinWrite via into_bytes/from_bytes is fine, but please verify the produced byte order matches SMB’s little‑endian u32 layout for access masks. Add a golden test that round‑trips known masks (e.g., generic_read = 0x8000_0000, delete = 0x0001_0000) to ensure correct bit positions.
Also applies to: 45-52
92-92: paste → pastey migration looks goodMacro path updated to pastey::paste!, no functional change expected.
196-201: Rename fieldcrate_childtocreate_childaccess_mask! { pub struct ObjectAccessMask { - crate_child: bool, + create_child: bool,crates/smb-cli/src/watch.rs (1)
78-91: Async watch flow looks correctCancellable stream with take(number) and per‑event logging is appropriate.
Ensure tokio-util is enabled in this binary’s feature set to satisfy CancellationToken usage.
crates/smb/src/connection/worker/worker_trait.rs (1)
45-63: Cancellable receive helper: solid additionAsync variant races on CancellationToken; non‑async checks atomic flag. Good defaults when no cancel source is provided.
Also applies to: 65-82
crates/smb-fscc/src/query_file_info.rs (1)
49-58: FSCC info types expansion: looks correctBinrw annotations and field layouts match spec expectations at a glance.
Add a few binrw round‑trip tests for:
- FileAllInformation (composed fields)
- FileStreamInformationInner (name length calc)
- FileNetworkOpenInformation (attributes mapping)
Also applies to: 59-84, 85-113, 114-123, 144-175, 176-185, 186-197, 198-207, 208-231, 232-243, 252-276, 278-301, 302-316, 317-327, 328-339, 340-359, 361-377, 378-389
crates/smb-fscc/src/file_attributes.rs (1)
12-97: Bit layout/endianness: add golden testsThe 32‑bit field count sums to 32; mapping via into_bytes/from_bytes is fine. Please add tests asserting specific flags produce expected little‑endian u32 values (e.g., directory == 0x0000_0010).
crates/smb/src/resource/directory.rs (1)
425-441: Single-threaded iterator: ensure_watch_optionsis sync-capableThis path calls
_watch_options(...).into()from sync code. With the#[maybe_async]fix on_watch_options, this compiles and behaves correctly (sync). Without it, this fails to compile. Please add that annotation as above.
afiffon
force-pushed
the
110-add-config-for-default-sizes-of-query-directory-and-similar-ops
branch
from
bc790fb to
61d39a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/smb-msg/src/info/set.rs (1)
65-91: Avoid panicking in library API: make SetInfoData::to_req fallibleReplace the panic on mismatch with a Result-returning API (or add a new try_to_req), so callers can handle invalid class/data combinations without aborting.
Example direction:
- Change signature to
pub fn try_to_req(self, info_class: SetInfoClass, file_id: FileId, additional_info: AdditionalInfo) -> Result<SetInfoRequest, SetInfoError>.- Return a descriptive error on mismatch instead of
panic!.crates/smb-msg/src/dfsc.rs (1)
96-104: Docstring typo: “target_failbacl” → “target_failback”Minor docs fix.
- /// Whether DFS client target failback is enabled for all targets in this referral response. This value used only in version 4. - pub target_failbacl: bool, + /// Whether DFS client target failback is enabled for all targets in this referral response. This value is used only in version 4. + pub target_failback: bool,crates/smb-fscc/src/set_file_info.rs (1)
48-66: Defaulting delete_pending = true can be a footgun.Constructing
FileDispositionInformation::default()schedules deletion. ConsiderDefault= false to be safer, and provide::delete()helper for the true case. If you keep it true, at least document prominently in the type-level docs.crates/smb-fscc/src/common_info.rs (1)
45-49: Avoid silent truncation of EA value length.
as u16will truncate lengths > 65535. Usetry_into()withtry_calcto error early.Apply:
- #[bw(calc = match ea_value { - Some(v) => v.len() as u16, - None => 0 - })] - ea_value_length: u16, + #[bw(try_calc = ea_value.as_ref() + .map(|v| v.len()) + .unwrap_or(0usize) + .try_into())] + ea_value_length: u16,
♻️ Duplicate comments (10)
crates/smb-fscc/src/directory_info.rs (2)
72-81: Blocker: Layout bug — EaSize vs ReparseTag must be info-class specific, not attribute dependent (duplicate of prior review)The macro still conditionally reads ea_size/reparse_tag based on file_attributes.reparse_point(). FSCC defines these per information class, not per file attribute, so this causes misaligned reads (e.g., Extd variants already have an explicit reparse_point_tag field, and Full/Both classes must always include EaSize). Remove this conditional from the macro and add the correct field explicitly in each struct.
Apply this diff to drop the conditional block:
- #[br(if(!file_attributes.reparse_point()))] - // ea_size and reparse_tag are the same field, parsed differently, based on attributes. - #[bw(assert(reparse_tag.is_some() != ea_size.is_some()))] - /// The size of the extended attributes for the file. - pub ea_size: Option<u32>, - #[br(if(file_attributes.reparse_point()))] - // Must set file_attributes.reparse_point() to true for this to be some. - #[bw(assert(reparse_tag.is_some() == file_attributes.reparse_point()))] - /// The reparse point tag. If the file is not a reparse point, this value is 0. - pub reparse_tag: Option<ReparseTag>,Follow-up fixes per struct are proposed below.
127-132: Missing EaSize in FileFullDirectoryInformationFullDirectory must always contain EaSize. Add it explicitly.
- pub struct FileFullDirectoryInformation {} + pub struct FileFullDirectoryInformation { + /// The size of the extended attributes for the file. + pub ea_size: u32, + }crates/smb/src/resource/directory.rs (3)
424-433: Fixed: single_threaded watch_stream now uses _watch_optionsSwapping the removed _watch_timeout for _watch_options resolves the build issue in single_threaded builds.
Please run CI for both feature sets to ensure maybe_async transforms compile:
- async+mtt: cargo test --features async,multi_threaded
- sync+st: cargo test --no-default-features --features single_threaded
374-378: Surface Cleanup to the consumer as an error, not silent stopKeep behavior consistent with From and the multi_threaded iterator: send Err(Cancelled("watch cleaned up by server")) before exiting.
- Some(DirectoryWatchResult::Cleanup) => { - // Server cleaned up the watch, exit the loop. - log::debug!("Watch cleaned up by server. Stopping stream."); - break; - } + Some(DirectoryWatchResult::Cleanup) => { + log::debug!("Watch cleaned up by server. Stopping stream with error."); + let _ = sender.send(Err(Error::Cancelled("watch cleaned up by server"))).await; + break; + }
306-325: Reset async_msg_ids between watch iterations (parity with multi_threaded path)Reusing the same AsyncMessageIds across loops risks stale IDs. Reset after each _watch_options call.
result = directory ._watch_options(filter, recursive, receive_options.clone()) => { let should_stop = matches!(result, DirectoryWatchResult::Cancelled | DirectoryWatchResult::Cleanup); if watch_tx.send(result).await.is_err() { break; // Receiver dropped } + // Prepare for the next iteration with fresh async IDs. + if let Some(ids) = &receive_options.async_msg_ids { + ids.reset(); + } if should_stop { break; } }crates/smb-fscc/src/filesystem_info.rs (1)
46-51: Use try_calc to prevent silent u32 truncation for wide-string lengthsCurrent
as u32can truncate on overflow. Prefer fallible calc like elsewhere in the codebase. This mirrors prior feedback.- #[bw(calc = file_system_name.size() as u32)] + #[bw(try_calc = file_system_name.size().try_into())] pub file_system_name_length: u32, @@ - #[br(args { size: SizedStringSize::bytes(file_system_name_length) })] + #[br(args { size: SizedStringSize::bytes(file_system_name_length) })] pub file_system_name: SizedWideString,- #[bw(calc = volume_label.size() as u32)] + #[bw(try_calc = volume_label.size().try_into())] pub volume_label_length: u32, @@ - #[br(args { size: SizedStringSize::bytes(volume_label_length) })] + #[br(args { size: SizedStringSize::bytes(volume_label_length) })] pub volume_label: SizedWideString,Also applies to: 349-358
crates/smb-fscc/src/set_file_info.rs (1)
88-88: Restore public alias for backward compatibility.If older code referenced
FileRenameInformation2, keep it alive by aliasing to the new name.Apply:
+/// Back-compat alias for the previously-public SMB2 rename info name. +pub type FileRenameInformation2 = FileRenameInformation;crates/smb-fscc/src/common_info.rs (1)
69-89: Public API rename: provide deprecated aliases for prior “syncronous_*”.The field accessors changed from the misspelled names; downstream code will break. Add deprecated shims delegating to the corrected accessors.
Apply:
impl FileModeInformation { + #[deprecated(note = "Renamed to synchronous_io_alert()")] + pub fn syncronous_io_alert(&self) -> bool { self.synchronous_io_alert() } + #[deprecated(note = "Renamed to set_synchronous_io_alert()")] + pub fn set_syncronous_io_alert(&mut self, v: bool) { self.set_synchronous_io_alert(v) } + #[deprecated(note = "Renamed to synchronous_io_non_alert()")] + pub fn syncronous_io_non_alert(&self) -> bool { self.synchronous_io_non_alert() } + #[deprecated(note = "Renamed to set_synchronous_io_non_alert()")] + pub fn set_syncronous_io_non_alert(&mut self, v: bool) { self.set_synchronous_io_non_alert(v) } }crates/smb-fscc/src/query_file_info.rs (1)
381-388: Remove unused bw(import) or wire it up.
#[bw(import(has_next: bool))]isn’t used in FileGetEaInformation; drop it to avoid confusion, or consume it if ChainedItemList expects it.Apply:
-#[bw(import(has_next: bool))] pub struct FileGetEaInformation {crates/smb/tests/notify.rs (1)
11-15: Fix conflicting sleep imports across features; unify via helper.If both features "async" and "multi_threaded" are enabled,
sleepis imported twice (name clash). Also, calling.awaitunconditionally onstd::thread::sleepis risky across cfgs. Replace with a single helper using maybe_async.Apply:
-#[cfg(feature = "multi_threaded")] -use std::thread::sleep; -#[cfg(feature = "async")] -use tokio::time::sleep; +// unified helper below; remove per-feature sleep imports- sleep(std::time::Duration::from_secs(2)).await; + maybe_sleep(std::time::Duration::from_secs(2)).await;Add helpers (near other helpers):
#[maybe_async::async_impl] async fn maybe_sleep(d: std::time::Duration) { tokio::time::sleep(d).await } #[maybe_async::sync_impl] fn maybe_sleep(d: std::time::Duration) { std::thread::sleep(d) }Quick check:
#!/bin/bash # Ensures no duplicate `sleep` imports remain and helper is present. rg -nP 'use (std::thread|tokio::time)::sleep' crates/smb/tests/notify.rs rg -n '^\\s*(async\\s*)?fn\\s+maybe_sleep\\b' crates/smb/tests/notify.rsAlso applies to: 94-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/build.yml(2 hunks)crates/smb-dtyp/src/binrw_util.rs(1 hunks)crates/smb-dtyp/src/binrw_util/sized_wide_string.rs(2 hunks)crates/smb-fscc/src/access_masks.rs(1 hunks)crates/smb-fscc/src/common_info.rs(2 hunks)crates/smb-fscc/src/directory_info.rs(2 hunks)crates/smb-fscc/src/filesystem_info.rs(7 hunks)crates/smb-fscc/src/lib.rs(1 hunks)crates/smb-fscc/src/notify.rs(2 hunks)crates/smb-fscc/src/query_file_info.rs(5 hunks)crates/smb-fscc/src/set_file_info.rs(4 hunks)crates/smb-msg/src/create.rs(3 hunks)crates/smb-msg/src/dfsc.rs(2 hunks)crates/smb-msg/src/info/set.rs(2 hunks)crates/smb-msg/src/ioctl/fsctl.rs(2 hunks)crates/smb-msg/src/query_dir.rs(7 hunks)crates/smb-msg/src/tree_connect.rs(2 hunks)crates/smb-rpc/src/pdu.rs(6 hunks)crates/smb/src/connection/config.rs(3 hunks)crates/smb/src/resource/directory.rs(7 hunks)crates/smb/tests/notify.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
crates/smb/src/resource/directory.rs (2)
crates/smb/src/resource.rs (6)
new(872-876)send_cancel(842-851)std(483-483)std(521-521)std(713-713)file_id(306-314)crates/smb/src/msg_handler.rs (8)
new(33-43)new(75-77)new(93-100)new(201-203)new(386-390)default(135-140)default(249-260)send(299-301)
crates/smb-fscc/src/filesystem_info.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
size(22-24)args(95-95)bytes(36-38)
crates/smb-msg/src/ioctl/fsctl.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
size(22-24)args(95-95)bytes(36-38)
crates/smb-msg/src/dfsc.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
size(22-24)args(95-95)bytes16(42-44)
crates/smb-fscc/src/set_file_info.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
size(22-24)args(95-95)bytes(36-38)
crates/smb-fscc/src/directory_info.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
size(22-24)args(95-95)bytes(36-38)
crates/smb-fscc/src/common_info.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
size(22-24)args(95-95)bytes(36-38)
crates/smb-msg/src/query_dir.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
args(95-95)size(22-24)bytes16(42-44)
crates/smb-msg/src/create.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
args(95-95)size(22-24)bytes16(42-44)
crates/smb-msg/src/tree_connect.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
args(95-95)size(22-24)bytes16(42-44)
crates/smb-fscc/src/notify.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (2)
args(95-95)size(22-24)
crates/smb/tests/notify.rs (3)
crates/smb/src/resource/directory.rs (9)
std(568-568)new(26-33)new(669-696)new(787-800)new(862-870)new(940-1013)watch_stream(268-274)watch_stream(406-415)watch_stream(419-440)crates/smb/tests/common.rs (1)
make_server_connection(36-48)crates/smb-msg/src/notify.rs (1)
all(61-75)
crates/smb-rpc/src/pdu.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
args(95-95)size(22-24)bytes16(42-44)
crates/smb-fscc/src/query_file_info.rs (1)
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
size(22-24)args(95-95)bytes(36-38)
⏰ 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: test (multi_threaded)
- GitHub Check: test (async)
🔇 Additional comments (43)
crates/smb-msg/src/query_dir.rs (2)
34-34: LGTM: Explicit size directive improves clarity.The change to use
SizedStringSize::bytes16(file_name_length)makes the deserialization argument more explicit and type-safe compared to passing a raw value.
154-154: LGTM: Field name typo corrected.The field name has been correctly updated from
fild_idtofile_idacross all test data instances, aligning with the actual struct definition.Also applies to: 170-170, 186-186, 202-202, 218-218, 234-234
crates/smb/src/connection/config.rs (4)
145-153: Documentation improvement looks good.The expanded documentation clearly explains the credits concept and provides a helpful reference to the Windows Server configuration parameter. The documentation enhances understanding of this configuration option.
155-160: Well-documented new configuration field.The
default_transaction_sizefield is appropriately typed and comprehensively documented with clear references to all affected operations.
186-192: Good validation prevents zero-sized transaction buffers.The validation correctly catches zero values which would cause runtime issues. This addresses the validation concern from past reviews.
Consider adding an upper bound check to catch unreasonably large values that exceed typical SMB protocol limits (e.g., 8 MiB). However, since the maximum can vary based on server capabilities and negotiated parameters, a hard maximum might be too restrictive. If you choose to add one, ensure it's well-documented.
Based on learnings about typical SMB transaction sizes.
201-207: 64 KB default is appropriate for modern SMB.The
DEFAULT_TRANSACTION_SIZEof 64 KB (0x10_000) aligns well with SMB2.0+ specifications and addresses the past review concern about 4 KB being too low. The method naming is consistent with the field name, following the same pattern as thetimeout()method..github/workflows/build.yml (3)
85-85: Comment typo fixed. ✓The typo "prvent" has been corrected to "prevent".
88-98: Lockfile path flag removal resolves past issue. ✓The
--lockfile-path Cargo.mock.lockflags have been removed from all build commands, addressing the past critical issue where the file did not exist. Builds will now use standard dependency resolution.
123-123: No issues found. The license field is properly inherited from the workspace root.The
smb-testscrate correctly useslicense.workspace = trueto inherit thelicense = "MIT"from the workspace root's[workspace.package]section. Modern Cargo resolves workspace inheritance for metadata fields during publishing—theworkspace = truereferences are replaced with actual values when the crate is published to crates.io. All required metadata (description, repository, authors) is present and will be correctly published.crates/smb-fscc/src/lib.rs (3)
8-14: Docs improvement LGTM.Clear, task-oriented sections for Query/Set/Directory/Notify.
32-39: Re-export set is convenient; watch for name collisions.With
pub use ...::*;across many modules, ensure there are no duplicate type names exported at the crate root that could cause ambiguous imports for users. CI build +cargo docwill surface any collisions.
19-31: The review comment's concern is not supported by the codebase.All 16 imports across the workspace use either
use smb_fscc::*;(wildcard) or import specific types by name (e.g.,use smb_fscc::FileDispositionInformation;). No code imports via module paths likesmb_fscc::directory_info::Type.The re-exports in lib.rs make all types available at the crate root, and this pattern is consistently used throughout the codebase. Making these modules private does not break any existing code in the workspace.
Likely an incorrect or invalid review comment.
crates/smb-fscc/src/directory_info.rs (1)
154-165: No action required—all Extd variants already aligned.All five Extd variants (FileId64ExtdBothDirectoryInformation, FileId64ExtdDirectoryInformation, FileIdAllExtdBothDirectoryInformation, FileIdAllExtdDirectoryInformation, FileIdExtdDirectoryInformation) consistently use
u32forreparse_point_tag. No customReparseTagtype exists in the codebase. The alignment requested in the original review has been satisfied.crates/smb-msg/src/info/set.rs (1)
109-113: RenameInformation payload + make_guid! macro usage looks correctThe test now constructs FileRenameInformation and uses make_guid! for FileId; this matches the SetFileInfo variant and GUID alias changes.
Also applies to: 118-120
crates/smb-msg/src/tree_connect.rs (2)
54-57: Correct sizing: use bytes16(path_length) for TreeConnect pathSwitching to SizedStringSize::bytes16(path_length) matches the u16 PathLength field and keeps read/write symmetric with buffer.size().
Please confirm interop expectations: SMB2 TreeConnect Path buffer on the wire in your implementation includes the trailing NUL and PathLength counts it (the test vector ends with 0x00,0x00). If any servers expect non-NUL-terminated paths, we may need a toggle.
83-84: paste → pastey migration (macro expansion source)Macro source swap is mechanical; no behavior change visible here.
crates/smb-msg/src/dfsc.rs (2)
59-69: Length-as-bytes and bytes16() args are correct for UTF-16 fieldsUsing request_file_name.size()/site_name.size() with bytes16(length) fixes byte-vs-code-unit bugs and matches binrw_util expectations.
127-137: paste → pastey macro switch is fineCompile-time expansion source change only; enums/variants unchanged.
crates/smb/src/resource/directory.rs (2)
479-491: Robust _watch_options status handlingAllowed statuses and conversions (Success/Cancelled/NotifyCleanup/NotifyEnumDir) are correct; error propagation is clear, and buffer underflow is reported via NotifyEnumDir.
Also applies to: 496-526
535-573: Good: quota query min-size guard and pass-through to query_commonValidates provided buffer size against FileQuotaInformation::MIN_SIZE and cleanly defers size selection to query_common.
crates/smb-fscc/src/notify.rs (2)
1-4: Docs addition is clear and helpful.Nice cross-link to MS-FSCC 2.7; improves discoverability.
15-19: Correct: byte-sized read args for UTF‑16 file_name.Switching to args { size: SizedStringSize::Bytes(...) } enforces byte units and will reject odd sizes (non-multiples of 2). Good safety improvement.
crates/smb-msg/src/create.rs (3)
96-98: Correct size semantics for name.Using SizedStringSize::bytes16(name_length) matches the on‑wire u16 byte length and prevents char/byte confusion.
277-279: paste → pastey macro path migration.Looks good. Please ensure Cargo.toml/workspace switched to the pastey crate everywhere to avoid mixed macro origins.
359-390: Second pastey migration spot.Same as above; verify no remaining paste::paste! invocations across the workspace.
crates/smb-msg/src/ioctl/fsctl.rs (1)
402-404: PipeWaitRequest: consistent byte-sized length/args.
- name_length now derives from name.size() and read uses bytes(...). This matches MS‑FSCC and aligns with SizedWideString semantics.
Also applies to: 411-413
crates/smb-rpc/src/pdu.rs (4)
9-15: make_guid! import and usage LGTM.Switch to smb_dtyp::make_guid! keeps GUID construction consistent and readable.
24-26: pastey macro path migration.Good. Confirm all generated code sites now reference pastey to prevent duplicate macro crates.
259-263: BindAck port_spec sizing fix.bytes16(port_spec_len) correctly treats the length as bytes and validates width. Good improvement.
367-369: Tests: updated to make_guid!.Consistent with runtime changes; assertions remain deterministic.
Also applies to: 381-383, 389-391, 397-399, 463-465
crates/smb-dtyp/src/binrw_util.rs (1)
15-18: Prelude re-exports expanded appropriately.Re-exporting BaseSizedString*, SizedStringSize ensures downstream crates can use the new args/constructors without deep paths.
crates/smb-dtyp/src/binrw_util/sized_wide_string.rs (3)
78-82: NamedArgs read args: good choice.BaseSizedStringReadArgs with an explicit size removes prior ambiguity and makes call sites self-documenting.
200-213: Debug/Display impls are clean.Human-friendly outputs aid troubleshooting; no nits.
292-312: Unit tests cover equality and cloning paths.Using BaseSizedString::<u8/u16>::from(...) benefits from the type-alias impls—nice sanity checks.
crates/smb-fscc/src/set_file_info.rs (1)
118-127: LGTM: root_directory handling is correct for SMB2 link.You assert zero on read and calc zero on write. Matches SMB2 requirements.
crates/smb-fscc/src/query_file_info.rs (3)
64-83: FileAllInformation field order matches FSCC; nice.Order and types align with the spec; aggregation looks correct.
255-276: Named pipe enums: values align with spec.ByteStream/Message, Inbound/Outbound/FullDuplex, and state values look correct.
137-142: Wire compatibility verified—strong implementation.The code correctly preserves u32 on-wire layout:
FileAttributesuses#[bitfield]which auto-generatesinto_bytes()/from_bytes()methods that safely convert the bitfield to u32ReparseTaguses#[repr(u32)]with#[brw(repr(u32))]ensuring proper u32 serialization- Both types pack to exactly 32 bits with no data loss or alignment issues
crates/smb/tests/notify.rs (5)
18-21: LGTM: constants and test-macro scaffold.Prefix/COUNT constants and macro-generated tests look correct and readable.
Also applies to: 22-39
33-35: LGTM: generated test entrypoints.maybe_async + serialized tests are configured properly.
57-69: LGTM: batch file creation loop.Correct naming, create-new semantics, and proper close.
149-185: LGTM: sync deletion with mpsc barrier.Channel-based join is correct; no thread leaks, and errors propagate via
unwrap()here as intended for tests.Consider mirroring the async change above to avoid
unwrap()inside threads if you want uniform error reporting.
239-243: LGTM: notify filter and semaphore signaling.Only counting
Removedis correct for this test’s expectation.
| - name: Remove lock file | ||
| run: echo rm Cargo.lock # TODO: re-enable when resolving #136 |
There was a problem hiding this comment.
Fix lock file removal command—currently just echoing, not deleting.
Line 87 runs echo rm Cargo.lock, which only prints the text instead of executing the removal. This defeats the step's stated purpose. Either execute the removal or remove the step entirely until #136 is resolved.
Apply this diff to actually remove the lock file:
- - name: Remove lock file
- run: echo rm Cargo.lock # TODO: re-enable when resolving #136
+ - name: Remove lock file
+ run: rm -f Cargo.lock # TODO: re-enable when resolving #136Alternatively, if this step is incomplete, comment it out or remove it:
- - name: Remove lock file
- run: echo rm Cargo.lock # TODO: re-enable when resolving #136
+ # TODO: re-enable lock file removal when resolving #136
+ # - name: Remove lock file
+ # run: rm -f Cargo.lock📝 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.
| - name: Remove lock file | |
| run: echo rm Cargo.lock # TODO: re-enable when resolving #136 | |
| - name: Remove lock file | |
| run: rm -f Cargo.lock # TODO: re-enable when resolving #136 |
🤖 Prompt for AI Agents
.github/workflows/build.yml around lines 86 to 87: the workflow step intended to
remove Cargo.lock currently runs `echo rm Cargo.lock`, which only prints the
command instead of deleting the file; fix it by replacing the echo with the
actual removal command (run: rm Cargo.lock) or, if you prefer to defer, remove
or comment out the entire step until issue #136 is resolved so the workflow no
longer contains a no-op line.
| #![allow(unused_assignments)] | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unnecessary allow attribute.
#![allow(unused_assignments)] appears unused here; consider dropping to keep lints meaningful.
🤖 Prompt for AI Agents
In crates/smb-dtyp/src/binrw_util/sized_wide_string.rs around lines 1 to 2,
remove the unnecessary crate-level attribute `#![allow(unused_assignments)]` as
it isn't suppressing any warnings in this file; delete that line so lints remain
meaningful and allow the compiler to report unused assignment warnings if they
appear.
| #[derive(Debug, Clone, Copy)] | ||
| pub enum SizedStringSize { | ||
| Bytes(u64), | ||
| Chars(u64), | ||
| } | ||
|
|
||
| impl SizedStringSize { | ||
| /// [SizedStringSize::Bytes] factory for u32 size. | ||
| #[inline] | ||
| pub fn bytes(n: u32) -> Self { | ||
| SizedStringSize::Bytes(n as u64) | ||
| } | ||
|
|
||
| /// [SizedStringSize::Bytes] factory for u16 size. | ||
| #[inline] | ||
| pub fn bytes16(n: u16) -> Self { | ||
| SizedStringSize::Bytes(n as u64) | ||
| } | ||
|
|
||
| /// [SizedStringSize::Chars] factory for u32 size. | ||
| #[inline] | ||
| pub fn chars(n: u32) -> Self { | ||
| SizedStringSize::Chars(n as u64) | ||
| } | ||
|
|
||
| /// [SizedStringSize::Chars] factory for u16 size. | ||
| #[inline] | ||
| pub fn chars16(n: u16) -> Self { | ||
| SizedStringSize::Chars(n as u64) | ||
| } | ||
|
|
||
| #[inline] | ||
| fn get_size_bytes<T: Sized>(&self) -> binrw::BinResult<u64> { | ||
| let size = match self { | ||
| SizedStringSize::Bytes(b) => *b, | ||
| SizedStringSize::Chars(c) => *c * std::mem::size_of::<T>() as u64, | ||
| }; | ||
| if size % std::mem::size_of::<T>() as u64 != 0 { | ||
| return Err(binrw::Error::Custom { | ||
| pos: 0, | ||
| err: Box::new(format!( | ||
| "SizedStringSize {:?} is not a multiple of char width {}", | ||
| self, | ||
| std::mem::size_of::<T>() | ||
| )), | ||
| }); | ||
| } | ||
| Ok(size) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
SizedStringSize API is solid; tweak error context.
Constructors (bytes/bytes16/chars/chars16) are ergonomic. get_size_bytes returns a Custom error with pos: 0 on width mismatch—prefer propagating the reader position for better diagnostics (plumb from caller).
🤖 Prompt for AI Agents
In crates/smb-dtyp/src/binrw_util/sized_wide_string.rs around lines 27 to 76,
get_size_bytes currently returns a binrw::Error::Custom with a hardcoded pos: 0
on size/width mismatch; change get_size_bytes to accept a pos parameter (u64)
and use that pos when constructing Error::Custom so the actual reader position
is reported, and update all call sites to pass the reader's current position
(e.g., from the BinRead context or stream_position) when invoking
get_size_bytes.
| type Args<'a> = BaseSizedStringReadArgs; | ||
|
|
||
| fn read_options<R: Read + Seek>( | ||
| reader: &mut R, | ||
| endian: Endian, | ||
| size_bytes: Self::Args<'_>, | ||
| args: Self::Args<'_>, | ||
| ) -> BinResult<Self> { | ||
| // Size is in bytes, but we need to read in chars. | ||
| assert!( | ||
| size_bytes.0 % Self::CHAR_WIDTH == 0, | ||
| "Size must be a multiple of char width" | ||
| ); | ||
| let size_chars = size_bytes.0 / Self::CHAR_WIDTH; | ||
| let size_to_use = args.size.get_size_bytes::<T>()?; | ||
| if size_to_use == 0 { | ||
| return Err(binrw::Error::Custom { | ||
| pos: reader.stream_position()?, | ||
| err: Box::new(format!( | ||
| "BaseSizedString<{}> had invalid read arguments {:?} - all None or zero", | ||
| std::any::type_name::<T>(), | ||
| args | ||
| )), | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix outdated error message in read_options.
Message still mentions “all None or zero”, but args now always contains a size; only zero is invalid. Update for clarity.
Apply this diff:
- return Err(binrw::Error::Custom {
- pos: reader.stream_position()?,
- err: Box::new(format!(
- "BaseSizedString<{}> had invalid read arguments {:?} - all None or zero",
- std::any::type_name::<T>(),
- args
- )),
- });
+ return Err(binrw::Error::Custom {
+ pos: reader.stream_position()?,
+ err: Box::new(format!(
+ "BaseSizedString<{}> cannot read with zero size (args: {:?})",
+ std::any::type_name::<T>(),
+ args
+ )),
+ });📝 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.
| type Args<'a> = BaseSizedStringReadArgs; | |
| fn read_options<R: Read + Seek>( | |
| reader: &mut R, | |
| endian: Endian, | |
| size_bytes: Self::Args<'_>, | |
| args: Self::Args<'_>, | |
| ) -> BinResult<Self> { | |
| // Size is in bytes, but we need to read in chars. | |
| assert!( | |
| size_bytes.0 % Self::CHAR_WIDTH == 0, | |
| "Size must be a multiple of char width" | |
| ); | |
| let size_chars = size_bytes.0 / Self::CHAR_WIDTH; | |
| let size_to_use = args.size.get_size_bytes::<T>()?; | |
| if size_to_use == 0 { | |
| return Err(binrw::Error::Custom { | |
| pos: reader.stream_position()?, | |
| err: Box::new(format!( | |
| "BaseSizedString<{}> had invalid read arguments {:?} - all None or zero", | |
| std::any::type_name::<T>(), | |
| args | |
| )), | |
| }); | |
| } | |
| type Args<'a> = BaseSizedStringReadArgs; | |
| fn read_options<R: Read + Seek>( | |
| reader: &mut R, | |
| endian: Endian, | |
| args: Self::Args<'_>, | |
| ) -> BinResult<Self> { | |
| let size_to_use = args.size.get_size_bytes::<T>()?; | |
| if size_to_use == 0 { | |
| return Err(binrw::Error::Custom { | |
| pos: reader.stream_position()?, | |
| err: Box::new(format!( | |
| "BaseSizedString<{}> cannot read with zero size (args: {:?})", | |
| std::any::type_name::<T>(), | |
| args | |
| )), | |
| }); | |
| } | |
🤖 Prompt for AI Agents
In crates/smb-dtyp/src/binrw_util/sized_wide_string.rs around lines 88 to 106,
the error message in read_options incorrectly states "all None or zero" even
though args always contains a size now; update the error text to only mention
the zero-size case (for example "had invalid read arguments: size was zero") and
keep including type_name::<T>() and the args value and pos as before so the log
remains informative.
| access_mask! { | ||
| pub struct FileAccessMask { | ||
| file_read_data: bool, | ||
| file_write_data: bool, | ||
| file_append_data: bool, | ||
| file_read_ea: bool, | ||
|
|
||
| file_write_ea: bool, | ||
| file_execute: bool, | ||
| file_delete_child: bool, | ||
| file_read_attributes: bool, | ||
|
|
||
| file_write_attributes: bool, | ||
| #[skip] | ||
| __: B7, | ||
| }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Bit layout looks correct; add a brief note/tests for the reserved gap.
The 7-bit skip neatly fills object-specific bits up to 0..15. Please add a short doc comment stating these map to MS-FSCC object-specific bits and that 9..15 are reserved, plus a couple of unit tests to guard the mapping.
Example tests (new file tests/access_masks.rs):
use smb_fscc::{FileAccessMask, DirAccessMask};
#[test]
fn file_dir_roundtrip_preserves_bits() {
// Set a mixture of low and high rights via bytes.
let mut bytes = [0u8; 4];
// low object-specific + standard rights + generic: just a sample pattern
bytes.copy_from_slice(&0xA5A5_013F_u32.to_le_bytes());
let f = FileAccessMask::from_bytes(bytes);
let d: DirAccessMask = f.into();
assert_eq!(d.into_bytes(), bytes);
let f2: FileAccessMask = d.into();
assert_eq!(f2.into_bytes(), bytes);
}
#[test]
fn individual_flag_positions_match() {
let mut f = FileAccessMask::new();
f.set_file_read_data(true);
assert!(DirAccessMask::from(f).list_directory()); // same bit position
}Also applies to: 23-38
🤖 Prompt for AI Agents
In crates/smb-fscc/src/access_masks.rs around lines 6–21 (and similarly for
lines 23–38), add a brief doc comment on the FileAccessMask/DirAccessMask
structs stating these fields map to the MS-FSCC object-specific bits and that
bits 9..15 are reserved (the 7-bit skip fills object-specific bits up to 0..15);
then add unit tests (e.g. new tests/access_masks.rs) that assert round-trip
preservation of bytes between FileAccessMask and DirAccessMask and verify
individual flag positions (as in the review example) to guard the mapping and
reserved gap.
| #[maybe_async::maybe_async] | ||
| async fn do_test_smb_notify( | ||
| f_start_notify_task: fn(Arc<Semaphore>, Directory), | ||
| ) -> Result<(), Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
🧩 Analysis chain
Prevent race: ensure watcher is ready before deletions.
Deletes may land before the spawned watcher is effectively listening, causing hangs. Add a small readiness barrier (preferred) or a brief maybe_sleep before the first delete.
Minimal option:
- f_start_notify_task(notified_sem.clone(), dir);
+ f_start_notify_task(notified_sem.clone(), dir);
+ // Give the watcher a moment to register to avoid missing early events.
+ maybe_sleep(std::time::Duration::from_millis(100)).await;Better: extend the watcher starter to signal readiness (e.g., pass an Arc<Semaphore> or a oneshot), then wait for 1 permit before first delete.
Please run the test 20x to check flakiness:
Also applies to: 81-93
🏁 Script executed:
#!/bin/bash
# Run this particular test repeatedly (adjust package/test filter as needed).
for i in {1..20}; do
echo "--- run $i ---"
cargo test -p smb --tests notify --features "async multi_threaded" -- --nocapture || exit 1
doneLength of output: 123
I cannot execute cargo tests in the sandbox environment. Let me inspect the actual test code to determine if the race condition concern is valid.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the test file and the specific lines mentioned in the review
cat crates/smb/tests/notify.rs
Length of output: 7737
🏁 Script executed:
#!/bin/bash
# Also search for any sleep or semaphore-related mechanisms already in place
rg -n "maybe_sleep|semaphore|Semaphore" crates/smb/tests/notify.rs -C 2Length of output: 1255
🏁 Script executed:
#!/bin/bash
# Check if there are multiple test functions and understand the structure
rg -n "fn do_test|fn test_" crates/smb/tests/notify.rsLength of output: 31
🏁 Script executed:
#!/bin/bash
# Look for the function signature and understand watcher spawning patterns
ast-grep --pattern 'fn do_test_smb_notify($$$) -> $_ { $$$ }'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the broader test file for context on how watchers are managed
rg -n "f_start_notify_task" crates/smb/tests/notify.rs -B 3 -A 10Length of output: 932
🏁 Script executed:
#!/bin/bash
# Search for any delete operations to understand timing
rg -n "delete|remove" crates/smb/tests/notify.rs -i -C 2Length of output: 2303
🏁 Script executed:
#!/bin/bash
# Look for any barriers or wait mechanisms
rg -n "wait|barrier|notified" crates/smb/tests/notify.rs -C 2Length of output: 2508
🏁 Script executed:
#!/bin/bash
# Check if maybe_async is used and understand its implications
rg -n "maybe_async|#\[maybe" crates/smb/tests/notify.rs -C 1Length of output: 949
🏁 Script executed:
#!/bin/bash
# Check for any existing delays or sleep patterns in the file
rg -n "sleep|delay|Duration" crates/smb/tests/notify.rs -C 1Length of output: 212
🏁 Script executed:
#!/bin/bash
# Get a broader context of the entire test function(s)
sed -n '40,100p' crates/smb/tests/notify.rsLength of output: 1870
🏁 Script executed:
#!/bin/bash
# Check what features are defined and if async is actually in use
rg -n "feature|cfg" crates/smb/tests/notify.rs | head -20Length of output: 211
🏁 Script executed:
#!/bin/bash
# Look at Directory and Semaphore usage patterns
rg -n "Directory|notified_sem" crates/smb/tests/notify.rs -C 1Length of output: 1483
🏁 Script executed:
#!/bin/bash
# Check if there are known issues or TODOs related to timing/race conditions
rg -n "FIXME|TODO|race|flaky|timing" crates/smb/tests/notify.rs -iLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are Cargo.toml features related to async/threading
cat crates/smb/Cargo.toml | grep -A 20 "features"Length of output: 2132
🏁 Script executed:
#!/bin/bash
# Verify the existence and content of lines 43-46 and 81-93 specifically
sed -n '43,46p' crates/smb/tests/notify.rs
echo "---"
sed -n '81,93p' crates/smb/tests/notify.rsLength of output: 579
The race condition is real and the review comment is accurate; add readiness synchronization before first deletion.
The code spawns watcher tasks at line 81 using tokio::spawn() (or std::thread::spawn()) which are fire-and-forget operations that return immediately. The watcher functions (legacy_watch, stream_iter_watch) return right after spawning without signaling that the watch loop has actually started. Deletions begin immediately at lines 86-90 before the spawned task has reached its watch setup call (r.watch() or Directory::watch_stream()), causing notifications from the first batch to be missed. The test then hangs at line 92 waiting for notifications that never arrive.
The 2-second sleep at line 94 protects the second batch but not the first. A readiness mechanism (semaphore or oneshot channel) is preferable to a sleep, but at minimum, a small delay before the first deletion is necessary to eliminate the flakiness.
🤖 Prompt for AI Agents
In crates/smb/tests/notify.rs around lines 43 to 46, the test spawns watcher
tasks that return immediately and then performs the first deletions before the
watcher has actually started watching, causing a race that drops notifications;
modify the test to wait for a readiness signal from the watcher before
performing the first deletion: update the watcher-start helper to signal
readiness (preferably via the existing Arc<Semaphore> or a oneshot channel)
right after calling r.watch() / Directory::watch_stream(), and in the test
acquire/wait on that readiness permit before executing the first deletion batch
(the existing 2s sleep can be removed or kept as a fallback); ensure the same
readiness wait is applied before the first deletion so notifications are not
missed.
| sleep(std::time::Duration::from_secs(2)).await; | ||
| delete_many_files( | ||
| TestConstants::DEFAULT_SHARE, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid fixed 2s sleep between batches; prefer event-driven pacing.
After awaiting the first batch’s permits, the watcher is active; consider reducing to a small delay or removing entirely to speed tests and reduce flakiness.
🤖 Prompt for AI Agents
In crates/smb/tests/notify.rs around lines 94 to 96, there's a fixed
sleep(Duration::from_secs(2)).await between batches which slows tests and causes
flakiness; remove the hard 2s sleep and instead either (a) eliminate it entirely
if the prior await on the first batch’s permits already guarantees the watcher
is active, or (b) replace it with a short, bounded wait (e.g. 50–200ms) or,
preferably, an event-driven synchronization (await a notification/oneshot or
barrier that the watcher has started) so the test proceeds only once the watcher
is ready.
| #[maybe_async::async_impl] | ||
| async fn delete_many_files(share_path: &'static str, rng_numbers: &[u32]) -> smb::Result<()> { | ||
| // for each number, iterate and call delete_from_another_connection. Wait for all at the end. | ||
| use futures_util::future::join_all; | ||
|
|
||
| // Connect the client: | ||
|
|
||
| let (client, share_path) = make_server_connection( | ||
| share_path, | ||
| ConnectionConfig { | ||
| encryption_mode: EncryptionMode::Disabled, | ||
| ..Default::default() | ||
| } | ||
| .into(), | ||
| ) | ||
| .await?; | ||
|
|
||
| let client = Arc::new(client); | ||
|
|
||
| let futures = rng_numbers.iter().map(|&i| { | ||
| let share_path = share_path.clone(); | ||
| let client = client.clone(); | ||
| tokio::spawn(async move { | ||
| delete_file_from_another_connection( | ||
| client, | ||
| share_path, | ||
| &format!("{}_{i}.txt", NEW_FILE_NAME_UNDER_WORKDIR_PREFIX), | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| }) | ||
| }); | ||
|
|
||
| let results = join_all(futures).await; | ||
| for r in results { | ||
| r.expect("delete task panicked"); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good: join results are now checked; also propagate inner errors.
You handle panics via expect. Don’t unwrap() inside the spawned tasks; return the Result and assert after join so inner I/O errors surface clearly.
Apply:
- let futures = rng_numbers.iter().map(|&i| {
+ let futures = rng_numbers.iter().map(|&i| {
let share_path = share_path.clone();
let client = client.clone();
- tokio::spawn(async move {
- delete_file_from_another_connection(
- client,
- share_path,
- &format!("{}_{i}.txt", NEW_FILE_NAME_UNDER_WORKDIR_PREFIX),
- )
- .await
- .unwrap();
- })
+ tokio::spawn(async move {
+ delete_file_from_another_connection(
+ client,
+ share_path,
+ &format!("{}_{i}.txt", NEW_FILE_NAME_UNDER_WORKDIR_PREFIX),
+ )
+ .await
+ })
});
- let results = join_all(futures).await;
- for r in results {
- r.expect("delete task panicked");
- }
+ let results = join_all(futures).await;
+ for r in results {
+ let inner = r.expect("delete task panicked");
+ inner.expect("delete failed");
+ }📝 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.
| #[maybe_async::async_impl] | |
| async fn delete_many_files(share_path: &'static str, rng_numbers: &[u32]) -> smb::Result<()> { | |
| // for each number, iterate and call delete_from_another_connection. Wait for all at the end. | |
| use futures_util::future::join_all; | |
| // Connect the client: | |
| let (client, share_path) = make_server_connection( | |
| share_path, | |
| ConnectionConfig { | |
| encryption_mode: EncryptionMode::Disabled, | |
| ..Default::default() | |
| } | |
| .into(), | |
| ) | |
| .await?; | |
| let client = Arc::new(client); | |
| let futures = rng_numbers.iter().map(|&i| { | |
| let share_path = share_path.clone(); | |
| let client = client.clone(); | |
| tokio::spawn(async move { | |
| delete_file_from_another_connection( | |
| client, | |
| share_path, | |
| &format!("{}_{i}.txt", NEW_FILE_NAME_UNDER_WORKDIR_PREFIX), | |
| ) | |
| .await | |
| .unwrap(); | |
| }) | |
| }); | |
| let results = join_all(futures).await; | |
| for r in results { | |
| r.expect("delete task panicked"); | |
| } | |
| Ok(()) | |
| } | |
| #[maybe_async::async_impl] | |
| async fn delete_many_files(share_path: &'static str, rng_numbers: &[u32]) -> smb::Result<()> { | |
| // for each number, iterate and call delete_from_another_connection. Wait for all at the end. | |
| use futures_util::future::join_all; | |
| // Connect the client: | |
| let (client, share_path) = make_server_connection( | |
| share_path, | |
| ConnectionConfig { | |
| encryption_mode: EncryptionMode::Disabled, | |
| ..Default::default() | |
| } | |
| .into(), | |
| ) | |
| .await?; | |
| let client = Arc::new(client); | |
| let futures = rng_numbers.iter().map(|&i| { | |
| let share_path = share_path.clone(); | |
| let client = client.clone(); | |
| tokio::spawn(async move { | |
| delete_file_from_another_connection( | |
| client, | |
| share_path, | |
| &format!("{}_{i}.txt", NEW_FILE_NAME_UNDER_WORKDIR_PREFIX), | |
| ) | |
| .await | |
| }) | |
| }); | |
| let results = join_all(futures).await; | |
| for r in results { | |
| let inner = r.expect("delete task panicked"); | |
| inner.expect("delete failed"); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In crates/smb/tests/notify.rs around lines 108–146, the spawned tasks call
.unwrap() which hides I/O errors; change the spawned async closure to return a
Result (e.g. smb::Result<()>) instead of unwrapping, so each tokio::spawn
returns a JoinHandle<Result<()>>; after join_all, iterate the join results,
first assert/handle any JoinError (task panic) and then propagate or assert the
inner Result so I/O errors surface (use ? to return the error from
delete_many_files or map_err to convert and fail the test). Ensure the function
signature and error handling propagate the inner errors instead of panicking
inside the task.
| fn legacy_watch(sem: Arc<Semaphore>, r: Directory) { | ||
| tokio::spawn(async move { | ||
| for notification in r.watch(filter, true).await.unwrap() { | ||
| if notification.action == NotifyAction::Removed { | ||
| sem.add_permits(1); | ||
| break; | ||
| loop { | ||
| for notification in r.watch(NotifyFilter::all(), true).await.unwrap() { | ||
| on_notification(sem.clone(), notification); | ||
| } | ||
| } | ||
| r.close().await.unwrap(); | ||
| }); | ||
| } | ||
| #[maybe_async::sync_impl] | ||
| fn start_notify_task(sem: Arc<Semaphore>, r: Directory) { | ||
| let filter = NotifyFilter::new() | ||
| .with_file_name(true) | ||
| .with_dir_name(true) | ||
| .with_attributes(true) | ||
| .with_last_write(true) | ||
| .with_last_access(true); | ||
| fn legacy_watch(sem: Arc<Semaphore>, r: Directory) { | ||
| std::thread::spawn(move || { | ||
| for notification in r.watch(filter, true).unwrap() { | ||
| if notification.action == NotifyAction::Removed { | ||
| sem.add_permits(1); | ||
| break; | ||
| loop { | ||
| for notification in r.watch(NotifyFilter::all(), true).unwrap() { | ||
| on_notification(sem.clone(), notification); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Background watcher has no shutdown path (possible test leaks/hangs).
The infinite loop never exits; the spawned task/thread outlives the test and holds resources. Add a cancellation signal (e.g., Arc<AtomicBool> or a oneshot) and break the loop when tests are done.
🏁 Script executed:
#!/bin/bash
# View the test file to understand context around lines 188-206
echo "=== Full test file context ==="
cat crates/smb/tests/notify.rsLength of output: 7768
🏁 Script executed:
#!/bin/bash
# Search for how legacy_watch is called in tests
echo "=== Usages of legacy_watch ==="
rg -n "legacy_watch" crates/smb/tests/notify.rs -A 5 -B 5
echo -e "\n=== Check for test cleanup/teardown patterns ==="
rg -n "#\[tokio::test\]|#\[test\]|fn test_" crates/smb/tests/notify.rs -A 10Length of output: 1054
🏁 Script executed:
#!/bin/bash
# Check Semaphore usage and on_notification function
echo "=== on_notification function and Semaphore pattern ==="
rg -n "fn on_notification|Semaphore" crates/smb/tests/notify.rs -B 2 -A 5
echo -e "\n=== Check if there's any thread/task joining or waiting ==="
rg -n "join|await|wait" crates/smb/tests/notify.rs -B 2 -A 2Length of output: 5028
🏁 Script executed:
#!/bin/bash
# Check if there are other watchers or background tasks in the codebase with proper cleanup
echo "=== Search for other tokio::spawn or thread::spawn patterns ==="
rg -n "tokio::spawn|thread::spawn" crates/smb/tests/ -A 3 | head -50
echo -e "\n=== Check for AbortHandle, JoinHandle, or task tracking patterns ==="
rg -n "JoinHandle|AbortHandle|join\(" crates/smb/tests/notify.rsLength of output: 1879
🏁 Script executed:
#!/bin/bash
# Verify: in the sync version, is the thread joined anywhere after f_start_notify_task call?
echo "=== Check all lines after f_start_notify_task call in do_test_smb_notify ==="
sed -n '81,105p' crates/smb/tests/notify.rsLength of output: 845
🏁 Script executed:
#!/bin/bash
# Check Cargo.toml for test configuration and dependencies
echo "=== Test framework configuration ==="
cat Cargo.toml | grep -A 10 "\[dev-dependencies\]"
echo -e "\n=== Check for tokio test config ==="
rg -n "tokio::test" crates/smb/tests/notify.rsLength of output: 143
Fix spawned watcher threads/tasks: both async and sync implementations lack proper cleanup.
The legacy_watch (lines 188–205) and stream_iter_watch spawned tasks/threads are never awaited or joined. In the sync version, std::thread::spawn creates threads that outlive the test function and remain blocked in r.watch() indefinitely. The async version benefits from tokio runtime cleanup on shutdown, but lacks explicitness.
Compare with delete_many_files (lines 130, 170), which correctly joins spawned threads via join_all() (async) or mpsc channel + recv() loop (sync). Apply the same pattern: either capture JoinHandle and join before returning, or use a cancellation signal (Arc<AtomicBool> or oneshot channel) to exit the watch loop cleanly.
| #[maybe_async::async_impl] | ||
| fn stream_iter_watch(sem: Arc<Semaphore>, r: Directory) { | ||
| use futures_util::TryStreamExt; | ||
| tokio::spawn(async move { | ||
| let r = Arc::new(r); | ||
|
|
||
| Directory::watch_stream(&r, NotifyFilter::all(), true) | ||
| .unwrap() | ||
| .try_for_each(|notification| { | ||
| let value = sem.clone(); | ||
| async move { | ||
| on_notification(value, notification); | ||
| Ok(()) | ||
| } | ||
| }) | ||
| .await | ||
| .unwrap_or_else(|_| panic!("Error in notification stream")); | ||
| r.close().await.unwrap(); | ||
| }); | ||
| } | ||
| #[maybe_async::sync_impl] | ||
| fn stream_iter_watch(sem: Arc<Semaphore>, r: Directory) { | ||
| std::thread::spawn(move || { | ||
| let r = Arc::new(r); | ||
| for notification in Directory::watch_stream(&r, NotifyFilter::all(), true).unwrap() { | ||
| on_notification(sem.clone(), notification.unwrap()); | ||
| } | ||
| r.close().unwrap(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Iterator watcher lacks cancellation; r.close() is unreachable.
The iterator runs until canceled; since no canceller is used, the loop never ends and r.close() won’t run. Use the cancellable iterator handle (see NotifyDirectoryIteratorCanceller) and call cancel when done, or add a stop condition.
I'll verify the cancellation concerns in the review comment by examining the codebase implementation and test context.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the Directory struct and watch_stream implementation
rg -n "impl.*Directory" --type rs -A 30 | head -100
Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for watch_stream method definition
rg -n "fn watch_stream" --type rs -B 5 -A 20Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for NotifyDirectoryIteratorCanceller
rg -n "NotifyDirectoryIteratorCanceller" --type rs -B 2 -A 5Length of output: 26
🏁 Script executed:
#!/bin/bash
# Get full context of the test function
rg -n "stream_iter_watch" --type rs -B 20 -A 50Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for how other tests handle stream cleanup
rg -n "watch_stream" --type rs -B 3 -A 10Length of output: 26
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for watch_stream method definition (corrected file type)
rg -n "fn watch_stream" --type rust -B 5 -A 20
Length of output: 5904
🏁 Script executed:
#!/bin/bash
# Search for NotifyDirectoryIteratorCanceller
rg -n "NotifyDirectoryIteratorCanceller" --type rust -B 2 -A 5Length of output: 4682
🏁 Script executed:
#!/bin/bash
# Get full context of the test function in notify.rs
cat crates/smb/tests/notify.rs | head -250 | tail -100Length of output: 2991
🏁 Script executed:
#!/bin/bash
# Search for how watch_stream is used across the codebase
rg -n "watch_stream" --type rust -B 3 -A 10Length of output: 9585
🏁 Script executed:
#!/bin/bash
# Look at the full notify.rs test file to understand test structure
wc -l crates/smb/tests/notify.rs && head -50 crates/smb/tests/notify.rsLength of output: 1390
---
🏁 Script executed:
```shell
#!/bin/bash
# Look for test timeout or task cleanup in test file
cat crates/smb/tests/notify.rs | tail -100
Length of output: 2795
🏁 Script executed:
#!/bin/bash
# Check Drop implementation of NotifyDirectoryIterator
rg -n "impl Drop for NotifyDirectoryIterator" --type rust -A 10Length of output: 631
🏁 Script executed:
#!/bin/bash
# See full do_test_smb_notify function to understand test cleanup
rg -n "async fn do_test_smb_notify" --type rust -A 80Length of output: 4882
🏁 Script executed:
#!/bin/bash
# Check for any test timeout configuration
rg -n "timeout|Duration" --type rust crates/smb/tests/notify.rsLength of output: 54
🏁 Script executed:
#!/bin/bash
# Look for Semaphore implementation and how test waits for completion
rg -n "struct Semaphore" --type rust -B 2 -A 15 | head -50Length of output: 2032
Rewrite: Correct - r.close() is unreachable without explicit cancellation.
The watch_stream() iterator is designed to run indefinitely. For the multi-threaded variant, the returned iterator implements NotifyDirectoryIteratorCancellable, providing get_canceller() to stop the loop. For the async variant, the stream requires a CancellationToken to cancel. Without calling canceller.cancel() or providing a triggered token, both loops run indefinitely and r.close() remains unreachable.
Use the canceller pattern shown in crates/smb-cli/src/watch.rs (line 110): extract the iterator, get its canceller via get_canceller(), and call cancel() when done—or provide a triggered CancellationToken to the async stream to signal completion.
🤖 Prompt for AI Agents
In crates/smb/tests/notify.rs around lines 208 to 237, the watch_stream loops
are unconditionally blocking so r.close() is never reached; you must wire up
cancellation: for the sync/threaded variant, call Directory::watch_stream(...)
to get the iterator, call get_canceller() on it and store the canceller so you
can call canceller.cancel() when you want the loop to exit (then break the for
loop and call r.close()); for the async/tokio variant, pass or create a
CancellationToken and give it to watch_stream (or extract the stream's canceller
equivalent) and trigger token.cancel() (or call the canceller) when you want the
stream to stop so the try_for_each finishes and r.close(). Ensure you
unwrap/handle errors as before after cancellation.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements