https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5095
--- Comment #2 from Bill Meier <wmeier@xxxxxxxxxxx> 2010-08-11 14:42:52 EDT ---
Some comments:
1. Please put the contents of the packet-etch.h file into the packet-etch.c
file.
Since the .h file is not used elsewhere there is no need for a separate
file.
2. #ifdef DISSECTOR_WITH_GUI doesn't seem necessary
#include <stdio.h> doesn't appear to be needed.
3. Wireshark has a standard set of functions for "value to string" lookups.
See doc/README.developer section: 1.7.1 match_strval and val_to_str.
These should be used instead of the code in read_type & etc.
4. Names of the form g_... are used by GLib.
I suggest using something like gbl_... instead of g_... for global vars
defined in packet-etch.c.
5. Looking at the code in proto_register and proto_reg_handoff I suspect
that a "TCP port preference" is desired (rather than the
hard-wiring of port 4001).
However, proto_reg_handoff... is only called once at Wireshark
startup since you've not specified it as a callback in
etch_module = prefs_register_protocol(proto_etch, etch_prefs_apply_cb);
The normal way to handle multiple preferences for which an action
is needed when the preference is changed is:
a. Specify the proto_reg_handoff fcn as the prefs callback;
b. Include whatevefr actions are required in the proto_reg_handoff fcn.
In this case, I think you might need to keep track of th current value
of the keytab_filename so you would know if it has been changed when
proto_reg_handoff is called when there is a pref change.
doc/README.developer has some information on prefs callbacks and etc.
In any case, TCP port 4001 seems to be already assigned.
(See http://www.iana.org/assignments/port-numbers).
Is there a standard (maybe IANA assigned) port used for etch ??
6. In general, all functiona and variables other than proto_register... and
proto_reg_handoff... should be static so as to prevent any name clashes
with other dissectors.
7. Unless I'm missing something, it looks to me like there's a memory
leak each time the "hash/symbol" files are reread in that g_symbols_count
is reset to 0 but the g_malloc'd storage is not freed up.
g_symbols[g_symbols_count].symbol = (char *) g_malloc(strlen(symbol) + 1);
8. dissect_etch() needs to verify that at least 4 bytes are available
otherwise the following tvb_memeql will cause an exception.
use 'if (tvb_length(...) < 4) return FALSE;'
9. Initializing g_old_frame_num in proto_register won't work as intended
since proto_reg_handoff is called only *once* whne Wireshark is started.
If you need to initialize a variable each time a file is completely
redissected or (re)read, you'll need to register an init fcn.
See the register_init_routine in doc/README.developer.
--- There are undoubtedly more comments but the above comments are a start.
Also: Can you provide a pointer to a specification for the protocol ?
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
