wskibum has asked for the wisdom of the Perl Monks concerning the following question:

Hello again,
You guys were such a great help on my last problem that I've returned with a new one. I'm new to Perl and am trying my hand at a concatenation script. It's a bit clunky and I want to add some features but am not sure how. Any help or suggestions is most welcome and I thank you in advance.

My objective here is to:

1) Clean this script up

2) SCP errors not being handled correctly

3) Do something with the "Number of days to go back" entry so I can just put in 1 number for a week, month, etc.

4) create a tar file with a date stamp on the name that holds all the files being concatenated and the resulting file from the process, then move the tar file to an archive directory.

Here's the script:

# THIS SCRIPT CONCATENATES DATA FOR 1 WEEK OF RELIANT TRUST DATA AND S +ENDS TO NDM # $MT='my.email.com'; # Mail Alias used for sending emails $ASL='app-server'; @BFL=('name1.log','name2.log'); # START TIME AND DATE VARIABLES # $now = time(); $yesterday = $now - 3600*24; ($sec,$min,$hour,$mday,$mon,$year,$wday,$yday,$isdst) = localtime($yes +terday); $yestermday=sprintf("%02d", $mday); $yesterwday=$wday; $lf="$0.$yesterwday.log"; open(LF,">$lf"); $year += 1900; $mon++; $mon = sprintf("%02d", $mon); $mday = sprintf("%02d", $mday); # # END TIME AND DATE VARIABLES # Saturday only ($wday=Friday), push concatenation of 7 days data if ($yesterwday == 5) { # compute last 7 days of file names # cat the files foreach $f (@BFL) { $catfile = "$f\.$year$mon$yestermday"; unlink $catfile; # in case we run 2x in one day foreach $i (5,6,7,8) { for $dd (7,6,5,4,3,2,1) { # Number of days to go bac +k for process $then = $now - $dd*3600*24; ($sec,$min,$hour,$mday,$mon,$year,$wday,$yday,$isdst) += localtime($then); $mday = sprintf("%02d", $mday); $year += 1900; $mon++; $mon = sprintf("%02d", $mon); $fileext = "$f\.myservername$i\.$year$mon$mday"; $command = "cat $fileext.$ASL$i >> $catfile 2>/dev/nul +l"; if ($debug) { print "WOULD $command\n"; } else { $output = `$command`; } $exit_value = $? >> 8; if ($exit_value == 2) { print LF "Assuming it's not there: $fileext.$ASL$i +\n"; } elsif ($exit_value) { print LF "Trouble with command: $command\n"; print LF "$output\n"; $errs++; } else { print LF "OK: $command\n"; } } } } # the cat-ed file is produced: scp it $filetoput = "/home/echxfr/$catfile"; @cmd = ( '/usr/bin/scp', "$catfile", "cduser\@ndm:$filetoput", ); $rv = system(@cmd); if ($rv == -1) { print("Unable to execute scp: $!\n"); } elsif ($rv & 127) { printf("scp to died with signal %d\n", ($rv & 127)); } elsif ($rv >> 8) { printf("scp exited with error %d\n", ($rv >> 8)); } else { print("scp was successful\n"); } $fname = "$catfile"; if (-z $fname ) { `/bin/mailx -s "ERROR: Reliant SCP Log file $fname sent to ndm + is empty" $MT`; } }

Replies are listed 'Best First'.
Re: Help with Concatenates script
by moritz (Cardinal) on Apr 03, 2008 at 17:18 UTC
    I won't clean the script up for you, but I have a few hits:
    1. Remove the newlines between two related statements
    2. Align the closing curly braces to the same column where their opening counterpart is
    3. Don't do all the date calculation manually. Use Date::Simple or Date::Calc instead.

    After that you script will be much shorter and better readable, and you can start to think about enhancements.

      Can't load modules on target host.
Re: Help with Concatenates script
by leocharre (Priest) on Apr 03, 2008 at 21:48 UTC
    I have to double what was said here. Break up the script into subs. Even if you're just calling it one time. It will make more sense to you, and to other people viewing your code. Break up what happens into tasks. I have started the process here for you.. this is not functional code.. But it illustrates some examples.
    #!/usr/bin/perl use strict; # THIS SCRIPT CONCATENATES DATA FOR 1 WEEK OF RELIANT TRUST DATA AND S +ENDS TO NDM # my $MAIL_ALIAS='my.email.com'; # Mail Alias used for sending emails my $ASL='app-server'; # what the heck is ASL?????????!!!! my @LOG_FILENAMES=('name1.log','name2.log'); my($yyyy,$mm,$dd) = deduct_date_values( timestamp_days_back(1) ); my $abs_log_yesterday = "$0.$dd.log"; sub secsday { return (3600 * 24) } # to give you an idea how far you c +ould consider breaking this up sub deduct_date_values { my $timestamp = shift; $timestamp or die; # expect the 'impossible' require Time::Format; my $date_string = Time::Format::time_format('yyyy_mm_dd',$timestamp +); my($yyyy,$mm,$dd) = split( /_/, $date_string ); # make sure $yyyy or die; $mm or die; $dd or die; return ($yyyy,$mm,$dd); } sub timestamp_days_back { my $numdays = shift; $numdays or die; my $then = ( time - ( $numdays * secsday() ) ); return $then; } if ($yesterwday == 5) { # compute last 7 days of file names # cat the files foreach $logfilename (@LOG_FILENAMES) { my $abs_cat_target = "$logfilename.$year$mm$dd"; unlink $abs_cat_target; # in case we run 2x in one day for my $i (5,6,7,8) { for my $days_back (7,6,5,4,3,2,1) { # Number of days + to go back for process my($yyyy,$mm,$dd) = deduct_date_values( timestamp_days +_back($days_back) ); my $abs_file = "$logfilename.myservername$i\.$year$mm$ +dd"; $command = "cat $fileext.$ASL$i >> $catfile 2>/dev/nul +l"; if ($debug) { print "WOULD $command\n"; } else { $output = `$command`; }

    Now, some serious pointers. Things you may think trivial but are actually make or break. Label your scalars/symbols descriptively. Don't use shotcuts like 'f' to mean absolute path to logfile , call it $absolute_path_to_logfile

    Use strict. Don't play russian roulette with this. Maybe the data you are handling is actually valuable information.

    Do what the pros tell you to do.
    The more time passes, the more I come to agree with the higher monks. Try out what they suggest, understand it, then reject it if you still want to- after you've done it, used it.

    There's a *reason* for the anal retentiveness of things like using strict and naming variables properly- and it's not to make our life harder or to patronize the noobs.
    It's to make your life better and easier.

    If you ignore people telling you to use strict- and you continue using perl, you will be using strict. It will just take longer to come around- and slow down your learning curve. I think that to learn to do as much as you did, you must have been exposed to 'use strict' a lot of times. Believe it. Don't doubt it.

    I hope this is not going too far up your ***. I would have cleaned out your script but I have to tell you my friend, it's impossible unless I'm you- because of how the code is written.
    I would let most of this go for a noob. But from looking at your code, I *know* you're new to perl but not to computers. You know some shiznit. You need a peer to smack you upside the head- here it is.

    If you see yourself using perl in the future, you need Damian Conway's Perl Best Practices, you can get it for $5 at amazon.com.

Re: Help with Concatenates script
by apl (Monsignor) on Apr 03, 2008 at 17:36 UTC
    and you can start to think about enhancements.

    Like encapsulating the @cmd construction and processing into a sub.

Re: Help with Concatenates script
by chrism01 (Friar) on Apr 04, 2008 at 01:03 UTC
    ... and also use either

    #!/usr/bin/perl -w
    or
    #!/usr/bin/perl
    use warnings;

Re: Help with Concatenates script
by jwkrahn (Abbot) on Apr 04, 2008 at 03:51 UTC
    My objective here is to: 1) Clean this script up
    #!/usr/bin/perl use warnings; use strict; # THIS SCRIPT CONCATENATES DATA FOR 1 WEEK OF RELIANT TRUST DATA AND S +ENDS TO NDM # my $MT = 'my.email.com'; # Mail Alias used for sendin +g emails my $ASL = 'app-server'; my @BFL = qw( name1.log name2.log ); # START TIME AND DATE VARIABLES # my ( $mday, $mon, $year, $wday ) = ( localtime $^T - 3600 * 24 )[ 3 .. + 6 ]; # Saturday only exit 0 if $wday != 5; # # END TIME AND DATE VARIABLES for my $f ( @BFL ) { my $catfile = sprintf '%s.%d%02d%02d', $f, $year + 1900, $mon + 1, + $mday; open my $CAT, '>', $catfile or die "Cannot open '$catfile' $!"; for my $i ( 5 .. 8 ) { for my $dd ( reverse 1 .. 7 ) { # Number of days to go back + for process my ( $mday, $mon, $year ) = ( localtime $^T - $dd * 3600 * + 24 )[ 3 .. 5 ]; my $fileext = sprintf '%s.myservername%d.%d%02d%02d', $f, +$i, $year + 1900, $mon + 1, $mday; open my $IN, '<', "$fileext.$ASL$i" or warn "Cannot open ' +$fileext.$ASL$i' $!"; print $CAT while <$IN>; } } # the cat-ed file is produced: scp it my $filetoput = "/home/echxfr/$catfile"; my @cmd = ( '/usr/bin/scp', $catfile, "cduser\@ndm:$filetoput", ); my $rv = system @cmd; if ( $rv == -1 ) { print "Unable to execute scp: $!\n"; } elsif ( $rv & 127 ) { printf "scp to died with signal %d\n", $rv & 127; } elsif ( $rv >> 8 ) { printf "scp exited with error %d\n", $rv >> 8; } else { print "scp was successful\n"; } if ( -z $catfile ) { 0 == system '/bin/mailx', '-s', "ERROR: Reliant SCP Log file $ +catfile sent to ndm is empty", $MT or warn "system 'mailx' failed: $?"; } }