Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 2252] Lon-Protocol

[bugzilla-daemon@xxxxxxxxxxxxx]

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.