[collectd] patch for email stats from postfix and amavisd-new, dns stats from powerdns

[Luke Heberling]

Sebastian Harl wrote:
> I hope, I don't sound too picky ;-)
I've seen worse.  :-)
>> + * EXAMPLE
> [...]
>
> Is there a specific reason for including this in the source code as well? Imho
> is perfectly fine to include it in the manpage only - this would save you/us
> from keeping documentation up to date in two different places (which is kind
> of error prone).
>
Only that I had it in the source from the very beginning and only
wrote the manpage as I was preparing the patch.  I'll take it out of
the source in my next patch.
> Please don't add prototypes of library functions to your source code. Simply
> include the appropriate headers. If functions are only available if certain
> defines have been set, rethink if you really want to use those functions.
> We're not very happy about any architecture / distribution / whatever specific
> extensions (especially GNU does some really strange things some times...) as
> this limits portability a lot. And yes, collectd currently has been reported
> to run on quite a few non-GNU platforms. If you really cannot live without any
> such extensions add appropriate checks to the configure script (or ask us to
> do so).
Yes you're right.  Geez that was ugly.  I'll sort that out.
>> +/* constant value inlined in module_register for 
>> + * benefit of debian maintainer script
>> + */
>> +#define PLUGIN_NAME "amafix"
>
> Just out of curiosity: what's the benefit you're talking about? ;-)
>
check_plugins.pl was confused by the lack of quotation marks around the 
plugin name argument to the plugin_register_* family of functions.  It's 
the regular expression on line 57 of debian/bin/check_plugins.pl.
>> +av_passed_clean		value:COUNTER:0:U
>> +av_passed_spammy	value:COUNTER:0:U
>> +av_passed_spam		value:COUNTER:0:U
> [...]
>
> This does not fit into collectd's data definition concept. types.db only
> contains the definition of different data types and somewhat descriptive but
> general names. A closer specification of the collected data happens through
> the use of the (plugin name, plugin instance, type instance) tuple. In your
> case you somewhat mixed the plugin name (e.g. "av") with a type instance (e.g.
> "spam") to create new data types. You should probably introduce new data types
> like "email_counter" ("email_count" is available already, but unfortunately
> it's defined as a GAUGE value :-/), "email_bytes" and the like and pass any
> other information using appropriate type instances (and possibly plugin
> instances).
>
I wondered about this. I clearly was breaking the naming convention, but 
some of the names were fairly generic and I did not want to prevent them 
from being used differently by other plugins. Perhaps a worthwhile 
feature would be the ability to override a given type declaration.
For example:
email_count          value:GAUGE:0:U          #Default for all plugins
email_count.amavis   value:COUNTER:0:U        #for amavis plugin

>
> I hope I didn't scare you off but could give you some useful hints :-)
Not at all. As if it weren't apparent, I'm rather a novice when it comes 
to C, and took on this project in part to cut my teeth, as it were. All 
useful hints are appreciated. I'll be making all of the changes you 
suggested and will have a set of patches for you by probably early next 
week.

Luke Heberling