[bugzilla-daemon@xxxxxxxxxxxxx]

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.