[bugzilla-daemon@xxxxxxxxxxxxx]

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

--- Comment #6 from Bill Meier <wmeier@xxxxxxxxxxx> 2011-01-22 17:54:48 EST ---
> (Cont'd; replaces item #7 in the previous comments)
> (I keeping hitting the "commit" before I'm finished :(  )
> 
>  7. Display of "message payload":
>      a. If I understand the code correctly, the result of the
>         first part of the code is to display
>         up to 6 items of 24 bytes each as FT_BYTES with a different 
>         text label for each.
> 
>         Is the intent just to display the payload as hex bytes ?
> 
>         If so, is this needed since the bytes will (normally) appear
>         in the hex pane ?
> 
>         Does breaking the display into 24 byte chunks reflect
>         any underlying data structure ?

        It also looks like following the hex display that only 
        the O2NET_MSG_MAGIC message type is dissected.
        The others are not (yet?):
          O2NET_MSG_STATUS_MAGIC
          O2NET_MSG_KEEP_REQ_MAGIC
          O2NET_MSG_KEEP_RESP_MAGIC

        Is the intention to eventually add code to the dissector for these
        message types ?

        Suggestion: 
        1. Dissect O2NET_MSG_MAGIC as now.
        2. For the others (until dissection code added) call
           the data dissector to show the hex bytes.
           (See below).


8. Unused variables should be marked with _U_ in the function definition:

   That is:

  void dlm_am_flags_handler(proto_tree *tree, tvbuff_t *tvb, guint offset,
                        void *priv _U_, struct dlm_msg_field_def *fld)


   so the following is not needed:
    priv = priv; /* stop warning */


9. Calling the data dissector: Something like

   call_dissector(data_handle, tvb_new_subset_remaining(tvb, offset), pinfo,
           subtree);


   Also needed: 

   a global variable:

         static dissector_handle_t data_handle;


    and in  proto_reg_handoff_ocfs2():

         data_handle = find_dissector("data");

10. AFAIKT in the dlm_struct_defs array the fields 
        const char *s_name;
    const char *s_description;
    gint *s_ett_index;
    are never referenced; Thus it appears that none of the ett variables
    other than ett_ocfs2 are ever used.

11. General principle: If a field is of a fixed length and of a certain
      number of bytes and should be present, then there's no need
      to check if the bytes are actually present. 

      proto_tree_add_item(..., offset, length, ...) will automatically
      generate a "malformed" entry for the field if the bytes are
      not present. (The dissector will also be exited if this occurs).

      So; It would seem to me that most(all?) usage of tvb_offset_exists() is
      probably unnecessary.

12. Many of the "handler" functions seem to have identical code;
    Maybe consider using one function for the identical cases ?

13. struct ocfs2_msg  is unused and should be #if 0'd out (remaining
    only for documentation) ?

    ditto for various other structs ?

=========

Also:

1. For testing purposes, we'll need a capture file or two containing all the
various fields.

2. Has this dissector been "fuzz-tested" ?

   See: Section 2.9.4 of the Wireshark Developer's guide:

http://www.wireshark.org/docs/wsdg_html_chunked/ChSrcContribute.html


Thanks for your work 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.