Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 3488] Add support for decoding CAPWAP Control Packet (RFC5415)

[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=3488


Stig Bjørlykke <stig@xxxxxxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |stig@xxxxxxxxxxxxx




--- Comment #3 from Stig Bjørlykke <stig@xxxxxxxxxxxxx>  2009-05-25 14:03:49 PDT ---
Short review:

1. You should use the standard proto_tree_add_item() in many places where you
use proto_tree_add_uint/boolean/string

Example:

This line:
  proto_tree_add_uint(capwap_control_header_tree,
hf_capwap_control_header_seq_number, tvb, offset, 1, tvb_get_guint8(tvb,
offset));

Should be written as:
  proto_tree_add_item(capwap_control_header_tree,
hf_capwap_control_header_seq_number, tvb, offset+plen, 1, FALSE);

2. Note the offset bug in the previous line

3. You shold not check "if (tree)" when adding values, as this will produce
different output when toggeling "Colorize Packet List"

4. The hf-blurb shall be NULL if not different from the name

5. You no long er need to call check_col()

6. Your switch statements should have a default, even if it's empty, just to
clarify "no default action"

7. I think the dtls should be added to the top level tree, because it's another
protocol

8. Your bit values is a bit hard to read when comparing with the packet bytes,
but that may be a matter of taste...


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