[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2935





--- Comment #12 from David Castleford <david.castleford@xxxxxxxxxxxxxxxxxx>  2008-10-09 08:46:36 PDT ---
(In reply to comment #9)
> First: Thanks for your effort so far on this dissector !
>
thanks for your feedback, hope I'm getting there! please see updated diff file

> Some additional comments:
> 
> 1.   I don't believe the 'if (..._tree)' for the various subtrees is req'd
>      since proto_item_add_subtree will always return a non-null value 
>      (as long as the parent is non-null)
> 
ok, just followed the examples, removed

> 2.  The following don't appear to be used:
>    - mikey_tcp_port
>    - proto_register(): 'int i = 0;'
> 
see new patch, removed a lot of obsolete / unused things

> 3. pvaluedec is defined as a guint16 and is later calculated 
>    as 100 * <a guint16>.
>    This seems like a potential for overflow.
> 
corrected, guint32 used now

> 4. The 'if (...==-1)' in proto_register.... is not needed.
>
ok 
> 5. The code to build 'param_value' uses strcat and sprintf which are "memory
> unsafe" functions and thus not allowed in Wireshark.
>
removed altogether and used tvb_bytes_to_str, saw in your next comment you
prefer the extra space, be my guest to change if you wish

> I suggest instead displaying the "param value" fields by using
> FT_BYTES and BASE_HEX in the relevant hf[] entries. If you would rather have
> the hex strings shown with spaces between each byte, then I guess you'll 
> need to build param_value as now but instead maybe 
> using g_strlcat & g_snprintf.
>
done

> See the glib reference for info on g_strlcat &etc 
> (http://library.gnome.org/devel/glib/2.16/glib-String-Utility-Functions.html)
> 
> (There may be a simpler way which someone else can suggest).
> 
> Note: for future reference I suggest reviewing the doc/README.developer
> document (if you haven't already done so) which discusses such things as not
> using strcat & etc.
> 
> 6. With respect to proto_reg_handoff:
> 
>    As you note in comment #7, the code involving tcp_port and global_tcp port
>    is similar to that used in other dissectors. It needs one additional line 
>    to be OK:
> 
>   if (!initialized) {
>     simulcrypt_handle = create_dissector_handle(dissect_simulcrypt, ...
>     dissector_add("tcp.port", global_simulcrypt_tcp_port, ...
> >>  tcp_port = global_simulcrypt_tcp_port;
>     initialized = TRUE;
>   }
>
corrected, thanks 
> ---
> 
>    The code which seems unneeded is:
>                
> if(!strcmp(tab_ecm_inter[i].protocol_name,CA_SYSTEM_ID_MIKEY_PROTO))
>   {
>     tab_ecm_inter[i].ca_system_id=ca_system_id_mikey;
>     tab_ecm_inter[i].ecmg_port=-1;
>   }
> datagram_handle=find_dissector(tab_ecm_inter[i].protocol_name);
> dissector_delete("tcp.port", tcp_port, datagram_handle);
>
don't ask me why, but with the code disabled the ECM_datagram corresponding to
set ca_system_id is not dissected as MIKEY protocol

> Let try to explain:
> a. As far as I can tell, The simulcrypt dissector never actually registers 
>    a handle for tab_ecm_inter[i].protocol_name on tcp_port and thus 
>    a dissector_delete is not required. 
>    (In actuality, altho the Wirehark documentation may not 
>    be clear, all that dissector delete really does is to deregister *whatever*
>    handle is currently registered on the specified tcp port; 
>    the 'handle' argument is currently ignored).
>    So: the find_dissector() ... delete_dissector lines are not required.
> 
> b. ecmg_port is being used elsewhere to apparently determine when a
>    sub-dissector should be called (find_dissector & etc).
> 
>    I would note that I think it's the case that find_dissector need 
>    be called only once during proto_reg_handoff for each of the protocols in
>    tab_ecm_inter[] (with the handle being stored in another variable 
>    in tab_ecm_inter.
> 
>    I'm not sure when ecm_port needs to be initialized to -1; maybe for each new
>    capture via an init function registered using 'register_init_routine' ? 
>    (The registered init function will be called each time a capture file 
>    dissection is done).
> 
>    Please let me know if I'm missing something
> 
beyond my grasp, afaik the current code works, please see new diff file and let
me know - if you can remove things and still have it working, fine by me :-)

note that use of ecm_interpretation structure allows a new protocol to be added
to a corresponding ca_system_id (done for a private dissector)


-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.