Bug 5507 - [review] Unhelpful diagnostics when site rules directory is inaccessible
Summary: [review] Unhelpful diagnostics when site rules directory is inaccessible
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: 3.2.0
Hardware: All All
: P3 normal
Target Milestone: 3.2.4
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard: can be commited
Keywords:
Depends on:
Blocks:
 
Reported: 2007-06-10 09:15 UTC by Mark Martinec
Modified: 2008-09-25 08:51 UTC (History)
0 users


Attachment Type Modified Status Actions Submitter/CLA Status
the promised patch patch None Mark Martinec [HasCLA]
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Martinec 2007-06-10 09:15:11 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.
Comment 1 Mark Martinec 2007-06-10 09:16:13 UTC 
Created attachment 3983 [details]
the promised patch
Comment 2 Mark Martinec 2007-08-13 08:21:32 UTC 
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.
Comment 3 Justin Mason 2007-08-13 08:42:46 UTC 
+1 for application to 3.2.x, if you want
Comment 4 Mark Martinec 2007-08-22 06:48:51 UTC 
> +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.* .
Comment 5 Mark Martinec 2007-10-18 01:32:59 UTC 
(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.
Comment 6 Doc Schneider 2007-10-18 05:46:04 UTC 
+1
Comment 7 Mark Martinec 2007-10-18 05:54:09 UTC 
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.
Comment 8 Tom Schulz 2007-10-18 08:23:06 UTC 
Should a new bug be opened for comment #4 ?
Comment 9 Mark Martinec 2007-10-18 08:54:43 UTC 
> 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.
Comment 10 Mark Martinec 2008-09-25 08:51:00 UTC 
> 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