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;
}

• mpapec: Dispatch definition should be out of the loop.
• choroba: @mpapec: Then you wouldn't have closures over $line and @fields...
• choroba: @mpapec: Check the update.
• mpapec: Yes, you're out of scope but recreating hash and subs for each iteration doesn't look optimal. gist.github.com/mpapec/e1d1199a3f030a2f26e4c83c9637f44e