[collectd] [PATCH] Madwifi plugin

[Ondrej Zajicek]

On Sun, Jul 19, 2009 at 06:26:20PM +0200, Florian Forster wrote:
> > diff -uprN collectd-4.6.3/configure.in collectd-4.6.3-mod/configure.in
> 
> I was able to rebase the patch on the master branch without any
> problems. If you want I can push that commit for you to use as a base
> for further changes.

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

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

> > +#include "madwifi.h"
> 
> You're shipping that header file with structure declarations. Is it
> possible to use already existing header files to get those declarations?
> They are later used for `ioctl', so I'd rather use the systems
> declarations to avoid problems should those structures ever be changed..

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.

> > +static inline void
> > +macaddr_to_str(char *buf, int bufsize, const uint8_t mac[IEEE80211_ADDR_LEN])
> > +{
> > +	snprintf (buf, bufsize, "%02x:%02x:%02x:%02x:%02x:%02x",
> > +		mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> > +}
> 
> Please use the correct format string for uint8_t here. It's
>   snprintf (buf, bufsize,
>     "%02"PRIx8":%02"PRIx8":%02"PRIx8":%02"PRIx8":%02"PRIx8":%02"PRIx8,
>     mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);

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.

> Also, `bufsize' should be a size_t.

OK

> > --- collectd-4.6.3/src/types.db	2009-06-02 11:17:47.000000000 +0200
> > +++ collectd-4.6.3-mod/src/types.db	2009-07-17 21:57:50.000000000 +0200
> 
> You are adding *a lot* of new types. This is not the usual way in which
> data is structured in collectd, see [0].
> 
> Please use
>   type = "if_frames"; type_instance = "tx-noack";
> instead of
>   type = "ath_hw_tx_noack"; type_instance = NULL;
> or
>   type = "if_errors"; type_instance = "rx-badcrypt";
> instead of
>   type = "ath_hw_rx_badcrypt"; type_instance = NULL;
> and so on.
> 
> 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.

- 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')?).

- 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" ?)

-- 
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/20090720/5e04bee4/attachment.pgp