[bugzilla-daemon@xxxxxxxxxxxxx]

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





--- Comment #10 from Jeff Morriss <jeff.morriss.ws@xxxxxxxxx>  2009-01-08 06:44:10 PDT ---
Sorry for the delay.  Here are a few more comments:

The main dissector code consists of a lot of 'if' statements like:

+       if( (value == GED125_FAILURE_CONF_VALUE) && size == 16 )

1) You should probably rename the variable 'value' to 'message_type' since
that's what it is.
2) Why check the size for each message type?  That means if the message is
truncated (either due to an error or, since size is based on tvb_length()
instead of tvb_reported_length(), because a snapshot length was used when
capturing) then the dissector won't even attempt to dissect the message. 
Attempting to and failing is actually a good thing because the user will see
the packet as malformed or truncated.  I'd suggest changing that to a switch
statement while dropping the size comparisons.


A lot of the dissection code extracts the data value and then (only) adds it to
the tree:

+               value = tvb_get_ntohl(tvb, offset);   
+
+               proto_tree_add_uint(y_tree, hf_ged125_InvokeID, tvb, offset,
LEN4, value);
+
+               offset+=LEN4;
+
+               value = tvb_get_ntohl(tvb, offset);

It would save (a lot of) code and time to just add all those values to the tree
with proto_tree_add_item(); you should only need to tvb_get() values that you
then use in your code (like the message type).  That's a global comment for the
whole dissector.


There are still some hard-coded values out there (this one in particular
already has a define):

+       if(mess_type != 47 && check_col(pinfo->cinfo, COL_INFO))
+               /*don't want to overwrite the service control column info*/

For the above code, wouldn't it work to just set COL_INFO before dissecting the
messages and then let the service control dissection overwrite it?  There's at
least one other place where COL_INFO was set at the end of the function but
generally it makes sense to do it at the beginning (unless, of course, you
don't have all the necessary info yet).


This code:

+       if(invalid_length)
+               proto_tree_add_string(j_tree, hf_ged125_General_Error, tvb,
offset, -1,
+                                        GED125_IMPOSSIBLE_LENGTH_ERROR);

could also set an expert info, see
http://wiki.wireshark.org/Development/ExpertInfo

(That's just a suggestion, it's not necessary.)



Also: since this dissector is not used by any others, I'd suggest getting rid
of the header file and putting all the defines and hf_ and ett_ variables at
the top of the .c file (like the rest of the dissectors).

One last comment: I think the last couple of attachments were supposed to
contain a sample PCAP file but they don't ('svn diff' won't include a PCAP
file).


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