Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 5929] New protocol dissector for "CIP Motion"

[bugzilla-daemon@xxxxxxxxxxxxx]

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

--- Comment #6 from Chris Maynard <christopher.maynard@xxxxxxxxx> 2011-08-02 18:12:21 PDT ---
(In reply to comment #3)
> Created an attachment (id=6616)
 --> (https://bugs.wireshark.org/bugzilla/attachment.cgi?id=6616) [details]
> Revised patch
> 
> Code modified per comments, fuzz tested against 5 capture files and 13 cycles.

Some additional feedback:
1) Can you clean up some of the whitespace?  (e.g., hf_cip_control_status
declaration and others don't line up with the rest, value_string declarations,
etc.)

2) Consider using extended value strings, particularly for the large value
string arrays.

3) Use ENC_LITTLE_ENDIAN (or ENC_NA as appropriate) instead of TRUE as the
endian argument to proto_tree_add_*

4) All your defines for things like CONTROL_MODE, FEEDBACK_CONFIG, etc., those
are defines for offsets to those fields.  Personally, I don't think using
defines makes it any clearer and would prefer to see the offset values
themselves, but if you're going to use defines, might I suggest
CONTROL_MODE_OFFSET, FEEDBACK_CONFIG_OFFSET, ...

5) Speaking of those defines, I don't think it's necessary to have a separate
packet-cipmotion.h header file since everything contained within it is only
used within packet-cipmotion.c itself.

6) In dissect_devce_event(), you don't need to check "if ( nots > 0 )", since
if it's not, then the for() loop won't be entered.

7) If at all possible, eliminate proto_tree_add_text() as any field added using
it won't be filterable.  I'm referring to code such as:

proto_tree_add_text(header_tree, tvb, offset +
GET_AXIS_ATTR_REQ_NUM_ATTRIBUTES, 2, "Number of attributes: %d",
attribute_cnt);

... and others like it.

8) In dissect_cntr_service(), dissect_devce_service(), etc., consider a switch
instead of an if, else if, else if, else construct.

9) In dissect_var_inst_header(), you pass in a guint32* inst_number, but then
assign a guint8 to it.  Should you instead pass in a guint8*, or should you be
assigning a 32-bit value to it instead of an 8-bit value?  Same goes for the
other args like cyc_size, cyc_blk_size, evnt_size, and servc_size.  Similar for
dissect_var_cont_conn_header() as well, and others.

10) Patch failed to apply cleanly.

11) checkhf.pl finds the following problem:
$ tools/checkhf.pl epan/dissectors/packet-cipmotion.c
ERROR: NO ARRAY: epan/dissectors/packet-cipmotion.c, hf_cip_accel_cmd

Looks like a copy-and-paste bug where you have incorrectly used hf_cip_vel_cmd
twice instead of using hf_cip_accel_cmd.

12) checkAPIs.pl finds the following problems:
$ tools/checkAPIs.pl epan/dissectors/packet-cipmotion.c
Warning: epan/dissectors/packet-cipmotion.c does not have an SVN Id tag.
Error: the blurb for field "Instance Count" ("cipm.instancecount") matches the
f
ield name in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Last Update Id" ("cipm.lastupdate") matches the
fiel
d name in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Node Status" ("cipm.nodestatus") matches the field
n
ame in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Node Control" ("cipm.nodecontrol") matches the
field
 name in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Node Faults and Alarms" ("cipm.fltalarms") matches
t
he field name in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Time Data Set" ("cipm.timedataset") matches the
fiel
d name in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Group Sync Status" ("cipm.syncstatus") matches the
f
ield name in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Command Data Set" ("cipm.cmdset") matches the field
name in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Actual Data Set" ("cipm.actset") matches the field
n
ame in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Status Data Set" ("cipm.stsset") matches the field
n
ame in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Axis Status" ("cipm.axisstatus") matches the field
n
ame in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Axis I/O Status" ("cipm.axisiostatus") matches the
f
ield name in epan/dissectors/packet-cipmotion.c
Error: the blurb for field "Axis Safety Status" ("cipm.safetystatus") matches
th
e field name in epan/dissectors/packet-cipmotion.c

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