Evan Huus
changed
bug 8579
| What |
Removed |
Added |
| CC |
|
eapache@gmail.com
|
Comment # 2
on bug 8579
from Evan Huus
Hi Marko, thanks for the patch. Some notes from an initial review:
- The indentation is inconsistent which makes it difficult to review. Adding
modelines to the bottom of the file
(https://www.wireshark.org/tools/modelines.html) may help depending on which
editor you use.
- You do a great deal of proto_tree_append_text - I'm not familiar with the
protocol, but generally most cases are handled better with properly named and
typed fields. Wireshark's formatter does a good job already with common types.
Since you appear to have commonly-repeated custom types (which I think are what
FIELD_PART_* are?) then you may be better off using BASE_CUSTOM functions
(check README.developer).
- You wmem_alloc a string-buffer and do some operations on it. Why not just
create a wmem_strbuf and use that? If there's some behaviour or API that you
would need it to do I can look at extending it.
- I'm a bit confused by the twos_complement function - could you provide some
sort of example in the comment about what it's supposed to take and return? I
feel like there might be an easier way to do what you're trying to accomplish,
and hopefully one that works on little-endian systems as well.
- Generally prefer tvb_reported_length to tvb_length since the former correctly
handles truncated packets in captures.
- The functions proto_register_asterix and proto_reg_handoff_asterix should be
at the bottom of the file by convention so that they can be picked up properly
by some scripts we run in the build process.
- Can you provide a better explanation of what the uap array is? It's not
obvious to me as I don't know the protocol.
Thanks,
Evan
You are receiving this mail because:
- You are watching all bug changes.
