Wireshark-bugs: [Wireshark-bugs] [Bug 4118] PacketBB (RFC 5444) support for Wireshark
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4118
--- Comment #7 from Henning Rogge <hrogge@xxxxxxxxxxxxxx> 2009-10-14 02:33:16 PDT ---
(In reply to comment #4)
> The following comments based upon a quick "first pass" review...
Thanks for this quick review, I think I addressed most comments in the second
patch.
> - C++ comments should be changed to C style;
I searched the whole patch but did not find any "// ...." comments.
> - COL_INFO never gets filled with anything ....: Is this as intended ???
I'm not sure what I should put into the field. Maybe the packet sequence number
if available and the number of messages ?
> - 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).
PacketBB is a very "variable" packet format, so there are lot's of things that
can go wrong. That's why I inserted all this "not enough/too many" error
messages.
> - 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.
PacketBB allows to define a fixed prefix/postfix for an address block, so I
have to assemble the whole address from three parts.
> if (maxoffset - offset < head_length):
> what if head_length==0: Will this work ??
"head_length = 0" is the default case (and works). The if() should only trigger
if there are not enough bytes left in the stream for the head_length fixed
address prefix.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
