How would you go about it?

pcassell has asked for the wisdom of the Perl Monks concerning the following question:

So I have been playing around with perl the last week and trying to get a handle on it. Im hardly able to write anything decent without having perlmonks or perldoc open though, but I am learning!

Wanted to know what/how I could improve the following code or in what ways you would have done it differently. I really like seeing more than one approach, and I need help pointing out my pitfalls.

Its very simple code, just traverses a specified directory and changes the permissions for files and/or directories. It is my perl way of doing the following with GNU Find.

find [base directory] -type [f|d] -exec chmod [perm] {} \;
[download]

Here it is, input greatly appreciated. Im still a newbie though.

#!/usr/bin/perl -w
use strict;
use Getopt::Long;
use File::Find;

my %opts = (
    verbose => '', 
    directory => './', 
    filemode => '', 
    dirmode => ''
);

if(!GetOptions('verbose' => \$opts{'verbose'},
                    'filemode:s' => \$opts{'filemode'},
                    'dirmode:s' => \$opts{'dirmode'})) {
    usagedie();
}
#make sure each argument passed is either a file or directory
foreach (@ARGV) {
    usagedie() if (!-f || !-d);
}
#we need to perform at least one option
usagedie() if (!$opts{'filemode'} && !$opts{'dirmode'});

#use default directory if argument not set on command line
find(\&fileop, $opts{'directory'}) unless @ARGV;
find(\&fileop, @ARGV) if @ARGV;

sub fileop {
    #set the right permissions based on if a file or a directory
    #only set permissions of the mode is set
    chmod oct($opts{'filemode'}), $_ if -f && $opts{'filemode'};
    chmod oct($opts{'dirmode'}), $_ if -d && $opts{'dirmode'};
    print $File::Find::name . "\n" if $opts{'verbose'} && (-f || -d);
}

sub usagedie {
    print "Usage: setperm.pl [-v] [-f perm] [-d perm] [Directory]\n";
    print "-v, --verbose\t show files changed by script\n";
    print "-f, --filemode\t octal mode to change files to\n";
    print "-d, --dirmode\t octal mode to change directories to\n";
    exit(0);
}
[download]

Updated Code:

#!/usr/bin/perl -w
use strict;
use Getopt::Long;
use File::Find;

my %opts = (
    verbose => '', 
    filemode => '', 
    dirmode => ''
);

if (!GetOptions(\%opts, 'verbose', 'filemode:s', 'dirmode:s')) {
    usagedie();
}
#make sure each argument passed is either a file or directory
foreach (@ARGV) {
    usagedie() if (!-f && !-d);
}
#we need to perform at least one option
usagedie() if (!$opts{'filemode'} && !$opts{'dirmode'});
#make sure correct octal values were used
$opts{'filemode'} = chkperm('File', $opts{'filemode'});
$opts{'dirmode'} = chkperm('Directory', $opts{'dirmode'});

#use default directory if argument not set on command line
find(\&fileop, @ARGV ? @ARGV : './');

sub fileop {
    #set the right permissions based on if a file or a directory
    #only set permissions if the mode is set
    if (-f && $opts{'filemode'}) {
        chmod $opts{'filemode'}, $_;
    }
    elsif (-d && $opts{'dirmode'}) {
        chmod $opts{'dirmode'}, $_;
    }
    else {
        #dont print if we didnt modify file perms
        return;
    }
    print $File::Find::name . "\n" if $opts{'verbose'};
}
sub chkperm {
    my ($type, $perm) = @_;
    #zero length strings are defined values
    #our $perm 's by default are set to ''
    if ($perm) {
        return ($perm =~ /^[0-7]{4}$/) 
            ? oct($perm) 
            : die "Incorrect $type mode $perm\n";
    }
    return;
}
sub usagedie {
    print "Usage: setperm.pl [-v] [-f perm] [-d perm] [Directory]\n";
    print "-v, --verbose\t show files changed by script\n";
    print "-f, --filemode\t octal mode to change files to\n";
    print "-d, --dirmode\t octal mode to change directories to\n";
    exit(1);
}
[download]

Thanks to all of ya'll who helped out. I tested it a few times and it worked perfect, I will test more, but I wanted to show what the combined efforts did to the script before it went off the front page.

I'll be adding an exclude option soon too. This will allow you to exclude directories or files. Thanks!

Comment on How would you go about it? Select or Download Code

Replies are listed 'Best First'.
Re: How would you go about it? by Zaxo (Archbishop) on Jul 26, 2004 at 00:36 UTC
That looks pretty good as it is. The File::Find usage looks fine. I think your @ARGV handling is unnecessary. You have already arranged for command line options to set the default modes. Those options are removed from @ARGV when Getopt::Long parses them. You could arrange for @ARGV to be a list of directories to apply to if you wished. Kudos for including a usage function. You may be interested in Pod::Usage. It automatically extracts usage functions from the pod - which I recommend you add to your script. You can then read the pod with `perldoc myscript` and you can easily extract a man.1 page from your literate perl on installation. Good work, I wouldn't guess you'd been using perl such a short time. After Compline, Zaxo	[reply]
Re^2: How would you go about it? by pcassell (Sexton) on Jul 26, 2004 at 18:27 UTC
My @ARGV handling is in case you want to include multiple directories/files on the command line, and I didnt want my script to end nastily if you mistyped a directory or file. Thanks for the info on Pod::Usage, I will be checking that out!	[reply]
Re: How would you go about it? by Aristotle (Chancellor) on Jul 26, 2004 at 02:18 UTC
Check out The Dynamic Duo --or-- Holy Getopt::Long, Pod::UsageMan! and GetOpt::Long usage style. Put together they're how I do all my script writing. Here are some minor points I'd do differently. (I'll be using my variables instead of your `$opts` hash, because that affords me the safety of strict. See aforementioned link.) `foreach (@ARGV) { usagedie() if (!-f \|\| !-d); }` [download] I'd prefer to write this as `usagedie() if grep not( -f or -d ), @ARGV;` [download] The following is needlessly repetitive: `find(\&fileop, $opts{'directory'}) unless @ARGV; find(\&fileop, @ARGV) if @ARGV;` [download] It could be written simpler like so: `find( \&fileop, @ARGV ? @ARGV : $opt_directory );` [download] In this part you're checking `-f` and `-d` three or four times per entry, and you're repeating yourself in the conditionals. You also print the directory or file names, even if the corresponding mode option was not set, and they were therefor not touched — probably a minor bug. `sub fileop { #set the right permissions based on if a file or a directory #only set permissions of the mode is set chmod oct($opts{'filemode'}), $_ if -f && $opts{'filemode'}; chmod oct($opts{'dirmode'}), $_ if -d && $opts{'dirmode'}; print $File::Find::name . "\n" if $opts{'verbose'} && (-f \|\| -d); }` [download] How about the following? It's longer, but less redundant — there is only one location at which to update or add any single condition. `sub fileop { if( -f and $opt_filemode ) { chmod oct( $opt_filemode ), $_; } elsif( -d and $opt_dirmode ) { chmod oct( $opt_dirmode ), $_; } else { return; # avoid falling thru to print() } print $File::Find::name, "\n" if $opt_verbose; }` [download] Finally, instead of pulling the values through oct every time through the function, I'd do that once at the beginning of the script, as part of input validation that should check whether they're valid modes. Something like `sub check_perm { my ( $mode, $perm ) = @_; if( defined $perm ) { return ( $perm =~ /^[0-7]{3}$/ ) ? oct( $perm ) : die "Malformed $mode mode $perm\n"; } return; } $opt_filemode = check_perm file => $opt_filemode; $opt_dirmode = check_perm directory => $opt_dirmode;` [download] Makeshifts last the longest.	[reply] [d/l] [select]
Re^2: How would you go about it? by pcassell (Sexton) on Jul 26, 2004 at 18:41 UTC
This is great stuff, much appreciated. I am glad you talked about the repeated conditionals, since they seem like they would be obnoxious to debug and toubleshoot in a larger script. Thanks.	[reply]
Re^3: How would you go about it? by Aristotle (Chancellor) on Jul 26, 2004 at 18:52 UTC
NP. :) Repetition in general invites all manner of problems (see my other reply further down the thread). Avoid it wherever you can do so with reasonable effort. Makeshifts last the longest.	[reply]
Re: How would you go about it? by Errto (Vicar) on Jul 26, 2004 at 01:58 UTC
If I'm reading it right I think you have a small bug: `usagedie() if (!-f \|\| !-d);` should probably be `usagedie() if (!-f && !-d);` Otherwise that line will always call usagedie() if passed an argument. Quite well-written, though. My only nits are that you should validate that the numbers you're passed are actually valid file modes (otherwise they just become 000, which is not what you want), and that you probably don't need to set `$opts{directory}` since that's not a command-line option.	[reply] [d/l] [select]
Re^2: How would you go about it? by pcassell (Sexton) on Jul 26, 2004 at 18:35 UTC
Yes you are completely right, I got mixed up because originally I was using unless statements instead of negated if statements, but I'm used to using `if (!exp)` so I changed it and it got lost in the conversion. Thanks for pointing that out. I will look into validating octals thanks ya'll.	[reply] [d/l]
Re: How would you go about it? by graff (Chancellor) on Jul 26, 2004 at 02:20 UTC
Getopt::Long lets you provide a reference to a hash as its first parameter, so that instead of this: `GetOptions('verbose' => \$opts{'verbose'}, 'filemode:s' => \$opts{'filemode'}, 'dirmode:s' => \$opts{'dirmode'})` [download] You can do this: `GetOptions( \%opts, 'verbose', 'filemode:s', 'dirmode:s' );` [download] I suppose different people may have different preferences, but the short form seems very appealing to me. Setting modes on directories vs. data files is sometimes a pain in unix, especially if you're doing something like "enable group write access" on a directory tree and its files, and the files are all data (not executables): if you're using numeric mode flags, you have to do two passes, so that directories will be "executable" (i.e. searchable) while data files will not. So, it ~~would be nice if~~ is really nice that the script could just take care of this distinction and do both types in one pass -- or but it might also be nice to allow the use of symbolic mode flags, like "g+w", so that this isn't an issue. (Updated after I fully understood what the OP's code was doing -- nice work.)	[reply] [d/l] [select]
Re^2: How would you go about it? by Aristotle (Chancellor) on Jul 26, 2004 at 02:42 UTC
Setting modes on directories vs. data files is sometimes a pain in unix, especially if you're doing something like "enable group write access" on a directory tree and its files, and the files are all data Not particularly if you RTFM, provided you don't want to set completely different modes for files vs directories. `chmod -R a=rX,ug+w foo/` [download] Note the uppercase `X` mode which means "execute bit but only if it's a directory". Makeshifts last the longest.	[reply] [d/l]
Re: How would you go about it? by ysth (Canon) on Jul 26, 2004 at 01:10 UTC
oct is a poor tool to use on user input; while it will nicely allow entry of octal (default), hex, or binary numbers, it will also silently return 0 on garbage. You might do the conversion up front and die if the value is 0 and the input string wasn't `/^(0[bx])?0+\z/`. Update: That's not good enough; it will complain if the string starts 0x or 0b and there are illegal hex or binary digits, but not complain if there are valid octal digits followed by garbage :(.	[reply] [d/l]
Re: How would you go about it? by saberworks (Curate) on Jul 26, 2004 at 05:13 UTC
If the command line tool works perfectly, why not just use your perl script to accept arguments and run the command-line tool through exec() or backticks or something?	[reply]
Re^2: How would you go about it? by pcassell (Sexton) on Jul 26, 2004 at 18:44 UTC
I am mainly doing this as a learning excersize. And, I'd rather run my setperm.pl script than remember that long find command (not too hard I know, but I hate remembering stuff). The next step is to create some samba scripts to set permissions on a domain share, create new samba users, etc so I figured I'd try to get the basics down first.	[reply]
Re: How would you go about it? by pbeckingham (Parson) on Jul 26, 2004 at 16:14 UTC
Nice code. I saw something in your code that I was doing, and lately converted. These lines: `#use default directory if argument not set on command line find(\&fileop, $opts{'directory'}) unless @ARGV; find(\&fileop, @ARGV) if @ARGV;` [download] Your use of `if` and `unless` are creating a more compact form of `if (...){} else {}` for calling `&find`. I love statement modifiers, but I now use the following form: `#use default directory if argument not set on command line find(\&fileop, @ARGV ? @ARGV : $opts{'directory'});` [download] That way, with a true `if/else` variant, I never screw up the logic and inadvertently create overlapping conditions, which were difficult to debug. Yes, Aristotle said the same thing, but the key, I think, is not the needless redundancy of the code, but the danger of creating those overlapping conditions.	[reply] [d/l] [select]
Re^2: How would you go about it? by Aristotle (Chancellor) on Jul 26, 2004 at 17:57 UTC
the key, I think, is not the needless redundancy of the code, but the danger of creating those overlapping conditions. You're contradicting yourself. `:-)` Try: "the key is the needless redundancy of the code because of the danger of creating those overlapping conditions". It's the Don't Repeat Yourself principle. The potential for overlapping conditions is just one manifestation of the same basic danger: if you make multiple copies, you risk having them run out of synch. That's why you should always strive to do things Once And Only Once. Makeshifts last the longest.	[reply]
Re: How would you go about it? by eyepopslikeamosquito (Archbishop) on Jul 26, 2004 at 22:46 UTC
Just a couple of minor points on your `usagedie` function. First, `exit(1)` is more correct, since the command has failed due to incorrect user input. Second, using a here document is more enjoyable than one print statement after another, in my experience. For example: `sub usagedie { print <<'END_USAGE'; Usage: setperm.pl [-v] [-f perm] [-d perm] [Directory] -v, --verbose\t show files changed by script -f, --filemode\t octal mode to change files to -d, --dirmode\t octal mode to change directories to END_USAGE exit(1); }` [download] Perl's here documents are like shell's, but much more powerful; they are a general quoting mechanism. You can even do things like this: `SomeFunction(<<'PARAM1', 42, <<"PARAM3"); this is the text for param1 PARAM1 and this is the text for param3 with $i variable interpolation this time PARAM3` [download] See `perlop` for more information on Perl here documents.	[reply] [d/l] [select]

Back to Seekers of Perl Wisdom