Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl Monk, Perl Meditation

Re: First Unix Admin Script - Request for Constructive Critisism

by tachyon (Chancellor)
on Feb 08, 2003 at 16:18 UTC ( [id://233729] : note . print w/replies, xml ) Need Help??

in reply to First Unix Admin Script - Request for Constructive Critisism

Well here is a list that springs to mind:

You are not using warnings or strict, see use strict, warnings and diagnostics or die You don't need them for trivial scripts but once you add a few more lines they will save you from all manner of trivial errors that will otherwise make life miserable.

Although you are not using strict you have a single my statement which forms a bug (it would be fine if you did had been consistent!)

my $scan = 'foo'; # here $scan == 'foo'; if ( $something ) { my $scan = 'bar'; # here $scan == 'bar'; } # now the $scan we defined in the if() {} is out of scope, # gone, defunct, forgotten, inaccessible so $scan == 'foo'

In your code by localising $scan within the if and then later trying to access it you have ensures that the -s option can never be run as $scan will always be undefined. strict and warnings would have helped you avoid this bug.

You use somelongvarname and other_long_varname which irks me because it is inconsistent. Either use one or the other or evenDoTheCamelHumptyDumpty but do it all the one way and you will not have to go ??? did I use _ in that var_name or not???

When you want to print a bunch of literal stuff it is easier on the eye and fingers to do stuff like:

print " ======================================= Here is my really groovy script I hope you like it Call it with --help to get the synopsis ======================================= ";

Note we are just using literal tabs and newlines.

Speaking of a synopsis it is useful to include one so that if the script is called with no args (if it needs them) or a ?, -h, --help or unknown argument you spew it out.

With simple scripts you can parse @ARGV yourself. For complex command line syntax Getopt::Std and Getopt::Long are the standards (I don't like them much myself but that is another story....)

Without strict you can call a function like some_func but this is called a 'bare word' as perl has to make the assumption that this represents a function call. Typically you will call functions like some_func() if it takes no args or some_func(@ARGS) if it does take args. You can also call subs like &some_func or &some_func() which have some very subtle differences that won't worry you so I will just mention that they exist.

You are checking the return codes from your system calls which is good. You don't need to print($string) and then die. You can simply die $string which will print the string to STDERR and then die in the one line:

print "Oops"; die; # might as well be die "Oops";

When you open a file you should check that the call worked like so:

open FILE, $somefile or die "Could not read $somefile, perl says the e +rror is $!\n"; # note if you want to use || rather than or you need to be aware # that it binds tighter than the , operator so you need parnes thusly open (FILE, ">$somefile") || die "Could not write $somefile, $!\n";

Almost lastly you can just do:

while ( my $line=<FILE> ) { blah }

The defined() is implicit in the while loop.

Second lastly you can/should use the multi arg version of system rather than interpolating into a string as this will escape shell meta chars for you automagically and prevents hacking by editing a file so that a line contains something like:

volume ;

If I inserted this line into mountpointdata then your system call would exec my script (probably as root ;-) This is because the string interpolation occurs before the system call. If you use multiarg system this won't work as ; is not a valid arg.

Lastly you have a lot of recurrent code after your system calls so why not write a sub so you can do:

system(@stuff) check_call($custom_message,@stuff); sub check_call { my $custom_message = shift; my @call = @_; # check for 0 return status return unless $?; # bugger, we have an error so tell all # Work out return code my $rc = $? >> 8; die qq! System call failed Error code $rc $custom_message Actual call was system( "@call" )!; }

As your scripts get more complex you will find that the Carp module comes in handy as it give you a stack trace which can ofen be a lot more useful than just dying in a sub as it tells you how the sub got called. I would suggest using it for the example above as it will then add details of where in the script the error handler was called.

use Carp; one(); sub one { two() } sub two { three() } sub three{ confess "It took a while to get here, but I know just how I + did!" } __DATA__ It took a while to get here, but I know just how I did! at script line + 7 main::three() called at script line 6 main::two() called at script line 5 main::one() called at script line 3

If you are going to be doing a lot of system type scripts I would just put this sub in a module say called Error::Custom (see Simple Module Tutorial then How to Make a CPAN Module Distribution and finally A Guide to Intalling Modules so you can just go:

#!/usr/bin/perl -w use strict; use Error::Custom; synopsis() unless @ARGV; system(@args); check_call('custom message',@args); # this lives in Error::Custom system(@args1); check_call('more custom messages',@args1); exit; sub synopsis { # note that $0 contains the script name so the name will # always be the correct one no matter what you call the script print " Synopsis $0 SOME_FLAG SOME_ARG blah blah\n\n"; exit; }

The module would look like:

package Error::Custom; use strict; use Exporter; use Carp; use vars qw($VERSION @ISA @EXPORT @EXPORT_OK ); $VERSION = 1.00; @ISA = qw(Exporter); @EXPORT = qw(check_call); @EXPORT_OK = qw(check_call); sub check_call { my $custom_message = shift; my @call = @_; # check for 0 return status return unless $?; # bugger, we have an error so tell all # Work out return code my $rc = $? >> 8; confess qq! System call failed Error code $rc $custom_message Actual call was system( "@call" ) !; } 1;




Replies are listed 'Best First'.
Re: Re: First Unix Admin Script - Request for Constructive Critisism
by D.Millin (Beadle) on Feb 08, 2003 at 21:40 UTC
    Hi tachyon,

    Thank you for taking the time to answer so fully. I have rejigged the script to use strict, and -w. After a bit of cleaning up, the script is working again, but I am a bit confused at how to approach the error handling.

    The script will be a part of a set of scripts to take snap shots of LUN's(Logical Units of Storage) on an SAN. Your check_call approach is exactly what I need for the standard unix commands, but EMC does not appear to use non-standard return codes in their software i.e. With admsnap(for taking snapshots), 13 is returned when a snapshot is activated, which complicates the issue.

    The other complicating factor is that on some machines, the scripts will be run by operators from a menu system. This means that if they encounter any problems, we have to provide them with a plain english explanation of the problem, before returning them to the menu.

    Any advise on this, or style would be deeply appreciated,
Re: Re: First Unix Admin Script - Request for Constructive Critisism
by logan (Curate) on Feb 08, 2003 at 18:57 UTC
    OT, I suppose, but...

    Wow, Tachyon. I wish I could vote twice on your response. I have rarely seen a response to an RFC that was so informative or complete. I've bookmarked it for future reference.

    "What do I want? I'm an American. I want more."

Re: Re: First Unix Admin Script - Request for Constructive Critisism
by D.Millin (Beadle) on Feb 08, 2003 at 23:37 UTC
    Hi tachyon,

    I have been given some thought to your suggestion to have a check_call sub. I guess one approach would be to adapt check_call each type, or class of command. Reporting would still need to be done in plain english for the operators, so instead of passing @stuff, I could always pass in appropriate parameters. BTW, is there any way of redirecting STDOUT,STDERR when system is called in a list context?

      You ask about redirecting STDOUT and STDERR for system calls given as lists. A good way to do this is with the module IPC::Run.
      use IPC::Run qw( run ) ; run \@cmd, \$in, \$out, \$err;

      One idea you might be able to use for parameterizing your error messages for various commands would be to pass in a hash reference containing explanatory messages indexed by return codes. Also see perlvar for how you can use the variable $! to obtain error message strings after a failed system call.

      Finally, the reason spelled-out file handles are considered "bad style" compared to IO::File is that they create global names, which might conflict with other modules. IO::File uses unique private symbols instead.