how can this be improved?

[blueflashlight]

hi, monks ...

In the never-ending quest to become a less lame writer of perl programs (hoping to someday be able to proudly have a JAPH sig), I would appreciate any and all comments on this script, which I am posting below.

This program works fine (but is only useful on AIX, which incidently, is NOT being canned in favor of Linux, contrary to postings on various sites, including those who use only non-alphanumeric characters in their names (but I digress))

However, I'm sure that some of ways that I'm doing things are awkward, inefficient, or just plain dumb. I hope that y'all would like to help me be less awkward, inefficient, and dumb.

thanks, and try to be gentle (well, not *too* gentle, but I don't think I have to worry about that. :-)

-blueflashlight

#!/usr/local/bin/perl -w

#=====================================================================
# cinnamon -- a perl script that translates the sense data from SIM
#             and MIM messages posted by IBM 3590 tape drives into
#             human-readable format, and sends the messages via email
#---------------------------------------------------------------------
# $Id: cinnamon,v 1.1 2001/08/21 04:48:51 ssklar Exp $
#=====================================================================

use strict;

$|++;

unless ($^O =~ /aix/) {
    die "Cinnamon is only useful on AIX systems.  Sorry.\n"
};

#---------------------------------------------------------------------
+-
# the email address(es) that the formatted error message is to be sent
# to.  Don't forget to backslash any "@" signs ...
#---------------------------------------------------------------------
+-

my $recipient = "root";

unless (defined($recipient)) { $recipient = "root" }

#---------------------------------------------------------------------
+-
# the sequence number of the error log entry that we were invoked for
# will be passed as the single argument ...
#---------------------------------------------------------------------
+-

my $sequence_number = shift ||
    die "cinnamon: sequence number needed as argument\n";

#---------------------------------------------------------------------
+-
# read in the full unformatted error log entry with the specified
# sequence number ...
#---------------------------------------------------------------------
+-

open (ERROR, "/usr/bin/errpt -g -l $sequence_number |");

#------------------------------------------------------------------
# pull out the detail data from the error log entry ...
#------------------------------------------------------------------

my %message;

while (<ERROR>) {

    if (/^el_nodeid/) {
    
        (undef, $message{host}) = split (/\s+/, $_); next    
    };
    
    if (/^el_resource/) {
    
        (undef, $message{drive}) = split (/\s+/, $_); next
    };    
    
    
    if (/^el_detail_data/)    {

        (undef, $message{detail}) = split (/\s+/, $_); last
    };
    
};

close (ERROR);

#------------------------------------------------------------------
# get the "Machine Type" and convert it from hex to ascii ...
#------------------------------------------------------------------

substr($message{detail}, 128, 10) =~ /(\w\w)(\w\w)(\w\w)(\w\w)(\w\w)/;

$message{machine_type} = pack ("CCCCC", hex($1), hex($2), hex($3), hex
+($4), hex($5));

#------------------------------------------------------------------
# get the "Model" and convert it from hex to ascii ...
#------------------------------------------------------------------
            
substr($message{detail}, 138, 6) =~ /(\w\w)(\w\w)(\w\w)/;

$message{model} = pack ("CCC", hex($1), hex($2), hex($3));

#------------------------------------------------------------------
# get the "Model and Microcode Level" and convert it from hex
# to ascii ...
#------------------------------------------------------------------   
+ 
    
substr($message{detail}, 32, 8) =~ /(\w\w)(\w\w)(\w\w)(\w\w)/;

$message{mml} = pack ("CCCC", hex($1), hex($2), hex($3), hex($4));

#------------------------------------------------------------------
# get the "Message Code" and look up it's meaning ...
#------------------------------------------------------------------

my %message_code = (

    3030    =>    "No Message",
    3430    =>    "Operator Intervention Required",
    3431    =>    "Device Degraded",
    3432    =>    "Device Hardware Failure",
    3433    =>    "Service Circuits Failed, Operations not Affected",
    3535    =>    "Clean Device",
    3537    =>    "Device has been cleaned",
    3630    =>    "Bad Media, Read-Only Permitted",
    3631    =>    "Rewrite Data if Possible",
    3632    =>    "Read Data if Possible",
    3634    =>    "Bad Media, Cannot Read or Write",
    3732    =>    "Replace Cleaner Cartridge"

);

$message{code} = $message_code{substr($message{detail}, 40, 4)} || "UN
+KNOWN";

#------------------------------------------------------------------
# determine if we're dealing with a SIM or a MIM ...
#------------------------------------------------------------------

if (substr($message{detail}, 16, 2) eq "01") {

    #--------------------------------------------------------------
    # it's a SIM ...
    #--------------------------------------------------------------
    
    $message{type} = "SIM";
        
    #--------------------------------------------------------------
    # convert the FID Severity Code into something meaningful ...
    #--------------------------------------------------------------
            
    my %fid_severity_code = (
        33        =>    "1 -- Acute",
        32        =>    "2 -- Serious",
        31        =>    "3 -- Moderate",
        30        =>    "4 -- Service"
    );
    
    $message{severity} = $fid_severity_code{substr($message{detail}, 5
+2, 2)} || "UNKNOWN";
    
    #--------------------------------------------------------------
    # get the FID (FRU Identification Number), and convert it from
    # hex to ascii ...
    #--------------------------------------------------------------
                            
    substr($message{detail}, 64, 4) =~ /(\w\w)(\w\w)(\w\w)/;
    
    $message{fid} = pack ("CC", hex($1), hex($2));
    
    #--------------------------------------------------------------
    # get the "First FSC" (Fault Symptom Code), and convert it from
    # hex to ascii ...
    #--------------------------------------------------------------
    
    substr($message{detail}, 68, 8) =~ /(\w\w)(\w\w)(\w\w)(\w\w)/;
    
    $message{first_fsc} = pack ("CCCC", hex($1), hex($2), hex($3), hex
+($4));
    
    #--------------------------------------------------------------
    # get the "Last FSC" (Fault Symptom Code), and convert it from
    # hex to ascii ...
    #--------------------------------------------------------------
    
    substr($message{detail}, 76, 8) =~ /(\w\w)(\w\w)(\w\w)(\w\w)/;
    
    $message{last_fsc} = pack ("CCCC", hex($1), hex($2), hex($3), hex(
+$4));
            

} else {

    #--------------------------------------------------------------
    # it's a MIM ...
    #--------------------------------------------------------------
    
    $message{type} = "MIM";
    
    #--------------------------------------------------------------
    # convert the MIM Severity Code into something meaningful ...
    #--------------------------------------------------------------
            
    my %mim_severity_code = (
        31        =>    "3 -- Moderate: high temporary read or write e
+rrors have occurred",
        32        =>    "2 -- Serious:  permanent read or write errors
+ have occurred",
        33        =>    "1 -- Acute: tape directory errors have occurr
+ed"
    );
    
    $message{severity} = $mim_severity_code{substr($message{detail}, 5
+2, 2)} || "UNKNOWN";
    
    #--------------------------------------------------------------
    # get the VOLSER (Volume Serial Number), and convert it from
    # hex to ascii ...
    #--------------------------------------------------------------
    
    
    substr($message{detail}, 68, 12) =~ /(\w\w)(\w\w)(\w\w)(\w\w)(\w\w
+)(\w\w)/;
    
    $message{volser} = pack ("CCCCCC", hex($1), hex($2), hex($3), hex(
+$4), hex($5), hex($6));
    

};

#------------------------------------------------------------------
# format the data and store it in the array @mail ...
#------------------------------------------------------------------

my @mail;

push (@mail, sprintf("Subject:  %s posted by %s: %s\n", $message{type}
+, $message{drive}, $message{code}));

push (@mail, sprintf("%-16s: %-20s\n", "Sequence Number", $sequence_nu
+mber));
push (@mail, sprintf("%-16s: %-20s\n", "Host",            $message{hos
+t}));
push (@mail, sprintf("%-16s: %-20s\n", "Drive",           $message{dri
+ve}));
push (@mail, sprintf("%-16s: %-20s\n", "Model",           $message{mod
+el}));
push (@mail, sprintf("%-16s: %-20s\n", "Microcode",       $message{mml
+}));
push (@mail, sprintf("%-16s: %-20s\n", "Message Type",    $message{typ
+e}));
push (@mail, sprintf("%-16s: %-20s\n", "Message Code",    $message{cod
+e}));
push (@mail, sprintf("%-16s: %-20s\n", "Severity",        $message{sev
+erity}));

if ($message{type} eq "SIM") {

    push (@mail, sprintf("%-16s: %-20s\n", "First FSC",   $message{fir
+st_fsc}));
    push (@mail, sprintf("%-16s: %-20s\n", "Last FSC",    $message{las
+t_fsc}));
    
} else {

    push (@mail, sprintf("%-16s: %-20s\n", "VOLSER",       $message{vo
+lser}));
    
};

push (@mail, "\n\nRaw Sense Data:\n$message{detail}\n" . "-" x 72 . "\
+n\n");

#------------------------------------------------------------------
# open a pipe to sendmail and sent the message ...
#------------------------------------------------------------------

open (SENDMAIL, "|/usr/sbin/sendmail $recipient") or
    die "cinnamon: couldn't open sendmail: $!";
    
print SENDMAIL @mail;

close (SENDMAIL);

exit 0;

#---------------------------------------------------------------------
+-
# documentation
#---------------------------------------------------------------------
+-


=pod

=head1 NAME

cinnamon -- translates the sense data from SIM and MIM messages 
posted by IBM 3590 tape drives to the AIX error log into human-
readable format, and sends the messages via email.


=head1 DESCRIPTION


B<cinnamon> (so named because I thought it sounded like "sim-mim-
mon", which was my original name for the program) parses and mails 
AIX error log entries posted with the identifier B<D1A1AE6F>, which is
+ 
the ERROR ID for B<SIM_MIM_RECORD_3590>.


SIM and MIM records are part of the "Statistical Analysis and Reportin
+g 
System" (SARS), and are messages created by IBM 3590 tape drives that
 report on the condition of the drive (a SIM) or of the medium (a MIM)
+.  
These records are presented by the operating system in different ways.
  In AIX, they are recorded in the error log, but in a difficult-to-re
+ad format.


This script breaks down the sense data where the information is stored
+, 
parses it into a human-understandable format, and mails it to root.


=head1 SETUP/USAGE

If the user wants emails sent by this program to be sent to any other 
address than B<root>, the variable B<$recipient> should be changed 
(about line number 26) to that address (or those addresses, comma-
separated but all contained within double-quotes, with any "@" signs 
back-slashed.


Additionally, this program is meant to be used as B<errnotify method>,
 added to the ODM, so that B<cinnamon> will be invoked each time an 
error log entry that matches the descriptor values of a 3590 SIM or MI
+M 
message.


To create the B<errnotify method>, save the following text to the file
+ B</tmp/cinnamon.add>:


    errnotify:
        en_name = "cinnamon"
        en_persistenceflg = 1
        en_label = "SIM_MIM_RECORD_3590"
        en_class = "H"
        en_type = "INFO"
        en_method = "/usr/local/bin/perl /usr/local/sbin/cinnamon $1"
        
(Note: use the proper paths to your perl executible and to this progra
+m
 in the above "en_method" line.)


After saving the above text to a file, run the command:

    odmadd /tmp/cinnamon.add
    
The error notification object will be added to the ODM.  To verify tha
+t 
the object was added to the ODM properly, run the command:


    odmget -q"en_name='cinnamon'" errnotify
    
To remove the object from the ODM (why would you want to do that?), 
run the command:

    odmdelete -q"en_name='cinnamon'" -o errnotify
    
    
=head1 AUTHOR

    Sandor W. Sklar
    Unix Systems Administrator
    Stanford University ITSS
    
    mailto:[e-mail]
    
    http://whippet.stanford.edu/~ssklar/
    
If this script is useful to you, or even if it is useless to you,  or 
+you have 
some changes/improvements/questions/large sums of cash and nothing
 to do with it, please send me an email.
    
=head1 FOR MORE INFORMATION

Most of the things that this script does were taken from the IBM 
publication "Statistical Analysis and Reporting System User Guide", wh
+ich
 can be downloaded from <http://www.storage.ibm.com/hardsoft/tape/pubs
+/pubs3590.html>.

Information about creating custom error notification objects can be 
found in Chapter 4 of the IBM manual "General Programming Concepts:
 Writing and Debugging Programs", available online at 
< http://www.rs6000.ibm.com/doc_link/en_US/a_doc_lib/aixprggd/genprogc
+/error_notice.htm>

=head1 COPYRIGHT

This program is free software; you may redistribute it and/or modify i
+t
 under the same terms as Perl itself.

=cut

#---------------------------------------------------------------------
+-
# version history
#---------------------------------------------------------------------
+-

# $Log: cinnamon,v $
# Revision 1.1  2001/08/21 04:48:51  ssklar
# Initial revision
#

[mugwumpjism]

Not bad. One "error" I can see: you're not checking the return code from sendmail (which will be available in $? after the close).

I would also strongly consider making a new module for "raising alert messages" for your system, so that if later on you decide that sending an email when there's a problem get unmanagable, you can get it to post it to your favourite error processing tool by changing your system in one place. Another approach is to raise a critical event to the system log with something like Sys::Syslog, and use a program like xlogmaster to catch them after that.

[blueflashlight]

Thanks for the advice; I'll do that return code checking (I always confuse $! and $? ... maybe I should tatoo them on my arm)

I did think about breaking the script into a "modular" form, but because of the way it will be used (called as an event handler by the error reporting subsystem, via an ODM lookup), I thought it best to keep it all self-contained.

thanks again!

[Hofmator]

Nice job, imho, but allow me two random remarks:

• This line is useless:

  my $recipient = "root";
  unless (defined($recipient)) { $recipient = "root" } # useless
  

  and I would - if it wasn't useless - prefer to write the line as $recipient = "root" unless (defined($recipient)); but that's more a matter of taste...
• These lines can be simplified:

  substr($message{detail}, 32, 8) =~ /(\w\w)(\w\w)(\w\w)(\w\w)/;
  $message{mml} = pack ("CCCC", hex($1), hex($2), hex($3), hex($4));
  
  # simply
  $message{mml} = pack ("H*", substr($message{detail}, 32, 8));
  

-- Hofmator

[busunsl]

$recipient = "root" unless (defined($recipient));

Or even shorter with this common idiom:
$recipient ||= "root";

[dragonchild]

Those are not the same statement. The first only sets $recipient to "root" if $recipient is undef. The second sets it if $recipient is "" or 0, as well.

One big suggestion I have is to break things out into functions and put those functions in a module. Use that module and your script becomes much smaller. But, that's not the big savings. Say, for instance, you want to do something similar, but not quite the same. You just re-use a bunch of functions and your development time is cut dramatically. In addition, you already know that the funtions in this module are debugged correctly, so you can trust them! :)

------
/me wants to be the brightest bulb in the chandelier!

Vote paco for President!

[blueflashlight]

The idea behind the ...



my $recipient = "root";
unless (defined($recipient)) { $recipient = "root" }


is that, if the user chooses not to edit the script and put their email address in the var $recipient, the mail would go to root by default. I guess my execution of that idea leaves something to be desired.

Thanks much for the regex/pack/substr reduction ... I was sure that there was a way to do that, but I just couldn't figure it out!

[maverick]

The only thing I see that others have not mentioned falls into the 'never trust user input' category:

my $sequence_number = shift ||
    die "cinnamon: sequence number needed as argument\n";

open (ERROR, "/usr/bin/errpt -g -l $sequence_number |");

You're passing whatever the first parameter is directly to the open, and you're not checking to see if the open failed. Here's a modification with a little more paranoid error checking.

my $sequence_number = shift;
unless ($sequence_number =~ /^\d+$/) {
    # I am assuming it must be a number
    die "cinnamon: sequence number needed as argument\n";
}

open (ERROR, "/usr/bin/errpt -g -l $sequence_number |") || die "couldn
+'t open errpt: $!";

/\/\averick
perl -l -e "eval pack('h*','072796e6470272f2c5f2c5166756279636b672');"

[blueflashlight]

excellent catch! thank you ... I'll ensure that the argument is validated.

[japhy]

This is mainly superficial, but there are some noteworthy comments.

unless ($^O =~ /aix/) { die ... }
# might be better as


die ... unless $^O =~ /aix/;

I'm not sure about that whole $recipient thing. Are you expecting an argument?

my $sequence_number = shift || die ...;
# technically, you want 'or' instead of '||' here

open (ERROR, "/usr/bin/errpt -g -l $sequence_number |");
# you don't check to see if it opened properly...
# nor do you ensure the safety of the variable
# if I enter '; rm -rf' here, you're in trouble


my ($sequence_number) = shift =~ /(\d+)/
  or die ...;
open ERROR, "/usr/bin/errpt -g -l $sequence_number |"
  or die "can't run errpt -g -l $sequence_number: $!";

I'm going to idiomatize your while-loop.

while (<ERROR>) {
  $message{host} = (split)[1], next if /^el_nodeid/;
  $message{drive} = (split)[1], next if /^el_resource/;
  $message{detail} = (split)[1], next if /^el_detail_data/;
}

Your hex-to-ascii stuff can be replaced by using "H*" instead of "C". The only other thing I'd change is your @mail array and ALL those sprintf()s.

printf SENDMAIL "%-16s: %-20s\n" => @$_ for
  ["Sequence Number" => $sequence_number],
  [ Host => $message{host} ],
  [ Drive => $message{drive} ],
  [ Model => $message{model} ],
  [ Microcode => $message{mml} ],
  [ "Message Type" => $message{type} ],
  [ "Message Code" => $message{code} ],
  [ Severity => $message{severity} ],

  $message{type} eq "SIM" ? (
    [ "First FSC" => $message{first_fsc} ],
    [ "Last FSC" => $message{last_fsc} ],
  ) : (
    [ VOLSER => $message{volser} ],
  );

It's condensed somewhat, and the data structure still represents the pairing between text and variable. And, if you need to change the format, you change it once.

_____________________________________________________
Jeff[japhy]Pinyan: Perl, regex, and perl hacker.
s++=END;++y(;-P)}y js++=;shajsj<++y(p-q)}?print:??;

[blueflashlight]

>>> die ... unless $^O =~ /aix/;

I like it. thanks.

>>>I'm not sure about that whole $recipient thing. Are you expecting an argument?

The idea is that, if the user wishes to edit the script and change the value of $recipient, then that value will be used, but if not, then "root" will be used. I'll make that clearer in the code and the pod.

>>> the only other thing I'd change is your @mail array and ALL those sprintf()s.

thanks! I like that suggestion.

[blueflashlight]

if anyone is interested, the final code (well, final as of today) can be found at http://whippet.stanford.edu/~ssklar/aix/cinnamon/

thanks to everyone for the great advice and comments!