Wireshark-bugs: [Wireshark-bugs] [Bug 5929] New protocol dissector for "CIP Motion"
Date: Thu, 15 Sep 2011 17:03:42 -0700 (PDT)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5929

--- Comment #9 from Bill Meier <[email protected]> 2011-09-15 20:03:39 EDT ---
Michael: I guess you've updated the patch on behalf of Benjamin Stocks.

Thanks !


IMO, there are some more changes which need to be done ...

============================================
(In reply to comment #2)
> Quick review:
> 
> Mixed tab/multi space indentation has to go.
> 

There's still a lot of mixed tab/multi-space indentation and inconsistent
indentation (e.g. 6 spaces in some cases). 

There's also a fair amount of what I call "4 space tabs". Please
replace same by 4 spaces.

Please remove all trailing blanks.


============================================

(In reply to comment #7)
> Created an attachment (id=6981)
 --> (https://bugs.wireshark.org/bugzilla/attachment.cgi?id=6981) [details]
> Updated patch
> 
> Addressed all issues except #2 & #9.

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.)

As noted above: still needs work.

> 2) Didn't know when to use "extended" value string vs. "regular" value string

ans: for "large" value_string arrays (where "large" is somewhat arbitrary):)

I'd start with:
cip_gs_vals

Speaking of which:
cip_gs_vals in packet-cipmotion.c is the same as that in packet-cip.c except
that the last two entries are missing. Do you know:  is this correct or should
the value_string arrays be identical ?  If they are identical, then please use
one common value_string; (actually just the extended value string pointer would
need to be public).

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.

$Id$ still missing;

=============
Additional notes:

1. There's no prefs so prefs_register_protoco() need not be called (and
#include <prefs.h> is not needed, etc.

also: proto_reg_handoff...() doesn't need 'if (!initialized) ...' since no
prefs and thus this fcn is only called at Wireshark startup. 

cip_motion_handle isn't actually used so no need to call
create_dissector_handle()

2. Forward ref declaration for proto_register...() not needed.

=====

Re:

> I (somewhat intentionally) didn't include the change to Makefile.common to add
> this dissector to the build.  It almost always has to be merged, and I figure
> its just as easy to just manually add it.

In my experience, including this in the patch almost always works OK (assumimg
the patch is based upon a somewhat recent SVN).

Note: epan/CMakeLists.txt should also be updated.

-- 
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.