SA Bugzilla – Bug 5507
[review] Unhelpful diagnostics when site rules directory is inaccessible
Last modified: 2008-09-25 08:51:00 UTC
I'm attaching a small patch to improve diagnostics when site rules directory was present during install, but was inaccessible to a process running SA. It took us some running around in circles (on a mailing list) when a user reported SA failing after a fresh install and reporting config: could not find site rules directory even though the directory /usr/local/etc/mail/spamassassin was present and accessible to anyone. It turned out that one directory above, the /usr/local/etc/mail was too strongly protected, and a sloppy -e() test in Perl does not distinguish between 'does not exist' and 'is inaccessible'. The attached patch turns a: config: could not find site rules directory into a: config: path "/usr/local/etc/mail/spamassassin" is inaccessible: Permission denied config: could not find site rules directory Please incorporate the patch to facilitate future troubleshooting.
Created attachment 3983 [details] the promised patch
committed to trunk: $ svn -m '(bug5507) Unhelpful diagnostics when site rules directory is inaccessible' ci Sending lib/Mail/SpamAssassin.pm Transmitting file data . Committed revision 565376.
+1 for application to 3.2.x, if you want
> +1 for application to 3.2.x, if you want Perhaps, although there are dozens of other cases which test for -f, -d, -e and similar, without calling stat() or lstat() first, and therefore can not distinguish between (for example) not being a directory or not existing or an access violation, and not being able to be specific whether a test should follow a symbolic link or not. In the least this causes a misleading diagnostics, or at worst can cause a wrong decision. If you think this can be a good idea, I'd like to undertake another global sweep across modules and replace simple tests like -f $file with a: stat($file); ... -f _, along the lines of the patch above or the following code section: + use Errno qw(ENOENT EACCES); - if (... -d $path) { ... } + my($errn) = stat($path) ? 0 : 0+$!; + if ($errn == ENOENT) { ... } # does not exist + elsif ($errn) { info("config: \"$path\" is inaccessible: $!") } + elsif (-d _) { ... } # is a directory ... The amount of change would make this suitable for 3.3.0, and probably not worth fixing a single case for 3.2.* .
(In reply to comment #3) > +1 for application to 3.2.x, if you want Ok, let's do it, needs one more vote. We'll leave saving the rest of the world to 3.3.0.
+1
Thanks. spamassassin-3.2$ svn -m 'Unhelpful diagnostics when site rules directory is inaccessible, Bug 5507' ci Sending lib/Mail/SpamAssassin.pm Transmitting file data . Committed revision 585950.
Should a new bug be opened for comment #4 ?
> Should a new bug be opened for comment #4 ? Perhaps, if you (pl.) like. I won't forget about my intent anyway - have notes scribbled in many places on the printed listing of SA code, which I reviewed during my summer vacation.
> I won't forget about my intent anyway - have notes scribbled in > many places on the printed listing of SA code, which I reviewed > during my summer vacation. Topic continues on Bug 5981