[collectd] [PATCH] Plugin for Linux Software-RAID devices

[Michael Hanselmann]

Hi Florian

On Wed, Oct 6, 2010 at 2:17 PM, Florian Forster <octo at collectd.org> wrote:
> On Wed, Oct 06, 2010 at 12:26:24AM +0200, Michael Hanselmann wrote:
>> It reports the number of component devices, number of devices in
>> array, number of active, working, failed and spare disks.
>
> What's the difference between "number of component devices", "number of
> devices in array", and "number of active disks"? From looking at the
> "mdu_array_info_t" struct is looks like
>
>  active == working + failed + spare
>
> and that "active_disks" is equal to either "nr_disks" or "raid_disks".

Unfortunately I can't really explain (I'm no expert on md). I did some
tests using different RAID levels and the numbers depend on the level.
What I know of sure is that “number” is the total number of disks
involved, including failed and spares. Should I go back, do more
tests, and write it down?

I guess the most important attribute for most users (including me)
will be “failed”.

> Your code looks very clean and readable, great job! :) I have some
> comments nonetheless …

Thanks a lot for the review!

>> +static const char *config_keys[] =
>> +{
>> +     "Device",
>> +     "IgnoreSelected",
>> +     NULL
>> +};
>> +static int config_keys_num = STATIC_ARRAY_SIZE (config_keys);
>
> You should remove the NULL entry from this array (preferred) or subtract
> one from "config_keys_num" here.

Removed NULL from the list.

>> +             goto out_close;
>
> Please remove that "goto". Calling "close" and "return" in each case is
> only one line more for each case, totalling in three more lines (for
> which you can remove four lines at the end of the function ;).

Ah, you noticed the “goto”. :-) Replaced them with what you proposed.

>> +     if (st.st_rdev != makedev (MD_MAJOR, minor))
>> +     {
>> +             WARNING ("md: Major/minor of %s are %i:%i, should be %i:%i",
>> +                      path, (int)major(st.st_rdev), (int)minor(st.st_rdev),
>> +                      (int)MD_MAJOR, minor);
>> +             goto out_close;
>> +     }
>
> Since "major" and "minor" are macros defined by the (GNU) libc, having
> variables of the same name around is very confusing IMHO. To be honest,
> I'm surprised the preprocessor is fine with this.

Looking at it now I'm also surprised. This is the definition of “major”:

#define major(dev) gnu_dev_major (dev)

I guess the preprocessor only replaces it if parentheses follow the
name. “disk.c” already uses “major” and “minor” as variable names.
Should I change the name nonetheless or leave it?

>> +     if (ioctl (fd, GET_ARRAY_INFO, &array) < 0) {
>> […]
>> +     assert (array.md_minor == minor);
>> […]
>> +     md_submit (minor, "md_disks", "number", array.nr_disks);
>
> I think I wouldn't assert this here. If you want to make absolutely
> certain that the ioctl didn't return something else, you should check
> using a normal "if" condition and return an error.

I just figured that if the kernel reports wrong data on this ioctl
after the minor is already checked using fstat(2), bigger things are
broken and the code here shouldn't worry. Removed assertion.

Won't resend just yet, I'll wait for your answers to my questions.

Regards,
Michael

-- 
http://hansmi.ch/