Wireshark-bugs: [Wireshark-bugs] [Bug 4757] New Protocol Submission for IEEE 802.1Qat (Multiple
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4757
--- Comment #10 from Chris Maynard <christopher.maynard@xxxxxxxxx> 2010-05-13 10:27:54 PDT ---
A few more comments ...
1) The ETHERTYPE_MSRP should probably be defined in epan/etypes.h with the rest
of the Ethertypes. Speaking of Ethertype, it doesn't look like this is a
registered Etheretyp with the IEEE
(http://standards.ieee.org/regauth/ethertype/eth.txt). Were you planning on
registering it?
2) Regarding the documentation, for now, maybe you could just add a link to
where to obtain it somewhere in the source code. Hopefully later on it will be
freely available from http://standards.ieee.org/getieee802/.
3) Consistent naming convention. Most #defines are prepended with MSRP, but
some are not. They should be consistently named.
4) Whitespace: I don't want to start a religious war here, but it looks like
most of your dissector is indented using 4 spaces; however, there are tabs in
various places, particularly the block from lines 554-625, which doesn't match
the rest of the dissector's formatting. Can you replace all tabs with spaces
so the dissector is consistent throughout?
5) You might consider replacing the for loop (lines 501-510) with a call to
val_to_str() right in 511, i.e.
proto_item_append_text(msg_tree, ": %s (%d)", val_to_str(attribute_type,
attribute_type_vals, "<Unknown>"), attribute_type);
6) The calls to check_col() are deprecated in new code. You can safely omit it
now. README.developer doesn't explicitly indicate this anywhere, but the
packet-PROTOABBREV.c example does show that col_set_str() is called without the
check.
7) The switch at line 601 has no dissection for the default case. Is that
intentional? You might consider adding the raw, unknown data using
proto_tree_add_text(), and indicated as an "Unknown attribute" or something,
rather than just skipping it. In any case, have you tested how your dissector
would handle an invalid/unknown attribute_type in that case? It should handle
it gracefully. There are tools available for fuzz testing. Take a look in
tools/ for them and run it against some of your dissector to find out how
robust it is at handling mal-formed packets and invalid/unknown data.
8) The four_packed_event thing: It looks like you should make a subtree when
adding these, that way it'll be more obvious that the byte is actual 4
different 2-bit bitfields. Then you may want to introduce either 4 sub-events
(hf_msrp_four_packed_event1, hf_msrp_four_packed_event2,
hf_msrp_four_packed_event3, and hf_msrp_four_packed_event4) so you can filter
on them separately ("mrp-msrp.four_packed_event1",
"mrp-msrp.four_packed_event2", "mrp-msrp.four_packed_event3" and
"mrp-msrp.four_packed_event4"), or at least one more sub-event that covers all
4 so that you can at least filter on the entire byte using the filter you
assigned to hf_msrp_four_packed_event, namely "mrp-msrp.four_packed_event" or
you could filter on a sub-event, using the filter you assign to the sub-events.
For example, hf_msrp_four_packed_event_sub with filter
"mrp-msrp.four_packed_event_sub". [Disclaimer: I don't have the spec in front
of me so if this doesn't make sense when bounced off the spec or if the
language I've used above doesn't quite match, then be sure to use the
terminology from the spec. instead of mine.]
9) Same goes for the three_packed_event thing.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
