PrimeLord has asked for the wisdom of the Perl Monks concerning the following question:

Fellow monks I was hoping some of you could take a look at a piece of code I wrote earlier today and give me some feedback on it.

The code will provide a Unix user with a very short menu to either establish a ppp connection or logoff the system. The code will be run out of the users .login. I am wondering if there is a better way to accomplish this task then what I have come up with and if anyone can see any ways that I have missed to break out of this menu. Thanks for any help and suggestions you can offer. 

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

use strict;

sub main_block {
        my $username = getlogin;
        my $ppid = getppid;
        my $result = generate_menu($username);
        until ($result == 1 || $result == 2) {
                print "Invalid option.\n";
                sleep 2;
                $result = generate_menu($username);
        }
        user_choice($result, $ppid);
}

sub generate_menu {
        my $username = shift;
        system("clear");
        print "Welcome $username\n\n";
        print "Please chose from the following options:\n";
        print "1) Establish PPP Session\n";
        print "2) Logoff\n";
        print "------------------------\n";
        print "Choice: ";
        chomp (my $result = <STDIN>);
        return $result;
}

sub user_choice {
        my ($result, $ppid) = @_;
        if ($result == 1) {
                print "\nServer to connect to: ";
                chomp (my $server = <STDIN>);
                ppp_connect($server);
        } else {
                kill 9 => $ppid;
        }
}

sub ppp_connect {
        my $server = shift;
        system("clear");
        system("ppp -direct $server");
        main_block;
}

$SIG{INT} = 'IGNORE';
$SIG{TSTP} = 'IGNORE';
main_block;

[download]


-Prime

Replies are listed 'Best First'.
Re: Shell Menu Code Review
by sauoq (Abbot) on Jul 25, 2002 at 19:08 UTC

    I just glanced at it, but you aren't catching or ignoring SIGQUIT. Users could probably drop out of it with a ^\.

    Update: Ok in addition to that, you get $server directly from the user and then use it in double quotes in a system call. That's a huge no-no. Consider what might happen if your user enters: || /bin/sh when asked for the server. 

    -sauoq
"My two cents aren't worth a dime.";
[reply]
[d/l]
[select]
Re: Shell Menu Code Review
by Chmrr (Vicar) on Jul 25, 2002 at 19:14 UTC

    If this is meant to restrict user access (ie users are not allowed a command line) then you might want to be more leery about your system("ppp -direct $server") -- what if they type "some.server.org && bash" as the server to connect to? The multiple argument form of system will tend to avoid such problems.

    Other than that, looks good.

    perl -pe '"I lo*`+$^X$\"$]!$/"=~m%(.*)%s;$_=$1;y^`+*^e v^#$&V"+@( NO CARRIER'

[reply]
[d/l]
      saouq and Chmrr,

      Thanks for pointing that out. I hadn't even thought about the fact they could pass anything they wanted into that system call. I am going to change that so it just presents them with a list of servers to connect from.

      -Prime
[reply]
Re: Shell Menu Code Review
by hossman (Prior) on Jul 25, 2002 at 19:21 UTC
    One thing that jumps out at me...

    There's a lot of really tight connections between your subs: main_block assumes that there are exactly 2 options, user_choice assumes it knows exactly what option 1 and option 2 are, etc.... Subs are really usefull from a design perspective because they setup a "black box" that will take some input, perform some actions, and produce some output. But by interconnecting them so much, you're losing all of their power -- no one can ever modify one of your subs without touching all of the other ones -- which kinda defeats the point.

    That's not to say that they way you are doing things is inheriently bad given the scope of your script. I just think you are missleading anybody who ever reads your code in the future. You're script would accutally be more readable if you eliminated the subs, and had a basic sequential series of code.

[reply]
      I think the subs are useful but hossman is right about how you set the subs up
      Here's a sample script that I came up with and maybe you can tinker with it a little so you can have two flavors, this code did work on linux but I only recently tested in windows Xp, hope it helps! 
      #!/usr/bin/perl -w
use strict;

my $choice = ""; 

# main program
while ($choice !~ /q/i) {
    $choice = &mainmenu();
  SWITCH: {
      $choice =~ /^1/ && do { &option1(); last SWITCH; };
      $choice =~ /^2/ && do { &option2(); last SWITCH; };
  }
}

# main menu
sub mainmenu {
    my $input = "";
    print "\nUTILITIES\n\n";
    print "Please choose from the following options (or Q to quit):\n\
+n";
    print "1. Establish PPP Session\n";
    print "2. Logoff\n";
    print "-------------------------\n";
    while () {
    print "\nYour choice --> ";
    chomp($input = <STDIN>);
    if ($input =~ /^\d$/ || $input =~ /^q$/i) {
        return $input;
    } 
    else {
        print "Not a choice.  1-2 or Q to quit, please,\n";
        }
    }
}
# connect to PPP session
sub option1 {
    print "Connecting to PPP session\n";
}
# log off session
sub option2 {
    print "Logging off\n";
}

      [download]
[reply]
[d/l]
      I see what you mean. I may have to see if I can play with that. I wrote the subs like that so I would have an easy way to regenerate the menu if they chose a wrong option or once the ppp exited, but I guess I could just do that with format too.

      Thanks,
      -Prime
[reply]
Re: Shell Menu Code Review
by greenFox (Vicar) on Jul 26, 2002 at 02:23 UTC
    On top of what others have said-
    I suggest turning on Perl's taint mode (-T) see perlman:perlsec for the details. With your code as is taint mode will barf unless you have sanitized $server. Taint mode will also pick up another potential security hole in your code - you call "clear" without specifying a path, which means that the first program called clear found in the search path will be executed, which may not be clear at all :) I always undefine $ENV{PATH} and specify the full path to each program I want to call. A tip I picked up from the Perl Cookbook:

    my $clear=`/path/to/clear`;
# and then whenever you want to clear the screen-
print $clear;

    [download]

    --
    Until you've lost your reputation, you never realize what a burden it was or what freedom really is. -Margaret Mitchell

[reply]
[d/l]
Re: Shell Menu Code Review
by Sifmole (Chaplain) on Jul 26, 2002 at 11:35 UTC 
            print "Welcome $username\n\n";
        print "Please chose from the following options:\n";
        print "1) Establish PPP Session\n";
        print "2) Logoff\n";
        print "------------------------\n";
        print "Choice: ";

    [download]
    One thing that shows up again and again are blocks of print statements like this; and of course Perl has an easier way for you to do it. For instance, you can chain print statements together with commas: 
            print "Welcome $username\n\n",
         "Please chose from the following options:\n",
         "1) Establish PPP Session\n",
         "2) Logoff\n",
         "------------------------\n",
         "Choice: ";

    [download]
    And of course Perl will allow you to get rid of all those extra quotes and commas: 
    print "Welcome $username

Please chose from the following options:
1) Establish PPP Session
2) Logoff
------------------------
Choice: ";

    [download]
    Or probably even better, use a HERE doc 
    print <<EOP;
Welcome $username

Please chose from the following options:
1) Establish PPP Session
2) Logoff
------------------------
Choice:
EOP

    [download]
[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom