Wireshark-bugs: [Wireshark-bugs] [Bug 5553] Merge Request: Dissector for the OCFS2 network proto
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5553
--- Comment #4 from Bill Meier <wmeier@xxxxxxxxxxx> 2011-01-22 14:49:47 EST ---
More comments(in no particular order):
1. It appears that #includes of stdio.h, string.h & memory.h are not needed.
2. All the #includes after epan/packet.h aren't needed.
(packet.h handles some of the#includes; Others, such as prefs.h
just aren't needed.
3. Parts of the standard Wireshark copyright text are missing.
(See README.developer)
...
* Wireshark - Network traffic analyzer
* By Gerald Combs <gerald@xxxxxxxxxxxxx>
* Copyright 1998 Gerald Combs
...
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
*/
4. I'm not at all a fan of using goto's unless really needed.
E.G.,I don't think they are really needed in dissect_ocfs2() &
dissect_dlm_query_join_request().
For ocfs2_find_header() see comment below.
5. dissect_ocfs2(): a "new-style" dissector should return either
0 or "the amount of bytes successfully processed".
Returning a negative number is (I think) essentially treated as
indicating that the dissector succeeded.
6. ocfs2_find_header():
a. It seems to be trying to find two "magic" bytes by
searching thru the buffer.
However, after ocs_find_header() is called, AFAICS dissect_ocfs2()
is coded that the "magic" is always at offset 0 in the buffer.
b. Reassembly of the PDU is being done by setting some variables in the
pinfo struct as described in README.developer section 2.7.2.
Given that your protocol rides only over TCP, the simpler cleaner
way to do reassembly is to use tcp_dissect_pdus() as described
in section 2.7.1 of README.developer.
7. Display of "message payload":
a. General principle: If a field is of a fixed length and of a certain
number of bytes and should be present, then there's no need
to check if the bytes are actually present.
proto_tree_add_item(..., offset, length, ...) will automatically
generate a "malformed" entry for the field if the bytes are
not present. (The dissector will also be exited if this occurs).
b. If I understand the code correctly, the result is to dfisplay
up to 6 fields as FT_BYTES with a different Text label for each.
I think a simpler way to do this would be as follows:
for (i=0; i<6; i++)
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
