If you're defining your hash before you've set the value of $file etc you're not going to get a useful command line (and were you using strict and warnings it would have griped at you to boot). Print out the string you're passing to system and you'll see the problem.

I suspect your key issue is that you are expecting that the variables in the command strings will get substituted at the time system is called. They don't. They get substituted at the time the hash is constructed. Instead you may want to use place holders and substitute the actual parameters in at system execution time. There are many ways you could achieve that - here's one:

use strict;

my %systemcmd = (
    add       => "addqueue -h P0 -q P1 -i 3",
    remove    => "removequeue -f -q P0",
    status    => "lpstat -pP0",
    cancel    => "cancel -e P0",
    add_slurp => "addqueue -h P0 -q P1 -i 3"
);

my $file = <<FILE;
add wibble ping

cancel wobble
flibble one
FILE

open INFILE, '<', \$file;

while (<INFILE>) {
    chomp;
    next unless length;

    my ($command, @params) = split;

    unless (exists $systemcmd{$command}) {
        warn "Unrecognised command: $command in line $.\n";
        next;
    }

    my $cmdline = $systemcmd{$command};

    $cmdline =~ s/(P$_)/$params[$_]/e for 0 .. $#params;
    print qq{system ("$cmdline");\n};
}

close INFILE;
[download]

Prints:

system ("addqueue -h wibble -q ping -i 3");
Unrecognised command: flibble in line 4
system ("cancel -e wobble");
[download]

Aside from that there are some major issues in your code. In particular you use the variable $file in about six different ways. The intent of your code is not at all clear and the chance that it is correct is 0%.

if ($file) {
    ...
    open my $file, "< $file",
      or print &reasons();
    ...
        foreach my $file (@all_lines) {
[download]

Why do you have three different variables named $file?

    while (<$file>) {
        my @all_lines = <$file>;
        foreach my $file (@all_lines) {
[download]

When you enter the while loop the first line is read into the $_ variable and then the rest of the lines are read into the @all_lines variable. @all_lines does not contain all of the lines because the first line is missing.

It looks like you may want something like this:

my %systemcmd = (
    add       => sub { 'addqueue', '-h', $_[ 0 ], '-q', $_[ 0 ], '-i',
+ '3' },
    remove    => sub { 'removequeue', '-f', '-q', $_[ 0 ] },
    status    => sub { 'lpstat', "-p$_[0]" },
    cancel    => sub { 'cancel', '-e', $_[ 0 ] },
    add_slurp => sub { 'addqueue', '-h', $_[ 0 ], '-q', $_[ 0 ], '-i',
+ '3' },
    );


if ( $file ) {
    $file = lc $file;
    open my $FH, '<', $file or die reasons();
    print "Please wait... Reading from the file... \n";
    sleep 5;

    <$FH>;  # remove first line
    while ( my $line = <$FH> ) {
        if ( -e "/var/spool/lp/request/$line" ) {
            die "This printer is already defined!\n";
            }
        else {
            print "Adding queue \"$line\"\n";
            sleep 1;
            my @args = $systemcmd{ add_slurp }->( $line );
            0 == system @args or warn "system @args failed: $?";
            }
        }
    close $FH;
    exit 0;
    }
[download]

I appreciate everyone willing to help! I have tried many different methods and I'm still having issues dealing with Perl reading the file and dealing with addqueue properly. The reason why I had $file mentioned so much probably is silly to you guys but I'm using Getopt::Long, I setup an option called -f which stores as \$file, if $file is called then I'll execute my code, unfortunatly I couldn't see another way to open the file handle other than $file since of course the string rely's on <STDIN>.

The purpose of this program is to make the process of adding print queues standard and seemless, (we have various flavors of *nix), if I can make it intuitive enough; I can pass the workload to the applications teams :).

I wanted to have the functionality of a user throwing a text file in wherever they want and then call the option. Such as mkpq -f /tmp/printers.txt, of course the /tmp/printers.txt will be stored as $file and that's why it was mentioned so much.

I am a new Perl programmer I'm trying to read as much as possible but with no programming background it makes it tough to do things correct. Albeit I do appreciate constructive criticism, I'll keep poking around with this chunk of code, as the rest of my program works great!

Thanks again Monks.

Here is the product of your guys' advice, thanks again!



if ($file) {
    lc($file);
    if ( -e "$file" ) {
        my $fh;
        open( $fh, '<', $file or die &reasons() );
        my @lines = <$fh>;

        foreach my $q (@lines) {
            chomp($q);
            if ( -e "/var/spool/lp/request/$q" ) {
                die "Printer \"$q\" is already defined!\n";
            }
            else {
                print "Adding queue \"$q\": ";

                # "addqueue -h QUEUENAME -q QUEUENAME -i 3"
                my %file_cmd = ("addqueue -h $q -q $q -i 3");
                system( $file_cmd {'$file_cmd'} );

            }
        }

        close($fh);

    }
    else {
        &reasons();
    }

}
[download]