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

Hi.

As indicated by 192323, I am trying to write an auto-admin of sorts. This is a VERY rough draft of it, to which I am seeking suggestions, comments, more feature requests, etc.

what i need ideas on is.. right now it can only handle either a 1 IP (which makes it pretty useless) or a class C of IPs. (or any CIDR thats less than class C)

for ($i = 0; $i <= 254; $i++) this line is the problem. a bunch of nested if's (if you arent understanding my idea of how nested if's would fix this.. you shouldn't be reading this anyway) would fix this. but i dont want them, its too ugly.

so..

help. suggest. make feature requests. flames to /dev/null please.

#!/usr/bin/perl
use NetAddr::IP;
use Net::SSH::Perl;

$configfile = "admin.conf";

open(CONFIG, $configfile) or die "Couldn't open config file: $!\n";
while (<CONFIG>) {
  chomp;
  s/#.*//;
  s/^\s+//;
  next unless length;
  ($var, $value) = split(/\s*=\s*/, $_, 2);
  $User_Preferences{$var} = $value;
}

chop($User_Preferences{IP});

$ip = new NetAddr::IP "$User_Preferences{IP}";
@server_list = $ip->hostenum;

for ($i = 0; $i <= 254; $i++) {
  if ($server_list[$i] =~ /(\d+)\.(\d+)\.(\d+)\.(\d+)/) {
    $period = ".";
    $server_list[$i] = $1 . $period . $2 . $period . $3 . $period . $4
+;
    print "$server_list[$i]\n";
   }
}


sub test {
  foreach $server (@server_list) {
    $testcmd = "touch /tmp/success.$server";
    $ssh = Net::SSH::Perl->new($server);
    $ssh->login($User_Preferences{USER}, $User_Preferences{PASSWORD});
    ($stdout, $stderr, $exit) = $ssh->cmd($testcmd);
  }
}

sub restartapache {
  foreach $server (@server_list) {
    $apacherestartcmd = "$User_Preferences{APACHEPATH} restart";
    $ssh = Net::SSH::Perl->new($server);
    $ssh->login($User_Preferences{USER}, $User_Preferences{PASSWORD});
    ($stdout, $stderr, $exit) = $ssh->cmd($apacherestartcmd);
  }
}



if ($User_Preferences{TEST} == 1) { test(); }
if ($User_Preferences(RESTARTAPACHE} == 1 { restartapache(); }

[download]

Edited: ~Tue Aug 27 23:10:27 2002 (GMT) by footpad: Converted plaintext URL to hyperlink (per Consideration) and added HTML formatting, for readability.

Replies are listed 'Best First'.
Re: Auto-admin (finally, the code is here! rough draft)
by sauoq (Abbot) on Aug 27, 2002 at 21:53 UTC
    if you arent understanding my idea of how nested if's would fix this.. you shouldn't be reading this anyway

    I almost decided to move on upon reading this... but then I figured, what the heck, I'm always diving into things I don't understand. I'm not going to listen to anybody who tells me what I should or shouldn't be reading.

    So, after reading the docs and actually playing with NetAddr::IP a little I took another look.

    $ip = new NetAddr::IP "$User_Preferences{IP}";
@server_list = $ip->hostenum;

for ($i = 0; $i <= 254; $i++) {
  if ($server_list[$i] =~ /(\d+)\.(\d+)\.(\d+)\.(\d+)/) {
    $period = ".";
    $server_list[$i] = $1 . $period . $2 . $period . $3 . $period . $4
+;
    print "$server_list[$i]\n";
   }
}

    [download]

    First suggestion: don't use indirect object notation. Someone else mentioned this advice in another post today and it's good advice worth mentioning again. So change

    $ip = new NetAddr::IP $User_Preferences{IP};
    to
    $ip = NetAddr::IP->new($User_Preference{IP});

    Second suggestion: Your for loop is the wrong way to do what you are trying to. You variable @server_list holds a list of the hosts within the subnet specified in $User_Preference{IP}. If you only specify one host, for instance, that array isn't going to have 255 different entries; it'll have one. What you really want to do is just iterate through the list. foreach my $server (@server_list) { ... } will do nicely.

    Third suggestion: All you are doing inside that loop is removing the "/32" off the hosts because NetAddr::IP::hostenum returns them in CIDR notation. Why not just use s!/32!!; rather than that god awful matching, capturing, and recombining? Even if you still wanted to do that, using $period = "."; is false hubris. There would be nothing wrong with: "$1.$2.$3.$4" but don't do it. Use s/// instead.

    Fourth suggestion: Quit worrying yourself about nested ifs and such. I've checked and found NetAddr::IP::hostenum does a fine job of handling CIDR notation for subnets greater than a class C.

    Fifth suggestion: Instead of having two separate functions for test() and restartapache() it would be better to just iterate over your server list once. I say so because opening an SSH connection to each host is not an inexpensive operation. Try it with one loop and check your flags to see what you should do after you make the connection.

    Sixth suggestion: Putting the username and password of a shell account in a config file is a bad idea. Perhaps you can avoid it by using ssh-agent? If not, be careful to keep the permissions on that config file tight.

    Seventh suggestion:: You assumed that if people didn't understand what you were thinking, they couldn't help you. The truth is that usually others can help if you put in the effort to understand what they are thinking. (I still have no idea why you would want to use nested ifs.)

    Edit: Minor reformatting. Changed "any bozo" to "anybody." Added seventh suggestion. 

    -sauoq
"My two cents aren't worth a dime.";
[reply]
[d/l]
[select]
Re: Auto-admin (finally, the code is here! rough draft)
by Zaxo (Archbishop) on Aug 27, 2002 at 20:46 UTC

    One test I'd suggest is `/usr/sbin/httpd -t` as a sanity check of your config writing. That won't mean that the config is correct, but it eliminates an easy source of error.

    After Compline,
    Zaxo

[reply]
Re: Auto-admin (finally, the code is here! rough draft)
by vxp (Pilgrim) on Aug 27, 2002 at 20:10 UTC
    here is the admin.conf for this thing.
    Read more... (608 Bytes)
[reply]
[d/l]

Back to Seekers of Perl Wisdom