https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4590
Bill Meier <wmeier@xxxxxxxxxxx> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |NEW
CC|wmeier@xxxxxxxxxxx |
--- Comment #3 from Bill Meier <wmeier@xxxxxxxxxxx> 2010-03-28 15:27:43 PDT ---
In general the dissector seems reasonably OK.
Some comments (in no particular order):
1. As Jaap Keuter indicated not too long ago re another dissector submission:
"if(tree)" may never be around "col_set_str()"
2. I think the code which checks for Malformed ... by using tvb_bytes_exist is
not needed. As the dissector accesses bytes in the packet a "Malformed"
exception will be thrown if the access "goes off the end". Also: the error
message will indicate the nature of the issue: whether there are not enough
bytes in the packet or whether the packets captured were truncated when saved
(snaplen).
3. Please use consistent whitespace and indentation.
Eg: Either spaces (somewhat preferred) or tabs for indentation.
4. hf[]: FT_ETHER needs a display type of BASE_NONE. Wireshark now has
additional validation of the hf[] fields.
5. hf[]: I'm not sure why many of the entries have a filter name of "".
They should all have names since the fields will appear in the filter list
but will be unusable w/o a name. (Click on "Expression..." on the filter
bar and look at the ancp... entries).
6. I suggest using proto_tree_add_item with a bitmask specification in the
related hf[] entry for the result, code, i_flag and i_submsg fields.
[ Also: '(guint8) res_code >> 12' causes a compiler error ]
You can then use FT_BOOLEAN for the i_flag field with a
standard tfs_set_notset structure.
proto_tree_add_item(ancp_tree, hf_ancp_result, tvb, offset, 1, FALSE);
proto_tree_add_item(ancp_tree, hf_ancp_code, tvb, offset, 2, FALSE);
...
proto_tree_add_item(ancp_tree, hf_ancp_i_flag, tvb, offset, 1, FALSE);
proto_tree_add_item(ancp_tree, hf_ancp_submsg_num, tvb, offset, 2, FALSE);
---------
{ &hf_ancp_result,
{ "Result", "ancp.result",
FT_UINT8, BASE_DEC,
VALS(resulttype_names), 0xF0,
NULL, HFILL }
},
{ &hf_ancp_code,
{ "Code", "ancp.code",
FT_UINT16, BASE_HEX,
VALS(codetype_names), 0x0FFF,
NULL, HFILL }
},
...
{ &hf_ancp_i_flag,
{ "I Flag", "ancp.i_flag",
FT_BOOLEAN, 8,
TFS(&tfs_set_notset), 0x80,
NULL, HFILL }
},
{ &hf_ancp_submsg_num,
{ "SubMessage Number", "ancp.submessage_number",
FT_UINT16, BASE_DEC,
NULL, 0x7FFF,
NULL, HFILL }
},
7. 'if checkcol()' is no longer required around col_set_str() and col_clear().
8. In dissect_ancp_message() the initial tvb_length check is not needed.
dissect_ancp_message() will only be called if at least ANCP_MIN_HDR
are available due to that length having been specified in the call to
tcp_dissect_pdus.
9. 'static' is not required for the handle in proto_reg_handoff...
10. I tried the Statistics ! ANCP ! Packet Types stats. The numbers
shown did not look correct.
11. Please add the URLs for the protocol documents as a comment in the source.
12. It would be greatly appreciated if you could add a page about the ANCP
protocol to the protocol reference section of the Wireshark Wiki.
See: http://wiki.wireshark.org/HowToEdit
and http://wiki.wireshark.org/ProtocolReference
Thanks !
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.