[collectd] Madwifi plugin v2

[Ondrej Zajicek]

On Wed, Aug 12, 2009 at 02:44:56PM +0200, Florian Forster wrote:
> Hi Ondrej,
> 
> On Sun, Aug 09, 2009 at 05:21:04PM +0200, Ondrej Zajicek wrote:
> > After some time i managed to make a new version of Madwifi plugin. The
> > main change is that it is possible to finely tune the set of monitored
> > statistics and just the most important statistics are monitored by
> > default. Also the number of new data types is reduced (by using type
> > instances).
> 
> thank you very much for your patch :) I've applied it to the master
> branch of the Git repository and fixed some best practices while
> reviewing the code.
> 
> The biggest change I did was to rename the ???DisableSysfs??? option to
> ???Source???. The reason for this is that ???DisableSysfs??? is a negated
> option, which is confusing to use. Just think about:
> 
>   DisableSysfs false

I originally wanted to make it zero parameter option, just
'DisableSysfs'. But it seems that one parameter for each option
is forced by core code, so i added boolean argument.

This change is OK to me, but i have objections to some other changes:

@@ -765,9 +771,11 @@ check_devname (const char *dev)
        i = readlink (buf, buf2, sizeof (buf2) - 1);
        if (i < 0)
                return 0;

-       buf2[i] = 0;
+       buf2[sizeof (buf2) - 1] = 0;


This is not correct, because readlink does not terminate returned
string by zero. Therefore, this change adds some trash after the end of 
the string.


+               ssnprintf (ti2, sizeof (ti2), "antenna%i", i);
+               submit_counter (dev, "ath_stat", name, ti2,
+                               (counter_t) vals[i]);

In this case, type instance is created from 'name' and 'ti2'.
As 'name' already contains 'ant' for antenna, it is IMHO unnecessary
duplication. Old type instance name is like 'ast_ant_rx-1', new type
instance name is like 'ast_ant_rx-antenna1'.

-       if (ioctl (sk, IEEE80211_IOCTL_STA_INFO, &iwr) < 0)
+
+       status = ioctl (sk, IEEE80211_IOCTL_STA_INFO, &iwr);
+       if (status < 0)
+       {
+               ERROR ("madwifi plugin: Sending IO-control "
+                               "IEEE80211_IOCTL_STA_INFO to device %s "
+

Error paths for ioctls in madwifi plugin are intentionally silent,
because there are two classes of devices generated by madwifi driver
(low level device usually called wifiX and high level device usually 
called athX) and some ioctl calls are successful on devices from the
first class and other ioctl calls are successful on devices from the
second class. The enumeration of network devices does not differentiate
between them.


Other changes are OK to me.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20090814/c1f7b805/attachment.pgp