Document and format smb-msg #157
Conversation
Remove leftover conditional server/client compilation
Add note on auth to bug report template
- ChainedItemList simplified, support for items without Read+Write - Fix more client/server feature stuff in smb-msg
mroe reserved fields fixups templatize negotiation ctx getters
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughLarge workspace refactor: adds two proc-macro crates, migrates many bitfield/binrw derives to new mbitfield and smb_msg_derive macros, rewrites numerous SMB message types and ChainedItemList, adds #[forbid(unsafe_code)] across crates, updates CI, manifests, docs, and test macros. Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as Proc-macros (smb_msg_derive / smb_dtyp_derive)
participant Item as Source Item (struct/enum)
participant Generated as Generated Item (with injected fields/attrs)
participant Binrw as binrw / runtime IO
Item->>Proc: attribute macro invocation (e.g., #[smb_request], #[mbitfield])
Proc-->>Generated: inject size/reserved/temp attrs, derives, cfgs
Generated->>Binrw: uses generated BinRead/BinWrite mapping
Binrw-->>Generated: serialize / deserialize per injected logic
sequenceDiagram
participant Caller
participant Old as Old ChainedItemList (per-item wrapper)
participant New as New ChainedItemList (Vec<T> + custom BinRead/BinWrite)
participant IO
Caller->>Old: read item sequence (wrapper per item)
Old->>IO: read next_offset, read wrapper+value, repeat
Caller->>New: read items until next_entry_offset == 0
New->>IO: read next_entry_offset, validate alignment, read value, push Vec<T>, repeat
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
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 |
Move smb-cli outside of crates/ Make the CI shorter
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (20)
crates/smb/tests/dialects.rs (1)
23-23: Fix typo in function name.The function name contains a typo:
encrpytionshould beencryption. While the code works because the typo is consistent between the call site and definition, it should be corrected for clarity.Apply this diff to fix the typo:
- pub async fn [<test_smbint_ $dialect:lower _e $encrypt_mode:lower>]() -> Result<(), Box<dyn std::error::Error>> { - test_smb_integration_dialect_encrpytion_mode(Dialect::$dialect, EncryptionMode::$encrypt_mode).await + pub async fn [<test_smbint_ $dialect:lower _e $encrypt_mode:lower>]() -> Result<(), Box<dyn std::error::Error>> { + test_smb_integration_dialect_encryption_mode(Dialect::$dialect, EncryptionMode::$encrypt_mode).await-async fn test_smb_integration_dialect_encrpytion_mode( +async fn test_smb_integration_dialect_encryption_mode(Also applies to: 46-46
crates/smb/src/dialects.rs (2)
183-195: Signing algorithm selection only considers the first advertised entryThis is functionally fine, but if a server advertises multiple
signing_algorithmsand the first one is unsupported while a later one is supported, negotiation will fail even though a compatible option exists. Consider scanning the list for the first supported algorithm instead of hard-failing on the first entry.For example, conceptually:
- let signing_algo = if let Some(signing_algo) = - ctx_signing.and_then(|ctx| ctx.signing_algorithms.first()) - { - if !crypto::SIGNING_ALGOS.contains(signing_algo) { - return Err(Error::NegotiationError( - "Unsupported signing algorithm selected!".into(), - )); - } - Some(signing_algo) - } else { - None - }; + let signing_caps = response.get_ctx_signing_capabilities(); + let signing_algo = signing_caps.and_then(|ctx| { + ctx.signing_algorithms + .iter() + .find(|algo| crypto::SIGNING_ALGOS.contains(algo)) + }); + if signing_caps.is_some() && signing_algo.is_none() { + return Err(Error::NegotiationError( + "No supported signing algorithm offered by server".into(), + )); + }You can still keep
state.signing_algo = signing_algo.copied();as-is.
208-220: Encryption handling is reasonable; confirm behavior for unsupported-but-optional ciphersThe logic correctly:
- Validates the (first) advertised cipher against
crypto::ENCRYPTING_ALGOS.- Fails negotiation when encryption is required but the server provides no ciphers.
One behavioral nuance: if
encryption_modeis not required and the server advertises only unsupported ciphers, negotiation will currently fail with “Unsupported encryption algorithm received” instead of silently downgrading to no encryption. That might be intended (strict) or you might prefer to mirror the “scan for first supported cipher” approach and only error when encryption is required and no supported cipher exists.If you want the more tolerant behavior, analogous to signing:
- Search
ctx.ciphersfor the first supported cipher.- Only treat “no supported cipher” as fatal when
encryption_mode.is_required()is true.crates/smb-fscc/src/chained_list.rs (2)
1-5: Top‑level docs still describe a per‑item wrapper, not the new list‑centric APIThe intro comments talk about “this struct wraps the value, and the offset”, which no longer matches the design now that only
ChainedItemList<T, OFFSET_PAD>exists andTis stored directly. There’s also a small typo (“genric”).Consider updating these docs to describe the list‑level encoding (per‑entry prefix
next_entry_offset, alignment viaOFFSET_PAD, andVec<T>-like ergonomics) and fixing the typo so that users aren’t misled by the oldChainedItemwording.
34-57: Offset/alignment logic looks sound, butOFFSET_PAD == 0is a footgunThe new vec‑backed
ChainedItemList<T, OFFSET_PAD>and itsBinWrite/BinReadimplementations look correct:
- Writing:
- For each item, you emit a placeholder
next_entry_offset, writeT, then (for non‑last items) pad up to the nextOFFSET_PADboundary.- You then patch
next_entry_offsettoposition_after - position_before, so the offset spans prefix + payload + padding.- For the last element you rely on the initial placeholder
0and skip the padding, which matches the “0 == no next entry” convention.- Reading:
- You correctly detect an empty list by comparing current position to a computed
stream_end(nice fit withtake_seek).- You assert
position_before % OFFSET_PAD == 0for each entry, enforce that invariant, and usenext_item_offsetto jump to the next entry.Two edge cases are worth tightening:
OFFSET_PAD == 0will panic:
- Both the alignment assert and padding computation do modulo by
OFFSET_PADand will panic if it’s zero.- Since
OFFSET_PADis a const generic with a default of 4, callers can accidentally instantiateChainedItemList<T, 0>.Initial alignment is only enforced on read:
BinReadasserts every item’s start is aligned, butBinWritedoesn’t check the start position of the first item.- You currently rely on the surrounding field using
align_before(as in the CREATE contexts). Adebug_assert_eq!(writer.stream_position()? % OFFSET_PAD as u64, 0)before the loop would make misuse easier to catch.Suggestion:
- At minimum, document that
OFFSET_PADmust be non‑zero and that callers are responsible for aligning the start of the list.- Preferably, add a
debug_assert!(OFFSET_PAD > 0)and adebug_assert_eq!(writer.stream_position()? % OFFSET_PAD as u64, 0)insidewrite_optionsto surface configuration mistakes early.Also applies to: 59-113, 116-167
crates/smb-msg/src/create.rs (2)
19-53:FileIdconversions rely on invariants but introduceunwrap()panicsThe new
FileIdhelpers are convenient, but they do rely on assumptions:
From<[u8; 16]>:
- Uses
Self::read_le(&mut cursor).unwrap(). With#[binrw::binrw]on a simpleu64, u64struct and an in‑memoryCursor<[u8; 16]>, this should be infallible, but any future change to the binrw attributes (e.g., extra asserts) could turn this into a panic.From<Guid>:
- Serializes the GUID into a
Vec<u8>and then doestry_into().unwrap()to force it into a[u8; 16]. This assumes the GUID always serializes to exactly 16 bytes.Given these invariants are effectively guaranteed by the types (
Guidbeing 128‑bit and the binrw layout being fixed), this is probably acceptable, but the panic surface is non‑obvious.If you want to make this more robust, you could:
- Use
expect("FileId::read_le must succeed for 16 bytes")with a descriptive message, or- Provide fallible versions (e.g.,
try_from_bytes) alongside theseFromimpls and keep the panicking ones as thin wrappers.
309-405: CREATE context macro: offset math withCHAINED_ITEM_PREFIX_SIZEand helper APIsThe
create_context_half!macro is doing a lot, but the pieces line up:
Struct layout:
_name_offsetand_data_offsetarePosMarker<u16>fields with comments and seek logic that treat the base of the chained entry as “start of entry minusCHAINED_ITEM_PREFIX_SIZE”.- With
CHAINED_ITEM_PREFIX_SIZE = size_of::<NextEntryOffsetType>() == 4, the expression
seek_before = _name_offset.seek_from(_name_offset.value as u64 - CHAINED_ITEM_PREFIX_SIZE as u64)
resolves tobase + name_offset, wherebaseis the start of the entry (right where theNextEntryOffsetlives).- The analogous expression for data,
seek_before = _name_offset.seek_from_if(_data_offset.value as u64 - CHAINED_ITEM_PREFIX_SIZE as u64, _data_length.value > 0),
similarly yieldsbase + data_offsetwhen data is present.- Using
_name_offset.posas the reference for both is fine becausebase = _name_offset.pos - CHAINED_ITEM_PREFIX_SIZE.Write side:
PosMarker::write_roff_plusandwrite_roff_size_b_pluswithoffset_plus = CHAINED_ITEM_PREFIX_SIZE as u64match the same “offset from entry start” convention for the on‑wireNameOffset/DataOffsetfields.align_before = 8and the_data_offset.value % 8 == 0assert keep the payloads 8‑aligned as required.
Dataenum and helpers:
#[br(pre_assert(name.as_slice() == CreateContextType::FOO.name()))]gives a nice type‑safe dispatch on context name.as_*andfirst_*helpers are convenient; thefirst_*implementation walks aVec<CreateContext...>and matches onctx.data, which is straightforward.Two API nits:
The
first_*helpers take&Vec<CreateContext...>:
- Given that the rest of the code now uses
ChainedItemList<CreateContext..., 8>, it would be more flexible to take&[CreateContext...]instead, allowing direct use on bothVecandChainedItemListvia itsDeref<[T]>.
TryInto<$req_type>error reporting:
- The
UnexpectedContenterror hasactual: ""with a TODO about exposing a stringified name.- It might be worth at least wiring through
CreateContextType::from_name(self.data.name())and formatting that, even if you don’t convert it to UTF‑8 yet.Overall, the offset math and list integration look correct; the main changes I’d consider are those small API tweaks to make the helpers easier to use and errors more informative.
Also applies to: 407-433
crates/smb-msg/src/message.rs (1)
6-21: Fix small docstring spacing inmake_message!
concat!("This struct represents all the ", stringify!($name), "message types.")produces “Requestmessage”/“Responsemessage” without a space. Consider inserting a space before" message types.":- #[doc = concat!("This struct represents all the ", stringify!($name), "message types.")] + #[doc = concat!("This struct represents all the ", stringify!($name), " message types.")]crates/smb-msg/src/info/set.rs (1)
86-118: Fix the proposed error construction; useself.name()instead ofself.info_type().into()The review correctly identifies that
to_req()should returnResult<SetInfoRequest, SmbMsgError>instead of panicking. However, the proposed diff has an implementation bug:
info_type()returnsInfoTypeenum, not&'static str- The error field
actualexpects&'static strInfoTypehas noInto<&'static str>implementationThe codebase already provides a
name()method (generated by the same macro at line 89–95 ofcommon.rs) that returns&'static str. Use that instead:- _ => { - return Err(SmbMsgError::UnexpectedContent { - expected: "matching SetInfoClass and SetInfoData variant", - actual: self.info_type().into(), - }); - } + _ => { + return Err(SmbMsgError::UnexpectedContent { + expected: "matching SetInfoClass and SetInfoData variant", + actual: self.name(), + }); + }There is one caller at
crates/smb/src/resource.rs:416that will need updating to handleResult:let data = data.into().to_req(cls, self.file_id()?, additional_info)?;crates/smb-msg/src/compressed.rs (2)
99-106: Fix type mismatch inbwattribute on line 130The
add_original_size_to_total_lengthfunction expects&CompressionAlgorithm, but line 130'sbwattribute passescompression_algorithmby value instead of by reference. Line 129'sbrattribute correctly uses&compression_algorithm.- #[bw(write_with = PosMarker::write_size_plus, args(&length, add_original_size_to_total_length(compression_algorithm)))] + #[bw(write_with = PosMarker::write_size_plus, args(&length, add_original_size_to_total_length(&compression_algorithm)))]Consider also guarding the subtraction on line 129 against underflow for malformed input.
25-38:CompressedMessage::total_sizeundercounts chained message sizesFor the
Chainedvariant,total_sizeonly sumspayload_data.len()plusm.items.len() * 4and the top-levelSTRUCT_SIZE. This omits:
- The fixed per-item header (
CompressionAlgorithm+flags+length= 2 + 2 + 4 = 8 bytes per item).- The optional
original_size(u32, 4 bytes) for algorithms whereoriginal_size_required()is true.Current code only accounts for 4 bytes per item instead of 8, and completely misses the conditional 4-byte
original_sizefield. The reported size is too small for every item.Consider something along these lines instead:
- pub fn total_size(&self) -> usize { - match self { - CompressedMessage::Unchained(m) => { - m.data.len() + CompressedUnchainedMessage::STRUCT_SIZE - } - CompressedMessage::Chained(m) => { - m.items.iter().map(|i| i.payload_data.len()).sum::<usize>() - + m.items.len() * 4 - + CompressedChainedMessage::STRUCT_SIZE - } - } - } + pub fn total_size(&self) -> usize { + match self { + CompressedMessage::Unchained(m) => { + CompressedUnchainedMessage::STRUCT_SIZE + m.data.len() + } + CompressedMessage::Chained(m) => { + let item_header_base = + std::mem::size_of::<CompressionAlgorithm>() + + std::mem::size_of::<u16>() + + std::mem::size_of::<u32>(); + + CompressedChainedMessage::STRUCT_SIZE + + m.items + .iter() + .map(|item| { + let extra_original = if item.compression_algorithm.original_size_required() { + std::mem::size_of::<u32>() + } else { + 0 + }; + item.payload_data.len() + item_header_base + extra_original + }) + .sum::<usize>() + } + } + }crates/smb-msg/src/info/query.rs (1)
132-167:QueryQuotaInfoconstructors leave required fields uninitialisedBoth
QueryQuotaInfo::new()andQueryQuotaInfo::new_sid()constructors initialise only 4 fields but the struct has 8. The missing fields (reserved,sid_list_length,start_sid_length,start_sid_offset) cause compilation errors.Initialise all fields explicitly in both constructors:
impl QueryQuotaInfo { pub fn new( return_single: bool, restart_scan: bool, content: Vec<FileGetQuotaInformation>, ) -> Self { Self { return_single: return_single.into(), restart_scan: restart_scan.into(), + reserved: 0, + sid_list_length: PosMarker::default(), + start_sid_length: PosMarker::default(), + start_sid_offset: PosMarker::default(), get_quota_info_content: Some(content.into()), sid: None, } } pub fn new_sid(return_single: bool, restart_scan: bool, sid: SID) -> Self { Self { return_single: return_single.into(), restart_scan: restart_scan.into(), + reserved: 0, + sid_list_length: PosMarker::default(), + start_sid_length: PosMarker::default(), + start_sid_offset: PosMarker::default(), get_quota_info_content: None, sid: Some(sid), } } }Lines 169–197 in
crates/smb-msg/src/info/query.rs.crates/smb-msg/src/negotiate.rs (1)
215-228:impl From<$name> for NegotiateContextomits required field initializationsThe macro-generated
Fromimplementation at lines 269–276 only initializescontext_typeanddata:impl From<$name> for NegotiateContext { fn from(val: $name) -> Self { NegotiateContext { context_type: NegotiateContextType::$name, data: NegotiateContextValue::$name(val), } } }This is a compile error;
NegotiateContexthas four fields and bothdata_lengthandreservedmust be initialized:impl From<$name> for NegotiateContext { fn from(val: $name) -> Self { NegotiateContext { context_type: NegotiateContextType::$name, + data_length: PosMarker::default(), + reserved: 0, data: NegotiateContextValue::$name(val), } } }Additionally,
RdmaTransformCapabilities(lines 482–492) declares the fieldreservedtwice with different types (u16andu32), which is a syntax error:pub struct RdmaTransformCapabilities { #[bw(try_calc(u16::try_from(transforms.len())))] transform_count: u16, reserved: u16, // ← duplicate name reserved: u32, // ← duplicate name #[br(count = transform_count)] pub transforms: Vec<RdmaTransformId>, }Rename one of them to match the SMB2 specification (likely the first should be
reserved1: u16).crates/smb-msg/src/smb1.rs (1)
11-52: Code will not compile due to duplicate field names and incomplete Default impl — all identified issues confirmedThe review comment is accurate and identifies genuine critical compilation errors:
Duplicate
reservedfields confirmed: The struct contains three fields all namedreserved: u16(lines 32, 39, 41), which is a Rust compilation error.Macro renaming strategy fails: The
common_struct_changesmacro usesproc_macro2::Span::call_site().start().lineto generate unique identifiers for reserved fields. However,call_site()returns the macro invocation location (the#[smb_request_binrw]line), not the individual field locations. All three reserved fields will be renamed to the same identifier (e.g.,_reserved15), still causing collisions after macro expansion.Default impl missing required fields confirmed: The
Defaultimplementation (lines 63-84) initializes only 6 fields but the struct has 14 fields total. Missing:_command,_pid_high,_tid,_pid_low,_word_count, and all threereservedfields. Even fields with#[br(temp)]or#[bw(calc = ...)]attributes must be initialized inSelf { ... }.The recommended fixes are appropriate: rename the duplicate fields to unique names before macro expansion (e.g.,
uid_reserved,mid_reserved), updatecommon_struct_changesto generate genuinely unique identifiers (e.g., using a per-field counter), and initialize all struct fields inDefault.crates/smb-msg/src/tree_connect.rs (2)
80-181: RemotedIdentityTreeConnect and Blob/Array helpers look well-structured; consider auto-calculating sizesThe
TreeConnectContext+make_remoted_identity_connect!macro,BlobData<T>,ArrayData<T>,SidAttrData,SidAttrSeGroup, andLuidAttrDataform a coherent representation of the remoted identity TREE_CONNECT context. The offset/size handling withPosMarkerandwrite_roff_bis a nice fit for the spec’s layout.One thing to consider for robustness:
RemotedIdentityTreeConnect.ticket_sizeandBlobData::blob_sizeare currently caller-controlled; there’s no#[bw(calc = ..)]or assert tying them to the actual serialized size. If a caller passes inconsistent values, you can end up emitting malformed packets.If feasible, adding
bw(calc = ..)and/orbr(assert(..))on these size markers would make the encoding more self-consistent and reduce manual bookkeeping for users of these types.Given the complexity of the remoted identity layout, please sanity‑check these helpers against the examples in MS-SMB2 2.2.9.2.1 and existing interop traces before tightening any assertions.
26-78: Address flag-context consistency to prevent malformed wire protocolThe concern is valid. MS-SMB2 specifies that when the RemotedIdentity parameter is true, the client MUST add a tree connect request extension and MUST set the SMB2_TREE_CONNECT_FLAG_EXTENSION_PRESENT bit in the Flags field. Additionally, if the EXTENSION_PRESENT bit is set, the extension structure MUST be added at the beginning of the Buffer field.
The current implementation creates an asymmetry: read operations check
flags.extension_present()while write operations check!tree_connect_contexts.is_empty(). Sinceflagsandtree_connect_contextsare public fields, code can construct invalid states (e.g., flag=false with non-empty contexts, or flag=true with empty contexts).The suggested
#[bw(assert(...))]would catch these at serialization time. However, since this is a protocol-critical invariant, consider also enforcing it during construction through a builder pattern or validation method to prevent invalid intermediate states.Verify test coverage includes both base (no extension) and extension-present cases to ensure round-trip consistency.
crates/smb-msg/src/notify.rs (2)
16-100: ChangeNotifyRequest, NotifyFlags, and NotifyFilter definitions look goodThe
ChangeNotifyRequestlayout (flags, output_buffer_length, file_id, completion_filter, reserved) and theNotifyFlags/NotifyFilterbitfields closely follow the MS-SMB2 2.2.35 structures, and theNotifyFilter::all()helper is a convenient way to express “monitor everything”.Only minor potential improvement: similar to other protocol structs, you might want to make the trailing
reserved: u32write as zero automatically via#[bw(calc = 0)]and either drop or assert it on read.If you tighten the reserved handling, please verify that existing fixtures (especially the
ChangeNotifyRequesttest) still match their expected hex.
125-181: ServerToClientNotification/Notification plumbing is correct; consider enforcingstructure_sizeThe
ServerToClientNotification,NotificationType,Notification(with importednotification_type), andNotifySessionClosedtypes correctly express the SMB2 server-to-client notification pattern and allow future extension with new notification variants.For extra safety you might:
- Add a
#[bw(calc = 9)]and#[br(assert(structure_size == 9))]onServerToClientNotification::structure_sizeto match the spec’s fixed structure size and prevent malformed messages.Example:
#[smb_response_binrw] pub struct ServerToClientNotification { - /// Size of the SMB2_SERVER_TO_CLIENT_NOTIFICATION structure. - structure_size: u16, + /// Size of the SMB2_SERVER_TO_CLIENT_NOTIFICATION structure. + #[bw(calc = 9)] + #[br(assert(structure_size == 9))] + structure_size: u16,After adding such an assertion, please ensure any existing notification fixtures still round‑trip correctly.
crates/smb-msg/src/dfsc.rs (1)
107-117: Bitfield definitions: duplicate__names and a typo intarget_failbacl
ReferralHeaderFlagsuses field nametarget_failbacl, which looks like a typo fortarget_failbackand will be part of the public API.- Both
ReferralEntryFlagsandReferralEntryFlagsV4declare two#[skip]fields with the same identifier__in the same struct. Withsmb_dtyp::mbitfield(wrappingmodular_bitfield), duplicate field names are at best confusing and are likely to cause compilation or tooling issues.Consider:
- Renaming
target_failbacltotarget_failback.- Giving the skipped fields distinct names (e.g.
__reserved0,__reserved1) or merging them into a single wider skipped field if layout permits.Also applies to: 262-269, 382-391
crates/smb-msg/src/ioctl/msg.rs (1)
176-248: Avoidunwrap()inIoctlResponse::parse_fsctlto prevent panics on malformed data
parse_fsctlcurrently does:let mut cursor = std::io::Cursor::new(&self.out_buffer); Ok(T::read_le(&mut cursor).unwrap())This will panic on any
BinReaderror (truncated buffer, malformed server response), even though the method returnscrate::Result<T>and already handles one error case (MissingFsctlDefinition) gracefully. Since IOCTL responses come from the network, panicking here is a reliability and safety issue.Consider mapping the
BinReaderror into your error type instead, e.g.:pub fn parse_fsctl<T>(&self) -> crate::Result<T> where T: FsctlResponseContent, { if !T::FSCTL_CODES.iter().any(|&f| f as u32 == self.ctl_code) { return Err(crate::SmbMsgError::MissingFsctlDefinition(self.ctl_code)); } let mut cursor = std::io::Cursor::new(&self.out_buffer); T::read_le(&mut cursor).map_err(Into::into) }(or another appropriate conversion into
crate::SmbMsgError).
📜 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 (72)
.github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/copilot-instructions.md(1 hunks).github/workflows/build.yml(1 hunks)Cargo.toml(2 hunks)crates/smb-dtyp-derive/Cargo.toml(1 hunks)crates/smb-dtyp-derive/src/lib.rs(1 hunks)crates/smb-dtyp/Cargo.toml(1 hunks)crates/smb-dtyp/src/binrw_util/boolean.rs(1 hunks)crates/smb-dtyp/src/binrw_util/helpers.rs(1 hunks)crates/smb-dtyp/src/binrw_util/multi_sz.rs(1 hunks)crates/smb-dtyp/src/lib.rs(2 hunks)crates/smb-dtyp/src/security/ace.rs(8 hunks)crates/smb-dtyp/src/security/acl.rs(1 hunks)crates/smb-dtyp/src/security/security_descriptor.rs(2 hunks)crates/smb-fscc/Cargo.toml(2 hunks)crates/smb-fscc/src/chained_list.rs(4 hunks)crates/smb-fscc/src/common_info.rs(3 hunks)crates/smb-fscc/src/directory_info.rs(5 hunks)crates/smb-fscc/src/file_attributes.rs(1 hunks)crates/smb-fscc/src/filesystem_info.rs(5 hunks)crates/smb-fscc/src/lib.rs(1 hunks)crates/smb-fscc/src/query_file_info.rs(3 hunks)crates/smb-fscc/src/quota.rs(2 hunks)crates/smb-fscc/src/set_file_info.rs(2 hunks)crates/smb-msg-derive/Cargo.toml(1 hunks)crates/smb-msg-derive/src/lib.rs(1 hunks)crates/smb-msg/Cargo.toml(2 hunks)crates/smb-msg/README.md(1 hunks)crates/smb-msg/src/cancel.rs(1 hunks)crates/smb-msg/src/compressed.rs(3 hunks)crates/smb-msg/src/create.rs(13 hunks)crates/smb-msg/src/dfsc.rs(15 hunks)crates/smb-msg/src/echo.rs(2 hunks)crates/smb-msg/src/encrypted.rs(2 hunks)crates/smb-msg/src/error.rs(2 hunks)crates/smb-msg/src/file.rs(5 hunks)crates/smb-msg/src/header.rs(6 hunks)crates/smb-msg/src/info.rs(1 hunks)crates/smb-msg/src/info/common.rs(3 hunks)crates/smb-msg/src/info/query.rs(8 hunks)crates/smb-msg/src/info/set.rs(5 hunks)crates/smb-msg/src/ioctl.rs(1 hunks)crates/smb-msg/src/ioctl/fsctl.rs(32 hunks)crates/smb-msg/src/ioctl/msg.rs(2 hunks)crates/smb-msg/src/lib.rs(1 hunks)crates/smb-msg/src/lock.rs(1 hunks)crates/smb-msg/src/message.rs(2 hunks)crates/smb-msg/src/negotiate.rs(11 hunks)crates/smb-msg/src/notify.rs(4 hunks)crates/smb-msg/src/oplock.rs(2 hunks)crates/smb-msg/src/plain.rs(5 hunks)crates/smb-msg/src/query_dir.rs(2 hunks)crates/smb-msg/src/session_setup.rs(3 hunks)crates/smb-msg/src/smb1.rs(4 hunks)crates/smb-msg/src/test.rs(2 hunks)crates/smb-msg/src/tree_connect.rs(9 hunks)crates/smb-rpc/Cargo.toml(2 hunks)crates/smb-rpc/src/interface/srvsvc.rs(1 hunks)crates/smb-rpc/src/pdu.rs(7 hunks)crates/smb-transport/Cargo.toml(2 hunks)crates/smb-transport/src/lib.rs(1 hunks)crates/smb-transport/src/rdma/smbd.rs(4 hunks)crates/smb/Cargo.toml(1 hunks)crates/smb/src/compression.rs(1 hunks)crates/smb/src/dialects.rs(3 hunks)crates/smb/src/lib.rs(1 hunks)crates/smb/src/session.rs(1 hunks)crates/smb/src/session/encryptor_decryptor.rs(1 hunks)crates/smb/src/session/signer.rs(1 hunks)crates/smb/tests/dialects.rs(1 hunks)crates/smb/tests/long_dir.rs(0 hunks)smb-cli/Cargo.toml(2 hunks)
💤 Files with no reviewable changes (1)
- crates/smb/tests/long_dir.rs
🧰 Additional context used
🧬 Code graph analysis (35)
crates/smb-dtyp/src/lib.rs (1)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-fscc/src/file_attributes.rs (1)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/test.rs (1)
crates/smb-rpc/src/pdu.rs (1)
test_request_write(512-552)
crates/smb-rpc/src/interface/srvsvc.rs (1)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-transport/src/rdma/smbd.rs (1)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-rpc/src/pdu.rs (1)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/message.rs (1)
crates/smb-msg-derive/src/lib.rs (2)
smb_request_binrw(230-232)smb_response_binrw(238-240)
crates/smb-msg/src/info/common.rs (2)
crates/smb-msg-derive/src/lib.rs (1)
smb_message_binrw(246-248)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/cancel.rs (1)
crates/smb-msg-derive/src/lib.rs (1)
smb_request(206-208)
crates/smb-msg/src/smb1.rs (1)
crates/smb-msg-derive/src/lib.rs (1)
smb_request_binrw(230-232)
crates/smb-msg/src/info/set.rs (1)
crates/smb-msg-derive/src/lib.rs (3)
smb_request(206-208)smb_message_binrw(246-248)smb_response(214-216)
crates/smb-msg/src/tree_connect.rs (2)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)crates/smb-msg-derive/src/lib.rs (4)
smb_request(206-208)smb_request_binrw(230-232)smb_response(214-216)smb_response_binrw(238-240)
crates/smb-msg/src/echo.rs (1)
crates/smb-msg-derive/src/lib.rs (1)
smb_request_response(222-224)
crates/smb-fscc/src/common_info.rs (1)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/lock.rs (2)
crates/smb-msg-derive/src/lib.rs (3)
smb_request(206-208)smb_request_binrw(230-232)smb_response(214-216)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-dtyp/src/security/ace.rs (1)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-dtyp/src/security/security_descriptor.rs (1)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/error.rs (1)
crates/smb-msg-derive/src/lib.rs (2)
smb_response(214-216)smb_response_binrw(238-240)
crates/smb-msg/src/encrypted.rs (1)
crates/smb-msg-derive/src/lib.rs (1)
smb_message_binrw(246-248)
crates/smb-msg/src/query_dir.rs (4)
crates/smb-msg-derive/src/lib.rs (2)
smb_request(206-208)smb_response(214-216)crates/smb-dtyp/src/binrw_util/sized_string.rs (2)
size(22-24)bytes16(42-44)crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_aoff(344-363)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/session_setup.rs (3)
crates/smb-msg-derive/src/lib.rs (2)
smb_request(206-208)smb_response(214-216)crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_aoff(344-363)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/header.rs (2)
crates/smb-msg-derive/src/lib.rs (2)
smb_message_binrw(246-248)smb_request_response(222-224)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/dfsc.rs (2)
crates/smb-msg-derive/src/lib.rs (1)
smb_request_binrw(230-232)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/compressed.rs (2)
crates/smb-msg-derive/src/lib.rs (1)
smb_message_binrw(246-248)crates/smb-msg/src/negotiate.rs (1)
original_size_required(413-421)
crates/smb-msg/src/negotiate.rs (4)
crates/smb-msg-derive/src/lib.rs (3)
smb_request(206-208)smb_response(214-216)smb_message_binrw(246-248)crates/smb-dtyp/src/binrw_util/sized_string.rs (3)
size(22-24)try_from(185-188)try_from(256-258)crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_aoff(344-363)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/info/query.rs (3)
crates/smb-msg-derive/src/lib.rs (5)
smb_request(206-208)smb_request_binrw(230-232)smb_message_binrw(246-248)smb_response(214-216)smb_response_binrw(238-240)crates/smb-dtyp/src/binrw_util/pos_marker.rs (2)
write_aoff_size_a(545-570)write_size(602-620)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/ioctl/msg.rs (3)
crates/smb-msg-derive/src/lib.rs (3)
smb_request(206-208)smb_request_binrw(230-232)smb_response(214-216)crates/smb-dtyp/src/binrw_util/pos_marker.rs (2)
write_aoff_size(573-597)write_aoff(344-363)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/ioctl/fsctl.rs (3)
crates/smb-msg-derive/src/lib.rs (3)
smb_message_binrw(246-248)smb_request_binrw(230-232)smb_response_binrw(238-240)crates/smb-dtyp/src/binrw_util/pos_marker.rs (2)
write_size(602-620)fmt(652-657)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-fscc/src/chained_list.rs (1)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (2)
write_options(92-105)read_options(73-83)
crates/smb-fscc/src/filesystem_info.rs (1)
crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/plain.rs (1)
crates/smb-msg-derive/src/lib.rs (2)
smb_request_binrw(230-232)smb_response_binrw(238-240)
crates/smb-msg/src/file.rs (3)
crates/smb-msg-derive/src/lib.rs (3)
smb_request(206-208)smb_response(214-216)smb_request_binrw(230-232)crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_aoff(344-363)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/create.rs (3)
crates/smb-msg-derive/src/lib.rs (6)
smb_request(206-208)smb_request_binrw(230-232)smb_response(214-216)smb_response_binrw(238-240)smb_message_binrw(246-248)smb_request_response(222-224)crates/smb-dtyp/src/binrw_util/pos_marker.rs (4)
write_roff_size(421-445)seek_from(24-26)write_roff_size_b_plus(481-506)seek_from_if(29-35)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
crates/smb-msg/src/notify.rs (3)
crates/smb-msg-derive/src/lib.rs (3)
smb_request(206-208)smb_response(214-216)smb_response_binrw(238-240)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_aoff_size(573-597)
crates/smb-msg/src/oplock.rs (2)
crates/smb-msg-derive/src/lib.rs (4)
smb_request_response(222-224)smb_response(214-216)smb_message_binrw(246-248)parse(49-69)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-19)
🪛 GitHub Actions: Build
crates/smb-msg/src/smb1.rs
[warning] 5-5: unused import: binrw::io::TakeSeekExt
crates/smb-msg/src/dfsc.rs
[warning] 5-5: unused import: io::TakeSeekExt
🪛 Gitleaks (8.29.0)
crates/smb-msg/src/oplock.rs
[high] 152-152: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
crates/smb-msg/README.md
[uncategorized] ~22-~22: The official name of this software platform is spelled with a capital “H”.
Context: ...dingly (you can see the instructions in .github/copilot-instructions.md :)). My tip f...
(GITHUB)
[style] ~26-~26: Consider a different adjective to strengthen your wording.
Context: ...fficial documentation, so you can get a deep understanding of the protocol that way....
(DEEP_PROFOUND)
.github/copilot-instructions.md
[style] ~18-~18: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1432 characters long)
Context: ...example, or bitfield structs - of course! - NEVER document reserved fields.
(EN_EXCESSIVE_EXCLAMATION)
afiffon
force-pushed
the
144-document-smb-msg
branch
from
1955c22 to
f0c051e
Compare
afiffon
force-pushed
the
144-document-smb-msg
branch
from
f0c051e to
070a7e3
Compare
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/smb-msg/src/error.rs (2)
65-92: Reduce duplication betweenas_u32andas_u64with a small helper (optional)The
as_u32andas_u64methods are nearly identical aside from element size and conversion function. The current implementation is correct and safe (the length check guarantees theunwrap()), but you could slightly simplify and centralize the length check.One possible refactor:
impl ErrorResponseContext { - /// Interprets the error data as a u32, if possible. - /// Returns an error if the data length is not 4 bytes. - pub fn as_u32(&self) -> crate::Result<u32> { - if self.error_data.len() == std::mem::size_of::<u32>() { - Ok(u32::from_le_bytes( - self.error_data.as_slice().try_into().unwrap(), - )) - } else { - Err(crate::SmbMsgError::InvalidData( - "Invalid error data length for u32".into(), - )) - } - } - - /// Interprets the error data as a u64, if possible. - /// Returns an error if the data length is not 8 bytes. - pub fn as_u64(&self) -> crate::Result<u64> { - if self.error_data.len() == std::mem::size_of::<u64>() { - Ok(u64::from_le_bytes( - self.error_data.as_slice().try_into().unwrap(), - )) - } else { - Err(crate::SmbMsgError::InvalidData( - "Invalid error data length for u64".into(), - )) - } - } + fn expect_len(&self, expected: usize, type_name: &str) -> crate::Result<&[u8]> { + if self.error_data.len() == expected { + Ok(self.error_data.as_slice()) + } else { + Err(crate::SmbMsgError::InvalidData( + format!("Invalid error data length for {type_name}").into(), + )) + } + } + + /// Interprets the error data as a u32, if possible. + /// Returns an error if the data length is not 4 bytes. + pub fn as_u32(&self) -> crate::Result<u32> { + let bytes: [u8; std::mem::size_of::<u32>()] = + self.expect_len(std::mem::size_of::<u32>(), "u32")? + .try_into() + .unwrap(); + Ok(u32::from_le_bytes(bytes)) + } + + /// Interprets the error data as a u64, if possible. + /// Returns an error if the data length is not 8 bytes. + pub fn as_u64(&self) -> crate::Result<u64> { + let bytes: [u8; std::mem::size_of::<u64>()] = + self.expect_len(std::mem::size_of::<u64>(), "u64")? + .try_into() + .unwrap(); + Ok(u64::from_le_bytes(bytes)) + } }This keeps behavior identical while making future additions (e.g.,
as_i32) simpler.
107-116: Add tests covering non-empty error contexts to lock in ByteCount / alignment behaviorThe existing
error_simpletest only exercises the empty‑context case (error_data: vec![]), which doesn’t validate:
ErrorContextCountandByteCountinteraction when there are one or moreErrorResponseContextentries.- 8‑byte alignment of successive contexts.
- Round‑trip correctness of
ErrorResponseContextlayouts for SMB 3.1.1.The
// TODO(TEST): Add a test with added context items.comment (lines 115–116) is spot‑on. I recommend adding at least one test that:
- Builds an
ErrorResponsewith a singleErrorResponseContextand asserts the encoded hex matches expectations (correctErrorContextCount,ByteCount, and data).- Optionally, another with two contexts, to verify alignment and cumulative
ByteCount.If helpful, I can sketch concrete test cases following the existing
test_response!pattern.
♻️ Duplicate comments (2)
crates/smb-msg/src/ioctl.rs (1)
1-1: Fix typo in module doc comment (previously flagged).This issue was already noted in a prior review. The typo "requessts" should be "requests":
-//! IOCTL requessts and responses implementation, and FSCTLs. +//! IOCTL requests and responses implementation, and FSCTLs.crates/smb-msg/src/echo.rs (1)
14-17: Doc’d aliases for EchoRequest/EchoResponse are a good public API shapeRe‑exporting
EchoMessageasEchoRequest/EchoResponsewith///comments both matches the spec terminology and ensures these aliases show up properly in generated docs, addressing the earlier documentation nit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
crates/smb-msg/src/create.rs(13 hunks)crates/smb-msg/src/echo.rs(2 hunks)crates/smb-msg/src/error.rs(3 hunks)crates/smb-msg/src/ioctl.rs(1 hunks)crates/smb/src/resource.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T16:25:23.358Z
Learnt from: afiffon
Repo: afiffon/smb-rs PR: 157
File: crates/smb-msg/src/file.rs:14-25
Timestamp: 2025-11-21T16:25:23.358Z
Learning: In the smb-rs codebase, multiple fields named `reserved` with different types can appear in the same struct when using the `#[smb_request]`, `#[smb_response]`, or similar derive macros from smb_msg_derive. The macro system automatically renames these to unique identifiers (e.g., `_reserved1`, `_reserved2`) during expansion, so what appears to be duplicate field names is intentional and compiles correctly.
Applied to files:
crates/smb-msg/src/echo.rscrates/smb-msg/src/error.rscrates/smb-msg/src/create.rs
🧬 Code graph analysis (3)
crates/smb-msg/src/echo.rs (1)
crates/smb-msg-derive/src/lib.rs (1)
smb_request_response(222-224)
crates/smb-msg/src/error.rs (2)
crates/smb-msg-derive/src/lib.rs (2)
smb_response(214-216)smb_response_binrw(238-240)crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_size(602-620)
crates/smb-msg/src/create.rs (4)
crates/smb-msg-derive/src/lib.rs (6)
smb_request(206-208)smb_request_binrw(230-232)smb_response(214-216)smb_response_binrw(238-240)smb_message_binrw(246-248)smb_request_response(222-224)crates/smb-dtyp/src/binrw_util/sized_string.rs (2)
size(22-24)bytes16(42-44)crates/smb-dtyp/src/binrw_util/pos_marker.rs (6)
write_aoff(344-363)write_roff_size(421-445)seek_from(24-26)write_roff_plus(222-240)write_roff_size_b_plus(481-506)seek_from_if(29-35)crates/smb-dtyp-derive/src/lib.rs (1)
mbitfield(8-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). (5)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (windows-latest, 1.85)
- GitHub Check: test (single_threaded)
- GitHub Check: test (async)
- GitHub Check: test (multi_threaded)
🔇 Additional comments (7)
crates/smb-msg/src/ioctl.rs (1)
3-9: Module structure and exports look good.The module organization (common, fsctl, msg) and public re-exports are clean and well-structured.
crates/smb-msg/src/echo.rs (2)
3-11: EchoMessage macro usage and layout look sound
#[smb_request_response(size = 4)]on a singlereserved: u16field, plusDefault, is consistent with a 4‑byte SMB2 Echo body (structure size + reserved) and gives a sensible zeroed default message. No issues from a layout or API perspective.
25-27: Binrw test correctly validates the EchoMessage wire formatThe
test_binrw!case forEchoMessageexpecting"04000000"matches the 4‑byte little‑endian layout (structure size 4 + reserved 0), so this gives good coverage for the derived encoding/decoding.crates/smb/src/resource.rs (1)
157-165: LGTM! Improved context handling by preserving the container.The removal of
.into()is a beneficial change that avoids consumingresponse.create_contexts, allowing the container to be reused for potential additional context inspections without re-materialization.crates/smb-msg/src/error.rs (3)
10-14: ByteCount / ErrorContextCount wiring now looks correct and well-documentedThe updated
ErrorResponsedefinition is in good shape:
- Docs clearly describe SMB 3.1.1 behavior and the meaning of
ErrorContextCount/ErrorData._error_context_countis derived fromerror_data.len()on write, ensuring the count tracks the vector length._byte_count: PosMarker<u32>is used symmetrically:
- On read:
map_stream = |s| s.take_seek(_byte_count.value.into())limits parsing to the declaredByteCount.- On write:
#[bw(write_with = PosMarker::write_size, args(&_byte_count))]causesByteCountto reflect the serialized size of all contexts (including alignment/padding).- This resolves the earlier issue where
ByteCountwas not tied toerror_data.No changes needed here; this is a solid, spec-aligned shape.
Also applies to: 16-21, 23-23, 25-27, 29-33
36-41: ErrorResponseContext layout and length handling are consistent with SMB 3.1.1
ErrorResponseContext’s structure looks correct:
#[brw(align_before = 8)]on_error_data_lengthensures each context starts at an 8‑byte boundary relative to the error data area, matching the spec’s alignment requirement._error_data_lengthis automatically calculated fromerror_data.len()on write, and used as thecountwhen readingerror_data, keeping length metadata consistent.- The use of
#[smb_response_binrw]plusErrorIdas a separate repr(u32) enum is a clean separation of concerns.This layout should interoperate well with the surrounding
ErrorResponseparsing and ByteCount handling.Also applies to: 47-55
95-105: ErrorId enum shape and documentation look goodThe
ErrorIdenum is well-defined:
#[smb_response_binrw]plus#[brw(repr(u32))]give it a clear and stable on‑wire representation.Default = 0andShareRedirect = 0x72645253match the common SMB2 ERROR Context identifiers, and the doc comments describe their usage.No changes needed here.
| #[cfg(feature = "client")] | ||
| use binrw::io::TakeSeekExt; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify TakeSeekExt gating vs. unconditional .take_seek usage
error_data’s map_stream closure calls s.take_seek(_byte_count.value.into()) (lines 31–32), which relies on TakeSeekExt, but the import is gated behind #[cfg(feature = "client")] (lines 5–6). If this module is compiled without the client feature and is not itself cfg‑gated, this will fail to compile due to the extension trait not being in scope.
Please confirm that either:
- the
errormodule (or this struct) is also#[cfg(feature = "client")], or TakeSeekExtis otherwise brought into scope for non‑client builds, or- the
cfgon theuseis not needed and can be dropped.
You can quickly check how error.rs is included by running:
Also applies to: 31-32
🏁 Script executed:
#!/bin/bash
# Inspect how the error module is wired and whether it's cfg-gated
rg -n "mod error" crates/smb-msg/src -C3 || trueLength of output: 306
Now I need to directly examine the error.rs source code to see if the error_data field and its map_stream closure are feature-gated or unconditional.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Examine error.rs lines 1-50 to see the structure and whether map_stream is feature-gated
cat crates/smb-msg/src/error.rs | head -50Length of output: 1976
TakeSeekExt must be imported unconditionally or the map_stream closure must be feature-gated
The issue is confirmed: TakeSeekExt is imported only when feature = "client" (lines 5–6), but the error_data field's map_stream closure calls s.take_seek() unconditionally (line 31). Since the error module is not feature-gated in lib.rs, the struct and its field compile regardless of features. Without the client feature enabled, the trait will not be in scope when the binrw macro expands the closure, causing a compile error.
Fix by one of:
- Remove the
#[cfg(feature = "client")]guard on theTakeSeekExtimport (lines 5–6), or - Gate the
error_datafield (and any other code usingtake_seek) behind#[cfg(feature = "client")], or - Gate the entire
ErrorResponsestruct behind#[cfg(feature = "client")].
🤖 Prompt for AI Agents
crates/smb-msg/src/error.rs (around lines 5–6 and ~31): the TakeSeekExt trait is
only imported under #[cfg(feature = "client")] but the error_data.map_stream
closure calls s.take_seek() unconditionally, causing a missing-trait compile
error when the client feature is off; fix by either remove the #[cfg(feature =
"client")] on the TakeSeekExt import so the trait is always in scope, or gate
the error_data field (and any closures calling take_seek) with #[cfg(feature =
"client")] or gate the entire ErrorResponse struct with #[cfg(feature =
"client")] — pick one and apply it consistently so calls to take_seek are only
compiled when the trait is available.
✅ Actions performedReviews paused. |
1 similar comment
✅ Actions performedReviews paused. |
Add more documentation to the smb-msg crate, and improve code formatting.
Bug Fixes & Improvements
Chores
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.