dovecot - Aug 2016 - [patch] Improved error checking for the dovecot-antispam-plugin

Hi,

Hopefully this is the right channel for such a patch. I have a minor
enhancement to submit for the antispam plugin

  http://hg.dovecot.org/dovecot-antispam-plugin

It adds minimal error checking for the sendmail_binary, otherwise the
reported error in case of a missing binary or one with missing
permissions is generic and not useful.

Thanks,

Robert

-- 
http://robert.muntea.nu/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: validate_sendmail.patch
Type: text/x-patch
Size: 1088 bytes
Desc: not available
URL:
<http://dovecot.org/pipermail/dovecot/attachments/20160816/66891231/attachment.bin>

On 8/16/16 1:24 PM, Robert Munteanu wrote:
> Hi,
> 
> Hopefully this is the right channel for such a patch. I have a minor
> enhancement to submit for the antispam plugin
> 
>   http://hg.dovecot.org/dovecot-antispam-plugin
> 
> It adds minimal error checking for the sendmail_binary, otherwise the
> reported error in case of a missing binary or one with missing
> permissions is generic and not useful.
> 
> Thanks,
> 
> Robert
Robert, I like that you did this.

Beyond that and without even looking at the actual code, I'm curious why
you:

+    if (access(cfg->binary, F_OK) == -1)
+    {
+    mail_storage_set_error(storage, MAIL_ERROR_TEMP, "mail_sendmail
file does not exist");

instead of finding a way to include the value of cfg->binary in the
error message string.

This might not be needed if it's really obvious from the config file
what the path to the executable is, but if there is any doubt it might
be friendlier to show the exact path with the problem.  I'd also be
inclined to show the decoded value of errno instead of assuming that
'mail_sendmail file does not exist'.

Perhaps something along the lines of:

	"access(%s, F_OK) failed: %m", cfg->binary

if that makes sense.

H

On 17.08.2016 02:18, Harlan Stenn wrote:
> On 8/16/16 1:24 PM, Robert Munteanu wrote:
>> Hi,
>>
>> Hopefully this is the right channel for such a patch. I have a minor
>> enhancement to submit for the antispam plugin
>>
>>   http://hg.dovecot.org/dovecot-antispam-plugin
>>
>> It adds minimal error checking for the sendmail_binary, otherwise the
>> reported error in case of a missing binary or one with missing
>> permissions is generic and not useful.
>>
>> Thanks,
>>
>> Robert
> Robert, I like that you did this.
>
> Beyond that and without even looking at the actual code, I'm curious
why
> you:
>
> +    if (access(cfg->binary, F_OK) == -1)
> +    {
> +    mail_storage_set_error(storage, MAIL_ERROR_TEMP, "mail_sendmail
> file does not exist");
>
> instead of finding a way to include the value of cfg->binary in the
> error message string.
>
> This might not be needed if it's really obvious from the config file
> what the path to the executable is, but if there is any doubt it might
> be friendlier to show the exact path with the problem.  I'd also be
> inclined to show the decoded value of errno instead of assuming that
> 'mail_sendmail file does not exist'.
>
> Perhaps something along the lines of:
>
> 	"access(%s, F_OK) failed: %m", cfg->binary
>
> if that makes sense.
>
> H
Hi!

Thank you for your patch, we'll take it under consideration!

Aki

Hi Harlan,

On Wed, Aug 17, 2016 at 2:18 AM, Harlan Stenn <harlan at pfcs.com>
wrote:
> On 8/16/16 1:24 PM, Robert Munteanu wrote:
>> Hi,
>>
>> Hopefully this is the right channel for such a patch. I have a minor
>> enhancement to submit for the antispam plugin
>>
>>   http://hg.dovecot.org/dovecot-antispam-plugin
>>
>> It adds minimal error checking for the sendmail_binary, otherwise the
>> reported error in case of a missing binary or one with missing
>> permissions is generic and not useful.
>>
>> Thanks,
>>
>> Robert
>
> Robert, I like that you did this.
>
> Beyond that and without even looking at the actual code, I'm curious
why
> you:
>
> +    if (access(cfg->binary, F_OK) == -1)
> +    {
> +    mail_storage_set_error(storage, MAIL_ERROR_TEMP, "mail_sendmail
> file does not exist");
>
> instead of finding a way to include the value of cfg->binary in the
> error message string.
>
> This might not be needed if it's really obvious from the config file
> what the path to the executable is, but if there is any doubt it might
> be friendlier to show the exact path with the problem.  I'd also be
> inclined to show the decoded value of errno instead of assuming that
> 'mail_sendmail file does not exist'.
>
> Perhaps something along the lines of:
>
>         "access(%s, F_OK) failed: %m", cfg->binary
>
> if that makes sense.
Thanks for the review . I was not sure that it's OK to show the path
to the script in an error message which will be shown to the user. But
I have no issue in resending a new version of the patch with better
error reporting, will do so in the following days.

Robert



-- 
http://robert.muntea.nu/