Convert modbus to rust v0 #5675
Conversation
Autogenerated files and changes by setup-app-layer script.
- Add constants; - Add header structure; - Add simple unit test to check header size.
|
Just to add a comment on the obvious, I've left some questions marked as TODO, so it's easier to find where I have doubts (and am already aware about them)... |
|
|
||
| /* Modbus Read/Write function and Access Types. */ | ||
| // TODO I think those exactly the same as the last three above, can we delete them, therefore? | ||
| // pub const MODBUS_TYP_WRITE_SINGLE MODBUS_TYP_WRITE | MODBUS_TYP_SINGLE; |
| // Unless it's being used like that because the length field in the MBAP header contains the length | ||
| // including next field, which is unit address (1 byte), in which case, yes, if we compare | ||
| // with that in mind, max is 254. but, then, min len would be... 3? still confused | ||
| // TODO should this really be usize? I'm having trouble using it, because header length is |
There was a problem hiding this comment.
I think usize can be used but maybe its better to use defined types wherever you can?
Would be interested to know about this more from @jasonish though.
You can use explicit type conversion later in the code to handle operations among variables with different datatypes.
There was a problem hiding this comment.
I was reading about those types, and also checked what was done in some other of ours (like dcerpc and http2) and saw that in some places usize was chosen, and apparently this is the choice when we are talking about memory size?
I had some trouble with the type conversion, Rust complained of all my tries, so I'll have to do some research, in case usize is indeed the best choice here.
| } | ||
| } | ||
|
|
||
| //TODO will the simplest parser come here? Will I need a simpler method that will be struct independent? most likely... |
There was a problem hiding this comment.
nom parsers should go in parser.rs. Here, you can have a function to make a call to that parser.
You'll have to write a little more code to understand where would it be appropriate to call the parser. (e.g. A lot of times, you'd need to call the parser (from parser.rs) in State struct's implementation to access the results easily)
| #[test] | ||
| fn test_max_pdu_length() { | ||
| // TODO what's the best way to test this? | ||
| unimplemented!("Still to understand best way to check this. PCAPs?"); |
There was a problem hiding this comment.
I think you can also do this with unittests. But, you'll have to have a header parsing function first that parses the length of a PDU from the protocol header.
There was a problem hiding this comment.
Thanks, makes sense!
|
This is a good start, Juliana. :) |
|
Closing this, as it was found that there was some duplicated effort for working on the same task. |
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3957
Describe changes:
--- bring constants from C files;
--- add header structure;
--- add a simple unit test to check header size.
#suricata-verify-pr:
#suricata-verify-repo:
#suricata-verify-branch:
#suricata-update-pr:
#suricata-update-repo:
#suricata-update-branch:
#libhtp-pr:
#libhtp-repo:
#libhtp-branch: