Re: [PATCH] connection_time: make compatible with tcpserver deployment

2012-06-04 Thread Ask Bjørn Hansen 
Applied.

Re: [PATCH] connection_time: make compatible with tcpserver deployment

2012-06-04 Thread Charlie Brady 



On Sun, 3 Jun 2012, Matt Simerson wrote:

 ---
  plugins/connection_time |   18 ++
  1 file changed, 14 insertions(+), 4 deletions(-)
 
 diff --git a/plugins/connection_time b/plugins/connection_time
 index bfac4d2..9cff7f9 100644
 --- a/plugins/connection_time
 +++ b/plugins/connection_time
 @@ -26,9 +26,10 @@ Adjust the quantity of logging for this plugin. See 
 docs/logging.pod
  use strict;
  use warnings;
  
 -use Time::HiRes qw(gettimeofday tv_interval);
  use Qpsmtpd::Constants;
  
 +use Time::HiRes qw(gettimeofday tv_interval);
 +

Is the change in ordering here accidental, gratuitous, or is there some 
hidden functional significance?

  sub register {
  my ($self, $qp) = shift, shift;
  if ( @_ == 1 ) {  # backwards compatible
 @@ -43,18 +44,27 @@ sub register {
  }
  else {
  $self-{_args} = { @_ }; # named args, inherits loglevel
 -}
 +};
  }
  
  sub hook_pre_connection {
 -my ($self, @foo) = @_;
 +my $self = shift;
 +$self-{_connection_start} = [gettimeofday];
 +$self-log(LOGDEBUG, started at  . $self-{_connection_start} );
 +return (DECLINED);
 +}
 +
 +sub hook_connect {
 +my $self = shift;
 +# this method is needed to function with the tcpserver deployment model
 +return (DECLINED) if defined $self-{_connection_start};
  $self-{_connection_start} = [gettimeofday];
  $self-log(LOGDEBUG, started at  . $self-{_connection_start} );
  return (DECLINED);
  }
  
  sub hook_post_connection {
 -my ($self, @foo) = @_;
 +my $self = shift;
  
  if ( ! $self-{_connection_start} ) {
  $self-log(LOGERROR, Start time not set?!);

Re: [PATCH] connection_time: make compatible with tcpserver deployment

2012-06-04 Thread Matt Simerson 

On Jun 4, 2012, at 9:32 AM, Charlie Brady wrote:

 On Sun, 3 Jun 2012, Matt Simerson wrote:
 
 ---
 plugins/connection_time |   18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)
 
 diff --git a/plugins/connection_time b/plugins/connection_time
 index bfac4d2..9cff7f9 100644
 --- a/plugins/connection_time
 +++ b/plugins/connection_time
 @@ -26,9 +26,10 @@ Adjust the quantity of logging for this plugin. See 
 docs/logging.pod
 use strict;
 use warnings;
 
 -use Time::HiRes qw(gettimeofday tv_interval);
 use Qpsmtpd::Constants;
 
 +use Time::HiRes qw(gettimeofday tv_interval);
 +
 
 Is the change in ordering here accidental, gratuitous, or is there some 
 hidden functional significance?

No functional significance. Except where functionality matters, I have been 
ordering them in all plugins as:

pragma declarations

local dependencies

external dependencies

and in alphabetical order, within the dependency groups.


 sub register {
 my ($self, $qp) = shift, shift;
 if ( @_ == 1 ) {  # backwards compatible
 @@ -43,18 +44,27 @@ sub register {
 }
 else {
 $self-{_args} = { @_ }; # named args, inherits loglevel
 -}
 +};
 }
 
 sub hook_pre_connection {
 -my ($self, @foo) = @_;
 +my $self = shift;
 +$self-{_connection_start} = [gettimeofday];
 +$self-log(LOGDEBUG, started at  . $self-{_connection_start} );
 +return (DECLINED);
 +}
 +
 +sub hook_connect {
 +my $self = shift;
 +# this method is needed to function with the tcpserver deployment model
 +return (DECLINED) if defined $self-{_connection_start};
 $self-{_connection_start} = [gettimeofday];
 $self-log(LOGDEBUG, started at  . $self-{_connection_start} );
 return (DECLINED);
 }
 
 sub hook_post_connection {
 -my ($self, @foo) = @_;
 +my $self = shift;
 
 if ( ! $self-{_connection_start} ) {
 $self-log(LOGERROR, Start time not set?!);

Re: [PATCH] connection_time: make compatible with tcpserver deployment

2012-06-04 Thread Ask Bjørn Hansen 

On Jun 4, 2012, at 9:32, Charlie Brady wrote:

 -use Time::HiRes qw(gettimeofday tv_interval);
 use Qpsmtpd::Constants;
 
 +use Time::HiRes qw(gettimeofday tv_interval);
 +
 
 Is the change in ordering here accidental, gratuitous, or is there some 
 hidden functional significance?

I think Matt has been trying to make loading other Qpsmtpd modules go first and 
then external dependencies second.  Almost all the time that's fine (no 
functional change) and it is neater.

(It's however also what makes reviewing these cleanup patches so much work, 
each change has to be considered -- was this just reformatting?  Does it change 
functionality?   Was it intentional?  What are the side-effects? -- in other 
words, help reviewing would be most welcome!  :-)  )


Ask

Re: [PATCH] connection_time: make compatible with tcpserver deployment

2012-06-04 Thread Matt Simerson 

On Jun 4, 2012, at 10:16 AM, Ask Bjørn Hansen wrote:

 On Jun 4, 2012, at 9:32, Charlie Brady wrote:
 
 -use Time::HiRes qw(gettimeofday tv_interval);
 use Qpsmtpd::Constants;
 
 +use Time::HiRes qw(gettimeofday tv_interval);
 
 Is the change in ordering here accidental, gratuitous, or is there some 
 hidden functional significance?
 
 I think Matt has been trying to make loading other Qpsmtpd modules go first 
 and then external dependencies second.  Almost all the time that's fine (no 
 functional change) and it is neater.

Precisely.

 (It's however also what makes reviewing these cleanup patches so much work, 
 each change has to be considered -- was this just reformatting?  Was it 
 intentional?  

Almost all of my non-functional changes are ones you'll find listed in PBP, and 
they resolve around making code easier to read and maintain. Quite often I get 
into a bit of code and find it's no small task to figure out what's happening.  
Especially long chains of conditionals. They carry a high cognitive load. Most 
of the changes I make are covered by these few principles:

Code in commented paragraphs.
Explicit returns.
Linear coding (Reject as many iterations as possible, as early as possible.)
Avoid negative control statements

I try to make these changes first, without changing any functionality.  Then I 
write the tests.  Finally, I go in and start changing how things work, which is 
mostly extending the functionality so that it also works the way I want.  But 
in such a way that it's easy for someone else to change it to work the way they 
want.

For example, the changes in github have scores of changes that revolve around 
how a plugin rejects a message. Scores of them.  Because there's no standard 
way to change how a plugin rejects messages without altering the plugin.  With 
a small additional to Plugins.pm 
(https://github.com/smtpd/qpsmtpd/pull/23/files), all those changes go away, 
any qpsmtpd user (or developer) change how all their plugins handle mail, and 
every plugin supports these options: 

plugin reject [ 0 | 1 | naughty ]

plugin reject_type [ perm | temp | disconnect | temp_disconnect ]

That covers 99% of the changes that have been made to plugins to alter how they 
reject mail. Answers to FAQ's like how do I use spam to train my spam plugins 
can be written.  Stuff that requires a developer now can be done by a user.

 Does it change functionality?
 What are the side-effects?

That's what test coverage answers. :-)

 -- in other words, help reviewing would be most welcome!  :-)  )

Another solution is just to review changes less.  If it comes in with tests, 
and it doesn't stomp all over the guidelines laid out in docs/*.pod, then just 
let them pass.

I realize there's only one branch of qp, but git is a VCS.  If a commit breaks 
things, it's easy enough to back out. Ideas that don't work out are easy enough 
to prune later.  Making room for other people to change and use your software 
in ways you didn't expect is, after all, the UNIX way. 

Matt

[PATCH] connection_time: make compatible with tcpserver deployment

2012-06-03 Thread Matt Simerson 
---
 plugins/connection_time |   18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/plugins/connection_time b/plugins/connection_time
index bfac4d2..9cff7f9 100644
--- a/plugins/connection_time
+++ b/plugins/connection_time
@@ -26,9 +26,10 @@ Adjust the quantity of logging for this plugin. See 
docs/logging.pod
 use strict;
 use warnings;
 
-use Time::HiRes qw(gettimeofday tv_interval);
 use Qpsmtpd::Constants;
 
+use Time::HiRes qw(gettimeofday tv_interval);
+
 sub register {
 my ($self, $qp) = shift, shift;
 if ( @_ == 1 ) {  # backwards compatible
@@ -43,18 +44,27 @@ sub register {
 }
 else {
 $self-{_args} = { @_ }; # named args, inherits loglevel
-}
+};
 }
 
 sub hook_pre_connection {
-my ($self, @foo) = @_;
+my $self = shift;
+$self-{_connection_start} = [gettimeofday];
+$self-log(LOGDEBUG, started at  . $self-{_connection_start} );
+return (DECLINED);
+}
+
+sub hook_connect {
+my $self = shift;
+# this method is needed to function with the tcpserver deployment model
+return (DECLINED) if defined $self-{_connection_start};
 $self-{_connection_start} = [gettimeofday];
 $self-log(LOGDEBUG, started at  . $self-{_connection_start} );
 return (DECLINED);
 }
 
 sub hook_post_connection {
-my ($self, @foo) = @_;
+my $self = shift;
 
 if ( ! $self-{_connection_start} ) {
 $self-log(LOGERROR, Start time not set?!);
-- 
1.7.9.6