[collectd] PATCH: ipmi

[Florian Forster]

Hi Peter,

thanks a lot for your patch :)

On Tue, Aug 12, 2008 at 06:09:22PM +0200, Peter Holik wrote:
> * Sensor names
>   Because i had twice sensor names i changed them to
>   current-Current 1 power_supply (10.1).rrd

This is not backwards compatible, but I think I'll make an exception
here. Having one name multiple times seems to be quite common with IPMI,
at least with Dell machines..

> * I store sensor names/type in c_ipmi_sensor_list_t
>   Because i thought why every time build the sting after got a value.

Okay, good point.

> * Added notification configureable for
>   * NotifySensorNotPresent
>   * NotifySensorRemove
>   * NotifySensorAdd

Nice :)

Could you maybe separate this into two patches? One for the notification
stuff and one for the rest?

What does the global `c_ipmi_sensor_removed' variable do? It's set to
one in `sensor_list_remove' and then never reset to anything else.. Do
you want to send `ADD' notifications only after a `REMOVE' has been
reported? Wouldn't it make more sense to do something like:
-- 8< --
  static int c_ipmi_init_in_progress;
  ...
  sensor_list_add ()
  {
    if (c_ipmi_nofiy_add && (c_ipmi_init_in_progress == 0))
      /* send_notification */
  }
  ...
  c_ipmi_init ()
  {
    /* Don't send `ADD' notifications during startup */
    c_ipmi_init_in_progress = 1 + (10 / interval_g);
  }
  ...
  c_ipmi_read ()
  {
    sensor_list_read_all ();

    if (c_ipmi_init_in_progress > 0)
      c_ipmi_init_in_progress--;
    else
      c_ipmi_init_in_progress = 0;
  }
-- >8 --

You use this fancy C99 syntax to initialize the `notification_t' struct:
-- 8< --
  notification_t n = { .severity = NOTIF_WARNING,
    .time = time(NULL),
    .plugin = "ipmi",
    .plugin_instance = "",
    .meta = NULL };
-- >8 --
I agree that his sytnax is nice and handy, but unfortunately there are
popular compilers which can't cope with this syntax - I've stumbled upon
this problem with the Sun C compiler, for example. So could you change
this to the old
  notification_t n = { NOTIF_WARNING, time(NULL), "", "", "ipmi", "", "", "", NULL };
syntax please?

Last but not least: The entire C file is indented with spaces. Could you change
your code to use spaces rather than tabs, please?

Best regards,
-octo
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20080814/f126b92b/attachment.pgp