refactor(dapapi): encoding/decoding traits #402
Conversation
… add simple Encode trait;
…ement encoding/decoding for bind-related structures;
TheBestTvarynka
changed the title
|
Do not merge it yet :) currently, I'm testing it on my environment after the refactoring |
Everything works well 😊 |
…m with plain functions;
…m with plain functions;
crates/dpapi-core/Cargo.toml
Outdated
| num-derive.workspace = true | ||
| num-traits = { workspace = true, default-features = true } |
There was a problem hiding this comment.
note: I typically recommend against using num-derive and num-traits crates.
Two main reasons:
- For resilient parsing, we should typically avoid enums, and
- Otherwise, it’s typically not worth adding a proc-macro in a core crate. (This also applies to
thiserror.)
There was a problem hiding this comment.
Otherwise, it’s typically not worth adding a proc-macro in a core crate. (This also applies to thiserror.)
I just introduced a new crate: dpapi-pdu (#402 (comment)) and moved proc-macros (including thiserror) there.
There was a problem hiding this comment.
Note that I still recommend against using num-derive and num-traits crates even in dpapi-pdu. But if you are sure it covers your needs properly, and if it’s really saving you trouble, it’s fine!
crates/dpapi-core/src/lib.rs
Outdated
| pub mod gkdi; | ||
| mod marker; | ||
| mod padding; | ||
| pub mod rpc; |
There was a problem hiding this comment.
thought: It seems gkdi and rpc modules are not providing core building blocks, and do not belong to the dpapi-core crate.
There was a problem hiding this comment.
I thought about it. I explored the IronRDP crates structure more and decided to move structures encoding and decoding implementations into a separate crate: dpapi-pdu. Moreover, I added an alloc feature to the dpapi-core crate and put the EncodeVec extension trait behind it.
I just pushed a new commit. Can you have a look?
Surprisingly, it was easy to refactor. I think it's a sign of a good architecture 🙃
crates/dpapi-core/src/str.rs
Outdated
| @@ -27,11 +29,11 @@ pub fn from_utf16_le(data: &[u8]) -> Result<String> { | |||
| /// Encodes str into a UTF-16 encoded byte array. | |||
| /// | |||
| /// *Note*: this function automatically appends a NULL-char. | |||
| pub fn encode_utf16_le(data: &str) -> Vec<u8> { | |||
| /// *Panic*: panics when cursor's internal buffer doesn't have enough space. | |||
There was a problem hiding this comment.
note: Typically this kind of information is specified in a dedicated section. See: https://rust-lang.github.io/api-guidelines/documentation.html#function-docs-include-error-panic-and-safety-considerations-c-failure
praise: But first, let me thank you for documenting properly this kind of things! Too often we forget to do so 😆
Hi, I rework the encoding/decoding traits. Our goal is to support WASM. I took your past comment (#342 (comment)) into account and tried to make it similar to `IronRDP` and other projects. I didn't change any DPAPI-related logic. This PR contains only decoding/encoding refactoring.
Hi, I rework the encoding/decoding traits. Our goal is to support WASM. I took your past comment (#342 (comment)) into account and tried to make it similar to `IronRDP` and other projects. I didn't change any DPAPI-related logic. This PR contains only decoding/encoding refactoring.
Hi, I rework the encoding/decoding traits. Our goal is to support WASM. I took your past comment (#342 (comment)) into account and tried to make it similar to `IronRDP` and other projects. I didn't change any DPAPI-related logic. This PR contains only decoding/encoding refactoring.
Hi,
I rework the encoding/decoding traits. Our goal is to support WASM. I took your past comment (#342 (comment)) into account and tried to make it similar to
IronRDPand other projects.I didn't change any DPAPI-related logic. This PR contains only decoding/encoding refactoring.