feat(dpapi): implement basic RPC structures encoding/decoding #342
Conversation
deps: add picky-* crates to workspace dependencies;
…/encoding. implement `BindTimeFeatureNegotiationBitmask`;
| use crate::{DpapiResult, Error}; | ||
|
|
||
| trait Encode { | ||
| fn encode(&self, writer: impl Write) -> DpapiResult<()>; |
There was a problem hiding this comment.
note: In IronRDP again, the choice was made to not use impl genericity for a few reasons:
- https://github.com/Devolutions/IronRDP/tree/dd249909a894004d4f728d30b3a4aa77a0f8193b/crates/ironrdp-pdu#overview-of-encoding-and-decoding-traits (for object-safety)
- https://github.com/Devolutions/IronRDP/blob/master/ARCHITECTURE.md#avoid-io-wherever-possible (avoid I/O)
- https://github.com/Devolutions/IronRDP/blob/dd249909a894004d4f728d30b3a4aa77a0f8193b/STYLE.md#avoid-monomorphization (avoid monomorphization)
I think ideally this kind of library should be sans-I/O / I/O-free: https://sans-io.readthedocs.io/#
The I/O part is driven by a thin layer feeding bytes in and reading bytes out.
This approach is still fine as I/O is not hardcoded either. It’s possible to use Vec and other in-memory buffer to work.
The main benefit is that you don’t need to know the required number of bytes ahead of time because you can just plug the TcpStream and read bytes out of it as you need.
Just information for you to consider, but I won’t impose you to change anything.
There was a problem hiding this comment.
Yes, I agree with your points. Thank you for the additional links
The main benefit is that you don’t need to know the required number of bytes ahead of time because you can just plug the TcpStream and read bytes out of it as you need.
Yep. And &mut Write/Read also implements Write/Read. It's super convenient
but I won’t impose you to change anything.
😊
There was a problem hiding this comment.
note: Most of the changes you made to this file could have been in a separate PR as it seems unrelated to the dpapi feature.
There was a problem hiding this comment.
Hm, yes, you are right. These changes somehow leaked from upcoming PRs 😅 I didn't notice it
TheBestTvarynka
force-pushed
the
feat/dpapi-rpc-structures
branch
from
8fb48ec to
5600c83
Compare
crates/dpapi/src/error.rs
Outdated
| #[error("{0:?}")] | ||
| #[error("IO error")] | ||
| Io(#[from] std::io::Error), | ||
|
|
||
| #[error("UUID error: {0:?}")] | ||
| #[error("UUID error")] | ||
| Uuid(#[from] uuid::Error), | ||
|
|
||
| #[error("integer conversion error: {0:?}")] | ||
| #[error("integer conversion error")] | ||
| IntConversion(#[from] std::num::TryFromIntError), |
There was a problem hiding this comment.
note: It also occurred to me that thiserror has a error(transparent) attribute for situations where adding an additional message is not relevant.
https://docs.rs/thiserror/latest/thiserror/#details
Also a worthwhile read: https://www.howtocodeit.com/articles/the-definitive-guide-to-rust-error-handling
|
@TheBestTvarynka Can you switch to the published version? Devolutions/picky-rs#338 Feel free to merge the PR yourself after 🙂 |
Hi,
In this PR, I initialized the
dpapicrate and implemented basic RPC structure encoding/decoding.