Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl Monk, Perl Meditation
 
PerlMonks  

Shell Menu Code Review

by PrimeLord (Pilgrim)
on Jul 25, 2002 at 18:52 UTC ( [id://185282]=perlquestion: print w/replies, xml ) Need Help??

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;


-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.";
    
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'

      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
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.

      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"; }
      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
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;

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

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: ";
    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: ";
    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: ";
    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

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://185282]
Approved by djantzen
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others sharing their wisdom with the Monastery: (5)
As of 2024-03-29 12:15 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found