Re^3: Getopt question again

My code is finished.. Please give me some advise on what I did wrong(it is working code)... or what i didn't do it correctly or efficiently.. thanks guys!

#!/usr/bin/perl -w

use strict;
use diagnostics;
use Getopt::Long;


my @servers = qw(server1 sever2 server3 sever4);
my $help = 0;
my ($callid,$dn,$inbound);

sub usage {
         print <<"NL";

         usage: $0 sip/h323 [-h] -c <callid> -d <dialed number> -i <in
+bound id>

        syntax:
                --help -h
                       prints this help text and exists
                --callid -c
                       allows searching in SBC by callid ; will output
+ of calls and siptool -v
                --dn -d
                       allows searching in SBC by dialed number; will 
+output last found call from each server if applicable
                --inbound -i
                       allows searching in SBC by inbound ID ; will ou
+tput last found call from each server if applicable
                --time -t { Future implementation }
                       allows searching callid/dn/inbound in SBC by ho
+ur ; in 00:00 to 23:59 format
                       To search between 2PM and 3PM, 14:00-15:00

       example: $0 sip --dn 12125551212   or $0 sip  -d 12125551212
                $0 h323 --callid 12341234 or $0 h323 -c 12341234
                $0 sip --inbound 333333    or $0 sip  -i 33333

NL
         exit 0;

}

sub san_check_arg {
        usage () if $#ARGV  != 0;
        usage () if $ARGV[0] !~ /\bsip\b|\bh323\b/i;
        return ('CALLID', $callid)             if defined($callid);
        return ('DN',$dn)                      if defined($dn);
        return ('INBOUND ID',$inbound)         if defined($inbound);
}

sub wr_temp {
         my $wr_t = shift;
         open FF, ">/tmp/file_123456";
         print FF "$wr_t";
         close FF;
}

sub proc_it {
          my $proc = shift;
          my @meth =
                     $_[0] eq 'CALLID' ?  'CALLID' :
                     $_[0] eq 'INBOUND ID' ? 'INBOUND ID' : '';
          my $file_f;
          my ($h323_r,$sip_r);
          if ("$proc" eq 'h323') {
             for (@servers) {
                  print "\t going through server $_\n";
                  if ($meth[0] eq 'INBOUND ID') {
                     $h323_r = `ssh "$_" grep "'|$_[1]|'" /process/hom
+e/h323cc.[12]/paccalls | tail -1`;
                     next;
                  } else {
                     $h323_r = `ssh "$_" grep "$_[1]" /process/home/h3
+23cc.[12]/paccalls | tail -1`;
                  }
                  if (($h323_r) && ($meth[0] eq 'CALLID')) {
                      $file_f = wr_temp("$h323_r");
                      system "/n2p/bin/siptool -v /tmp/file_123456";
                      last;
                  } elsif ($h323_r) {
                      print "Foudn at least one call at $_\n";
                      print "$h323_r\n";
                      next;
                  }
             }
          } elsif ("$proc" eq 'sip') {
             for (@servers) {
                  print "\t going through server $_\n";
                  if ($meth[0] eq 'INBOUND ID') {
                     #my $yahoo = "\|$_[1]\|";
                     $sip_r = `ssh "$_" grep "'|$_[1]|'" /process/home
+/sipcc.[12]/sipcalls | tail -1`;
                     print "$sip_r\n";
                     next;
                  } else {
                     $sip_r = `ssh "$_" grep "$_[1]" /process/home/sip
+cc.[12]/sipcalls | tail -1`;
                  }
                  if (($sip_r) && ($meth[0] eq 'CALLID')) {
                       $file_f = wr_temp("$sip_r");
                       system("/n2p/bin/siptool -v /tmp/file_123456");
                       last;
                  } elsif ($sip_r) {
                      print "Found at least one call at $_\n";
                      print "$sip_r\n";
                  }
             }
          } else {
             print "Do no understand your input\n";
             usage();
          }
}

my $val1 = eval { GetOptions (
                        "help"      => sub { usage() },
                        "callid=s"  => \$callid,
                        "dn=s"      => \$dn,
                        "inbound=s" => \$inbound
)};
die                                   usage() if $@;  #alone

if (!$val1) {
      usage();
}

my @ar = san_check_arg();  #check users input sanity
proc_it(@ARGV,@ar);
[download]

Comment on Re^3: Getopt question again Download Code

Replies are listed 'Best First'.
Re^4: Getopt question again by graff (Chancellor) on Jan 28, 2008 at 04:55 UTC
When you say it's working, do you mean that you tested it on real inputs and confirmed that it does what it's supposed to do? Or do you just mean that it compiles and runs without crashes or error messages? A few odd things that I wouldn't normally do: Why put GetOptions inside an eval block? You can just check its return value (true means command line was ok, false means a bad arg). Why have @meth as an array when you only assign a single scalar value to it, and only use `$meth[0]` when checking its contents? Your "usage" message implies that a bunch of options can be used together (i.e. the first line shows "-c", "-d" and "-i" all being used in combination), but your option handling will only operate on one option. Your "san_check_arg" function does not check whether the args for "-c", "-d" or "-i" consist of just digits, and then you pass these unchecked strings from the command line directly into a subshell. This means you are asking for trouble. It's not so much a "security risk", but you are opening the door for simple typos to cause all sorts of havoc, because of what the subshell might do with "unexpected" characters in the unchecked user input. If you could figure out a way to do what is needed without runnin those ssh commands in backticks, it would be a lot nicer. How big are the files that you need to grep over? Would it make sense to coy them from the remove servers and do the grep locally (with the perl script)? Or you could do something like this in your "proc_it()" sub: `my $remote_file = $proc . 'cc.[12]/' . ( $proc eq 'sip' ? 'sipcalls' : + 'paccalls'; for my $server ( @servers ) { open( REMOTE, "-\|", "ssh $server cat /process/home/$remote_file" ) or die "ssh $server failed: $!"; while (<REMOTE>) { next unless /whatever/; ... } close REMOTE; }` [download] As long as you make sure that $proc has a proper value ("h323" or "sip"), that ssh command will be safe -- it contains no other "tainted" variables. But if the files are so big that you don't want all the data coming in over the remote connection, you should still try eliminate (or at least be more careful about) the use of quoted strings and user-supplied args in the command line being sent to a remote shell.	[reply] [d/l] [select]
Re^5: Getopt question again by convenientstore (Pilgrim) on Jan 28, 2008 at 06:01 UTC
hi fellow monk 1)eval.. I will have to look into that.. I used it w/out fully understanding them 2)I try to assign to $meth but it was getting assigned 1 when there was a value(context?) and wasn't sure how to get the actual word there so decide to use array 3)bunch of options being there comments---> this is something I have to work on.. but also have to understand more on getopt.. 4)san_check_arg comments.. this is good.. I will look into validate the user's input as digitis.. thank you! results from remote server is just one liner, but since it is dealing w/ dynamic file and argument has to be different so I figure doing the backtic to do direct ssh.. but I will also look into putting a script in remote server and apply arguments.. but woudln't that be almost same? overall, thank you and I will go over the script w/ your comments on my mind and repost the final one	[reply]
Re^5: Getopt question again by convenientstore (Pilgrim) on Jan 29, 2008 at 03:58 UTC
1)Eval block , I decide to keep as I like the way it looks.. But understand now that it's unecessary 2)@meth is now changed to $meth. For whatever reason, initially it was returning 1, which I thought meant I was using it as wrong context. Still not 100% sure why it was returning 1 but now returns proper value 3)usage () implying bunch of options can be used together. <-- this is something i still have to look into 4)san_check_arg now include, `return ('DN',$dn)if (defined($dn)&& "$dn" =~ /^\d+$/);` [download] Also, your new comments on proc_it(), this gives me more and better idea on how to implement stuff for future project.. once again, big thanks to you.	[reply] [d/l]


"be consistent"
	PerlMonks