Hi Harlan, On Wed, Aug 17, 2016 at 2:18 AM, Harlan Stenn <har...@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/