Beefy Boxes and Bandwidth Generously Provided by pair Networks
good chemistry is complicated,
and a little bit messy -LW
 
PerlMonks  

First Unix Admin Script - Request for Constructive Critisism

by D.Millin (Beadle)
on Feb 08, 2003 at 14:11 UTC ( [id://233702] : perlquestion . print w/replies, xml ) Need Help??

D.Millin has asked for the wisdom of the Perl Monks concerning the following question:

Hi Everyone,

The company where I work as a Unix system administrator, only uses the Korn shell, awk and sed. For technical reasons, I need to use Perl (5.005_03).

This script is to run on a server running Solaris, and will be used to scan for a point-in-time copy of a disk, import the volume manager disk group held on the disk, start the volumes and mount it on the corresponding mount points prior to a backup. The config file holds two columns, the first the volume name, the second the mount point. I am posting my code for review. I would appreciate any of you monks could give me advise/pointers on how I can improve the following code.

#!/usr/bin/perl ############################################################### # Variables ############################################################### $mountpointdata="/Backups/Config/mountpoint.config"; ############################################################### # Functions ############################################################### sub show_abort_banner{ print "\n\n\t\t\tAborting Import\n"; print "\t\t\t===============\n\n"; } ############################################################### # Main Script ############################################################### # Check if a scan required if ( $ARGV[0] =~ /-s/){ my $scan = 1; print "This script will search for specified disk groups\n"; shift; } print "\n\n\t\t Importing Disk Groups\n"; print "\t\t =====================\n\n"; while (<@ARGV>){ my $group = $_; # Check if the disk group is already imported $result=`vxdisk list | grep $group | wc -l | sed 's/ *//'`; if ($result > 0){ # Display Abort Banner show_abort_banner; print "\tThe disk group $group is already imported\n\n"; die; } if($scan){ print "\n\nActivating the $group session\n\n"; # Scan for a snapshot for the specified group system ("/usr/admsnap/admsnap activate -s $group > /dev/null 2>&1 +"); if ($?){ # Work out return code my $rc = $? >> 8; # Display Abort Banner show_abort_banner; # No new snapshots if ($rc == 11){ print "\tUnable to activate the snapshot: $group \n"; print "\tNo devices matching this session name were found\n\n +"; die; } else { print "\tFailed with untrapped error: $rc\n\n"; die; } } } print "\n\nImporting the Volume Manager disk group: $group\n"; system ("vxdctl enable"); system ("vxdg import $group > /dev/null 2>&1"); if ($?){ # Work out return code my $rc = $? >> 8; # Display Abort Banner show_abort_banner; # No disks found if ( $rc == 20 ){ print "\tUnable to import the disk group as there were\n"; print "\tNo valid disk(s) found containing disk group !!\n\n"; die; } } # Start the volumes system ("vxvol -g $group startall"); if ($?){ print "\n\n\t\t\tAborting Import\n"; print "\t\t\t===============\n\n"; print "\tCould not start the volumes in disk group $group\n\n"; die; } # Check if the mountpoint.config file is still there if ( ! -e "$mountpoint.config"){ # Display Abort Banner show_abort_banner; print "\tCould not find the mountpoint.config file\n\n"; die; } print "Mount the File Systems\n"; open MPCFG,"<$mountpointdata"; while ( defined ( my $line = <MPCFG>)){ if ( $line =~ /$group/ ){ ( $volume,$mountpoint ) = split /\s+/,$line; system ("mount -F ufs $volume $mountpoint"); if ($?){ print "\n\n\t\t\tAborting Import\n"; print "\t\t\t===============\n\n"; print "\tCould not mount $volume on mountpoint\n\n"; die; } } } }

Thanks, Darren

Replies are listed 'Best First'.
Re: First Unix Admin Script - Request for Constructive Critisism
by tachyon (Chancellor) on Feb 08, 2003 at 16:18 UTC

    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 ;my_hacking_script_that_will_now_get_executed.pl

    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 ;script.pl 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;

    cheers

    tachyon

    s&&rsenoyhcatreve&&&s&n.+t&"$'$`$\"$\&"&ee&&y&srve&&d&&print

      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,
      Darren
      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.

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

      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?


      Darren
        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.

Re: First Unix Admin Script - Request for Constructive Critisism
by tall_man (Parson) on Feb 08, 2003 at 16:37 UTC
    First, you really must "use strict" and "use warnings". There is a serious bug in your script that strict would have caught for you. The $scan in: my $scan = 1; is not the same as the $scan in: if ($scan) { The latter is actually $main::scan, since the former is out of scope, so your script will never obey "-s".

    Another problem: You don't test for success when you open MPCFG, and you open it without closing it. You could run out of open file handles, for one thing. Packages IO:File or FileHandle should be preferred for file handles; if you put them in my variables they close themselves.

    Some of the other things I noticed are a matter of style. I would combine the print messages before "die" with the die command itself. You print both ordinary trace messages and errors to STDOUT. Since this is a system utility you might want better control over logging and message levels; see Log::Log4perl.

    But please "use strict"!

      One minor nit pick:

      You don't test for success when you open MPCFG, and you open it without closing it. You could run out of open file handles

      Filehandles are global (another good reason for preferring IO::File as you suggest) so if open is called with a FILEHANDLE argument that refers to an already open file, the FILEHANDLE is closed first. The original script might not be good style, but it doesn't leak filehandles.

        Thanks for the head up on IO:File. You mention that the script isn't good style. I would appreciate any advise, in this area, as I am just leveraging perl into what I have seen in ksh.
Re: First Unix Admin Script - Request for Constructive Critisism
by zengargoyle (Deacon) on Feb 09, 2003 at 00:52 UTC
    $result=`vxdisk list | grep $group | wc -l | sed 's/ *//'`; # can be replaced by $result = grep /\Q$group/, `vxdisk list`; # to remove dependency on the systems grep/wc/sed # and the sed was not needed as ' 38' == '38' when # perl gets ahold of it.
Re: First Unix Admin Script - Request for Constructive Critisism
by graff (Chancellor) on Feb 08, 2003 at 22:57 UTC
    At first, I was a bit confused by this:
    while (<@ARGV>) { ... }
    If the intention is to read the contents of one or more files named on the command line, this is usually expressed as just:
    while (<>) { ... }
    When you do it this way, you are free to choose whether to name input files as command line args, or to pipe data to the script from some other process instead -- that is, the following two usages would be equivalent:
    your_script file1 file2 file3 cat file1 file2 file3 | your_script
    That's a dumb example, but the point is that if you happen to use some other process to generate those files, you could do:
    other_process | your_script
    It's just a nice kind of flexibility to have.
      Hi graff,

      Thanks for replying. The script is fed the names of volume manager disk groups that need to be imported from a snapshot. The script scans for the snapshot, activates it, them imports the disk group.

      Darren