Front page | perl.qpsmtpd |
Postings from June 2012
Re: [PATCH] unrecognized_commands: better log messages
Thread Previous
|
Thread Next
From:
Matt Simerson
Date:
June 4, 2012 12:30
Subject:
Re: [PATCH] unrecognized_commands: better log messages
Message ID:
C5CB0EA6-4E66-4106-ABD6-BBECAE91A032@tnpi.net
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
Thread Previous
|
Thread Next