Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 4590] ANCP (Access Node Control Protocol) Dissector

[bugzilla-daemon@xxxxxxxxxxxxx]

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.