Hi,
thx for advice, ye sit is part of code, code is working fine.
i will fix CAPS on variables soon. quote is fixed.
________________________________
From: Brandon McCaig <bamcc...@gmail.com>
To: Rajeev Prasad <rp.ne...@yahoo.com>
Cc: perl list <beginners@perl.org>
Sent: Tuesday, November 8, 2011 11:01 AM
Subject: Re: is this ssh happenign in parallel?
Hello:
I'm not familiar with Net::OpenSSH so I can't help with that, but
I can point out some questionable code and suggest improvements.
On Mon, Nov 07, 2011 at 03:56:17PM -0800, Rajeev Prasad wrote:
> my @hosts =qw(...); #all hosts
> my @allcmds = qw(...); #all commands
I don't think it really makes sense to use the qw operator for a
list of commands (unless the commands just happen to not require
arguments, but that's rare). :\ I'll just trust that you know
what you're doing. :P Otherwise, you should use Data::Dumper to
ensure that your @allcmds array is built as expected.
> for my $host (@hosts){ #loop to process all host in list
> chomp($host);
Since you are using the qw operator to assign @hosts it doesn't
really make sense to chomp it... I'll assume that you're actually
doing something different than what you've shown here.
> $ssh{$host} = Net::OpenSSH->new($host,
> port => $SSHPORT,
> user => $USER,
> password => $PASS,
> default_stderr_fh => $stderr_fh,
> default_stdout_fh => $stdout_fh,
> async => 1,
> master_opts => [-o => 'StrictHostKeyChecking=no',
> -o => 'ConnectTimeout 10'],
> );
> }
>
>
> open(MFOH,">>$MFRESULT");
You should generally use lexical file handles and the 3-argument
version of open. You should also be checking for and handling the
potential failure of open (e.g., with or die) (perhaps you're
using autodie instead, but it isn't shown so there's no way to
know).
> for my $host (@hosts) {
> foreach $CMD (@allcmds){
> chomp($CMD);
> ( @CMDRESULT, $CMDERR ) = $ssh{$host}->capture("$CMD");
These should probably be lexical variables (but it isn't clear
since parts of the program appear to be missing). That suggests
also that you aren't using the 'strict' pragma, but you should
be. It's possible that you've declared these variables globally,
but it doesn't seem warranted.
Also, you probably shouldn't be quoting $CMD here since IIRC it can
only be a string when assigned with qw. It's useless, potentially
wasteful, and in other circumstances potentially erroneous to
quote a variable.
Also, since @CMDRESULT is an array, it will consume all elements
of the list returned by Net::OpenSSH::capture. IOW, $CMDERR can
never get a value other than undef from the above assignment.
Lastly, it's distracting and probably bad style to use uppercase
identifiers here.
> print MFOH "\n$host COMMAND: $CMD\n";
> foreach my $line (@CMDRESULT) { print MFOH "$host $line"; }
> @CMDRESULT = "";
> if (defined $CMDERR ) {print MFOH "$host $CMDERR \n"; $CMDERR =
> "";}
This if statement is useless because as mentioned above $CMDERR
will never be defined.
> }
> }
> close MFOH;
>
> ....
HTH,
--
Brandon McCaig <bamcc...@gmail.com> <bamcc...@castopulence.org>
Castopulence Software <https://www.castopulence.org/>
Blog <http://www.bamccaig.com/>
perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'
