Fix minor issues and add more length checks to decoder#47
Fix minor issues and add more length checks to decoder#47313ctric wants to merge 1 commit intompowell90:mainfrom
Conversation
- Fixes some minor protocol issues - Add in decoders for IdentifyMode, DnsHostName and DnsDomainName - Add lots of extra length checks to deserialisation
|
Hey @313ctric, first of all, thanks the PR!
I'm happy with breaking public API changes and typos in enum variants in minor version releases.
Any further spec contributions are also massively appreciated. Would love for this crate to fully support all RDM specs and we are most of the way there. |
mpowell90
left a comment
mpowell90
left a comment
There was a problem hiding this comment.
Overall a great set improvements and fixes!
| macro_rules! check_len { | ||
| ($len:literal) => { | ||
| if bytes.len() < $len { | ||
| return Err(RdmError::InvalidMessageLength(bytes.len() as u8)); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
i'm not a fan of defining macros in function bodies and this macro is defined twice, here and in response.rs. However I can see why you've added it - it does clean up / simplify the length validation.
Perhaps a middle ground is to extract this macro out into a utils file. we would have to change it to accept the expected and actual length due to the function variables being out of scope:
macro_rules! check_len {
($actual:literal, $expected:literal) => {
if $actual < $expected {
return Err(RdmError::InvalidMessageLength($actual as u8));
}
};
}2 participants
Changes:
GetCommand->GetCommandResponse)ProtocolVersionfieldspubto allow it to be constructed in aconstmannerProductDetailandProductCategoryBitFieldandAsciifromConvertedParameterValue(per the spec these should always have the value set to 0x0000IdentifyModeSome of these are quite minor and also change the public API, so let me know if you'd rather not have any of them, or would like them doing differently, especially for the typos.
Also there's a new version of RDM (2025) and a new extra PIDS document (E1.37-5). I'd like to add those after this, but putting it all in one PR was far too messy.