search

prehor / amavisd-milter

Milter interface for the amavis spam filter engine.
BSD 3-Clause "New" or "Revised" License
8 stars 4 forks source link

Use {client_name} if available instead of hostname passed to xxfi_connect #3

Closed Bluewind closed 5 years ago

Bluewind commented 5 years ago

Spamassassin's RDNS_NONE check tests if the rdns field in the Received header is empty or "unknown". Postfix sets the client_name macro to "unknown" when the FcrDNS check fails, but the hostname passed to xxfi_connect is set to the IP address which means that RDNS_NONE will never trigger. Fix this by using client_name if available and only fall back to the hostname parameter otherwise.

Signed-off-by: Florian Pritz bluewind@xinu.at

prehor commented 5 years ago

Committed slightly modified version 6e55366 of this PR.

Bluewind commented 5 years ago

Thanks! A few things I noticed:

You commited the .CHANGES.swp file which you probably didn't intend to do. I'd suggest to remove it and add *.swp to .gitignore and possibly also to ~/.gitignore.

Also I'd argue that the INFO level log statement should have stayed for easier debugging if the macro is not defined. There's another INFO level log entry that contains the value if it is defined so clearly, INFO should only be enabled when people want to figure out what is happening. In that case they are most likely interested in knowing that they should/could define the client_name macro.

Apart from that, I also ask that, in the future, you put a short modification notice after the 'Signed-off-by" line so that it is clear that the commit has been changed and that it was not just merged as-is. Ideally that message even explains what was changed (e.g. "Add changelog, remove logging, fix indentation") and it is signed by you with another "Signed-off-by" (git commit -s) to show that you made those changes. It would not only have been helpful to me for finding the modifications just now, but it might also prevent some confusion as to who authored what, if someone else looks at the patch in the future. It's not too big of a deal, especially with this simple patch, but I figured I might as well say it when I'm already writing a comment.

Again, thanks for merging the patch and continuing to maintain this project!

prehor commented 5 years ago

ad 1) Thanks, the file is removed now.

ad 2) I'm sorry, but client_name is required by postfix only. Sendmail users will have their logs full of misleading messages. I have dived into the problem and the result is here #4.

ad 3) Thanks for suggestions. amavisd-milter is my first real GitHub project with contributors. I have to learn how to handle pull requests in the right way. Please be patient.