[bugzilla-daemon@xxxxxxxxxxxxx]

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

--- Comment #12 from Bill Meier <wmeier@xxxxxxxxxxx> 2010-10-26 14:11:17 EDT ---
OK: A few comments

1. Gearman looks like a fairly simple request/response protocol.

   I get the impression (from the protocol spec and from the sample capture)
   that any particular Gearman message will only have a single request
   or reply.

   Is this correct ?

   If so, I suggest you start out by simplifying your 
   code to reflect the above.

   That is: in dissect_binary_packet it seems to me that the following kind
   of code isn't needed:

        while (tvb_reported_length_remaining(tvb, offset) >= ....

    and 

        if (offset == 0)
        {
           ...
        }
        else
        {
         ...
        }

    and so on.

   Also: in general you do not need to keep checking if there's enough
         bytes; Just access the fields as needed. If you run off the 
         end of the data, Wireshark will throw an exception and show
         a "Malformed" message in the tree.

     IOW: you don't need to do stuff like:

        if (tvb_length_remaining(tvb, offset) >= ...

   Note: It presumably can be the case that a Gearman message
         can be longer than 1 packet can contain.
         Wireshark has a simple mechanism to handle this (for TCP).
         The code to handle this case can be added to your dissector
         quite simply once all other issues have been addressed.

2. match_strval will return NULL if the lookup fails.

   In your code, this will result in %s format being passed a NULL
   which is "not a good thing". Use val_to_str().

   See doc/README.developer for details.

3. All functions other than proto_register... and proto_reg_handoff...
   and all variables should be defined as static.

4. tvb_get_string does a g_malloc; Your usage of same results in memory
   leaks since the space allocated is never freed.
   Use tvb_get_ephemeral_string().
   Again: see doc/README.developer for details


I expect there are additional issues, but the above list is a start.   :)

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