Feedback on my second script

[Jackiejarvis]

Like in my previous post I would ask you to give some feedback on my second script. This script will start a rsync process between to places. I would like to get some feedback on this script so I can improved it or maybe you see some things that can go wrong that I don't see.

Many thanks!

#!/usr/bin/perl
# (c) Dieter Odeurs
# This script starts rsync for a back-up job

use strict;
use warnings;
use Getopt::Long;

my $path = "";
my $destination;
my $help;

#Show usage
sub showUsage{
print <<EOF;
    ----------------------------
    -- Usage for rsync script --
    ----------------------------
    Required:
    --path :
        The source directory, this is the directory you want to back-u
+p
        Example --path='/path/to/source'
    --destination
        This is the destination, where you want to put the back-up
        Example --destination='/path/to/destination'
    Optional:";
    --exclude
        Use this option if you want to exclude some directories
        Without this option the script will exclude only .etc and lost
++found
        Example: --exclude='test' --exclude='demo'
EOF
exit;
}

#Without exclude options the script automatically exclude .etc and los
+t+found
my @exclude = (
        ".etc",
        "lost+found"
        );

#You must specify a path, destination and if you want to exclude some 
+extra files it's possible with exclude
showUsage() if (!GetOptions ( 'path=s' => \$path,
                'exclude=s' => \@exclude,
                'destination=s' => \$destination,
                'help|?' => \$help,
                '<>' => \&showUsage ) or defined $help );

# Check if the path exist otherwise exit the program and give an error

if(defined $path){
        if (-d $path){
                print $path . " is a correct path \n";
        }else{
                print $path . " is a incorrect path \n";
                exit;
        }
}else{
        showUsage();
}

# Check if a destination is defined
unless (defined $destination) {   
    showUsage ();
}

print "exclude : @exclude \n";

# Create the command to start rsync with all the excludes and options
my $cmd =  "rsync -av --iconv=iso88591,utf8 --stats";
foreach my $exclude (@exclude) {
        $cmd = $cmd . " --exclude '$exclude'";
}
$cmd .= " $path $destination";
print $cmd . "\n";

# Run the program and show the output
my @result;
@result = `$cmd 2>&1`;
foreach my $result (@result) {
        print $result;
        chomp $result;
}

#exit the program
exit;

[Corion]

Two small things:

# Check if a destination is defined
unless (defined $destination) {   
    showUsage ();
}

I would like an improved error message here, like

# Check if a destination is defined
unless (defined $destination) {   
    print "No destination given.\n";
    showUsage ();
}

And at the place where you run rsync, you capture all of its (voluminous) output just to print it out right afterwards:

# Run the program and show the output
my @result;
@result = `$cmd 2>&1`;
foreach my $result (@result) {
        print $result;
        chomp $result;
}

I would change that to simply use system instead, which will do the printing directly. This would also add error checking so your script signals if the underlying rsync failed:

# Run the program and show the output
system( "$cmd 2>&1") == 0
    or die "rsync failed: $! / $?";

Of course, if you really want/need to do post-processing of the rsync output, as your chomp indicates, you will need to capture the output as you did.

[Jackiejarvis]

Thanks for the feedback. I have inserted the things you suggest. About the running rsync. In the future I would send a mail to the admin if there is an error with the error message included. How could i do something like this is it possible if i use system or not?

greetings!

[Corion]

Yes, if you want to extend the program later to send the output via mail (but only in the case of error), your current approach makes sense.

[jonadab]

I would send a mail... How could i do something like this

I've traditionally used Mail::Sendmail for that; although, Mail::Send may also be worth considering.

Note too that it's generally unsafe to allow the user to specify a destination address for mail (unless you have carefully authenticated the user). In simple cases like this, it's often best to hardcode the destination address. In more advanced cases, possible destination addresses can be stored in a configuration file or database that only system administrators can edit, and the user can be allowed to select from among them.

[karlgoethebier]

BTW, if using Getopt::Long why not also Pod::Usage?

Built-in documentation, e.g../acme.pl --help or perldoc acmel.pl. Easy to use and very handy (I'm reseller ;-)).

Read more... (3 kB)

Regards, Karl