Bugfix: set/query FILE_QUOTA_INFORMATION
#137
Conversation
WalkthroughAdds quota support across layers: new fscc::quota types, msg mapping to chained quota lists, Directory query/set API updated to use Vec, CLI flag to optionally display quota during traversal, conversion helpers and TryInto macro, and safer as_* accessors replacing unwrap_*. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as InfoCmd (smb-cli)
participant Iter as Iterator
participant Dir as Directory (smb)
participant Srv as SMB Server
User->>CLI: run info (show_quota=true)
CLI->>Iter: iterate path(s) with show_quota
loop For each directory
Iter->>Dir: query_quota_info(...)
Dir->>Srv: FSCTL/QueryInfo (expects ChainedItemList<FileQuotaInformation>)
Srv-->>Dir: chained quota response
Dir-->>Iter: Vec<FileQuotaInformation> (as_quota()?.into())
Note right of Iter: display_quota_info(...)
end
Iter-->>CLI: item info + quota (if any)
CLI-->>User: output
sequenceDiagram
autonumber
participant Dir as Directory (smb)
participant Msg as smb-msg
participant Fscc as smb-fscc::quota
participant Srv as SMB Server
Dir->>Msg: build Query (ChainedItemList<FileQuotaInformation>)
Msg->>Srv: send QueryInfo (Quota)
Srv-->>Msg: chained quota list
Msg-->>Dir: as_quota()?.into() -> Vec<FileQuotaInformation>
Dir->>Fscc: ChainedItemList::from(Vec<FileQuotaInformation>)
Dir->>Srv: SetInfo Quota with chained list
Srv-->>Dir: status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/smb-msg/src/info/common.rs (1)
72-79: Fix moved-self usage inas_*match
match self { … _ => … self.name() }usesselfafter it has been moved into the match, so this code will not compile. Bind the non-matching branch to a new identifier and callname()on it instead.- pub fn [<as_ $info_type:lower>](self) -> Result<$content, $crate::SmbMsgError> { - match self { - $name::$info_type(data) => Ok(data), - _ => Err($crate::SmbMsgError::UnexpectedContent { - expected: stringify!($info_type), - actual: self.name(), - }), - } - } + pub fn [<as_ $info_type:lower>](self) -> Result<$content, $crate::SmbMsgError> { + match self { + $name::$info_type(data) => Ok(data), + value => Err($crate::SmbMsgError::UnexpectedContent { + expected: stringify!($info_type), + actual: value.name(), + }), + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
crates/smb-cli/src/info.rs(6 hunks)crates/smb-dtyp/src/binrw_util/helpers.rs(1 hunks)crates/smb-fscc/src/lib.rs(2 hunks)crates/smb-fscc/src/quota.rs(1 hunks)crates/smb-msg/src/info/common.rs(3 hunks)crates/smb-msg/src/info/query.rs(5 hunks)crates/smb-msg/src/info/set.rs(2 hunks)crates/smb/src/resource.rs(3 hunks)crates/smb/src/resource/directory.rs(2 hunks)crates/smb/tests/dialects.rs(0 hunks)
💤 Files with no reviewable changes (1)
- crates/smb/tests/dialects.rs
🧰 Additional context used
🧬 Code graph analysis (4)
crates/smb-fscc/src/quota.rs (2)
crates/smb-fscc/src/chained_item.rs (1)
value(45-47)crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_size(480-497)
crates/smb-msg/src/info/query.rs (2)
crates/smb/src/resource.rs (2)
new(719-723)as_file(189-194)crates/smb-fscc/src/chained_item.rs (1)
new(41-43)
crates/smb/src/resource/directory.rs (1)
crates/smb-fscc/src/chained_item.rs (4)
from(125-130)from(162-164)from(173-177)from(222-224)
crates/smb-cli/src/info.rs (2)
crates/smb-msg/src/info/query.rs (1)
new(152-163)crates/smb/src/resource/directory.rs (3)
new(27-34)new(356-383)new(474-487)
⏰ 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). (4)
- GitHub Check: test (async)
- GitHub Check: test (multi_threaded)
- GitHub Check: test (single_threaded)
- GitHub Check: build-msrv
🔇 Additional comments (14)
crates/smb-dtyp/src/binrw_util/helpers.rs (1)
128-132: LGTM! Clean bidirectional conversion for Boolean wrapper.The
From<Boolean> for boolimplementation correctly provides ergonomic unwrapping of the Boolean newtype and automatically enablesInto<bool>via the standard library's blanket implementation. This complements the existingFrom<bool> for Booleanconversion nicely.crates/smb-msg/src/info/set.rs (1)
39-39: LGTM! Quota type mapping updated correctly.The change from
QueryQuotaInfotoChainedItemList<FileQuotaInformation>aligns with the new quota API introduced in thesmb-fscc::quotamodule. The validation logic at line 80 remains compatible with this change.crates/smb-fscc/src/lib.rs (2)
18-18: LGTM! Import cleanup.The removal of
TakeSeekExtfrom the top-level imports is appropriate as it's likely used only in specific submodules (likequota.rsat line 2) where it's directly imported.
29-29: LGTM! Quota module properly integrated.The new
quotamodule is correctly declared as public and its items are re-exported, making the quota API available to consumers of this crate. This follows the standard Rust module organization pattern seen with other modules in this file.Also applies to: 38-38
crates/smb-fscc/src/quota.rs (1)
32-45: LGTM! Quota information structures well-defined.The
FileGetQuotaInformationInnerstruct andFileGetQuotaInformationtype alias follow the established pattern for chained items. The use ofPosMarkerfor variable-length SID fields is appropriate, and the documentation correctly references the MS-FSCC specification.crates/smb-cli/src/info.rs (6)
43-46: LGTM! Quota flag added to CLI.The
show_quotaflag is properly integrated into theInfoCmdstruct with appropriate defaults and documentation.
88-90: LGTM! Initial quota query integrated.The quota information is queried for the root directory when
show_quotais enabled, using the new helper function.
152-158: LGTM! Size formatting enhanced with infinity handling.The addition of
u64::MAX => "∞"is a nice UX improvement for displaying unlimited quotas. The existing size formatting logic remains intact.
164-164: LGTM! Params structure extended correctly.The
show_quotafield is appropriately added toIterateParamsfor propagating the flag through the iteration logic.
230-234: Verify the recursion gating logic withshow_quota.Line 232 changes the early-return condition to
params.recursive < RecursiveMode::List && !params.show_quota. This means:
- If
show_quotaistrue, the function will not early-return and will continue to process subdirectories even whenrecursive == RecursiveMode::NonRecursive- This couples quota display with directory traversal in a potentially unexpected way
Question: Is this the intended behavior? Should enabling
show_quotaforce traversal of subdirectories when the user specified--recursive=non-recursive?If this is intentional (e.g., to query quota on each subdirectory even in non-recursive mode), consider adding a comment explaining this behavior:
- if params.recursive < RecursiveMode::List && !params.show_quota { + // Continue processing subdirectories if either: + // 1. Recursive mode is List (explicit recursion), OR + // 2. show_quota is enabled (to query quota on subdirectories) + if params.recursive < RecursiveMode::List && !params.show_quota { return None; }Otherwise, if quota should only be shown for directories that are already being traversed:
display_item_info(info, dir_path); - if params.recursive < RecursiveMode::List && !params.show_quota { + if params.recursive < RecursiveMode::List { return None; }And move the quota query to only execute during recursive traversal.
285-293: LGTM! Quota query helper is well-structured.The
try_query_and_show_quotahelper properly encapsulates quota querying with error handling, making the code more maintainable. The use ofQueryQuotaInfo::new(false, true, vec![])with an empty vector correctly requests all quotas as per the MS-SMB2 specification.crates/smb-msg/src/info/query.rs (3)
148-176: LGTM! Constructor methods provide a clean API.The two constructor methods
new()andnew_sid()provide clear, type-safe ways to buildQueryQuotaInfofor the two different query modes specified in MS-SMB2 2.2.37.1. The documentation references are helpful, and the implementations correctly handle the mutual exclusivity of the two options.
239-239: LGTM! Quota type mapping updated correctly.The change from
QueryQuotaInfotoChainedItemList<FileQuotaInformation>aligns with the new quota API in thesmb-fscc::quotamodule. This makes the quota response data structure consistent with other chained information types in the SMB protocol.
384-385: LGTM! Accessor pattern updated correctly.The change from
unwrap_file()toas_file().unwrap()aligns with the PR objective to removeunwrap_*methods in favor of fallibleas_*accessors. This pattern is more idiomatic Rust and provides better flexibility (e.g., allowingas_file()?.some_method()chains).Also applies to: 417-418
- Add CLI `info --show-quota` - Remove test (samba requires too much setup for now :() misc: - Remove `unwrap_*` methods in query/set cast types - Add some doc - Add `Boolean::Into<bool>` - Add macro for generating `Resource` casts
afiffon
force-pushed
the
130-query-quota-information
branch
from
f44e632 to
15ffa96
Compare
afiffon
force-pushed
the
main
branch
2 times, most recently
from
bae3bdf to
84f9da7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/smb-msg/src/info/common.rs (1)
40-109: Excellent safety improvement!Replacing the
unwrap_*methods withas_*methods that returnResult<Content, SmbMsgError>eliminates panic risks and provides better error handling. The addition ofinfo_type(),name(), andFrom<Content>implementations enhances the API ergonomics.The macro-generated code now:
- Returns descriptive errors via
SmbMsgError::UnexpectedContentinstead of panicking- Provides introspection via
info_type()andname()- Simplifies conversions with
Fromimplementations
♻️ Duplicate comments (5)
crates/smb-fscc/src/quota.rs (2)
15-17: Doc hint fix looks goodReference to ChainedItem/ChainedItemList is now correct; the confusing “Inner” hint is gone.
19-29: Add logical validation for quota fields (optional but helpful)Provide a validate() to catch quota_used/threshold exceeding quota_limit (treat u64::MAX as unlimited).
Example:
impl FileQuotaInformation { pub fn validate(&self) -> Result<(), &'static str> { if self.quota_limit != u64::MAX && self.quota_used > self.quota_limit { return Err("quota_used exceeds quota_limit"); } if self.quota_limit != u64::MAX && self.quota_threshold > self.quota_limit { return Err("quota_threshold exceeds quota_limit"); } Ok(()) } }crates/smb-msg/src/info/set.rs (1)
5-5: Check if NullByte is already re-exported via common:: (avoid redundant import)*If common::* exports NullByte, the explicit NullByte can be dropped; otherwise keep as-is.
#!/bin/bash # Show where NullByte is defined and whether it's re-exported from common. rg -nP --type=rust 'struct\s+NullByte|enum\s+NullByte|type\s+NullByte' crates/smb-msg/src/info -C2 rg -nP --type=rust 'pub\s+use.*NullByte' crates/smb-msg/src/info -C2crates/smb-cli/src/info.rs (2)
254-266: Simplify conversion and avoid re-binding nameMinor cleanup: use a temporary resource binding and a single match.
- let dir = dir_result.unwrap(); - - let dir: Directory = match dir.try_into() { + let resource = dir_result.unwrap(); + let dir: Directory = match resource.try_into() { Ok(dir) => dir, Err(e) => { log::warn!( "Failed to convert resource to directory {}: {}", path_of_subdir, e.0 ); return None; } };
131-145: Tighten skip logic and message for “no limits” quotasMessage says “no limit” but condition checks both limit and threshold. Make the log precise.
-fn display_quota_info(info: &Vec<smb::FileQuotaInformation>) { +fn display_quota_info(info: &[smb::FileQuotaInformation]) { for quota in info { - if quota.quota_limit == u64::MAX && quota.quota_threshold == u64::MAX { - log::trace!("Skipping quota for SID {} with no limit", quota.sid); + if quota.quota_limit == u64::MAX && quota.quota_threshold == u64::MAX { + log::trace!("Skipping quota for SID {} (no limits set)", quota.sid); continue; // No quota set }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
crates/smb-cli/src/info.rs(6 hunks)crates/smb-dtyp/src/binrw_util/helpers.rs(1 hunks)crates/smb-fscc/src/lib.rs(2 hunks)crates/smb-fscc/src/quota.rs(1 hunks)crates/smb-msg/src/info/common.rs(3 hunks)crates/smb-msg/src/info/query.rs(5 hunks)crates/smb-msg/src/info/set.rs(2 hunks)crates/smb/src/resource.rs(3 hunks)crates/smb/src/resource/directory.rs(2 hunks)crates/smb/tests/dialects.rs(0 hunks)
💤 Files with no reviewable changes (1)
- crates/smb/tests/dialects.rs
🧰 Additional context used
🧬 Code graph analysis (4)
crates/smb-cli/src/info.rs (1)
crates/smb-msg/src/info/query.rs (1)
new(152-163)
crates/smb-msg/src/info/query.rs (1)
crates/smb/src/resource.rs (1)
as_file(189-194)
crates/smb-fscc/src/quota.rs (1)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_size(480-497)
crates/smb/src/resource/directory.rs (1)
crates/smb-fscc/src/chained_item.rs (4)
from(125-130)from(162-164)from(173-177)from(222-224)
⏰ 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 (multi_threaded)
- GitHub Check: test (single_threaded)
- GitHub Check: test (async)
🔇 Additional comments (16)
crates/smb-dtyp/src/binrw_util/helpers.rs (1)
128-132: LGTM! Bidirectional conversion is now complete.The implementation correctly provides conversion from
Booleantobool, complementing the existingFrom<bool> for Boolean. This enables idiomatic usage of the wrapper type with automaticInto<bool>support via Rust's blanket implementation.crates/smb/src/resource.rs (2)
449-450: Good switch to typed accessorUsing as_security()? avoids panics and matches other as_* uses.
576-577: Good typed castas_filesystem()? is the safe cast counterpart; consistent with the new pattern.
crates/smb-msg/src/info/set.rs (1)
39-40: Quota payload type change is correctUsing ChainedItemList matches the on‑wire chained list format; aligns with the new FSCC quota types.
crates/smb-fscc/src/lib.rs (1)
29-39: LGTM: quota module exportAdding pub mod quota and pub use quota::* cleanly surfaces the new types.
crates/smb-cli/src/info.rs (6)
43-46: CLI flag addition looks goodshow_quota is optional and defaults to false; clear help text.
88-90: Good: conditional quota queryRunning quota query only when requested avoids extra IO.
152-158: Size formatting improvement approvedu64::MAX as “∞” and humanized sizes are helpful.
230-233: Traversal gating reads wellWhen not listing recursively, still open subdirs when show_quota is true. Intentional and clear.
268-283: Good: query quota per subdir when requestedCloses handle when not recursing; avoids leaks.
285-294: Graceful error handlingtry_query_and_show_quota logs and continues; no hard fail on unsupported servers.
crates/smb-msg/src/info/query.rs (3)
134-134: Nice improvement! Addresses the past review comment.The use of
is_some_andis cleaner and more idiomatic than the previous verbose condition or the suggestedmap_oralternative. This method is specifically designed for this use case.
148-176: LGTM! Well-documented constructors.The two constructors properly implement the two query options from the MS-SMB2 spec (2.2.37.1):
The mutual exclusivity is correctly enforced by the assertion at line 143, and the boolean conversions using
.into()are appropriate.
239-239: Significant but correct API change.The change from
QueryQuotaInfotoChainedItemList<FileQuotaInformation>aligns with the broader quota refactoring in this PR. The tests at lines 384-385 and 417-418 have been updated to use the newas_file()accessor, confirming the migration is consistent across the codebase.crates/smb/src/resource/directory.rs (2)
275-298: LGTM! Clean API with correct conversions.The method signature changes improve clarity:
- Input:
QueryQuotaInfoprovides structured query parameters- Output:
Vec<FileQuotaInformation>returns the quota data directlyThe implementation correctly:
- Extracts boolean flags from the
infoparameter (lines 290-291)- Uses the safe
as_quota()?accessor instead of unwrapping- Converts
ChainedItemList<FileQuotaInformation>toVec<FileQuotaInformation>via theFromimplementation (validated in relevant code snippets)
300-312: LGTM! Correct conversion and clean API.The simplified signature accepting
Vec<FileQuotaInformation>directly is more intuitive. The conversion toChainedItemListat line 304 is valid per theFrom<Vec<T>>implementation inchained_item.rs(lines 172-176 in relevant snippets).
| /// See [`FileGetQuotaInformation`]; use [`FileGetQuotaInformationInner::into`] to convert. | ||
| #[binrw::binrw] | ||
| #[derive(Debug)] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify conversion wording
“use FileGetQuotaInformationInner::into” -> say “use .into() to wrap into ChainedItem” for clarity.
-/// See [`FileGetQuotaInformation`]; use [`FileGetQuotaInformationInner::into`] to convert.
+/// See [`FileGetQuotaInformation`]; use `.into()` to wrap into `ChainedItem<FileGetQuotaInformationInner>`.📝 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.
| /// See [`FileGetQuotaInformation`]; use [`FileGetQuotaInformationInner::into`] to convert. | |
| #[binrw::binrw] | |
| #[derive(Debug)] | |
| /// See [`FileGetQuotaInformation`]; use `.into()` to wrap into `ChainedItem<FileGetQuotaInformationInner>`. | |
| #[binrw::binrw] | |
| #[derive(Debug)] |
🤖 Prompt for AI Agents
In crates/smb-fscc/src/quota.rs around lines 31 to 33, update the doc comment
that currently reads “use FileGetQuotaInformationInner::into” to instead say
“use .into() to wrap into ChainedItem<FileGetQuotaInformationInner>” so it
clearly instructs callers to call .into() to produce a
ChainedItem<FileGetQuotaInformationInner>; keep the surrounding text and
references unchanged.
| /// Generates TryInto implementations for Resource enum variants. | ||
| macro_rules! make_resource_try_into { | ||
| ( | ||
| $($t:ident,)+ | ||
| ) => { | ||
| $( | ||
|
|
||
| fn try_into(self) -> Result<File, Self::Error> { | ||
| match self { | ||
| Resource::File(f) => Ok(f), | ||
| _ => Err(Error::InvalidArgument("Not a file".into())), | ||
| } | ||
| } | ||
| } | ||
| impl TryInto<$t> for Resource { | ||
| type Error = (crate::Error, Self); | ||
|
|
||
| impl TryInto<Directory> for Resource { | ||
| type Error = crate::Error; | ||
|
|
||
| fn try_into(self) -> Result<Directory, Self::Error> { | ||
| fn try_into(self) -> Result<$t, Self::Error> { | ||
| match self { | ||
| Resource::Directory(d) => Ok(d), | ||
| _ => Err(Error::InvalidArgument("Not a directory".into())), | ||
| Resource::$t(f) => Ok(f), | ||
| x => Err((Error::InvalidArgument(format!("Not a {}", stringify!($t))), x)), | ||
| } | ||
| } | ||
| } | ||
| )+ | ||
| }; | ||
| } | ||
|
|
||
| make_resource_try_into!(File, Directory, Pipe,); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer TryFrom for idiomatic conversions (lets std provide TryInto automatically)
Implement TryFrom for each variant instead of TryInto; callers keep using .try_into(), but you get the blanket impl and a more idiomatic API.
Apply this diff:
-/// Generates TryInto implementations for Resource enum variants.
-macro_rules! make_resource_try_into {
+/// Generates TryFrom<Resource> implementations for enum variants and uses the blanket TryInto.
+macro_rules! make_resource_try_from {
(
$($t:ident,)+
) => {
$(
-
-impl TryInto<$t> for Resource {
- type Error = (crate::Error, Self);
-
- fn try_into(self) -> Result<$t, Self::Error> {
- match self {
- Resource::$t(f) => Ok(f),
- x => Err((Error::InvalidArgument(format!("Not a {}", stringify!($t))), x)),
- }
- }
-}
+impl TryFrom<Resource> for $t {
+ type Error = (crate::Error, Resource);
+ fn try_from(value: Resource) -> Result<$t, Self::Error> {
+ match value {
+ Resource::$t(v) => Ok(v),
+ other => Err((Error::InvalidArgument(format!("Not a {}", stringify!($t))), other)),
+ }
+ }
+}
)+
};
}
-make_resource_try_into!(File, Directory, Pipe,);
+make_resource_try_from!(File, Directory, Pipe,);Also applies to: 247-247
info --show-quotamisc:
unwrap_*methods in query/set cast typesBoolean::Into<bool>ResourcecastsSummary by CodeRabbit
New Features
Documentation