https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4704
--- Comment #2 from Anders Broman <anders.broman@xxxxxxxxxxxx> 2010-04-21 22:23:31 PDT ---
The author marked the bug as private making it accessabe to core developers
only.
I presume that covers the enclosed captures, I've copied the source code here.
And here's the relevant comments on it:
Jeff Morriss 2008-02-22 22:55:42 PST
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.[reply] [-]
Private Comment 7 Jeff Morriss 2008-02-22 22:56:59 PST
(From update of attachment 1451 [details])
Dissector should be reworked some before being checked in.[reply] [-] Private
Comment 8 Jaap Keuter 2008-09-30 00:12:29 PDT
Khalid, there are a number of comments on your code, but so far no updates have
been received. Are you still interested in getting your code in? If so, please
work the issues Jeff identified and resubmit a patch, preferably against the
current svn HEAD or latest stable release.[reply] [-] Private Comment 9 Jaap
Keuter 2008-12-30 13:32:26 PST
So far we have received no response. This way the code won't be going into
mainstream.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
