(I should stress before I start that I don't find this code horrible, but you asked for any constructive criticism. :)
die "Usage: permmy -[itcad] [directory] -f config-file\n" unless @ARGV >= 3; use vars qw/ $opt_t $opt_c $opt_d $opt_i $opt_f $opt_a/; getopts('tcdi:f:a:') or die "Usable options are -tcifa: $!\n";
Hmm, nothing explains what the options mean; the usage message implies that you can supply just 2 arguments but the code requires 3; the second die() gives the wrong list of supported arguments.
I prefix most of my scripts with a usage() routine something like:
sub usage { my $text = <<USAGE; $0 [ -i | -c | -t ] directory [, directory ... ] Frobnicate all the files in the specified directory/ies -t test only, and warn of discrepancies -i inclusive: unknown files ignored -c conclusive: unknown files deleted (default is to defrobnicate unknown files) USAGE die "$text\n@_\n" if @_; print $text; exit 0; }
I then call usage() without parameters if I see -h or -? options, and with an error message as a parameter if for some other reason the arguments were not acceptable. As well as being useful for the user, having this usage statement at the beginning of the script can be very helpful for the maintainer - these days I tend to write it first, and use it as a guide for development of the rest of the script.
if ($opt_t || $opt_d || $opt_c) { ... } [ snip 11 subroutines covering some 180 lines ] if ($opt_d) { ... }
I find this layout confusing; I prefer instead to keep the main code at the top together and with a clear
(or something) separating the main code from the subroutine declarations - it is quite difficult, either by eye or programmatically, to find the next active line of code when it is so far away. If the main code starts to become long (eg more than a screenful) I'll move stuff into otherwise unnecessary subroutines to minimise that, so that I can end up with:exit 0; # --- subroutines
since when maintaining it 6 months later, I usually want the main code to give me a clear overview of the structure, to make it as easy as possible to drill down to the actual bit of code I'm looking for.if ($opt_d) { configtest(); } elsif ($opt_i) { configurator("i", $perms_config_file, $dir_tree, \&wanted_ia); } elsif ($opt_a) { configurator("a", $perms_config_file, $add_tree, \&wanted_ia); } elsif ($opt_t) { configtest(); fulltest(); } elsif ($opt_c) { configtest(); changeperms(); # or whatever this actually does }
Sideroad: I try to name subroutines appropriately to help me think with them, and part of that is getting the right part of speech - if a subroutine returns TRUE when the argument is valid I'll call it valid or is_valid and use it like:
or, more likely:if (valid($foo)) { ... }
whereas if it does its validity checks but doesn't return a result I'll name it imperatively:if ($obj->valid) { ... }
validate($foo); # dies if invalid
sub configurator ($$$$) { ... exit; close CONFIG or die "Could not close config file $conf_file: $!\n" +; }
That looks wrong - perhaps the exit was a temporary diagnostic? I tend to put such things in the first column to highlight their (intended) temporary nature.
Sideroad: I assume there is documentation somewhere for the format of the config file.
my $nice_mode = &return_mode($mode);
The & is not needed there, and it makes me do a double-take - usually I only expect to see the & specifed when taking a reference to a subroutine.
sub change_mode ($$) { ...(oct($change_value) != oct($c_nice_mode))... }
Hmm, one day someone'll call change_mode($file, 0777) and everything will go horribly wrong. We have the underlying mode (a number) and the common external representation (an octal string) - I would normally prefer to use the underlying value throughout in the code, and convert to the external representation only when it is needed for output. This also makes things more consistent with perl builtins such as mkdir or sysopen which take a raw number for their mode arguments.
chown($num_uid, -1, $the_file) or warn "Could not change owner + of $the_file: $!\n"; print "Changed owner of $the_file from $c_pwuid to $change_val +ue\n";
After the warning is printed, the success message is printed anyway. Two common alternatives are:
or:if (chown(...)) { print "success"; } else { warn "failure"; }
chown(...) or do { warn "failure"; return }; print ...
if ($testy eq "dir-mode") { $d_mode = $config{$tes +t}{$tester}{$testy} } elsif ($testy eq "file-mode") { $f_mode = $config{ +$test}{$tester}{$testy} } ...
A string of tests like this immediately makes me think "use a hash". In this case, I think you want to end up with a hunk of code to replace sub wanted_c, by specifying a code block explicitly for each hash value or by encoding parameters to either build up some evalled code or to bind to a closure.
I note also that if the tests specify (say) both 'dir-mode' and 'dir-owner', the latter will be silently ignored: in such cases it is helpful to test and warn. You also need to be careful about booleans - I guess a mode of 000 is unlikely, but if you were to make a natural extension to allow owners to be specified by uid, an attempt to specify root as uid 0 would fail, since the
would always be false.elsif ((-d $_) && ($d_uid))
If you keep the wanted_c() subroutine as is, note also that it is doing a lot of unnecessary work repeatedly calling stat on the same file as each elsif test fails - that is probably most easily fixed by doing a single bare stat($_) at the beginning of the routine and using the filetests on _ (underscore) thereafter.
That's probably enough to be getting on with for now. :)
Hugo
In reply to Re: Permissions Utility Script for sysadmins - seeking comments
by hv
in thread Permissions Utility Script for sysadmins - seeking comments
by BuddhaNature
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |