Wireshark-bugs: [Wireshark-bugs] [Bug 5929] New protocol dissector for "CIP Motion"
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5929
--- Comment #9 from Bill Meier <wmeier@xxxxxxxxxxx> 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.
