http://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2252
--- Comment #6 from Jeff Morriss <jeff.morriss@xxxxxxxxxxx> 2008-02-22 22:55:42 GMT ---
I looked at this patch for a while today. Could you clean up the code some?
Here are some particular comments I have:
1) the indentation is way off (for example the code in decode_apdu() is fine
until it gets to the "selector =" line and then the code is starts being
indented *3* more tabstops. Are you maybe using a tab stop setting other than
8?
2) this dissector registers are "new style" (that is it should heuristically
determine if the packet was handled is really LON) but:
a) it doesn't do any such heuristics (it never returns 0)
b) it doesn't return *anything* at the end of the dissect_lon() function
(leading to a warning)
3) Why use append_text to append to a text item? Like:
m_list = proto_tree_add_text (reminder_tree, tvb, offset, 5, "M_LIST:
0x");
proto_item_append_text (m_list, "%08X%02X", mlist1, mlist2);
proto_treea_add_text() takes a variable number of arguments so you don't need
the append_text() at all.
4) In general hidden items are deprecated--because how does the user know if
they are there if they are hidden? This sequence of code is a bit odd because
it's adding a hidden (but filterable) item and then a visible (but not
filterable) text:
proto_tree_add_item_hidden (reminder_tree, hf_m_list_5byte, tvb, offset, 5,
FALSE);
m_list = proto_tree_add_text (reminder_tree, tvb, offset, 5, "M_LIST: 0x");
Why not use, in this case, proto_tree_add_bytes_format()?
5) There are warnings when compiling it. Here are some with my comments:
packet-lon.c: In function `decode_reminder':
packet-lon.c:520: warning: unsigned int format, different type arg (arg 8)
--> need to use G_GINT64_MODIFIER
packet-lon.c: In function `dissect_lon':
packet-lon.c:749: warning: suggest parentheses around comparison in operand of
|
packet-lon.c: In function `proto_register_lon':
packet-lon.c:1186: warning: unused variable `lon_module'
packet-lon.c: In function `decode_apdu':
packet-lon.c:348: warning: 'application_tree' might be used uninitialized in
this function
--> I wasn't sure how to fix that
packet-lon.c: In function `decode_reminder':
packet-lon.c:462: warning: 'mlist1' might be used uninitialized in this
function
packet-lon.c: In function `dissect_lon':
packet-lon.c:661: warning: 'bytes' might be used uninitialized in this function
--> I wasn't sure how to fix those either
packet-lon.c: At top level:
packet-lon.c:92: warning: 'safetylon_handle' defined but not used
packet-lon.c:218: warning: 'hf_lon_prio' defined but not used
packet-lon.c:219: warning: 'hf_lon_npdu' defined but not used
packet-lon.c:290: warning: 'hf_cod' defined but not used
packet-lon.c:296: warning: 'gPREF_HEX' defined but not used
6) Could you split dissect_lon() into multiple functions? There's way too much
code there (as evidenced by the number of local variables needed).
7) Is there any reason to keep all the commented-out code (and the instructions
left over from README.developer)? And the copied-from header (see the line
about "Copied from WHATEVER_FILE_YOU_USED") should be updated.
--
Configure bugmail: http://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
