You appear to be using a boatload of lexicals, all defined at the top of the script. This is only a marginal improvement over using global variables. My personal preference is to introduce them just before they are needed, and if possible, enclosed in blocks so that they can go out of scope quickly (when no longer needed).

For instance it would be better to say

my ($min, $hour, $day, $mon, $year) = (localtime(time))[1..5];
[download]

(undef, $min, $hour, $day, $mon, $year) = localtime(time);
[download]

But then all you do with them is produce a datestamp:

$starttime = "$mon"."$day"."$hour"."$min"."$year";
[download]

$starttime = "$mon$day$hour$min$year";
[download]

But all of that is pretty moot, because a better way to generate timestamps (better because it obviates the need for a slew of lexicals) is to use an anonymous sub to diddle the localtime values on the fly.

    my $starttime = sprintf '%4d%02d%02d%02d%02d',
                sub { $_[5]+1900, $_[4]+1, $_[3], $_[2], $_[1] }->(loc
+altime);
}
[download]

Hint: $_[3] et al. are scalars coming into the anonymous sub (via @_). These parameters are coming in from localtime. The subroutine is returning a list of scalars (reordering and munging various elements on the fly). The returned list is fed to sprintf and it just so happens that the list dovetails nicely into the format string.

$starttime = "$mon"."$day"."$hour"."$min"."$year";
[download]

here you are performing a null series of interpolations. You are better off saying

$starttime = "$mon$day$hour$min$year";
[download]

The real problem I see with the first one is writing "$var" when $var would do (this habit can actually hurt if you are dealing with things that aren't strings -- references, for example). FYI, both of these compile to the same code which is most like:

$starttime = $mon.$day.$hour.$min.$year;
[download]

Which isn't to say that the above line is a better way to write it than what you wrote. I find both equally valid, perhaps even preferring yours. I just wanted to emphasize the mistake of writing "$var", which is only useful when you want to convert something that might not be a string into a string.
        - tye (but my friends call me "Tye")

I'm afraid that I get quote-happy (comes from my old sh-scripting days); I'm sure that I'll continue to use unnecessary quotes until the day that I get bitten, just as you said above.

thanks! -s-

wow. thank you. I've put in your suggestions; as a bonus, I have now used my first anon sub (and I almost understand exactly what it is doing! :-)

many thanks. -s-

1. Where you have

    if ($opts{h} || $opts{u})  { exec "perldoc $0" };
   

   you might want to check out the Pod::Usage module. For example (copied straight from the docs)

    use Pod::Usage;
    use Getopt::Long;
    ## Parse options
    GetOptions("help", "man", "flag1") || pod2usage(-verbose => 0);
    pod2usage(-verbose => 1)  if ($opt_help);
    pod2usage(-verbose => 2)  if ($opt_man);
    pod2usage(-verbose => 2, -message => "$0: $error_msg.\n")
        if $some_error
   

   The neat thing about this module, is that it can handle your 'usage' message (runrig), or display a full man page without you having to do it your self, or setting off an exec to do it for you.

2. You having the following

    if ($opts{f})             { $conf = $opts{f} };        
    # if no conf file specified, use "/etc/errreporter.conf as
    # the default ...
    unless ($conf) { $conf = "/etc/errreporter.conf" 
   

   Simpler (or at least more idiomatic) may be

    $conf = $opts{f} if exists $opts{f};
    $conf ||= "/etc/errreporter.conf";
   

   or even

    $conf = $opts{f} || "/etc/errreporter.conf";
   

   Mind out for the gotcha though... $conf will get set to the default if $opts{f} doesn't exist, isn't defined, or if it is just logically false (0 or empty string)

3. You might want to load your config file using require. Its a lot safer, more robust, more portable and just plain easier

4. You seem to be fond of using regexps for testing string equality:

    if ($identifier =~ /$_/)...
    if ($ignore =~ /yes/)...
   

   What's wrong with plain old

    if ($ignore eq 'yes')...
   

   That's what 'eq' is for

Good luck with it :)

Update corrected $ignore eq /yes/ to $ignore eq 'yes'. Thanks chipmunk for pointing that out... it was a typo... honest!

whew! the learning never ends! I've used (most) of your suggestions, and if this stuff keeps up, I may actually become a half-decent perl person someday!

thanks so much to you, and everyone who has helped me out with this.

-s-

about the using "require" suggestion; I did read in my (camel | llama) book the very same thing. I ran into some problems, though when running under different versions of perl (5.5x vs. 5.6), and using the "cat" method worked everywhere.

I don't remember exactly what went wrong, I figure that since the script is only useful on aix, if /usr/bin/cat isn't there, there are bigger problems. :-) (i've added the full path to cat, now, just in case.)

-s-

(my $cmd = $0) =~ s#.*/##;
my $usage_str = <<EOT
Usage: $cmd [-hulf] [-f file]
'Explanation of above command and options'
EOT

getops('hulf:', \%opts) or die $usage_str;
[download]

thank you! I've incorprated your suggestion. -s-

: # -*- perl -*-
eval "exec perl -S $0 $*"
if $running_under_some_shell;

#=====================================================================
# errreporter - mails interesting entries from the AIX error log
#=====================================================================

use strict;
use File::Basename;
use Getopt::Std;

# declare vars in the conf file ...

my ($email, @ignore);

# declare some other vars used below ...

my ($identifier, $timestamp, $resource, $description, @error);

my $me = basename($0);
chomp (my $hostname = `/usr/bin/hostname`);

my @usage_string = <<ENDOFUSAGESTRING;
Usage: $me [-hul] [-f file]
Run "$me -h" for additional information.
ENDOFUSAGESTRING

my %opts;

getopts('hulf:', \%opts) or die @usage_string;

if ($opts{h} || $opts{u})  { exec "perldoc $0" };

if ($opts{l})              { exec '/usr/bin/errpt', '-t'
                               or die "$me: couldn't exec 'errpt -t'\n
+" };  

my $conf = $opts{f} || "/etc/errreporter.conf";

eval `/usr/bin/cat $conf 2> /dev/null` 
  or die "$me: couldn't read specified conf file $conf\n";

unless (defined ($email)) { die "$me: email not defined " }

# add the errpt header line to the ignore list ...

push (@ignore, "IDENTIFIER");

# get the current time, in the form used by the errpt command ...

my $starttime = sprintf '%02d%02d%02d%02d%02d',
  sub { $_[4]+1, $_[3], $_[2], $_[1], $_[5] % 100 } -> (localtime);

print STDOUT "$me: starting errreporter at $starttime, using conf file
+ $conf.\n";

# open an errpt process, reading the summary of errors posted
# after the start time of this script ...

open (ERRPT, "/usr/bin/errpt -c -s $starttime |") or
  die "$me: couldn't open errpt for reading!\n";

# while the above process is open ...

while (<ERRPT>) {

    chomp;

    # split the errpt entry into fields ...

    ($identifier, $timestamp, undef, undef, $resource, $description) =
      split (/\s+/, $_, 6);

    my $ignore = 'no';

    foreach (@ignore) {

        # check to see if the entry is on the ignore list ...

        if ($identifier =~ /$_/) { $ignore = 'yes' ; last }

    };

    unless  ($ignore eq 'yes') {

        # store the full entry in the array @error ...

        @error = `/usr/bin/errpt -a -s $timestamp`;

        # add a subject line to the @error array ...

        unshift (@error, "Subject: $hostname -- $resource -- $descript
+ion\n");

        #  and send the e-mail ...

        open (SENDMAIL, "|/usr/sbin/sendmail $email") or
          die "$me: couldn't open sendmail: $!";

        print SENDMAIL @error;

        close (SENDMAIL);

    };

};

#=====================================================================

=pod

=head1 NAME
... cut out the pod, which hasn't changed ...
=cut
[download]