This was my reply if it didn't get through your filters
Al
--
Albert Chu
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory
Hey Ryan,
For the most part looks fine. I assume the final patch will also clean
up the code comments to describe the CMC version packet :-) Just a few
notes:
+int
+ipmi_oem_dell_get_cmc_firmware_version (ipmi_oem_state_data_t
*state_data)
since this is an internal function it should be static and not have an
entry in the header file. For naming consistency, it should probably be
_get_dell_cmc_firmware_version() or something similar to the other
functions called by ipmi_oem_get_system_info().
+ if(rs_len != 20) {
+ pstdout_fprintf (state_data->pstate,
+ stderr,
+ "dell:get-system-info 'cmc-firmware-version'
option not supported on this system\n");
+ goto cleanup;
+ }
I don't think this check is really necessary, it'll be done via
ipmi_oem_check_response_and_completion_code(). I suppose you added it
for a nicer error message, but with ipmi-oem, I think there is a larger
acceptance of "you gotta know what you're doing" b/c it's so
OEM/motherboard/system/product specific (vs ipmi-sensors and more
standard tools). The manpage should document the motherboards its
confirmed against or types of motherboards its expected to work with.
Al
On Fri, 2012-05-18 at 10:34 -0700, Ryan Cox wrote:
> This patch adds "ipmi-oem dell get-system-info cmc-firmware-version".
> From a blade, it grabs the firmware version of the CMC for the
> enclosure that the blade is in. It appears to only show the active CMC
> firmware and not show anything for the standby CMC (if there is one). I
> have no idea what it would show if there is a redundant CMC using
> different firmware. This patch has worked for every Dell blade (10G,
> 11G, 12G) we have in an M1000e enclosure, including M1000e's with one
> and two CMC's.
>
> The version can be grabbed with: ipmi-raw 0 6 0x59 0 0xDF 1 0
> rcvd: 59 00 11 01 00 00 00 00 33 2E 30 33 00 00 00 00 00 00 00 00
>
> "3.03" in this case. I have tried this with several 3.x and 4.x
> versions and the version numbers are all 4 characters long.
>
> As far as I can tell, there is no easy way to grab this through existing
> generic functions in ipmi-oem-dell.c. Those functions appear to assume
> set selector and block selector 0, though the set selector is different
> in this case. I had to decide to either reinvent the wheel somewhat or
> modify the other functions to take selectors as parameters instead of
> assuming 0. I took the easy route and reinvented the wheel, though I'm
> open to changing that if needed. I figured I should at least get some
> feedback before submitting a patch that changes a whole lot of code.
>
> This patch may need some reworking, but it has been tested successfully
> on: M600, M610, M610x, M910, and a pre-prod 12G system. On a rackmount,
> it returns "dell:get-system-info 'cmc-firmware-version' option not
> supported on this system".
>
> Let me know if there's anything that needs to be changed and if you
> would prefer the slight duplication of code that I did versus adding set
> and block selector parameters to several functions.
>