Wireshark-bugs: [Wireshark-bugs] [Bug 4590] ANCP (Access Node Control Protocol) Dissector
Date: Sun, 28 Mar 2010 15:27:45 -0700 (PDT)
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.
- References:
- [Wireshark-bugs] [Bug 4590] New: ANCP (Access Node Control Protocol) Dissector
- From: bugzilla-daemon
- [Wireshark-bugs] [Bug 4590] New: ANCP (Access Node Control Protocol) Dissector
- Prev by Date: [Wireshark-bugs] [Bug 4624] New: TCP reassembly can call subdissector with incorrect TCP sequence number
- Next by Date: [Wireshark-bugs] [Bug 4590] ANCP (Access Node Control Protocol) Dissector
- Previous by thread: [Wireshark-bugs] [Bug 4590] ANCP (Access Node Control Protocol) Dissector
- Next by thread: [Wireshark-bugs] [Bug 4590] ANCP (Access Node Control Protocol) Dissector
- Index(es):
- Get Wireshark
- Download
- Code of Conduct