Re: [PATCH] don't print GeoIP country if not defined

On Apr 28, 2012, at 10:48 AM, Robert Spier wrote:

> On Mon, Apr 23, 2012 at 8:39 AM, Matt Simerson <[email protected]> wrote:
> 
>> 
>> On Apr 22, 2012, at 11:45 PM, Robert Spier <[email protected]> wrote:
>> 
>>> This change contains a bunch of unnecessary whitespace changes.  This
>> can make it hard to trace back to original changes.  I'm not saying there's
>> not a need for a whitespace cleanup, but I wonder if it would be better to
>> do it all at once?
>> 
>> I agree entirely.
>> 
>> If you will recall, I tried the 'do it all at once' approach a year ago. I
>> would love to see the entire code base run through perltidy and committed.
> 
> Maybe the time has come to "do a release" (0.9?) and then open things up
> for major cleanups/changes.
> 
> -R

I'm all in favor.

I'm doing more work on plugins right now and have a bunch more changes in the pipes. Most of the changes revolve around several practices which I think all plugins should adhere to:

  store operational results in notes

  use a 'reject' option: instead of rejecting messages that fail a test, offer an option for what to do.
	(ex: require_resolvable_fromhost becomes resolvable_fromhost with a reject  option
     this allows information to be collected from a plugin when it would otherwise be unusable

  log a reasonable amount, especially for LOGINFO.  I know, I know, reasonable is hard to pin down, but a little bit of guidance could go a long ways here:

	DEBUG -> fire hose, spray away logging anything and everything.
 	INFO      -> normal operational info:  goal is a one line summary per plugin
	NOTICE -> unusual results that are not errors

I also think logging anything more severe than a WARN event should have the option to automatically email a sysadmin. 

I'm working on a "Qpsmtpd Big Picture" document that contains a flowchart of a sample connection, a catalog of all the plugins, what each plugin does/sets/changes, and what information is made available to subsequent plugins via notes. It took me quite a while to wrap my head around this, and that learning curve presents a significant barrier to entry for potential qpsmtpd users. 

Speaking of opening things up to major changes and cleanups, I have to say I'm not terribly fond of the check_ prefix on so many of the plugin names. I think I get the idea behind it, but many of the plugins do more than check. Many also reject messages that fail their checks, by default.  Why not just drop the check_ prefix entirely, and have the plugin name be shorter?

Same goes for plugins like require_resolvable_fromhost.  I think its better to make the plugin resolvable_fromhost, and then offer config options that govern whether the plugin merely checks, or rejects based on the outcome of the test.

Matt


Matt