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 <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.
- Prev by Date: [Wireshark-bugs] [Bug 6343] PostgreSQL Startup message not properly supported by the PostgreSQL dissector
- Next by Date: [Wireshark-bugs] [Bug 5929] New protocol dissector for "CIP Motion"
- Previous by thread: [Wireshark-bugs] [Bug 5929] New protocol dissector for "CIP Motion"
- Next by thread: [Wireshark-bugs] [Bug 5929] New protocol dissector for "CIP Motion"
- Index(es):
- Get Wireshark
- Download
- Code of Conduct