Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 5730] Dissector for HSR and PRP-1

[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5730

--- Comment #5 from Stephen Fisher <steve@xxxxxxxxxxxxxxxxxx> 2011-10-20 13:24:51 MDT ---
Thanks for the updated patch!  Here are my comments, which are mostly for
visual reasons to be consistent with Wireshark but some are also functional
changes:

- Remove unnecessary include files stdio.h, stdlib.h, string.h and epan/prefs.h
from removed from epan/dissectors/packet-hsr-prp-supervision.c and packet-hsr.c

- Move the closing }; to its own line in the type_vals and hsr_laneid_vals and
prp_type_vals value string definitions

- Remove preferences code in proto_reg_handoff_hsr_prp_supervision() and
proto_reg_handoff_hsr() since the dissectors don't have preferences

- The proto_set_decoding can be removed along with hsr_enable_dissector since
it isn't doing anything except always enabling the dissector, which is
Wireshark's default.

- Remove unnecessary forward declaration of
proto_reg_handoff_hsr_prp_supervision() and proto_reg_handoff_hsr (Wireshark
will automatically generate the needed forward declarations to call these
functions when building and place them in epan/dissectors/register.c

- Change deprecated function name in packet-hsr.c: dissector_try_port ->
dissector_try_uint (takes same parameters)... this was found with the
tools/checkAPIs.pl script

- Added yourself to the AUTHORS file

- Remove the if(!tree) return and instead include most of the
dissect_hsr_prp_supervision() function in an if(tree) block.  I left the
COL_PROTOCOL and COL_INFO statements before this.  It isn't strictly necessary
to put the if(tree) at all though, because functions such as
proto_tree_add_item() will return if !tree and this would allow your code to
set the columns more accurately further down in the code.  It's really only
important to use if(tree) if you have complex code that will waste CPU cycles
or memory calculating things for the tree output when Wireshark doesn't even
need it.

- Remove the if(!tree) return from packet-hsr.c per last comment

- The while(1) in packet-hsr-prp-supervision.c for "ensure we can read 2 bytes
without exception" is unusual and should probably be avoided.  The
proto_tree_add_item, tvb_get_ and related functions will throw an exception if
it runs out of bytes in the packet, which is the proper behavior (because it
will be marked by Wireshark as malformed).

- Can sup_version be defined as a "guint16" (16-bit) instead of an "int" (32 or
64-bit) since you're assigning it with tvb_get_ntohs() (network to host
short/16-bit)?

- Can tlv_type and tlv_length be changed to guint8 (8 bit) instead of guint16
since you're fetching them with tvb_get_guint8?

- We don't typically put a list of revisions at the top of source files since
the revisions are kept in the version control (SVN) history

-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
You are watching all bug changes.