diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 5ac369f5..4e9622a6 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -29,19 +29,23 @@ Pay attention to the "**Architecture Invariant**" sections. library must be opt-in through a feature flag called `std` that is enabled by default. When the `alloc` crate is optional, a feature flag called `alloc` must exist to enable its use. +**Architectural Invariant**: no platform-dependant code (`#[cfg(windows)]` and such). + **Architectural Invariant**: no non-essential dependency is allowed. **Architectural Invariant**: no proc-macro dependency. Dependencies such as `syn` should be pushed as far as possible from the foundational crates so it doesn’t become too much of a compilation -bottleneck. The paper [Developer Productivity For Humans, Part 4: Build Latency, Predictability, +bottleneck. [Compilation time is a multiplier for everything][why-care-about-build-time]. +The paper [Developer Productivity For Humans, Part 4: Build Latency, Predictability, and Developer Productivity][developer-productivity] by Ciera Jaspan and Collin Green, Google -researchers, elaborates on why it is important to keep build times low. +researchers, also elaborates on why it is important to keep build times low. **Architectural Invariant**: unless the performance, usability or ergonomic gain is really worth it, the amount of [monomorphization] incured in downstream user code should be minimal to avoid binary bloating and to keep the compilation as parallel as possible. Large generic functions should be avoided if possible. +[why-care-about-build-time]: https://matklad.github.io/2021/09/04/fast-rust-builds.html#Why-Care-About-Build-Times [developer-productivity]: https://www.computer.org/csdl/magazine/so/2023/04/10176199/1OAJyfknInm [monomorphization]: https://rustc-dev-guide.rust-lang.org/backend/monomorph.html @@ -51,14 +55,14 @@ Meta crate re-exporting important crates. **Architectural Invariant**: this crate re-exports other crates and does not provide anything else. -_TODO_: clean up the dependencies - #### [`crates/ironrdp-pdu`](./crates/ironrdp-pdu) PDU encoding and decoding. _TODO_: talk about important types and traits such as PduDecode, PduEncode… +_TODO_: clean up the dependencies + #### [`crates/ironrdp-graphics`](./crates/ironrdp-graphics) Image processing primitives. diff --git a/crates/ironrdp-pdu/README.md b/crates/ironrdp-pdu/README.md index 3c573d58..dfa1c003 100644 --- a/crates/ironrdp-pdu/README.md +++ b/crates/ironrdp-pdu/README.md @@ -2,7 +2,14 @@ RDP PDU encoding and decoding library. -## WIP: overview of encoding and decoding traits +- [Overview of encoding and decoding traits](#overview-of-encoding-and-decoding-traits) +- [Difference between `WriteBuf` and `WriteCursor`](#difference-between-writebuf-and-writecursor) +- [Difference between `WriteBuf` and `Vec`](#difference-between-writebuf-and-vecu8) +- [Most PDUs are "plain old data" structures with public fields](#most-pdus-are-plain-old-data-structures-with-public-fields) +- [Enumeration-like types should allow resilient parsing](#enumeration-like-types-should-allow-resilient-parsing) +- [On bit flags](#on-bit-flags) + +## Overview of encoding and decoding traits It’s important for `PduEncode` to be object-safe in order to enable patterns such as the one found in `ironrdp-svc`: @@ -400,3 +407,68 @@ MUST always be rejected. In such cases, an `enum` is deemed appropriate. [RangedU32_get]: https://docs.rs/deranged/0.3.8/deranged/struct.RangedU32.html#method.get [deranged]: https://docs.rs/deranged/0.3.8/ [http-status-code]: https://docs.rs/http/0.2.9/http/status/struct.StatusCode.html + +## On bit flags + +The **TL;DR** is: + +- Use **both** `from_bits_retain` and `const _ = !0` when resilient parsing is required. + - `const _ = !0` ensures we don’t accidentally have non resilient or destructive parsing. In + addition to that, generated methods such as `complement` (`!`) will consider additional bits + and follow the principle of least surprise (`!!flags == flags`). + - `from_bits_retain` makes it clear at the call site that preserving all the bits is intentional. +- Use `from_bits` WITHOUT `const _ = !0` when strictness is required (almost never in IronRDP), and + document why with an in-source comment. + +Bit flags are used quite pervasively in the RDP protocol. +IronRDP is relying on the [`bitflags` crate][bitflags] to generate well-defined flags structures, +freeing us from worrying about bitwise logic. + +Three notable methods generated by the `bitflags` crate are: + +- [`from_bits`][from_bits], +- [`from_bits_truncate`][from_bits_truncate], and +- [`from_bits_retain`][from_bits_retain]. + +Within IronRDP codebase, `from_bits_retain` should generally be used over `from_bits`, and +`from_bits_truncate` is likely wrong. `from_bits_retain` will simply ignore unknown bits, but will +not unset them (unlike `from_bits_truncate`), i.e.: the underlying `u32` is set exactly to the value +received from the network. + +Rationale is: + +- PDU decoding and encoding logic should uphold the round-tripping property (`m = encode(decode(m))`), + and for this property to hold, parsing must be non-destructive (i.e.: lossless), + but `from_bits_truncate` is destructive (unknown bits are discarded). +- Resilient parsing is generally preferred, ignoring unknown values as long as they are not needed and/or as + long as ignoring them is causing no harm, but `from_bits` is not lenient + +Note that it’s okay to use `from_bits` if strictness is actually required somewhere, but such places must be +documented with a comment explaining why refusing unknown flags is better. + +`bitflags` v2.4 also introduced a new syntax in the `bitflags!` macro (): + +```rust +bitflags! { + pub struct Flags: u32 { + const A = 0b00000001; + const B = 0b00000010; + const C = 0b00000100; + + // The source may set any bits + const _ = !0; // <- This + } +} +``` + +There is crate-level documentation for this: + +This addition makes `from_bits_truncate` behave exactly like `from_bits_retain`, because all values +are considered to be known and defined. In such cases, `from_bits` also never fails therefore works +precisely the same as `from_bits_retain`, except it’s less ergonomic because it returns a `Result` +which must be needlessly handled. + +[bitflags]: https://crates.io/crates/bitflags +[from_bits]: https://docs.rs/bitflags/2.4.0/bitflags/example_generated/struct.Flags.html#method.from_bits +[from_bits_truncate]: https://docs.rs/bitflags/2.4.0/bitflags/example_generated/struct.Flags.html#method.from_bits_truncate +[from_bits_retain]: https://docs.rs/bitflags/2.4.0/bitflags/example_generated/struct.Flags.html#method.from_bits_retain