#85 Supermicro memory ecc error display

[K.C. Li]

This patch adds memory ecc error display for supermicro boards and adds a supermicro oem sensor type for SEL.

[Zdenek Styblik]

Hello,

• for code formatting, please, read https://sourceforge.net/p/ipmitool/wiki/coding_standards/
• global variable oemId ??? why exactly, because it seems totally unnecessary to me. Please, remove it!
• please, move function ipmi_get_oem_id() into 'lib/helper.c' - I think it will be useful in other modules as well

• the following code is duplicated in patch

  if (chipsetType == 1) {
    length = sizeof(supermicro_x9sb);
    for (i=0; i<length; i++)
    {
      if (oemId == supermicro_x9sb[i])
      {
        chipsetType = 2;
        break;
      }
    }
  }
  

• what exactly is the purpose of the code in question?

• also, so many loops and hoops, and switch-in-switch. Are you sure there isn't a better solution? Like stuff it into function.

• I'm not exactly happy about isSupermicro. The next thing you know, Dell comes and we'll have isDell. Then HP with isHP. Then Oracle ... If anything, variable like 'print_sensor = 1;' and set it to 0 in case of Supermicro.

  int isSupermicro = 0;
  switch (ipmi_get_oem(intf)) {
    case IPMI_OEM_SUPERMICRO:
    case IPMI_OEM_SUPERMICRO_47488:
      isSupermicro = 1;
      break;
  }
  

[K.C. Li]

Thank you for your review. I modified the uncomfortable code.

[Zdenek Styblik]

Hello KC,

please, check modified version of your patch. I've cleaned up unused variables and changed formatting here and there. I've also removed the following code as it did nothing:

if (rec->sel_type.standard_type.event_type == 0x6F) {
  /* ... */
} else {
  sensor_type = rec->sel_type.standard_type.event_type;
}

I remain confused over one thing, though.

sensor_type = rec->sel_type.standard_type.sensor_type;
if (sensor_type != 0x6F) {
  return NULL;
}
switch (sensor_type) {
  case SENSOR_TYPE_MEMORY:
    /* ... */
  case SENSOR_TYPE_SUPERMICRO_OEM:
    /* ... */

sensor_type is always 0x6F as you don't modify or work with it anywhere in the code. How can it match TYPE_MEMORY or TYPE_SUPERMICRO_OEM then? Please, clarify.

Thanks.

[K.C. Li]

I check the event_type whether it is 0x6F then check the sensor_type whether it is TYPE_MEMORY or TYPE_SUPERMICRO_OEM. I check the event and sensor two types.

[Zdenek Styblik]

KC, you're right. My bad and I have no idea how I could overlook it even though I've pasted the code in question. My excuse is an early morning.

Anyway, please, check attached patch with this issue fixed.

[K.C. Li]

Zdenek, the code is ok with me. Thank you for your help.

[Zdenek Styblik]

Committed into CVS.