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"; } }
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.
In reply to Re: Auto-admin (finally, the code is here! rough draft)
by sauoq
-sauoq
"My two cents aren't worth a dime.";
in thread Auto-admin (finally, the code is here! rough draft)
by vxp
For:
Use:
& & < < > > [ [ ] ]