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

[Florian Forster]

Hi Michael,

thank you very much for your patch :) Especially thanks for including
changes to the manpage and the build system!

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".

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

> +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.

> +		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 ;).

> +	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.

> +	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. And if you replace
"minor" with "array.md_minor" in the call to "md_submit", the worst that
can happen without the check is that an minor number is dispatched
although it was ignored using its device name.

Regards,
—octo
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x0C705A15
http://octo.it/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: Digital signature
URL: <http://mailman.verplant.org/pipermail/collectd/attachments/20101006/a88ce446/attachment.pgp>