https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5812
--- Comment #7 from Stephen Fisher <steve@xxxxxxxxxxxxxxxxxx> 2011-04-07 17:42:04 MDT ---
Thanks for the update. Here is a more detailed review:
- A comment would be nice above the v4prefix declaration and assignment to say
what it is (I'm guessing that it is a way to store an IPv4 address inside a
16-byte space that normally fits an IPv6 address right?)
- The comment above format_address() and format_prefix() states that they
return ephemeral strings, which is true other than the if(prefix == NULL)
return "corrupt" part. You could use return ep_strdup("corrupt") instead.
- In format_prefix(), instead of dynamically allocating a 50 byte ep_ buffer
then using g_snprintf with a limit of 50 characters, you can simplify it to use
ep_strdup_printf() to duplicate a string created using printf-style syntax (as
g_snprintf does) and the newly created string is ep_ allocated.
- While strictly not necessary, the protocol name "Babel" would typically be
all caps: "BABEL" in the call to col_set_str() for COL_PROTOCOL
- We try to stay away from C99 code such as declaring variables inside the
middle of a body of code and instead do it all at the top of the function for
maximum portability.
- Running tools/checkAPIs.pl lists these issues:
Error: the blurb for field "Body Length" ("babel.bodylen") matches the field
name in epan/dissectors/packet-babel.c
Error: the blurb for field "Message Type" ("babel.message.type") matches the
field name in epan/dissectors/packet-babel.c
Error: the blurb for field "Message Length" ("babel.message.length") matches
the field name in epan/dissectors/packet-babel.c
Error: the blurb for field "Nonce" ("babel.message.nonce") matches the field
name in epan/dissectors/packet-babel.c
Error: the blurb for field "Seqno" ("babel.message.seqno") matches the field
name in epan/dissectors/packet-babel.c
Error: the blurb for field "Address Encoding" ("babel.message.ae") matches the
field name in epan/dissectors/packet-babel.c
Error: the blurb for field "Raw Prefix" ("babel.message.prefix") matches the
field name in epan/dissectors/packet-babel.c
Error: the blurb for field "Prefix Length" ("babel.message.plen") matches the
field name in epan/dissectors/packet-babel.c
Error: the blurb for field "Metric" ("babel.message.metric") matches the field
name in epan/dissectors/packet-babel.c
Error: the blurb for field "Hop Count" ("babel.message.omitted") matches the
field name in epan/dissectors/packet-babel.c
(That just means that if you don't want a detailed FIELDDESCR description, set
it to NULL instead of the same ting as FIELDNAME)
It's a good idea (necessity actually) to run the dissector with the
tools/fuzz-test.sh script against a capture file containing the new dissector's
packets. This script intentionally corrupts packets to see if the dissector
will crash. I just had it crash on the 7th pass (seg fault) and will attach
the corrupted capture file in case you get a chance to track down the problem
before me.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
