[collectd] Madwifi plugin v2

[Florian Forster]

Hi Ondrej,

thanks for your feedback :)

On Fri, Aug 14, 2009 at 10:09:50PM +0200, Ondrej Zajicek wrote:
> >   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.

That is intentional, to keep the syntax consistent.

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

Oops, you're right. I've changed this to a ‘memset’ before the call to
‘readlink’. Also, I've changed the array dimensions to ‘PATH_MAX’.

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

Good point, I didn't see that from looking at the code. I've changed the
format string to simply "%i". Imho this is much easier to read than
  buf[0] = '0' + i; buf[1] = 0;

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

Okay, I've documented this in the code and unified the error handling in
the entire plugin. All ‘ioctl’s will now print a debugging message if
they fail, so users usually do not see those messages.

Thanks again for your feedback. By the way, I've created a wiki page for
the plugin [0]. It'd be great if you could proofread it.

Regards,
-octo

[0] <http://collectd.org/wiki/index.php/Plugin:MadWifi>
-- 
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/20090816/d1506885/attachment.pgp