-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TransmitCall
and TransmitReturn
#215
Merged
Merged
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
a110094
Removes Sync dependency for Sequence and downstream traits
77958e9
GetStatusChangeReturn
59f2726
ConnectCall
f232559
move ndr and rpce modules to their own files
df25a67
Adds an ndr::Encode and ndr::Decode trait
fe83ad4
Moves ReaderState to esc
02d811a
Fix and complete ConnectCall
1a7918b
Change ConnectCommon to ndr::Decode, add REDIR_SCARDHANDLE
edb6b10
impl ndr::Encode for ScardHandle and Connect_Return
31430fa
cleaning up rebase mistakes
5714992
HCardAndDispositionCall
b37c755
TransmitCall
343d666
Merge branch 'master' into feat/HCardAndDispositionCall
01cac1c
switch back to ndr::Encode/Decode
519c395
Merge branch 'feat/HCardAndDispositionCall' into feat/Transmit
e254770
TransmitReturn
fbbfe31
Merge branch 'master' into feat/Transmit
a8fb511
switch to from_bits_retain
18b05ca
cast_length
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: why
from_bits_truncate
wasn’t doing the job? I wonder iffrom_bits_retain
wouldn’t be a better alternative (keeping the original payload as-is)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was doing the job, I just noticed that
from_bits
was more precise so decided to switch to using that. I tried it here as wellIronRDP/crates/ironrdp-rdpdr/src/pdu/esc.rs
Lines 810 to 811 in fbbfe31
but it turns out that we actually do sometimes get junk bits in those fields 🤷♂️.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-thinking about this, I really think we should use
from_bits_retain
everywhere now. My understanding is that it wasn’t used up until now because this method didn’t exist yet in older versions of thebitflags
crate.As much as possible I want parsing to be lossless and to uphold the "round-trip" property (
m = encode(decode(m))
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand how
from_bits_retain
works. What if there are undefined bits?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those bits are ignored, but not unset (unlike
from_bits_truncate
), i.e.: the underlyingu32
is set exactly to the value received from the networkActually, let me correct myself: we should either use
from_bits
orfrom_bits_retain
, but neverfrom_bits_truncate
, and I would tend to reach forfrom_bits_retain
overfrom_bits
Rationale:
from_bits_truncate
is destructive (undefined bits are discarded)from_bits
is not lenientHowever, it’s okay to use
from_bits
if strictness is required at some place, but I would like such places to be documented with a comment explaining why we have to refuse unknown flagsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a8fb511
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, yes,
from_bits_retain
is much better than truncation viafrom_bits_truncate
, which may prevent a hard-to-catch bug for us in the future! We may want to make refactoring on the whole codebase, I created an issue to do refactoring in future #217