[bugzilla-daemon@xxxxxxxxxxxxx]

Evan Huus changed bug 8313

What	Removed	Added
Status	UNCONFIRMED	INCOMPLETE
CC		eapache@gmail.com
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.