[collectd] [PATCH] Madwifi plugin

[Florian Forster]

Hi Ondrej,

On Mon, Jul 20, 2009 at 04:05:07PM +0200, Ondrej Zajicek wrote:
> I tried to send patch against latest development release but i cannot
> regenerate autoconf/automake files in that version to test the patch
> (perhaps it requires newer autoconf version than i have in current
> Debian).

I'm building collectd on Debian Etch, Debian Lenny and Debian Testing
without problems. If you use Debian Testing, be sure to install a
version of libltdl that is compatible with your version of libtool. The
maintainer of libtool screwed that one up if care for my opinion by
making the correct version of libltdl a ``recommendation'' only instead
of a dependency.

> As i also need the madwifi plugin in stable version of collectd, i
> would prefer to send next versions of Madwifi plugin patch against
> version 4.6.3, until it would be merge-redy. (If that approach would
> not cause many problems to you.)

No, that's fine too.

> > > +#include "madwifi.h"
> 
> These declarations are not part of standard system header files, they
> are part of Madwifi header files (and it is uncommon to have that
> header files installed even if someone uses Madwifi driver). So i
> think that using original header files would be user's and packager's
> nightmare.

Well, then we'll have to cross our fingers they never change the
declaration of those structures, or we're screwed..

> > Please use the correct format string for uint8_t here.
> 
> I think that former version is also correct (suppose that sizeof int
> is at least 8) - according to C99 standard, integer promotion is
> applied to that arguments. But if you want, i can change the format
> string.

You're right, it should be possible to promote uint8_t to unsigned int
on all platforms. (`sizeof' returns at least 1, though ;) I'm a bit
cautious with format strings because they have shown to be a big problem
when it comes to portability. For example, using "%i" to print a size_t
will work well on Linux but will produce a warning on Mac OS X, Solaris
and probably every other UNIX ;)

Anyway, that's coding style and I don't care that much as long as it's
portable and readable.

> > In short, the `type' should always describe *what* is being counted/
> > collected/... and the `type_instance' should differenciate between
> > multiple values of the same nature (== type).
> 
> I thought about this, but there are several problems with that approach:
> 
> - You suggest generic types like "if_errors", but these might be
>   already defined incompatible (for example requires both rx and
>   tx values which is inappropriate for many ath_ values), so
>   i would use types with "ath_" prefix.

Of course, if there is no compatible type you need to add an appropriate
one, no question about that. It's just that your code adds one type for
each value it submits (and it submits quite a few ;) – this clearly
isn't the way the schema was designed.

There are two more types, already in use by the `netlink' plugin. They
are named "if_rx_errors" and "if_tx_errors". Maybe they are just what
you are looking for in this particular case.

> - The grouping of values seems to me as more or less arbitrary (for
>   example, if i use "ath_frames" type for sent frames, should i
>   alsouse "ath_frames" for frames not sent because of some error? If
>   not, and if i use "ath_errors" type for that, should i also use
>   "ath_errors" for errors that are not frame related (like 'periodic
>   calibration failed')?).

RFC 1925 clarifies: “It is more complicated than you think.”

Our naming schema has the same fundamental problem: The only way to
handle all and every possibility is to allow arbitrary text of infinite
length. At the same time, this is the least practical solution. The
easiest solution (from a programmers point of view) would be to
implement n categories and leave it at that – obviously not ideal
either. So the only possibility is to opt for a good compromise and
somehow fit the reality into it with some method or other.

So the only advice a can give is “do as you see fit”. Maybe a good
method would be to begin grouping values as you see them logically
related and stop when the abstraction starts to be not useful anymore.
“Relates to the same NIC” clearly is too abstract, for example ;) Maybe
assuming the users point of view is the best option: “What do I need to
see to recognize problems? Is it useful to see layer-1 problems separate
from layer-2 problem?”

> - On ath_sta_* types, type_instance is used for station MAC address.
>   in that case, i can use for example type = "ath_frames";
>   type_instance = "tx-ucast-11:22:33:44:55:66"; (or
>   "11:22:33:44:55:66-tx-ucast" ?)

Yes, that's what we usually do: The dash (`-') is not a valid character
in the plugin or type name, because it's used to separate plugin/type
from the instance. We therefore usually use it to separate “fields”
within the instance, for example in the `dbi' plugin when multiple
instance columns are defined.

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/20090721/e8a2a785/attachment.pgp