http://bugs.wireshark.org/bugzilla/show_bug.cgi?id=1430
------- Comment #3 from jeff.morriss@xxxxxxxxxxx 2007-03-09 14:50 GMT -------
A few comments:
1) Is there any reason for doing:
+ cmd = tvb_get_guint8(tvb, pos);
[...]
+ switch (cmd) {
+ case ECAT_CMD_APRD: {
+ cmd_str = "APRD";
+ } break;
[...]
+ if (check_col(pinfo->cinfo, COL_INFO)) {
+ col_append_fstr(pinfo->cinfo, COL_INFO, "%s[%d]", cmd_str, len);
+ if (next) {
+ col_append_str(pinfo->cinfo, COL_INFO, ", ");
+ }
+ }
Instead of using, say, a value string?
2) why isn't 'cmd' added to the tree?
3) some info about the protocol in the header of the file would be good (e.g.,
where does the protocol come from, etc.).
4) This variable isn't used, should it be or should it be removed? (There may
be others but I didn't try compiling it with gcc yet.)
+/* Global sample preference ("controls" display of numbers) */
+static gboolean gPREF_HEX = FALSE;
A previous post about the Ethercat protocol:
http://www.wireshark.org/lists/wireshark-dev/200609/msg06915.html indicates
that multiple protocols run on this ethertype. This makes me wonder if there
should be a "packet-beckhoff" dissector that then hands data to
"packet-ethercat" and the others. But I guess there's no need for that until
someone writes dissectors for the other protocols.
Beyond that, could you add a section in the Wiki about this protocol along with
a sample capture? The latter would be useful for fuzz testing the dissector
before checking it in.
--
Configure bugmail: http://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
