Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 1970] review_for_checkin : plugins IPMB protocol with GPL version 2

[bugzilla-daemon@xxxxxxxxxxxxx]

http://bugs.wireshark.org/bugzilla/show_bug.cgi?id=1970





--- Comment #52 from Jeff Morriss <jeff.morriss@xxxxxxxxxxx>  2008-02-25 22:12:05 GMT ---
Wow, there's a lot of code here.  For the most part it looks good, but here are
some comments:

- There are still C++ style comments in hpm.h (no big deal)
- Many of the filter names don't start with the protocol name ("ipmb" in this
case)--which is something we've been trying to clean up lately.  For example:

         { &hf_GetChassisStatus_datafield_miscChassis_Bit0, {
         "Bit 0", "GetChassisStatus.datafield.miscChassis.Bit0",
         FT_UINT8, BASE_HEX,
VALS(cmd_GetChassisStatus_data_miscChassis_Bit0_vals), 0x01,
         "Bit 0", HFILL }},

- I don't particularly like the widespread use of raw numbers.  Why not have
#defines for, for example, this:

application.c:                     case 0x37: /*Query Rollback status (QRS) */
application.c:                     case 0x37: /*Query Rollback status (QRS) */
application.c:                     case 0x37: /*Query Rollback status (QRS) */
application.c:                     case 0x37: /*Query Rollback status (QRS) */
packet-ipmb.c:   {0x2c,   0x37,   dissect_cmd_Query_Rollback_Status},
packet-ipmb.c:            case 0x37: /*Query Rollback status (QRS) */
packet-ipmb.c:            case 0x37: /*Query Rollback status (QRS) */
packet-ipmb.h:   { 0x37,   "Query Rollback Status" },

- there are a lot of redundant if(tree) calls like:

      /* PICMG Identifier */
      if (tree) {
         proto_tree_add_item( ipmb_tree, hf_GetIPMBLinkInfo_datafield_PICMGID,
                        tvb, (*poffset)++, 1, TRUE );
         len--;
      }
      /* IPMB-0 Link Number */
      if (tree) {

I suppose the compiler may optimize those out, but why not put one big 'if'
statement at the top?  (I only noticed this because of this warning:

picmg.c: In function `dissect_cmd_Get_IPMB_Link_Info':
picmg.c:1457: warning: 'requestByte2' might be used uninitialized in this
function

That wouldn't be a warning except for the excessive use of if's.)

- Why are there so many tests of the length remaining?  Like:

      if (tree && ( len > 0 ) ) {

Are the fields following such tests really optional?  If not, the tests can be
taken out because Wireshark will show the packet as malformed if the code tries
to access data beyond what is available (in the tvb buffer).

- Are all the hf_ entries really being used?

% grep -c hf_ packet-ipmb.h
857
% grep -c \&hf_ packet-ipmb.c
742

(I have to admit I'm a bit skeptical of all the duplication that has to happen
to have this dissector split into so many files, but...)


None of these are really showstoppers in my opinion--largely because I could
fix them all, but I don't really have the time at the moment.

Also, if you do update the source, please please don't attach 30 individual
files: it makes downloading all the files quite hard.  You can use 'tar'
(typically on UNIX) or WinZip (Windows) to put all the source files in one big
file.


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