Wireshark-bugs: [Wireshark-bugs] [Bug 7860] Add a dissector for the America Online protocol
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7860
Evan Huus <eapache@xxxxxxxxx> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |eapache@xxxxxxxxx
--- Comment #3 from Evan Huus <eapache@xxxxxxxxx> 2012-10-20 08:14:18 PDT ---
Hi Tim, thanks for the patch. It looks pretty good already, just a couple of
nits to pick:
- Column functions (col_append_fstr etc) and expert functions
(expert_add_info_format etc) should be called regardless of whether or not tree
is NULL (ie they shouldn't be inside an if(tree) block). The easiest way to fix
this is just to remove the if (tree) check completely - the dissector will run
a bit slower under certain cases, but everything will just work.
- hf_aol_type doesn't need proto_tree_add_uint - you can set the bitmask in the
hf_register_info array and then just use proto_tree_add_item as usual.
- hf_aol_end I think can use proto_tree_add_item as well, not sure why you're
using proto_tree_add_uint there
- in dissect_aol you use 0x5a as a magic number - it would probably be better
if it was given a name using #define
- You have references in comments in several places to what I assume are
sections of the standard. If the standard is available online somewhere, please
put a link to it in the comment header (where the copyright and $Id$ are). If
it isn't available, please put a note to that effect so people don't waste
their time googling for it :)
Otherwise it looks really good. The formatting is a bit painful for anyone
trying to read it on an 80-column screen, but that's a personal preference and
has nothing to do with the quality of the code.
Cheers,
Evan
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.
