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


avn@xxxxxxxxxxxxxxx changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |avn@xxxxxxxxxxxxxxx




------- Comment #47 from avn@xxxxxxxxxxxxxxx  2008-01-13 00:49 GMT -------
We have also developed a patch to enhance IPMI dissection in Wireshark, 
submitted in bug #2048. Thus, even though I am not a Wireshark committer,
I took the liberty of comparing both patches:

- It seems that your patch just adds an IPMB dissector that mostly 
duplicates the code in epan/dissectors/packet-ipmi.c. Wouldn't it be 
better to enhance the "generic" IPMI dissection code - so that IPMI 
over RMCP would benefit as well? Our patch enhances the generic IPMI 
dissection code, splitting it into "IPMI" and "IPMI session wrapper". 
The former can be used to implement IPMB dissector. I think, if patch
in the bug #2048 is integrated, your plug-in could be made much simpler
by handing off the dissection to generic IPMI code.

- We have implemented a request-response pairing, basing on Wireshark 
concept of "conversations". First of all, this feature allows for easy 
lookup of the responses to requests and vice versa - which could be a 
problem in busy shelves where response may be dozens of frames away 
from its corresponding request. Also, this allows for caching of data 
from the request so that the response could be parsed. You did 
something similar in Get LAN Configuration Parameters (GLCP), but your 
implementation (using static variables) has some flaws:

    * Consider a case where there are two GLCP requests followed by two
      GLCP responses: Req1, Req2, Resp1, Resp2. Your implementation will
      not match Req1 with Resp1 and Req2 with Resp2.
    * Your code that matches requests and responses doesn't account for
      LUNs of the source and destination. E.g., if the source LUN is not
      zero and destination LUN is zero in the request, the rqSeq will 
      not match rsSeq.
    * Worst of all, it seem that you don't account for the fact that 
      Wireshark runs packet dissector each time the packet is selected.
      That is, imagine the following scenario: Req1, Resp1, ... Req2,
      Resp2. Both Req1 and Req2 have the same source/destination/
      sequence (that could happen, since there are only 64 sequence 
      numbers in IPMI). If user selects Req2 and then Resp1, you will
      attempt to use wrong parameter selector.

- For some of the commands, dissectors are implemented, but not hooked
into the sub-dissector table (ipmi_cmd_table). Examples are Suspend BMC
ARPs, Get UDP/RMCP Stats.

- It looks somewhat strange to artificially separate the header fields
(hf_*) from the commands they're used in. In your implementation, header
fields are defined in ipmb.c and used in files that define sub-dissectors
(application.c, ...).

- The checks for command-specific completion codes are invalid. The
command-specific completion codes do not _replace_ the standard ones,
they _augment_ them. Your check (completionCode >= 80 && netfn == 0x2d)
will not handle standard completion codes (0xC0..0xFF) for HPM.1
commands. Moreover, the correct check would be against 0x80, not 80.

- The format of the message embedded into Send Message command
may vary depending on the medium of the target channel. Patch in
bug #2048 uses heuristics to guess that format, allowing to override
it to a specific variant ("IPMB"/"LAN"/"Unknown - do not dissect").

- It doesn't look right to interpret OEM netFns (0x30..0x3E)
unconditionally as Kontron commands, other OEMs may use this range
to implement their own commands. I'd suggest at least implementing a
configuration option to switch between possible interpretations
of OEM commands, Kontron being one of the possible variants.

- There are some vestiges of the code you copied: e.g.
Makefile.common states that it's for H.223 plugin.


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