Re^2: RFC: beginner level script improvement (various comments)

[smls]

Some random comments on your code follow. Please keep in mind that I only casually perused (parts of) your script, so don't consider this a proper code review by any means. I also don't really understand much of the underlying functionality, so this is restricted to Perl-specific aspects of the code.

storing config options in a list of scalars vs. a hash

In process_command_line_args you initialize a long list of variables, and pass them back as an array reference:

sub process_command_line_args {
    #set default values for dynamic config settings
    my ($layout,$wait_for_child,$wait_for_exec,$prompt,$configf,$quiet
+,$outf,$debug,$batchsize,$proto,$tacacs,$verbose,$header,@maildst) = 
+(0,120,3,1,"$name.cfg",1,"$name.csv",0,0,"none",0,0);

    GetOptions ( ...)
    ...

    my @args = ($layout,$wait_for_child,$wait_for_exec,$name,$prompt,$
+configf,$quiet,$outf,$debug,$batchsize,$proto,$tacacs,$verbose,$heade
+r,$mailref,$path);
    return \@args;
}

Then in various subs (Main, validate_command_line_args, populate_config_hash) you access its elements via array subscripts and by repeatedly unpacking it into a long list of scalars again.

This seems like a really cumbersome way to deal with those values (making your script more difficult to maintain).
A hash would serve them much better I think, especially since in populate_config_hash you end up creating the %CONFIG hash from them anyways.

One possibility would be to use %CONFIG right from the start:

sub Main {
    my %CONFIG;
    initialize_config_hash(\%CONFIG);
    get_command_line_args(\%CONFIG);
    validate_command_line_args(\%CONFIG);
    get_username_and_password_from_stdin(\%CONFIG) if $CONFIG{prompt};
    
    ...
}

sub initialize_config_hash {
    my $CONFIG = shift;
    %$CONFIG = (
        quiet          => 0      , cfg       => "$name.cfg"         ,
        verbose        => 0      , outcsv    => "$name.csv"         ,
        debug          => 0      , proto     => "none"              ,
        prompt         => 1      , header    => undef               ,
        layout         => 0      , maildst   => []                  ,
        wait_for_child => 120    , mail_to   => ""                  ,
        wait_for_exec  => 0      , mail_from => "[e-mail]" ,
        batchsize      => 0      , mail_sub  => ""                  ,
        tacacs         => 0      , mail_cc   => ""                  ,
        tacuser        => "user" , mail_data => ""                  ,
        tacpass        => "pass" ,
    );
}

sub get_command_line_args {
    my $CONFIG = shift;
    GetOptions (
        'layout|l'            => \$CONFIG->{layout},
        'file-based-auth|f'   => sub { \$CONFIG->{prompt} = 0; },
        'prompt-based-auth|p' => sub { \$CONFIG->{prompt} = 1; },
        ...
    )
    ...
}

calling 'next'/'last' to break out of a callback sub...

...is not a good idea.

In process_device you do:

$WARN = 0; #supress flow control warning in following block due to exi
+ting anon sub
LOOPCTRL: while ($lb < 10){
    $exp->expect(...
        [ ..., sub {...
            $lb++;
            next LOOPCTRL; }],
        ...
    );
    $exp->expect(...
        [ ..., sub {...
            last LOOPCTRL; }]
    );
}
$WARN = 1; # reset to default

The warning is there for a reason: The callback now never returns to the code that called it (in this case, Expect.pm), so if that code needs to do any cleanup etc. before passing control back to you, that will be skipped which might lead to hard-to-track bugs.

I don't have hands-on experience with the Expect.pm module, but the documentation suggests that expect() returns a positive number on success and undef otherwise, so it seems to me that this loop could be re-written like so:

while (...) {
    next if $exp->expect(...
        ...
    );
    last if $exp->expect(...
        ...
    );
}

Suppressing the warning becomes unnecessary, and so is the "LOOPCTRL" label btw.

(Also make extra sure that those expect calls + loop control really result in the intended behavior... I can't help you there, because I have no understading of the external program you're controlling.)

suppressing warnings

In my experience, suppressing core Perl warnings is rarely the best solution - usually they are an indicator that what you're trying to do can be done in a better/safer way.
But even when you do want to temporarily suppress Perl warnings, using a global variable together with a global signal handler is not the proper way to do it. Instead, simply disable the "warnings" pragma within an as-small-as-possible lexical scope:

use strict;
use warnings;

# warnings coming from here will be printed

{
  no warnings;
  
  # warnings coming from here will NOT be printed
}

# warnings coming from here will be printed

Or, even better, only turn off specific warning categories instead of all at once - for example:

  # Don't warn about exiting subs via loop control:
  no warnings 'exiting';
  
  # Don't warn about using undef in numeric or string operations:
  no warnings 'uninitialized';

To suppress warnings emitted by modules (rather than Perl itself), first look if the module provides an option to turn them off. If not, overriding the warn signal handler is the way to go, but this too is better done locally instead of for the whole program:

{
  local $SIG{__WARN__} = sub {};
  
  # call the noisy module function here
}

# warnings coming from here will not be affected

referencing + dereferencing

I see some contructs like this in your code:

\%{ $CONFIG }
\%$CONFIG

You can just write $CONFIG instead. The % dereferences the hash ref, but then the \ takes the reference again - i.e. they cancel each other out.
Of course if you're doing it on purpose, to remind yourself that what is being passed along is a hash ref - then don't let me keep you from doing so... :)

creating temporary files

In populate_config_hash you manually generate a hopefully-unique temporary file name like this:

my $range = '1000000';
my $rand = int(rand($range)); ##generate random temp-db filename to al
+low for simultanous script execution

...
  tempdb => "$path/$rand.db",

There is a module for safely creating temporary files: File::Temp (which is part of Perl since v5.6.1)

unpacking a subroutine argument

At the beginning of subroutines that only take one argument, you tend to write:

my $foo = $_[0];

There's nothing wrong with this technically, but I thought I'd point out that the following idioms are much more commonly used for that:

my $foo = shift;

my ($foo) = @_;

Using shift for this may seem particularly strange to those new to Perl, but it has a lot of tradition and is also very fast.

Edit: Fixed typo ("shift" not "unshift")

[georgecarlin]

First of all thank you very much for taking the time to read through and comment on the script.

I was very unhappy with the locally disabling warnings thing to begin with but failed to come up with a better solution and the entire loop construct is required so the different device output cases are tested and responded accordingly, with respective timeouts and further actions.
Using the return value of the expect method itsself hadn't occured to me at all, which is very sad considering how obvious it should have been (always is in hindsight when someone else did the thinking for you). Thank you very much for this hint. I will change that section and remove the ugly warnings thing.

I like some of the "useless" \%{$} and a-like constructs (as long as they don't constitute mistakes) as well as captitalization of some vars because it helps me keep track of where what comes from and why, what it contains and who accesses it, purely by naming and how it is referred to. However I still haven't fixed all inconsistency mistakes in the system.


The config-hash related changes you propose make a lot more sense than what I'm doing and I'll change the respective parts. Thank you very much for the pointers.
Same goes for the tempfile part.

regarding the unpacking a sub argument part
I read the unshift and sub documentation again and I still don't understand your hint.
As far as I understand the @_ is a list that contains all arguments the sub was called with and consequently $_[x] would contain the element of that array with index x. I would understand using any of the three below because they produce the identical result in regards to $bar_ref, (while I think shift only makes sense if something might or might not be present at a specific index after 0)

foo(\%bar);

sub bar {
    my $bar_ref = $_[0]; #I know what is at index 0 and that's what I 
+want
    my ($bar_ref) = @_; #same as above but I can not process anything 
+else from the list
    my $bar_ref = shift; # same as #1 but I want to continue processin
+g @_ and am unsure about indexes and/or presence
}

But the unshift thing I don't understand at all.

[smls]

"I read the unshift and sub documentation again and I still don't understand your hint."

Ah I meant shift of course, apologies for the confusion.




As far as I understand the @_ is a list that contains all arguments the sub was called with and consequently $_[x] would contain the element of that array with index x.

Correct.




while I think shift only makes sense if something might or might not be present at a specific index after 0
...
I want to continue processing @_ and am unsure about indexes and/or presence

Not sure what you mean by that. shift removes the first value off the list, so all remaining elements of the list move one index to the left (that's why it's called "shift"):

my @array = (2, 4, 6, 8);

my $first = shift @array;
# $first is 2
# @array is (4, 6, 8)

my $second = shift @array;
# $second is 4
# @array is (6, 8)
# $array[0] is 6
# $array[1] is 8

It works exactly the same when used on @_ (which is the default when shift is called without argument).




my ($bar_ref) = @_; #same as above but I can not process anything else from the list

Not sure what you mean by that, either. Unlike shift, the assignment does not modify @_. Its effect is identical to that of:
my $bar_ref = $_[0];
It's just a more "stream-lined" way of writing it.

When both sides of an assignment are lists, Perl goes from left to right, assigning the first element of the RHS ("right-hand-side") to the first element of the LHS, then the second RHS element to the second LHS element, and so on, for as many elements as there are on the LHS:

my ($first) = (2, 4, 6, 8);
my ($first, $second) = (2, 4, 6, 8);

# an array on the LHS will "eat up" all remaining elements:
my ($first, $second, @remaining) = (2, 4, 6, 8);

# undef can be used to "skip" elements you don't want to assign
# to anything:
my (undef, undef, $third) = (2, 4, 6, 8);

[georgecarlin]

As I said, I'm a self-taught perl user and best considered beginner level =)


I tried explaining my reasoning behind how I accessed the args in subs.
I've been looking into oop because in this script there's a lot of expect usage and I want to create re-uasable and well structured subs. (currently that'snot the case)
Below is a case where I have used shift.

sub set_timeout {
    my $self = shift;

    my $to = $self->{timeout};

    if (@_){
        $self->{timeout} = shift;
        
    }

    return $to;    
}

In this case shift makes sense imo, although it could have just as well been written like this.


sub set_timeout {
    my $self = $_[0];

    my $to = $self->{timeout};

    if ($_[1]){
        $self->{timeout} = $_[1];
        
    }

    return $to;    
}

The reason I didn't use the index based assignment is that shift feels better here and in the documentation I read about oop they exclusively used shift accessing. However in a different scenario like below I don't think using shift in the if clause makes sense.


sub set_stuff {
    my $self = shift;
    
    if (@_){    
        $self->{stuff1} = shift;    #3 lines for one task
        $self->{stuff2} = shift;
        $self->{stuff3} = shift;
        ($self->{stuff1},$self->{stuff2},$self->{stuff3}) = @_; #1 lin
+e, same result regardless of stuff presence
    }

    return 1;    
}

So what I'm saying is I used shift when I felt like it without having a clue whether it's faster or better than index based assignment. =)