https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4118
--- Comment #4 from Bill Meier <wmeier@xxxxxxxxxxx> 2009-10-13 14:42:45 PDT ---
Thanks for your effort on the dissector.
The following comments based upon a quick "first pass" review...
- The dissector needs a $Id$ near the top (See doc/README.developer);
- All fcns other than proto_register & proto_reg_handoff should be static;
- Not req'd:
void proto_register_packetbb();
void proto_reg_handoff_packetbb(); /* Causes Compiler warning on Windows */
/* due to missing (void). */
/* No matter since the forward ref */
/* is not needed. */
- In dissect_pbb_tlvblock:
indexEnd = addrCount;
gives Compiler warning on Windows:
conversion from 'gint16' to 'guint8', possible loss of data
- proto_register & proto_reg_handoff should be moved to the end of the file.
- C++ comments should be changed to C style;
- hf[]: "blurb" fields with "" should be replaced by NULL;
- Please use tfs_true_false (see epan/tfs.h) rather than your own
version of same.
- if (proto_packetbb == -1) not req'd;
- proto_reg_handoff: Code looks OK but it would be appreciated
if you could use the "standard idiom" for handling port-delete/port-add.
see packet-agentx.c for an example.
(Also: packetbb_handle can/should be local to proto_reg-handoff...)
- check_col is no longer req'd around col_set_str and col_clear
- COL_INFO never gets filled with anything ....: Is this as intended ???
- In general: I think The "too many octets"/"not enough octets" type
situations may be better handled by generating a
"Malformed" Expert message & etc:
However: I need to think a bit about what might be the best way to
do this in this dissector.
So: Let's leave this for a 2nd review. (or Maybe someone else
will have a suggestion).
- Don't use tvb->length & etc. The convention is to call one of
the tvb_... fcns;
In most cases: tvb_reported_length(...):
Using tvb_reported_length means that the case of a
"capture with truncated frames" will be handled properly with the correct
message displayed if the dissector tries to access a "valid" protocol field
which happens to not be present because the capture limited the length
of the each frame captured.
- ti = proto_tree_add_item(tree, proto_packetbb, tvb, 0, tvb->length, FALSE);
use -1 for length ("from offset to end") not tvb->length
- dissect_pbb_addressblock
I'm a bit nervous about all the tvb_get_ptr/memcpy stuff:
However: let's leave this for a 2nd review.
Why is address[255] static ?
if (maxoffset - offset < head_length):
what if head_length==0: Will this work ??
- Has this dissector been fuzz-tested with the test .pcap file ??
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
