https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7902
--- Comment #26 from Chris Bontje <cbontje@xxxxxxxxx> 2012-10-24 09:38:08 PDT ---
(In reply to comment #24)
> Created attachment 9425 [details]
> More updates to Modbus/TCP + RTU
>
> Here's what I did:
> 1. Added more "proto" data to get rid of some of the ugly global variables
> (note the modification of the CIP dissector)
> 2. Separated out the "classify packet function" since they were functionally
> different anyway (and Modbus RTU has some additional "classifying" to do when
> it comes to serial)
> 3. Reworked the display filter names so that "modbus" is the prominent
> dissector/filter. Modbus/TCP and RTU just have their "header" fields
> 4. Added filters for the diagnostic function values
> 5. Made "register values" filterable
>
I like all these changes; they cleaned things up very nicely! I'm not quite
clear as to where the modbus_request_info_t structure is defined (and how new
components like register_addr_type, etc got added); is it a dynamic data
structure we can just assign elements to? I was also considering going through
the filter names and adjusting things to split out the different categories - I
like the separate hf_* header field arrays you made for each dissector. Again,
very clean.
> The only thing that really needs to be done before the patch is accepted is to
> get rid of global_mbus_register_base. I see 2 solutions:
> 1. Not have register offsets displayed (which is how it worked previously)
> 2. Add "conversation data" with the transaction id. See
> README.request_response_tracking for implementation hints, although I think
> you'll run into problems with long captures where the transaction ID rolls over
> or a capture where transaction ID is always 0. Remember that transaction IDs
> aren't guaranteed to be sequential or you could have multiple requests before a
> single response so the "global variable" will do more harm than good.
>
With Modbus RTU (at least) we can be guaranteed that there will be no
situations where two requests are issued sequentially; normal conversations
will always go poll->response, poll->response, etc. In that case, with a
response, couldn't we reference the packet context fields to simply step
backwards through the packet lists (from our current point), find the last
query from current dst addr, and take the reference_number base from there?
This would just require us to store and then reference the reference_number -
or am I misunderstanding the capabilities of the packet context functionality?
"Transaction ID" only applies to Modbus/TCP messages and wouldn't to Modbus RTU
so we'll need a solution for both. I've also got captures from systems where
the Transaction ID is zero, as you noted above so we aught to not use that.
The Modbus/TCP systems I've worked with will typically still follow the same
poll->response pattern from the Modbus RTU world, do you have captures where
two different polls are issued sequentially? I'll attach a typical capture
that I see, it has a master polling a slave with 4 different holding register
polls, each one seems to be split?
> Some "nice to haves" while the Modbus dissectors are being modified (but I
> won't hold the patch up for them):
> 1. CRC validation for Modbus RTU (with on/off preference). Note that
> algorithms should be put in wsutil folder (maybe the Modbus CRC algorithm
> already exists there (in a generic form), I haven't checked)
> 2. Make a better attempt at getting the packet size for an RTU packet.
> Requires dissecting into function code but it better allows for multiple
> packets to be dissected in a single frame. Without this, tcp_dissect_pdus is
> less useful. It will do "one PDU across multiple frames", but not "multiple
> PDUs in a single frame" without better length calculations.
I'll look into both of these for a future iteration.
Chris
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.
