[bugzilla-daemon@xxxxxxxxxxxxx]

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

--- Comment #4 from Aniruddha <aniruddha.a@xxxxxxxxx> 2010-03-28 22:52:39 PDT ---
(In reply to comment #3)
> 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 !

Hi Bill,

Thanks for the detailed review,

1) Hope this is true for the col_append and the col_add as well, 
   I will remove the check in all those cases 

2) I will remove all "Malformed .." checks

3) I have "vim expandtab", and I have space-only indentation, no tabs
   if there are any specific Wireshark requirements for 
   indentation (or GNU indent with specific options), I can use that.

4) hf[]: Will change BASE_NONE for FT_ETHER

5) hf[]: I did not want filtering-on/display-of all the header fields available
   so, made some empty (""), is there is a different way to accomplish
   this ?

6) Will change to bitmasks

7) , 8) , 9) will change

10) I have verified the statistics, which counter did you feel was incorrect?

    With the sample capture that I have attached, 

    with the display filter ancp.mtype == 10 in use, we see 25 TCP packets
    with frame 7 having 2 ANCP packets (1 Syn and 1 SynACK) 
    i.e, 26 Adjacency packets total  (1 SynAck not seen in Info column
    because of 2 packets in same frame)

     Port Up - 4        (ancp.mtype == 80)
     Port Down - 2       (ancp.mtype == 81)
     Port Management - 4   (ancp.mtype == 32)

    (as shown in the stats)  

11) , 12) Will sure do


--
Ani

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