The bad news: I did introduce a bug. The good news: My tests caught it. Travis complained.
Pull Request: https://github.com/smtpd/qpsmtpd/pull/27 Details: This code does not increment $count correctly: + my $count = $self->connection->notes('unrec_cmd_count') || 0 + 1; + $self->connection->notes('unrec_cmd_count', $count ); The operator precedence is subtle. It bit me, and it's likely to bite others. So I changed it to this: + my $count = $self->connection->notes('unrec_cmd_count') || 0; + $self->connection->notes('unrec_cmd_count', ++$count ); Which does increment correctly. This also works (note the parens), which is how it was done before. + my $count = ($self->connection->notes('unrec_cmd_count') || 0) + 1; + $self->connection->notes('unrec_cmd_count', $count ); Which one is easier to read and maintain? Both seem equal to the task. Matt On Jun 3, 2012, at 11:46 PM, Matt Simerson wrote: > > On Jun 3, 2012, at 11:41 PM, Ask Bjørn Hansen wrote: > >> >> On Jun 3, 2012, at 19:00, Matt Simerson wrote: >> >>> removed hook_connect, unnecessary. >> >> I think it's there so if you log or otherwise use the note, it'll be >> reasonably initialized. > > It's not used anywhere else in qpsmtpd or plugins, except in > hook_unrecognized_command, where it already had code to initialize it, if it > wasn't already. > >>> - if ($badcmdcount >= $self->{_unrec_cmd_max}) { >>> - my $msg = "Closing connection, $badcmdcount unrecognized commands."; >>> - $self->log(LOGINFO, "fail: $msg"); >>> - return (DENY_DISCONNECT, "$msg Perhaps you should read RFC 2821?"); >>> - } >>> + if ( $count < $self->{_unrec_cmd_max} ) { >>> + $self->log(LOGINFO, "'$cmd', ($count)"); >>> + return DECLINED; >>> + }; >> >> Why is the new version better? (Other than just being different). > > One less hook to call (efficiency) > > Better error message (also logs count). > > Matt
