Evan Huus
changed
bug 8573
| What |
Removed |
Added |
| Status |
UNCONFIRMED
|
INCOMPLETE
|
| Ever confirmed |
|
1
|
Comment # 13
on bug 8573
from Evan Huus
Some additional notes on the updated patch for this bug:
- Indentation is still not consistent (in Makefile.common or in the dissector)
for some reason?
- You should add the files to epan/CMakeLists.txt as well.
- There's no need for #ifdef HAVE_CONFIG_H, we always have config.h now.
Additionally this should be with the other #includes, and not above the file
header.
- Rather than manually passing extra bytes off to the 'data' dissector, please
use a new-style dissector that returns the number of bytes dissected and let
Wireshark handle any extra as appropriate.
- There are still many unnecessary length checks - Wireshark automatically
handles cases where you try and go past the end of the packet, so you can just
assume there's enough data in all cases.
- Even in the case where you must fetch the value anyways, if there is no
modification then proto_tree_add_item is preferred (the one that caught my eye
was dissect_ptpIP_start_data; you do need to fetch dataLen yourself, but you
can still use proto_tree_add_item to put it in the tree).
- Adjusting offset by sizeof(variable) is a bit odd, though not really wrong.
It's probably better to hard-code 1,2,4,8 as necessary since then if you
accidentally use a guint instead of a guint32 (for example) it doesn't change
on different architectures. And the actual size you want will only change if
the actual protocol changes, so hard-coding them isn't terrible in this
particular case (the tvb_get_* calls would have to be changed in that case
anyways).
- The only things that go in the header should be things that might need to be
shared with other protocols (presumably PTP/USB). Even though function
prototypes might normally go in a header file, we try to put as much in the .c
file as possible to make it obvious what's public and what's not. I'm not sure
exactly what is supposed to be sharable, but I assume somethings at least can
be moved to the .c file.
Cheers,
Evan
You are receiving this mail because:
- You are watching all bug changes.