Comment # 9
on bug 8379
from Evan Huus
Hi Sebastiano, thanks for the patch and sample capture. The patch is pretty
solid already, but I do have a few additional thoughts:
- You provided a link to some protocol information - it would be helpful to put
this in a comment near the top of the dissector code as well.
- Please put a comment by get_hpfeeds_pdu_len that it is being passed to
tcp_dissect_pdus and therefore is actually necessary (otherwise it looks rather
trivial).
- Use tvb_reported_length instead of tvb_length in order to handle captures
that have truncated packets.
- There are a few confusing indentation issues (example: line 205, the
val_to_str call should probably be indented further to indicate that it is part
of the col_add_fstr call and not its own statement).
- In the default opcode case, the "Unknown Data" should probably be an expert
info instead of just a text item. Note that expert infos (like column info)
should be set regardless of if (tree), so you might need to restructure that a
bit more.
- Your solution to the port conflict is fine - the other way to do it would be
to have a preference for the port number that defaults to 0.
- The dissector currently fails to compile on my Linux machine, with the
following minor errors:
packet-hpfeeds.c: In function 'dissect_hpfeeds_publish_pdu':
packet-hpfeeds.c:133:17: error: variable 'it' set but not used
[-Werror=unused-but-set-variable]
packet-hpfeeds.c: In function 'dissect_hpfeeds_subscribe_pdu':
packet-hpfeeds.c:161:17: error: variable 'it' set but not used
[-Werror=unused-but-set-variable]
Cheers,
Evan
You are receiving this mail because:
- You are watching all bug changes.