persistent variables between subroutines (long)

[Tuna]

I have the dubious task of taking someone else's code, and making it work using warnings and strict pragma. The problem is (aside from not using strict and -w)that this person created variables which persist from subroutine to subroutine. Here's an example:

#!/usr/bin/perl

$program = "cluster_f$%k";
@list = ("Maleah");
$sshPort = 22;
require "flush.pl";

@taskSequence =
  (
   'establishSession~~connecting to $clusterHost'
);

foreach $clusterHost (@list) {

$executionStatus = &executeTaskSequence($clusterHost, @taskSequence) |
+|
$totalErrorCount++; 

 print "\n$prog_name: aborted reactivation of $clusterHost\n\n";
 
if  ($executionStatus || $cleanUpStatus ) {
    $totalErrorCount++;
    
    print "\n$prog_name: aborted reactivation of $clusterHost\n\n";
  }
  else {
    print "\n$prog_name: copied all files from $clusterHost\n\n";
}

if (@list) {
  
  local ($operations) = "host";
  
  printf "\n$prog_name statistics:\n";
  printf "    %d ${operations}s attempted\n", scalar(@list); 
  printf "    %d $operations(s) succeeded\n", @list - $totalErrorCount
+;
  printf "    %d $operations(s) failed\n", $totalErrorCount;
 }
#############################################################
sub establishSession {
  
  local @sshPort;
  
  if ($clusterHost =~ s/:(\d+)$// || $sshPort =~ /(\d+)/) {
    @sshPort = ("port", $1);
  }
  
  &xferOpen (SESSION, $clusterHost, "compression", "yes", @sshPort) ||
    return &error ("couldn't connect to $clusterHost: $xferError");
   
  return $success;
}
#############################################################
sub xferOpen
  {
  local ($sessionHandle, $remoteHost, %options) = @_;

  if ($xferSessionInfo{$sessionHandle})
     {$xferError = "session already open"; return 0}

  # find fastest supported cipher for host
  foreach $cipher (@xferSshCiphersToTryInOrderOfSpeed)
    {
      local ($xferError);
      $xferSessionInfo{$sessionHandle} =
    join("~", "host", $remoteHost, "cipher", $cipher, %options);

        &xfer_ssh("/bin/true") || return 1;
    }

  return 0;
  }    
#############################################################
sub xfer_ssh
  {
  local ($remoteCommand) = @_;
  local ($command, *READ, *WRITE, *ERROR, @error, $pid);

  local (%opt) = split(/~/, $xferSessionInfo{$sessionHandle});

  if (! %opt)
     {$xferError = "invalid session handle"; return 0}

  $command = $xferSshCommand;

  if ($opt{'port'})        {$command .= " -p $opt{'port'}"}
  if ($opt{'username'})    {$opt{'host'} = "$opt{'username'}\@$opt{'ho
+st'}"}
  if ($opt{'identity'})    {$command .= " -identity $opt{'identity'}"}
  if ($opt{'cipher'})      {$command .= " -c $opt{'cipher'}"}
  if ($opt{'compression'} eq "yes") {$command .= " '-oCompression yes'
+"}
  if ($opt{'compression'} eq "no")  {$command .= " '-oCompression no'"
+}

  $command .= " $opt{'host'} $remoteCommand"; print "command var in xf
+er_ssh = $command\n";

  $debug && print "xfer.pl: spawning $command\n";

  $pid = &open3 (WRITE, READ, ERROR, $command);

  close (WRITE);

  @output = <READ>;

  @error = <ERROR>;

  close (READ);

  close (ERROR);

  waitpid($pid, 0);

  $? || return 1;

  $xferError = "@error";print "xfererror = $xferError\n";

  return 0;
  }
#############################################################
sub error
  {
  local ($_) = @_;

  print "$program: ERROR: $_\n";

  return $failure;
  }

#############################################################
sub executeTaskSequence
  {
  local ($objectName, @taskSequence) = @_;
  local ($errorCount);

  foreach (@taskSequence)
    {
    ($task, $executionConstraints, $taskDescription) = split(/\s*~\s*/
+, $_);

    if (! $executionConstraints || eval "$executionConstraints")
       {
       if ($reportProgress && $taskDescription) 
          {&reportProgress ($objectName, $taskDescription)}

       &flush (STDOUT);

       &$task ($objectName) && ++$errorCount && last
       }

        &flush (STDOUT);
    }

  return ($errorCount)
  }
#############################################################
#sub reportProgress
  {
  local ($objectName, $taskDescription)=@_;

  if ($taskDescription = eval "\"$taskDescription\"")
     {printf "\n$program: %s (%s)\n", $taskDescription, $objectName}
  }
#############################################################

The code above, run as is, (assuming I haven't forgot to include all variables =) ), will attempt to establish an ssh connection to localhost (for testing). However, I need to incorporate this code, and about 800 lines just like it into another program, which runs using warnings and strict pragma.

The problem I am having, is sharing variables between each subroutine. I don't know how to do it.
For example, in &xferOpen, I need to make %xferSessionInfo available to &xfer_ssh, and a few other subroutines. If someone, could explain how to make variables available throughout each subroutine, and possibly some general advice regarding transposing code which was written without -w and strict, into a program using -w and strict. That would be MUCH appreciated!

Thanks,
Steve

[chipmunk]

To make variables persistent between subroutines, you can simply declare them outside the subroutines:

#!/usr/local/bin/perl -w

use strict;

BEGIN {
my $static = 0;

sub inc {
  ++$static;
}

sub dec {
  --$static;
}

}

$, = $\ = "\n";
print inc(), inc(), dec(), dec();

The block creates a private scope, so that $static is available only to the inc() and dec() functions. Adding BEGIN before the block ensures that $static is initialized before the subroutines are called.

[premchai21]

If you're using perl 5.6, then this is exactly what the 'our' statement is for. It, like my, is local at the block level, but it accesses package variables rather than creating lexicals. So, for instance:

sub init
{
    our ($foo);
    $foo = 0;
}

sub inc
{
    our ($foo);
    $foo++;     # same variable, $main::foo, as in init
}

But:

sub dec
{
    $foo++;     # not allowed because $foo not declared
}

So then, suppose you say:

init(); inc(); inc(); inc();
($main::foo == 3) && print "Three!\n";

would print "Three!".

[mugwumpjism]

Here's the general process.

Go through all of the subroutines and ensure that every global variable that should be a parameter to the function is actually passed as a parameter. Also ensure that all return information from the function is returned via return.

You now have a whole load of parts that you can prove individually must work, if their inputs are sane. This is good.

If you're confused about how to keep information persistent between subroutine calls, and you "outgrow" simple use of "return", here is my advice: Pass a reference to a data structures holding the information you want as the first argument to a function. eg:

sub xfer_ssh {
    my ($xferSessionInfo, @other_args) = (@_);

    # set something in %xferSessionInfo - note use of the
    # -> operator to dereference.
    $xferSessionInfo->{foo} = "bar";

    # exactly the same thing, written a way that some find
    # easier to grok.
    ${ $xferSessionInfo }{foo} = "bar";

    return "ok";
}

my %xferSessionInfo;

&xfer_ssh(\%xferSessionInfo, @other_args);

print $xferSessionInfo{foo};   # prints "bar"

Group your functions by what you are expecting to use as this first argument. eg, all of the functions that deal with "%xferSessionInfo" hashes together. Where possible, only make changes to the state variable that is the first argument to the function.

Practice this coding style for a while; it should be well within your reach. After you have mastered this technique, then read the perltoot man page (I wouldn't recommend reading it just yet).

[John M. Dlugosz]

Pass a reference to a data structures holding the information you want as the first argument to a function. ... Group your functions by what you are expecting to use as this first argument. eg, all of the functions that deal with "%xferSessionInfo" hashes together. Where possible, only make changes to the state variable that is the first argument to the function.

That is getting very close to using Perl Objects. Next step is to start blessing that hash.

[mugwumpjism]

Sshhh!! Don't confuse him yet :-) Didn't you read the last paragraph of my post?

[simon.proctor]

another method (than the change globals to parameters way) is to create an object which stores regularly used functionality and data from your programs. Naturally, this assumes that the coder may have repeated code all over the place.

You can then add this 'functionality object' to all your parameters or make it 'our' as you see fit. Call the accessor methods as required and generally make the program a bit smaller.

I'm not saying this is the best way but if I were presented with a long and badly written program I'd look to simplify it a little. You are essentially re-writing it anyway.

[perlknight]

Steve, Does this script automatically established a secure connection, since it uses port 22?

Thanks,
Alex

[Tuna]

Actually, in production, we will tie it to an obscure port. Since I am currently testing all of this locally, there's no reason not to use the ephemeral port.

UPDATE: Were you asking if that was what this program does? Yes, amongst many other functions, it will establish a non-interactive SSH/SCP session.

[John M. Dlugosz]

Many have said how you should do it. But here are some tips in migrating what you already have, after adding use strict and -w.

First of all, use warnings; too. This is lexical and selective. When in use, that region of code ignores -w and does what you said instead. So, you can turn off specific warnings just around the passages of "old" code that have not been cleaned up yet.

Likewise with use strict;. You can start by simply heeding the text of the message and qualifying the name. Then, anything that doesn't pass but isn't immediatly fixable can be worked-around by using no strict 'refs'; or whatever type of strictness you need to turn off, just around the line or lines in question.

Then, after it's stable again, you can revisit each section of "no..." and alter the code bit by bit, maintaining a running version all along.

—John