Thanks for your help, I will look carefully at both of your comments.
cheers
________________________________
From: John W. Krahn <jwkr...@shaw.ca>
To: Perl Beginners <beginners@perl.org>
Sent: Friday, 13 April 2012 6:08 AM
Subject: Re: Array of Hashes
Rob Dixon wrote:
>
> Hi Paul and welcome to the list.
>
> I can see a few things wrong with your code, but I have only a Windows
> machine so cannot test any changes I am suggestion so please beware.
>
> The reason you get the marked line in your output is because that is
> what you have written. This loop
>
> open(EXT,"/usr/sbin/pvdisplay $PV|");
> while(<EXT>) {
> print $_;
> if (/Free PE/) {
> $AllocatedPE = (split(/\s+/, <EXT>))[2];
> $rec->{$PV} = $AllocatedPE;
> print "$PV ";
> print $rec->{$PV};
> print " \n";
> push @vggroup, $rec;
> }
> }
>
> reads a line from the pvdisplay output and checks whether it contains
> 'Free PE'. If it does, this
>
> $AllocatedPE = (split(/\s+/, <EXT>))[2];
>
> reads another line (the 'Allocated PE' line), splits it on white space
> and puts the third field - the extent - into $AllocatedPE. The code then
> goes on to add a single hash element to $rec and then print the volume
> name and extent.
>
> The line read within the call to split is never displayed, and the loop
> goes on to read and display the line after it in the while loop.
>
> Another problem is that you are pushing the same hash reference $rec
> onto @vggroup several times. There is no need for the array, as the hash
> holds the information on all the volumes in the group on its own.
>
> Other advice that may help is:
>
> - Perl variables generally use lower-case letters and underscores, and
> should be declared at the point of first use
>
> - Lexical file handles should be used with the three-argument form of
> open, and the status of /all/ opens should be checked before the file
> handle is used
>
> - By default the split operator splits $_ on whitespace, so split on its
> own returns a list of separate non-whitespace fields found in $_. This
> is usually what is wanted, as is the case here
>
> - There may well be a module already written to do this. It is worh a
> look at http://search.cpan.org unless your purpose is to learn the language
>
> Below is a refactored version of your program taking these things into
> account. As I explained I am unable to test it so I can only confirm
> that it compiles.
>
> HTH,
>
> Rob
>
>
> use strict;
> use warnings;
>
> open my $cmd, '-|', "/usr/sbin/vgdisplay -v vg03" or die $!;
You might as well go all the way:
open my $cmd, '-|', '/usr/sbin/vgdisplay', '-v', 'vg03'
or die "Cannot open pipe from vgdisplay because: $!";
> my @pv;
>
> while (<$cmd>) {
> if (/PV Name/) {
> my $pv = (split)[2];
> push @pv, $pv;
> }
> }
And don't forget to verify that the pipe closed correctly:
close $cmd or warn $! ? "Error closing vgdisplay pipe: $!"
: "Exit status $? from vgdisplay";
Another way to populate @pv:
my @pv = `/usr/sbin/vgdisplay -v vg03` =~ /PV Name\s+(\S+)/g;
> print "@pv\n";
> my $rec;
>
> foreach my $pv (@pv) {
> open my $ext, '-|', "/usr/sbin/pvdisplay $pv" or die $!;
open my $ext, '-|', '/usr/sbin/pvdisplay', $pv
or die "Cannot open pipe from pvdisplay because: $!";
> while(<$ext>) {
> print;
> if (/Allocated PE/) {
> my $allocated_pe = (split)[2];
> $rec->{$pv} = $allocated_pe;
> print "$pv => $allocated_pe\n";
> }
> }
close $ext or warn $! ? "Error closing pvdisplay pipe: $!"
: "Exit status $? from pvdisplay";
> }
>
> use Data::Dumper;
> print Dumper $rec;
John
--
Any intelligent fool can make things bigger and
more complex... It takes a touch of genius -
and a lot of courage to move in the opposite
direction. -- Albert Einstein
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
