Perl script to set IP settings

Question

This is my first go at a Perl script to (semi-) automate setting the correct IP settings. I use it to configure a virtual machine after creation. It's also my first Perl script as a whole, so I'm looking for anyone who can give me pointers towards a better and more efficient coding style.

#!/usr/bin/perl 

use strict;
use warnings;

use Getopt::Long;
use Tie::File;

sub printMissingValues; 
sub gatherInputValues;
sub setValueForField;

my %resulthash = (
    "IPADDR"  => undef,
    "NETMASK" => undef,
    "GATEWAY" => undef,
     "DNS1"    => undef,
     "DNS2"    => undef,
);

my $filename = undef;


GetOptions ("filename=s"  => \$filename,
				"IP=s"        => \$resulthash{'IPADDR'},
            "NETMASK=s"   => \$resulthash{'NETMASK'},
            "GATEWAY=s"   => \$resulthash{'GATEWAY'},
            "DNS1=s"      => \$resulthash{'DNS1'},
				"DNS2=s"      => \$resulthash{'DNS2'}) 

or die ("Error in input values");

#gatherInputValues(\%resulthash);

open(my $fd, '>>', "$filename") 
    or die("Cannot open file $filename");

tie my @array, 'Tie::File',  $filename
    or die "Cannot tie file '$filename': $!";

for my $line (@array)
{
   my @fields = split /=/, $line;

    if ($fields[0] eq 'BOOTPROTO') {
         $fields[1] = '"static"';
        $line = join '=', @fields;
        }

    elsif ($fields[0] eq 'IPADDR' ) {
            $line = setValueForField('IPADDR', @fields);
    }

    elsif ($fields[0] eq 'NETMASK') {
            $line = setValueForField('NETMASK', @fields);
    }

    elsif ($fields[0] eq 'GATEWAY') {
            $line = setValueForField('GATEWAY', @fields);
    }

    elsif ($fields[0] eq 'DNS1') {
            $line = setValueForField('DNS1', @fields);
    }

    elsif ($fields[0] eq 'DNS2') {
            $line = setValueForField('DNS2', @fields);
    }
}

untie @array;

addMissingValues(\%resulthash);

close($fd);

##functions##

sub addMissingValues {
    my $hash = shift @_;

    while(my($key, $value) = each %{$hash}) {
        if (defined ($value)) {
            createNewField($key, $value);
        }
    }
}

 sub createNewField {
    my $key = shift @_;
    my $value = shift @_;
    print $fd (join '=', $key, $value);
    print $fd ("\n");
}

sub setValueForField {
    my $fieldname = shift;
    my @fields = shift;

    $fields[1] = $resulthash{$fieldname};

    return  (join '=', @fields);
}

sub gatherInputValues {

    my $result_hash = shift @_;

    print "IP4-address: ";
    $result_hash->{'IPADDR'} = <STDIN>;
    print "subnet-mask: ";
    $result_hash->{'NETMASK'} = <STDIN>;
    print "gateway: ";
    $result_hash->{'GATEWAY'} = <STDIN>;
    print "dns1: ";
    $result_hash->{'DNS1'} = <STDIN>;
    print "dns2: ";
    $result_hash->{'DNS2'} = <STDIN>;
}

This is the file the script operates on (standard configuration script for CentOS systems - /etc/sysconfig/network-scripts/ifcfg-etho)

TYPE="Ethernet"
PROXY_METHOD="none"
BROWSER_ONLY="no"
BOOTPROTO="static"
DEFROUTE="yes"
IPV4_FAILURE_FATAL="no"
IPV6INIT="yes"
IPV6_AUTOCONF="yes"
IPV6_DEFROUTE="yes"
IPV6_FAILURE_FATAL="no"
IPV6_ADDR_GEN_MODE="stable-privacy"
NAME="eth0"
UUID="7cabbc98-e67d-4a64-a132-7e8bcbeb579b"
DEVICE="eth0"
ONBOOT="yes

Answer 1

1. There's no need to predeclare the subs (sub whatever;).

2. There's no need to assign the initial values to %resulthash.

3. my $filename; does the same as my $filename = undef;. There's no need to specify the undef.

4. Hash keys are autoquoted if they are simple (i.e. follow the same rules as variable name). Instead of $resulthash{'IPADDR'} you can type just $resulthash{IPADDR}.

5. shift in a sub operates on @_. So you can just write my $hash = shift;.

6. Instead of shifting @_ several times, assign all the variables at the same time: my ($key, $value) = @_;.

Plus, you're comparing $fields[0] to various strings, maybe a "dispatch table" would be a bit clearer. See my version of the script:

#!/usr/bin/perl
use strict;
use warnings;

use Getopt::Long;
use Tie::File;

my %resulthash;
my $filename;

GetOptions ('filename=s' => \$filename,
            'IP=s'       => \$resulthash{IPADDR},
            'NETMASK=s'  => \$resulthash{NETMASK},
            'GATEWAY=s'  => \$resulthash{GATEWAY},
            'DNS1=s'     => \$resulthash{DNS1},
            'DNS2=s'     => \$resulthash{DNS2})
or die 'Error in input values';

open my $fd, '>>', $filename
    or die "Cannot open file $filename";

tie my @array, 'Tie::File',  $filename
    or die "Cannot tie file '$filename': $!";

for my $line (@array) {
    my @fields = split /=/, $line;

    { BOOTPROTO => sub { $fields[1] = '"static"';
                         $line = join '=', @fields; },
      IPADDR    => sub { $line = setValueForField('IPADDR', @fields); },
      NETMASK   => sub { $line = setValueForField('NETMASK', @fields); },
      GATEWAY   => sub { $line = setValueForField('GATEWAY', @fields); },
      DNS1      => sub { $line = setValueForField('DNS1', @fields); },
      DNS2      => sub { $line = setValueForField('DNS2', @fields); },
    }->{ $fields[0] }->();
}

untie @array;

addMissingValues(\%resulthash);

close $fd;

sub addMissingValues {
    my $hash = shift;

    while (my ($key, $value) = each %$hash) {
        createNewField($key, $value) if defined $value;
    }
}

sub createNewField {
    my ($key, $value) = @_;
    print {$fd} join '=', $key, $value;
    print {$fd} "\n";
}

sub setValueForField {
    my ($fieldname, @fields) = @_;

    $fields[1] = $resulthash{$fieldname};

    return join '=', @fields;
}

You can also keep the dispatch table outside of the loop, but you need to pass parameters to the callbacks.

my %DISPATCH = (
    BOOTPROTO => sub { $_[1] = '"static"';
                       join '=', @_ },
    IPADDR    => sub { setValueForField('IPADDR', @_) },
    NETMASK   => sub { setValueForField('NETMASK', @_) },
    GATEWAY   => sub { setValueForField('GATEWAY', @_) },
    DNS1      => sub { setValueForField('DNS1', @_) },
    DNS2      => sub { setValueForField('DNS2', @_) },
);

for my $line (@array) {
    my @fields = split /=/, $line;
    my $sub = $DISPATCH{ $fields[0] };
    $line = $sub->( @fields) if $sub;
}