Wireshark-bugs: [Wireshark-bugs] [Bug 5553] Merge Request: Dissector for the OCFS2 network proto
Date: Sat, 22 Jan 2011 11:49:50 -0800 (PST)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5553

--- Comment #4 from Bill Meier <[email protected]> 2011-01-22 14:49:47 EST ---
More comments(in no particular order):

1. It appears that #includes of stdio.h, string.h & memory.h are not needed.

2. All the #includes after epan/packet.h aren't needed.
   (packet.h handles some of the#includes; Others, such as prefs.h
   just aren't needed.

3. Parts of the standard Wireshark copyright text are missing.
   (See README.developer)

...
 * Wireshark - Network traffic analyzer
 * By Gerald Combs <[email protected]>
 * Copyright 1998 Gerald Combs

...

 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
 */

4. I'm not at all a fan of using goto's unless really needed.

   E.G.,I don't think they are really needed in dissect_ocfs2() & 
   dissect_dlm_query_join_request().

   For ocfs2_find_header() see comment below.

5. dissect_ocfs2():  a "new-style" dissector should return either
   0 or "the amount of bytes successfully processed".  
   Returning a negative number is (I think) essentially treated  as 
   indicating that the dissector succeeded.

6. ocfs2_find_header(): 
   a. It seems to be trying to find two "magic" bytes by 
      searching thru the buffer.

      However, after ocs_find_header() is called, AFAICS dissect_ocfs2()
      is coded that the "magic" is always at offset 0 in the buffer.

   b. Reassembly of the PDU is being done by setting some variables in the
      pinfo struct as described in README.developer section 2.7.2.

      Given that your protocol rides only over TCP, the simpler cleaner
      way to do reassembly is to use tcp_dissect_pdus() as described
      in section 2.7.1 of README.developer.


7. Display of "message payload":
   a. 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).

    b. If I understand the code correctly, the result is to dfisplay
       up to 6 fields as FT_BYTES with a different Text label for each.

       I think a simpler way to do this would be as follows:


       for (i=0; i<6; i++)

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