Evan Huus
changed
bug 8313
| What |
Removed |
Added |
| Status |
UNCONFIRMED
|
INCOMPLETE
|
| CC |
|
[email protected]
|
| Ever confirmed |
|
1
|
Comment # 1
on bug 8313
from Evan Huus
Hi Alex, thanks for the dissector, it looks pretty nice.
I haven't had time for a full review, but these are a few things (mostly minor)
that I found on a quick pass:
- Please add your dissector in the correct location (alphabetically) to the
Makefiles, not at the top.
- You link to a spec from the wiki page. Putting the same link in the comment
header of the dissector is generally helpful.
- 7259 is not an IANA-registered port for SML, so your dissector shouldn't
default to registering on that port (it's welcome to suggest that port to the
user in a tooltip though).
- Global variables are generally to be avoided when possible (excepting hf_ and
ett_ globals, which are effectively constants). Specifically, the globals
'data', 'length', and 'datatype_check' are confusing since they are set in one
function and then used in another with no obvious flow between the functions.
- Please use descriptive names for your functions (I have no idea what 'TL'
does or is supposed to do), and stay away from ALLCAPS - that is typically
reserved for macros.
- You don't need to keep calling tvb_ensure_bytes_exist() before you fetch data
from the packet - the tvb_get_* functions will safely throw exceptions when
appropriate.
- I'm not a fan of putting long statements on the same line as the 'else'
keyword without any braces (for example line 2594 and similar). Putting in
regular line-breaks and braces makes it easier to read.
- You don't need to add a preference to enable/disable the dissector -
Wireshark supports disabling individual dissectors automatically.
- It doesn't build on my linux system. GCC complains with the following:
packet-sml.c: In function 'decode_GetProfilePackRes':
packet-sml.c:1562:17: error: variable 'headerList_list' set but not used
[-Werror=unused-but-set-variable]
packet-sml.c: In function 'decode_GetProfileListRes':
packet-sml.c:1747:17: error: variable 'valTime' set but not used
[-Werror=unused-but-set-variable]
packet-sml.c: In function 'decode_GetListRes':
packet-sml.c:1893:17: error: variable 'valTime' set but not used
[-Werror=unused-but-set-variable]
packet-sml.c: In function 'dissect_sml_file':
packet-sml.c:2316:17: error: variable 'crc_msg' set but not used
[-Werror=unused-but-set-variable]
packet-sml.c:2315:17: error: variable 'crc_file' set but not used
[-Werror=unused-but-set-variable]
- Cppcheck finds the following additional (minor) issues:
epan/dissectors/packet-sml.c:1617: style: Variable 'headerList_list' is
assigned a value that is never used.
epan/dissectors/packet-sml.c:1800: style: Variable 'valTime' is assigned a
value that is never used.
epan/dissectors/packet-sml.c:1982: style: Variable 'valTime' is assigned a
value that is never used.
epan/dissectors/packet-sml.c:2675: style: Variable 'crc_file' is assigned a
value that is never used.
epan/dissectors/packet-sml.c:2578: style: Variable 'crc_msg' is assigned a
value that is never used.
- There are a few places where indentation is not quite correct. Adding
modelines from https://www.wireshark.org/tools/modelines.html will likely help.
You are receiving this mail because:
- You are watching all bug changes.
