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

Seeking guidance for more idiomatic way of (re)writing this script.

by perl514 (Pilgrim)
on Jan 14, 2013 at 19:03 UTC ( #1013260=perlquestion: print w/ replies, xml ) Need Help??
perl514 has asked for the wisdom of the Perl Monks concerning the following question:

Respected Monks

Given below is the script reads EMC Celerra NAS Array IPs from a text file and then logs into those NAS Arrays, runs a command and then gathers the output and sends out an e-mail. Please let me know if there is a better and more idiomatic way to write it. I tried running multiple commands using the "shell" option of Net::SSH2, but something isnt working. That portion is not given here as I want to try all the possibilities and error checks before I ask it here, but this is what I have as of now.

Here is the script

#!/usr/bin/perl use warnings; use strict; use Net::SSH2; use MIME::Lite; MIME::Lite->send ("smtp", "mail.server.com"); if (!open my $fh , "<" , "C:/path/to/nas_array_ip_list.txt") { my $msg = MIME::Lite->new ( From => 'name@email.com', To => 'name@email.com', Data => "Error:nas_array_ip_list.txt:$!.$^E\n", Subject => "IP List File - nas_array_ip_list.txt - Not Found + For $0 Script on $ENV{COMPUTERNAME}\n", ); $msg->send (); } else { print "Please wait. $0 script is being executed...\n"; open my $mailfh, ">", "C:/path/to/dmcheck.txt"; print $mailfh "\n############################################\n"; print $mailfh "\nUser: $ENV{USERNAME} running $0 from $ENV{COMPUT +ERNAME}\n\n"; print $mailfh "\n###########################################\n"; while (<$fh>) { next if (/^#/); my ($ipname, $ipaddr) = split /\t+|\s+/; my $username = 'username'; my $password = 'password'; my $ssh2 = Net::SSH2->new(); print $mailfh "\n----------------------------"; print $mailfh "\nData Mover Check For $ipname ($ipaddr)\n"; print $mailfh "----------------------------\n"; $ssh2->connect("$ipaddr") || die "PROBELM -$!"; $ssh2->auth_password("$username","$password") || die "Username +/Password not right"; my $chan = $ssh2->channel(); $chan->blocking(0); $chan->exec('/nas/sbin/getreason'); sleep 3; while (<$chan>) { chomp; next if (/10 - slot_0 primary control station/); if ($_ =~ /contacted$/) { print $mailfh "DM is OK: $_\n"; } else { print $mailfh "POSSIBLE DM FAILURE:Please check $ipname ($ +ipaddr): $_ POSSIBLE DM FAILURE:\n"; } }; $chan->close(); } close $mailfh; my $nasmailmsg = MIME::Lite->new( From =>'name@email.com', To => 'name@email.com', Subject => "Automated Check For NAS DataMover Health.", Type => 'Multipart/mixed', ); $nasmailmsg->attach ( Type => 'TEXT', Path => "dmcheck.txt", Filename => "dmcheck.txt", Disposition => 'inline', ); $nasmailmsg->send; system "del dmcheck.txt"; print "$0 execution completed.Please check your mailbox for Ma +il Titled - \n\"Automated Check For NAS DataMover Health.\"\n"; }

And here is the text file called "nas_array_ip_list.txt" that contains the IPs.

############################################################### # #Lines beginning with "#" will be ignored by the script. # #This file contains the NAS IPs. # #Please use tab or space to seperate the name and IP of #the NAS Array +s. # ########################################################### NASBOX1 127.0.0.1 NASBOX2 127.0.0.2 NASBOX3 127.0.0.3 NASBOX4 127.0.0.4

I intend to tweak it to perfection so that later some time, I can put this under "Cool Uses For Perl"

On another note, wanted to thank the PerlMonks and the authors of Learning Perl. This site and the book have been instrumental in getting me to a level where I could write a script like this. This script is already running in our production environment (which is why I had to fill the text file with localhost IPs as I couldnt share the actual IPs), and that feeling is really awesome. Just want to take the script to the next level.

Perlpetually Indebted To PerlMonks

Win 7 at Work. Living through it....Linux at home. Loving it.

Comment on Seeking guidance for more idiomatic way of (re)writing this script.
Select or Download Code
Re: Seeking guidance for more idiomatic way of (re)writing this script.
by keszler (Priest) on Jan 14, 2013 at 20:23 UTC
    Quick observations:
    • line 32: \s includes \t so tab not needed; split defaults to whitespace so \s not needed.
    • lines 34,35: if username/password are the same on all NAS arrays, set the variables before the loop
    • line 47: include $ipaddr in die message
    • line 61: m// defaults to checking $_ so if (/contacted$/) is sufficient
Re: Seeking guidance for more idiomatic way of (re)writing this script.
by Athanasius (Prior) on Jan 15, 2013 at 03:31 UTC

    Two suggestions:

    1. Wherever possible, avoid scattering hardcoded constants throughout the code. Instead, aim to make the script easily configurable. As a first step, move any data which could change into a hash:

      my %config ( smtp_destination => 'mail.server.com', nas_array_ip_list => 'C:/path/to/nas_array_ip_list.txt', smtp_msg_from => 'name@email.com', smtp_msg_to => 'name@email.com', mail_file => 'C:/path/to/dmcheck.txt', username => 'username', password => 'password', reason => '/nas/sbin/getreason', nasmailmsg_from => 'name@email.com', nasmailmsg_to => 'name@email.com', dm_check => 'dmcheck.txt', );

      and access the hash data as needed:

      MIME::Lite->send('smtp', $config{smtp_destination}); if (! open my $fh, '<', $config{nas_array_ip_list}) ...

      As the next step, you may find it useful to populate the hash by reading in the data from a separate configuration file.

    2. Avoid system calls where Perl has equivalent built-in functions. So, replace

      system 'del dmcheck.txt';

      with

      unlink $config{dm_check} or warn "Could not unlink file '$config{dm_ch +eck}': $!";

      See unlink.

    Hope that helps,

    Athanasius <°(((><contra mundum Iustus alius egestas vitae, eros Piratica,

Re: Seeking guidance for more idiomatic way of (re)writing this script.
by Anonymous Monk on Jan 15, 2013 at 11:03 UTC
    if (open ...) { print error } else { real code here } #EOF

    is better written as

    if (open ...) { print error exit(1); } real code here

    Besides that, you might want to try splitting the script's processing stages into well-named subroutines. It will help you reduce the amount of variables in the current scope, too. And perhaps rid of the temporary file -- I'm sure MIME::Lite can attach a "file" from a string variable.


      use autodie 'open';
      open ...; # autodie prints error message/exits on failure -- it dies
      ...
Re: Seeking guidance for more idiomatic way of (re)writing this script.
by perl514 (Pilgrim) on Jan 15, 2013 at 18:16 UTC

    Hi Keszler, Athanasius and Anonymous Monks,

    Thank you for your suggestions. I will surely try these and upload the new script.

    Athanasius - The lines  $config{smtp_destination} and $config{nas_array_ip_list} sort of flashed a light bulb in my head ( I can still hear it whizzing). I never knew this was possible.

    To the monk who suggested a better way of writing the if else loop, thank you very much.

    I think its high time PerlMonks should be renamed as Elite Perl Training Group.

    Perlpetually Indebted To PerlMonks

    Win 7 at Work. Living through it....Linux at home. Loving it.

Re: Seeking guidance for more idiomatic way of (re)writing this script.
by topher (Scribe) on Jan 16, 2013 at 05:49 UTC
    • You've got 2 places where you send e-mail. If you do something twice, it's a good candidate for splitting out into a function. I'd probably write a function called "send_mail()" or something, and put all of your e-mail work there. You can call it with an option to tell it what type of e-mail it should send.

    • As someone else mentioned, if most of your code is in an else {} block, that's a red flag that you might want to refactor. Since the initial if {} block is basically just checking to make sure you opened the file, I'd probably do something like:

      open(my $fh , "<" , $array_ip_list) or send_mail("ip_list_error"), die "IP List File - nas_array_ip_li +st.txt not found";

      Where send_mail() is the function mentioned earlier. The send_mail() function will check its argument, see "ip_list_error" and know what to do. This way, you either successfully open the file, or you send your e-mail and exit.

    • Definitely pull out any "global" settings and put them together at the front. That'll make it significantly easier to update later. As mentioned, a hash can be useful for this. You can use a single hash if you don't have too many things, or a couple of them if it makes logical sense.

    • Since you're deleting dmcheck.txt at the end and only using it as a temporary holding place for your e-mail, you'd be better off to use File::Temp. It's a standard module since 5.6.1, and gives you a safer way of using a temp file.

    • Minor style point: in my experience, most Perl programmers don't use parentheses around the conditional in a postfix if statement. For example:

      next if /foo/;

      If you're doing anything much more complicated than that, you'll want to use parens as appropriate of course.

    • Other than that, you're off to a good start already, and there's a bunch of other good suggestions here, too.

Re: Seeking guidance for more idiomatic way of (re)writing this script.
by perl514 (Pilgrim) on Jan 16, 2013 at 09:13 UTC

    Hi Topher,

    Thank you very much. I am yet to work out the subroutine part, but will do it asap. Thanks for all the suggestions. The arguments to a subroutine is something that I find a trifle confusing, but I'll work on that.

     

    Perlpetually Indebted To PerlMonks

    Win 7 at Work. Living through it....Linux at home. Loving it.

      By "arguments to a subroutine", I'm referring to passing a special value that the subroutine could match against to determine what it should do. For example, below is a quick attempt to pull out your e-mail handling and move it to a separate function, along with how I might call it.

      Here's where I would be making use of the function, which I called send_mail() just to stick with what I'd used previously:
      open( my $fh, "<", $array_ip_list ) or send_mail("ip_list_error"), die "IP List File - nas_array_ip_list.txt not found"; # [ . . . Do lots of stuff here . . . ] # If our external commands completed successfully, send our e-mail if ($result_from_nas_check_function) { send_mail( "results", $filename ); # [ Clean up after ourselves, delete our temp file, etc. ] exit 0; } else { # [ Notify e-mail that an error occured ] # [ Clean up after ourselves, delete our temp file, etc. ] die "Something went wrong."; }

      And here's an example of how I might define the send_mail() function, where I tell it what kind of e-mail it is sending, and then let it do the work:

      # Expects at least 1 argument matching the type of e-mail to send, and # optionally the additional arguments it might need. If this got much # more complicated, it might be worth passing the second argument as a # hash to simplify multiple values coming in. sub send_mail { # These could go at the beginning of your script, I just put them # here to make this a more complete example. use MIME::Lite; MIME::Lite->send( "smtp", "mail.server.com" ); my $type = shift; # what type of message should we send? my @args = @_; my $from = 'foo@example.com'; my $to = 'bar@example.com'; my $msg; if ( $type eq "ip_list_error" ) { $msg = MIME::Lite->new( From => $from, To => $to, Data => "Error:nas_array_ip_list.txt:$!.$ +^E\n", Subject => "IP List File - nas_array_ip_list +.txt - " . "Not Found For $0 Script on $ENV{COMPUTE +RNAME}\n", ); } elsif ( $type eq "results" ) { my $filename = $args[0]; $msg = MIME::Lite->new( From => $from, To => $to, Subject => "Automated Check For NAS DataMover Health.", Type => 'Multipart/mixed', ); $msg->attach( Type => 'TEXT', Path => $filename, Filename => $filename, Disposition => 'inline', ); } else { die "send_mail - bad type: $type\n" } # This will send our e-mail, regardless of which one we built. $msg->send; }

      Update: Added the final else, thanks to wfsp's excellent suggestion.

        topher++

        I'd consider having

        else { die "send_mail - bad type: $type\n" }
        at the end.
Re: Seeking guidance for more idiomatic way of (re)writing this script.
by perl514 (Pilgrim) on Jan 17, 2013 at 15:45 UTC

    Hi Topher,

     

    What a smacking pity I can upvote this only once. This is way too good. Maaan....I am dying for the weekend to dawn upon me so I can get updating this stuff

    Thank you very much.

    Perlpetually Indebted To PerlMonks

    Win 7 at Work. Living through it....Linux at home. Loving it.

Re: Seeking guidance for more idiomatic way of (re)writing this script.
by perl514 (Pilgrim) on Jan 31, 2013 at 14:11 UTC

    Hi Monks,

     

    All thanks to the suggestions put forth by topher, keszler, Athanasius and two anonymous monks that I was able to write a much better version of the original script and in the process learnt some new stuff. Thank you for all your suggestions. I have tried to incorporate them in the updated script uploaded in the Cool Uses For Perl Section. Thank you once again Monks.

    Perlpetually Indebted To PerlMonks

    use Learning::Perl; use Beginning::Perl::Ovid; print "Awesome Books";

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://1013260]
Approved by Corion
Front-paged by davido
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others rifling through the Monastery: (12)
As of 2014-04-21 16:06 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    April first is:







    Results (496 votes), past polls