Thanks shlomi ,working on your inputs...Thanks:)
On Tue, Jan 31, 2017 at 4:34 PM, Shlomi Fish <shlo...@shlomifish.org> wrote:
> Hi Uday,
>
> see my reply interim with your code.
>
> On Tue, 31 Jan 2017 15:31:59 +0530
> Uday Vernekar <vernekaru...@gmail.com> wrote:
>
> > Requirement:
> >
> > I have Test log files with filename as SerialNumber of tested Product and
> > the log file consist of
> > below data and other Text.the IP addresses can be repeated need to fetch
> > only mac starting with 00:05:95:XX:XX:XX
> >
> > <DATA>
> > Board IP address : 192.168.1.1:ffffff00
> > Host IP address : 192.168.1.100
> > Gateway IP address :
> > Run from flash/host/tftp (f/h/c) : f
> > Default host run file name : vmlinux
> > Default host flash file name : bcm963xx_fs_kernel
> > Boot delay (0-9 seconds) : 1
> > Default host ramdisk file name :
> > Default ramdisk store address :
> > Board Id (0-5) : 968500CHERRY
> > Number of MAC Addresses (1-32) : 10
> > Base MAC Address : 00:05:95:37:17:8b
> > PSI Size (1-64) KBytes : 24
> > Enable Backup PSI [0|1] : 0
> > System Log Size (0-256) KBytes : 0
> > Auxillary File System Size Percent: 0
> > Main Thread Number [0|1] : 0
> > MC memory allocation (MB) : 0
> > TM memory allocation (MB) : 0
> > <DATA>
> >
> > The data in the Test logs get appended everytime it is tested.
> >
> > I need to fetch "Base MAC Address : 00:05:95:37:17:8b"
> > from the each and evry logfile and put in another file with Respect to
> > SerialNumber of the Product.
> > If it doesnt find any Mac then it should display MAC NOT PROGRAMMED for
> > that SerialNumber.
> >
> > for Ex output in the file:
> > Serial Number Mac Programmed
> >
> > AG-0117-10-1740 00:05:95:37:17:8b
> > AG-0117-10-1741 00:05:95:37:17:8c
> > AG-0117-10-1742 00:05:95:37:17:8d
> > AG-0117-10-1743 MAC NOT PROGRAMMED
> > AG-0117-10-1744 00:05:95:37:17:8e
> >
> > I Have written the code Please help me correcting the Errors which I have
> > made in my code.
> >
> > <Start Code>
> >
> > #!/usr/bin/perl
> > use strict;
> > use warnings;
>
> It's good that you are using those.
>
> > use Cwd;
>
> Perhaps import getcwd/etc. explicitly or do «use Cwd ();».
>
> >
> > print "\tEnter the User Name:";
> > my $user = <STDIN>;
> > chomp($user);
> > print "\tEnter the Directory name where Logs are Stored:";
> > my $Directory = <STDIN>;
> > chomp($Directory);
>
> You have some duplicate code here.
>
> > system("mkdir -p /mnt/ONT");
>
> You should use https://metacpan.org/pod/File::Path or whatever instead of
> shelling to system which is less portable and may be slower. See:
>
> http://perl-begin.org/tutorials/bad-elements/#calling-the-shell-too-much
>
> (Note : I maintain that page).
>
> > my $userPath="/mnt/backup/$Directory";
> > chomp($userPath);
> >
>
> No need for chomp here.
>
> > my $scriptfile = "/mnt/ONT/MAC-REPORT-FILE.txt";
> > system("rm -f $scriptfile");
>
> again - you should use unlink.
>
> >
> > open(SCRIPT , ">>$scriptfile");
> >
>
> See http://perl-begin.org/tutorials/bad-elements/#open-function-style -
> you can
> also try using https://metacpan.org/pod/autodie .
>
> > print "\n Results are stored in /mnt/ONT/MAC-REPORT-FILE.txt\n";
> >
> > `rm /mnt/ONT/MAC-REPORT-FILE.txt`;
> >
>
> 1. Use unlink().
>
> 2. Don't use backticks instead of system -
> http://perl-begin.org/tutorials/bad-elements/#qx_for_command_execution
>
> > sub ScanDirectory {
> > my ($workdir) = shift;
> >
>
> You should do either:
>
> my ($work_dir) = @_;
>
> or:
>
> my $work_dir = shift;
>
> See scalar vs. list context.
>
> Also see http://perl-begin.org/tutorials/bad-elements/#
> calling-variables-file
>
> > my($startdir) = &cwd;
> >
>
> Don't call subroutines using ampersands :
>
> * http://perl-begin.org/tutorials/bad-elements/#
> ampersand-in-subroutine-calls
>
> * https://perlhacks.com/2015/04/subroutines-and-ampersands/
>
> > chdir($workdir) or die "Unable to enter dir $workdir:$!\n";
>
> I find that using chdir is usually a bad idea.
>
> >
> > opendir(DIR, "$workdir") or die "Unable to open $workdir:$!\n";
> > my @names = readdir(DIR);
> > closedir(DIR);
> >
> > foreach my $name (@names){
> > next if ($name eq ".");
> > next if ($name eq "..");
> >
>
> You want to use
> http://perl-begin.org/tutorials/bad-elements/#no_upwards_for_dirs
>
> Also see http://perl-begin.org/tutorials/bad-elements/#flow-
> stmts-without-labels
>
> > if (-d $name){
> > my $pwd=`pwd`;
>
> why not use Cwd.pm?
>
> > chomp($pwd);
> > my $path=$pwd."/".$name;
> > &ScanDirectory($path);
> > next;
> > }
> > unless (&CheckFile($name)){
> >
>
> Seems like you want to use File::Find or equivalent here -
> http://perl-begin.org/tutorials/bad-elements/#
> recursive_directory_traversal
>
> > }
> > }
> >
> > close(SCRIPT);
> > chdir($startdir) or die "Unable to change to dir $startdir:$!\n";
> > }
> >
> > my $date =`date`;
>
> use localtime() - http://perldoc.perl.org/functions/localtime.html
>
> > my $data_file2='/mnt/ONT/MAC-REPORT-FILE.txt';
> > open DATAOUT, ">>$data_file2" or die "can't open $data_file2 $!";
> > print DATAOUT "\n\t\t\tMAC-ID GENERATED REPORT\n";
> > print DATAOUT "\nGeneteated By:$user\n";
>
> you misspelled "Generated".
>
> > print DATAOUT "\nDATE: $date \n";
> > print DATAOUT "NOTE: THIS REPORT IS GENERATED BASED ON THE TEST LOGS
> > AVAILABLE IN THE SERVER\n";
> > print DATAOUT "\n";
> > print DATAOUT " SERIAL NUMBER\t\tMACADDRESS\n";
> > print DATAOUT "-----------------------------------------\n";
> > close(DATA);
> >
> > sub CheckFile{
> > my($name) = shift;
> >
> > my $datafile=$name;
> > #print "$datafile\n";
> > my $data_file2='/mnt/ONT/MAC-REPORT-FILE.txt';
> >
> > open DATA, "$datafile" or die" can't open $datafile $!";
> > my @array_of_data=<DATA>;
> >
> > #print @array_of_data;
> >
> > foreach my $i ($datafile)
> > {
>
> You're iterating over a scalar instead of a list.
>
> >
> > my $Result = `grep "Base MAC Address" \/mnt\/backup\/$Directory\/$name |
> > tail -n 1 `;
>
> don't use /bin/grep when you can use perl's file and text processing tools.
>
> > my ($str,@mac) = split(":",$Result);
> > if($mac[1] ne "05") {
> >
> > @mac=" MAC NOT PROGRAMMED\n";
> > #print "\n";
> > print DATAOUT " $name\t\t@mac\n" ;
> > }
> > else{
> > print DATAOUT " $name\t\t
> > $mac[0]:$mac[1]:$mac[2]:$mac[3]:$mac[4]:$mac[5]\n" ;
> > }
> > }
> > }
> >
> > &ScanDirectory($userPath);
> >
> > close(DATAOUT);
> >
> >
> > <END CODE>
> >
> >
>
> In short - this code should really be refactored, corrected, and
> modernised.
> Perhaps you should read http://www.onyxneon.com/books/
> modern_perl/index.html or
> a similar book or tutorial - see http://perl-tutorial.org/ and
> http://perl-begin.org/ .
>
> Regards,
>
> Shlomi Fish
>
>
--
*********************************************************
Don't ask them WHY they hurt you,
because all they'll tell you is lies and excuses.
Just know they were wrong, and try to move on.
**********************************************************
