Wireshark-bugs: [Wireshark-bugs] [Bug 7881] Plugin KNXnet/IP and cemisubdissect
Date: Sun, 21 Oct 2012 17:39:26 -0700 (PDT)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7881

--- Comment #7 from Michael Mann <mmann78@xxxxxxxxxxxx> 2012-10-21 17:39:26 PDT ---
Thanks for sample captures!  However the code still needs some more work.

1. These should really be built-in dissectors.  These dissectors won't be
applied to the 1.8 trunk, they will only be applied to the SVN trunk.  Because
the dissector files are new, that's not really a problem (from a patch
perspective), just an FYI.
2. There are still unnecessary function declarations at the top of the file
because the code isn't contructed to have "dependent" dissection functions
later in the code.
3. tvb_memcpy is still being used for "integer" values.  It should really only
be used for data intended as a byte array (and I didn't see any here)
4. There are a bunch of places where the code does:
foo = tvb_get_*()
proto_tree_add_uint(..., foo)

Those should be replaced with proto_tree_add_item, especially when "foo" isn't
otherwise used in the function.  Note that masks can be applied to the
hf_register_info array, so having a masked value doesn't preclude you from
using proto_tree_add_item.

5. proto_tree_add_item no longer uses TRUE/FALSE for the last argument.  Those
should be replaced with ENC_LITTLE_ENDIAN or ENC_BIG_ENDIAN.  The
fix-encoding-args.pl script can help you convert them.

6. I still don't see a need for many of the #defines.  Are they intended to go
into a switch statement in the dissection code?  Having them only repeated in
the value_strings is a bit of a waste.

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