Hi jlc!
On Tuesday 02 Mar 2010 06:38:09 Joseph L. Casale wrote:
> Hi,
> I should state first I don't have the luxury of using Perl often
> and am super rusty:)
>
> I have a script that sends snmp commands to a switch either by passing
> args directly or as stdin. It works when passing them directly, but
> fails when feeding them as stdin, which is ultimately the way it needs
> to be run.
>
> I would be grateful if anyone could lend a suggestion, go easy on me if
> you see something silly, I'd be glad to take any pointers.
>
> The only pre-reqs for this are that arguments are passes as follows:
> argument=value
> and the various return values, mostly 0|1. The input being passed is as
> the spec requires, and has all the values I otherwise pass by dashed args
> directly.
>
> Thanks,
> jlc
>
I'm commenting on your code below with some general remarks. Not sure if this
will fix your problem.
>
>
> #!/usr/bin/perl
>
> # Sets switch port on a procurve up or down
>
> use Getopt::Std;
> use Net::SNMP;
>
You should add «use strict;» and «use warnings;» and fix all the problems that
they give you. This will make your script more robust.
Why are you using "Getopt::Std" instead of Getopt::Long? G::L is much more
recommended.
> my $OID_ifAdminStatus = '1.3.6.1.2.1.2.2.1.7.';
>
> # Get the program name from $0 and strip directory names
> $_=$0;
Ahmmm... please don't assign this to $_. $_ can be modified too easily.
Do:
<<<
my $progname = $0;
>>>
Or whatever.
> $|=1;
Using IO::Handle's Flush would be more readable and more idiomatic here.
> s/.*\///;
> my $pname = $_;
OK, so you did it. Do:
my $pname = $0;
$pname =~ s/.*\///;
But better use File::Basename , File::Spec and friends.
>
> sub usage
> {
> print "Usage:\n";
> print "\n";
> print "$pname [options]\n";
> print "\n";
> print "Options:\n";
>
> print " -s <string> switch address\n";
> print " -p <int> port\n";
> print " -o <string> action (off(default)|on|[status|
monitor])\n";
> print " -a <string> authProtocol (md5 or sha1)\n";
> print " -A <string> authPassword\n";
> print " -u <string> secName\n";
> print " -x <string> privProtocol (des, 3des, aes or aes128)\n";
> print " -X <string> privPassword\n";
> print " -h help\n";
> print " -V version\n";
> print " -d debug mode\n";
>
> exit 0;
> }
>
Use a here-document here.
> sub fail
> {
> ($msg)=...@_;
> print $msg."\n" unless !defined $opt_d;
> $t->close if defined $t;
> exit 1;
> }
You need my $msg (which would be caught if you used strict.
>
> sub fail_usage
> {
> ($msg)=...@_;
> print STDERR $msg."\n" if $msg;
> print STDERR "Please use '-h' for usage.\n";
> exit 1;
> }
>
>
> sub version
> {
> print "$pname 1.0\n";
> exit 0;
> }
>
> if (@ARGV > 0)
> {
> getopts("s:p:o:a:A:u:x:X:hVd") || fail_usage ;
>
> usage if defined $opt_h;
> version if defined $opt_V;
>
> fail_usage "Unknown parameter." if (@ARGV > 0);
>
> $address = $opt_s if defined $opt_s;
> $port = $opt_p if defined $opt_p;
> $action = $opt_o if defined $opt_o;
> $authProtocol = $opt_a if defined $opt_a;
> $authPassword = $opt_A if defined $opt_A;
> $secName = $opt_u if defined $opt_u;
> $privProtocol = $opt_x if defined $opt_x;
> $privPassword = $opt_X if defined $opt_X;
> }
> else
> {
> get_options_stdin();
> }
>
Please convert this to Getopt::Long.
> $action = "off" unless defined $action;
>
> fail "failed: no switch address defined" unless defined $address;
> fail "failed: no port defined" unless defined $port;
> fail "failed: no authProtocol defined" unless defined $authProtocol;
> fail "failed: no authPassword defined" unless defined $authPassword;
> fail "failed: no secName defined" unless defined $secName;
> fail "failed: no privProtocol defined" unless defined $privProtocol;
> fail "failed: no privPassword defined" unless defined $privPassword;
> fail "failed: unrecognised action: $action" unless ($action =~
> /^(off|on|status|monitor)$/i);
This seem like a lot of duplicate functionality to the options assignment.
>
> sub get_options_stdin
> {
> my $opt;
> my $line = 0;
> while( defined($in = <>) )
while (my $in = <>) would be enough here.
> {
> $_ = $in;
Don't assign to $_. You can also do «while (<>)» but please avoid abusing $_
in serious scripts.
> chomp;
>
> # strip leading and trailing whitespace
> s/^\s*//;
> s/\s*$//;
>
> # skip comments
> next if /^#/;
>
> $line+=1;
$line++; is better.
> $opt=$_;
> next unless $opt;
>
> ($name,$val)=split /\s*=\s*/, $opt;
>
> if ( $name eq "" )
> {
> print STDERR "parse error: illegal name in option
$line\n";
> exit 2;
> }
> # DO NOTHING -- this field is not used by fenced
> elsif ($name eq "agent" ) { }
> elsif ($name eq "ipaddr" ) { $address = $val; }
> elsif ($name eq "port" ) { $port = $val; }
> elsif ($name eq "option" ) { $action = $val; }
> elsif ($name eq "authProtocol" ) { $authProtocol = $val; }
> elsif ($name eq "authPassword" ) { $authPassword = $val; }
> elsif ($name eq "login" ) { $secName = $val; }
> elsif ($name eq "privProtocol" ) { $privProtocol = $val; }
> elsif ($name eq "passwd" ) { $privPassword = $val; }
> elsif ($name eq "debug" ) { $opt_d = $val; }
You should place all these variables inside a hash and avoid varvarname.
>>>
> # FIXME should we do more error checking?
> # Excess name/vals will be eaten for now
> else { fail "parse error: unknown option \"$opt\""; }
> }
> }
>
> # Get status of port.
> # Return 0 if 'up', Return 2 if 'down' or Return 1 if otherwise.
> sub get_status
> {
> my ($session, $error) = Net::SNMP->session(
> -hostname => $address,
> -version=> 'snmpv3',
> -username => $secName,
> -authprotocol => $authProtocol,
> -authpassword=> $authPassword,
> -privprotocol => $privProtocol,
> -privpassword=> $privPassword,
> );
>
> if (!defined $session) {
> printf "ERROR: %s.\n", $error
> unless defined $opt_d;
> exit 1;
> }
Why can't you use perldoc -f die here?
>
> my $result = $session->get_request(
> -varbindlist => [ $OID_ifAdminStatus . $port],
> );
>
> if (!defined $result) {
> printf "ERROR: %s.\n", $session->error()
> unless !defined $opt_d;
> $session->close();
> exit 1;
> }
>
> printf "Port '%d' for switch '%s' is set to '%s'.\n",
> $port, $session->hostname(), $result->{$OID_ifAdminStatus .
$port}
> unless !defined $opt_d;
>
Don't access the global $opt_d from within this function like that, please.
> $session->close();
>
> if ($result==1)
> { return 0; }
> elsif ($result==2)
> { return 2; }
> else { exit 1; }
> }
>
> # Set switch port 'up' or 'down'
> # Return 0 if the status is as set, or non-zero on failure.
> sub set_status
> {
> print "About to set status as: @_\n" unless !defined $opt_d;
> my ($session, $error) = Net::SNMP->session(
> -hostname => $address,
> -version=> 'snmpv3',
> -username => $secName,
> -authprotocol => $authProtocol,
> -authpassword=> $authPassword,
> -privprotocol => $privProtocol,
> -privpassword=> $privPassword,
> );
>
> if (!defined $session) {
> printf "ERROR: %s.\n", $error
> unless defined $opt_d;
> exit 1;
> }
>
> my $result = $session->set_request(
> -varbindlist => [ $OID_ifAdminStatus . $port, INTEGER, @_ ],
> );
>
> if (!defined $result) {
> printf "ERROR: %s.\n", $session->error()
> unless !defined $opt_d;
> $session->close();
> exit 1;
> }
>
> printf "Port '%d' for switch '%s' was set to '%s'.\n",
> $port, $session->hostname(), $result->{$OID_ifAdminStatus .
$port}
> unless !defined $opt_d;
>
> $session->close();
> return 0;
> }
>
> # Perform Action
> if ($action =~ /^(status|monitor)$/i)
> {
> if (get_status==0)
> {
> exit 0;
> }
> else
> {
> exit 1;
> }
> }
> elsif ($action =~ /^off$/i)
> {
> if (set_status(2)==0)
> {
> exit 0;
> }
> else
> {
> exit 1;
> }
> }
> elsif ($action =~ /^on$/i)
> {
> if (set_status(1)==0)
> {
> exit 0;
> }
> else
> {
> exit 1;
> }
> }
> else
> {
> die "unknown action: $action";
> }
There's a lot of duplicate code here. Consider doing this:
sub myexit
{
my $status = shift;
exit($status == 0 ? 0 : 1);
}
And then do «myexit(get_status())» etc.
Regards,
Shlomi Fish
P.S: incidentally JLC is the Jerusalem Linux Club.
--
-----------------------------------------------------------------
Shlomi Fish http://www.shlomifish.org/
Optimising Code for Speed - http://shlom.in/optimise
Deletionists delete Wikipedia articles that they consider lame.
Chuck Norris deletes deletionists whom he considers lame.
Please reply to list if it's a mailing list post - http://shlom.in/reply .
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
