Robert Spier wrote:
Please try and wrap your lines at 80 characters.
done... at work we actually have an svn hook to catch long lines and
I've come to rely on it... but it looks for lines > 99 chars long :)
+ $self->log(LOGWARN,"Permissions on spool_dir $Spool_dir are not 2750")
I thought the point was to make the spool perms configurable.
Whoops, that was an artifact from our original forked code, it should
have been "Permissions on $Spool_dir are not $Spool_perms". Fixed.
+ unless sprintf('%o',(stat($Spool_dir))[2] & 07777) eq $Spool_perms;
Why are you doing a string comparison here? Much cleaner to just
do it numeric.
Well, all the numbers are octal, so my original thinking was that if we
don't have an sprintf here we need it in the warning. However, looking
at this I realized that since the previous patch does string comparison,
you would have to use ex. '700' rather than '0700' in the config file
for the thing to work at all. At best you could use sprintf('%04o') in
two places to support both but that would be even messier. So I've gone
ahead and made the switch, and used oct() twice in order to avoid having
to sprintf in the warning, which is longer and messier.
I think the code flow was cleaner in general before.
I personally disagree: the previous flow first checks just whether
spool_dir exists, which isn't the best check since it really needs to be
a directory; then it checks whether it' a directory and tries to mkdir
if it isn't. It's trivial, but why do one wrong check and one better
check on startup rather than a single check? It also seems unecessary
here to use 'foo or bar or die' to represent the more clear 'if (foo) {
bar or die }'. At any rate style isn't really that important to us vs.
having code that we don't have to fork to do what we need :) therefore:
spooldir_2.diff: shorter lines, avoid string mode comparison, fix
hard-coded mode in warning
spooldir_3.diff: same as above, but also avoid any changes to code flow
I also thought about re-introducing $mode against spooldir_2.diff for
the sake of readability, but I figured 2 diffs was enough...
-Jared
Index: lib/Qpsmtpd.pm
===================================================================
--- lib/Qpsmtpd.pm (revision 961)
+++ lib/Qpsmtpd.pm (working copy)
@@ -529,18 +529,16 @@
$Spool_dir =~ /^(.+)$/ or die "spool_dir not configured properly";
$Spool_dir = $1; # cleanse the taint
+ my $Spool_perms = $self->config('spool_perms') || '0700';
- # Make sure the spool dir has appropriate rights
- if (-e $Spool_dir) {
- my $mode = (stat($Spool_dir))[2];
- $self->log(LOGWARN,
- "Permissions on spool_dir $Spool_dir are not 0700")
- if $mode & 07077;
+ if (-d $Spool_dir) { # Make sure the spool dir has appropriate rights
+ $self->log(LOGWARN,
+ "Permissions on spool_dir $Spool_dir are not $Spool_perms")
+ unless ((stat $Spool_dir)[2] & 07777) == oct $Spool_perms;
+ } else { # Or create it if it doesn't already exist
+ mkdir($Spool_dir,oct $Spool_perms)
+ or die "Could not create spool_dir $Spool_dir: $!";
}
-
- # And finally, create it if it doesn't already exist
- -d $Spool_dir or mkdir($Spool_dir, 0700)
- or die "Could not create spool_dir $Spool_dir: $!";
}
return $Spool_dir;
Index: lib/Qpsmtpd.pm
===================================================================
--- lib/Qpsmtpd.pm (revision 961)
+++ lib/Qpsmtpd.pm (working copy)
@@ -529,17 +529,18 @@
$Spool_dir =~ /^(.+)$/ or die "spool_dir not configured properly";
$Spool_dir = $1; # cleanse the taint
+ my $Spool_perms = $self->config('spool_perms') || '0700';
# Make sure the spool dir has appropriate rights
if (-e $Spool_dir) {
- my $mode = (stat($Spool_dir))[2];
- $self->log(LOGWARN,
- "Permissions on spool_dir $Spool_dir are not 0700")
- if $mode & 07077;
+ my $mode = (stat($Spool_dir))[2] & 07777;
+ $self->log(LOGWARN,
+ "Permissions on spool_dir $Spool_dir are not $Spool_perms")
+ unless $mode == oct $Spool_perms;
}
# And finally, create it if it doesn't already exist
- -d $Spool_dir or mkdir($Spool_dir, 0700)
+ -d $Spool_dir or mkdir($Spool_dir,oct $Spool_perms)
or die "Could not create spool_dir $Spool_dir: $!";
}
