Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 3256] New dissector: gadu-gadu protocol

[bugzilla-daemon@xxxxxxxxxxxxx]

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





--- Comment #19 from Bill Meier <wmeier@xxxxxxxxxxx>  2009-09-14 11:51:37 PDT ---
My Comments

1.  Naming: 
    I think that "gadu-gadu" rather than "gg" should be be used 
    in the UI, in any dissector output and for any "external" symbols.
    I think "gadu-gadu" is much more meaningful than "gg".

    Now: Some places show "Gadu-Gadu" (eg: "decode as") and others show
         gg (filter names).

     Instances: 
        proto_gg = proto_register_protocol("Gadu-Gadu Protocol",
                                           "Gadu-Gadu",
                                           "gadu-gadu");

       proto_register_gadu_gadu()
       proto_reg_handoff_gadu_gadu()

       col_set_str(..., COL_PROTOCOL, ...)
       prefs_register_bool_preference(...)

       (Maybe) COL_INFO:  DISCONNECTING not GG_DISCONNECTING ??


     hf[]:
       - Field name:
          "type of packet" ==> "Packet Type"
          "length of packet" ==> "Packet Length"
          etc
       - Use gadu-gadu. for the initial string in the filter-name 
         (eg:gadu-gadu.len).
       - No need to use "Gadu-Gadu" in the blurb; 
         (Note: after "Gadu-Gadu" is removed in many cases the blurb is
          now essentially identical to the hf[] entry "Name" field and thus can
          be replaced with a NULL).

    For the same reason as above, I also think renaming the dissector 
    source file to be packet-gadu-gadu.c would be better.

2.  To me the use of "incoming" vs "outgoing" is possibly a bit confusing.
    Might "send" and "receive" be better terms to use ?
    (I guess I'm sort of neutral on this ....).

3.  In dissect_gg_pdu after the heuristic tests (if ( ...incoming ...)
    please add

        col_clear(pinfo->cinfo, COL_INFO);

    See doc/README.developer section 1.5.

4.  Please use tvb_reported_length_remaining rather than 
    tvb_length_remaining.
    For an explanation see:
    http://www.wireshark.org/lists/wireshark-dev/200807/msg00179.html

5.  #ifndef max/#endif is needed around the #define max.
    Compilation of this dissector fails in my Windows Wireshark
    environment w/o this.

6.  It appears that the following #includes are not required:

    #include <stdio.h>
    #include <stdlib.h>
    #include <errno.h>
    #include <ctype.h>
    #include <string.h>

7.  There are several /* XXX */ w/o any additional comment.
    Are these ToDo ? It would be nice if some additional text
    were added.

8.  gadu-gadu.type: 
    It seems to me that the send and receive fields should be kept
    separate since each field apparently has at least some values
    with different meanings depending upon whether send or receive.
    That is: use something like gadu-gadu.type.send & 
    gadu-gadu.type.recv for the filter_name in the
    hf[] entries for these fields.
    At the very least: keeping the fields separate would allow
          using a filter expression to find receive or send field values.
          eg:  gadu-gadu.type.recv == ... and gadu-gadu.type.send ==

    Also: There are multiple identical text value_strings for different
          type values.
          This leads to multiple identical choices when doing the 
          Filter Expression.
          EG: There are 5 instances of "Incoming status change"
          I think using a distinct text string for each type would be better
          (eg: "Incoming status change (v60)". Is this correct ? 
          Do '60", "77" and etc refer to versions ?).

9.  I'm not altogether thrilled about the use of varargs in
    gg_tree_add_item_except() along with the use of #defines to 
    specify some of the args for the calls to gg_tree_add_item_except()..

    I think it would be simpler to just declare a set of arrays 
    (eg: static const guint32 egg_...[]={...} ) and pass the 
    address of the appropriate array to gg_tree_add_item_except().

10. Unexcepted => Unexpected

11. (Minor) There appears to be inconsistent indentation for the 
    buddy/status/msgclass/msgattr entries in the hf[] array.
    (Or: are all of these supposed to be "Special" ?)

12. Unless I'm missing something, it appears to me that the following
    are not req'd in value_string gg_statuses_type[]:
        { GG_STATUS_FRIENDS_MASK, "Only for friends" },
        { GG_STATUS_VOICE_MASK, "VoIP enabled (GG 7.7)" },

13. Please do 'find_dissector("xml")' only once in proto_reg_handoff
    rather than potentially multiple repeated times in the
    individual dissector functions.

14. dissect_gg_login70()
    hf_login_uin_flags/hf_login_uin: It seems simpler to me to use
    two separate add_items: 
      flags: just low-order byte; 
      uin: the remaining 3 high-order bytes
    hf_login_uin_flags: if no flags: No need to show as "unknown".
    (This may apply elsewhere: Fields with no flags set presumably
    aren't unknown).

    Is login_version just the low-order byte ? 
    What's in the other bytes ? (see gg1.pcap)
    Currently login_version displays as a bit field. (Because mask 
     is supplied ??)
    I suspect that removing the mask will cause this field to display
     as an integer.

15. gg_comon_msgclass:
    shows in the displayed tree (in some cases):
      msg_class: chat (28)
        msg_class: Unknown
          ....

     Presumably the second line should not be displayed...
     (See frame #18 of gg1.pcap)

16. I suggest changing 
        if (!(can_be_incoming ^ can_be_outgoing)) {
    to
        if (can_be_incoming == can_be_outgoing)

    since "^" is really a bitwise operator not a logical operator.


Thanks for your work todate on this dissector ....


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