TransmitCall and TransmitReturn
#215
Conversation
moves ScardContext into the esc module and applies the traits to it
Coverage Report 🤖 ⚙️Past: New: Diff: -0.32% [this comment will be updated automatically] |
crates/ironrdp-rdpdr/src/pdu/esc.rs
Outdated
| let preferred_protocols = CardProtocol::from_bits_truncate(src.read_u32()); | ||
| let preferred_protocols = CardProtocol::from_bits(src.read_u32()) | ||
| .ok_or_else(|| invalid_message_err!("decode_ptr", "ConnectCommon", "invalid CardProtocol"))?; |
There was a problem hiding this comment.
Out of curiosity: why from_bits_truncate wasn’t doing the job? I wonder if from_bits_retain wouldn’t be a better alternative (keeping the original payload as-is)
There was a problem hiding this comment.
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 well
IronRDP/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.
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 the bitflags 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.
I don't quite understand how from_bits_retain works. What if there are undefined bits?
There was a problem hiding this comment.
Those bits are ignored, but not unset (unlike from_bits_truncate), i.e.: the underlying u32 is set exactly to the value received from the network
Actually, let me correct myself: we should either use from_bits or from_bits_retain, but never from_bits_truncate, and I would tend to reach for from_bits_retain over from_bits
Rationale:
- We want the round-tripping property to hold, and for this property to hold, parsing must be non destructive (lossless), but
from_bits_truncateis destructive (undefined bits are discarded) - We generally want lenient parsing, ignoring unknown values as long as we don’t need them and/or as long as ignoring them is causing no harm, but
from_bitsis not lenient
However, 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 flags
There was a problem hiding this comment.
Huh, yes, from_bits_retain is much better than truncation via from_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
Corresponds with gravitational/teleport#33282