6
\$\begingroup\$

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
\$\endgroup\$
7
\$\begingroup\$
  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;
}
\$\endgroup\$
  • \$\begingroup\$ Dispatch definition should be out of the loop. \$\endgroup\$ – mpapec May 20 '18 at 7:48
  • \$\begingroup\$ @mpapec: Then you wouldn't have closures over $line and @fields... \$\endgroup\$ – choroba May 20 '18 at 8:02
  • \$\begingroup\$ @mpapec: Check the update. \$\endgroup\$ – choroba May 20 '18 at 8:59
  • \$\begingroup\$ Yes, you're out of scope but recreating hash and subs for each iteration doesn't look optimal. gist.github.com/mpapec/e1d1199a3f030a2f26e4c83c9637f44e \$\endgroup\$ – mpapec May 20 '18 at 19:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.