*main:: added to function result

Hi,

I have written a function to lookup a threshold value from a cfg file. For
some reason, the value returned by the function has *main:: prepended to it,
eg.

C:\toolkit\Scripts\monitor\perl>fs_check.pl
*main::90

The lines from the code that produce the above output look like:

my $threshold = get_cfg_item($config,"df_threshold:" . @drv_lst[$i]);
print $threshold;

The function code is:

sub get_cfg_item
{
my ($config,$item_name)=@_;

my $item_len=length($item_name);

open(cfg, "$config") or die "\n\nCannot open configuration file:
$config\n\n";

my @cfg_line = ;

close(cfg);

my $j = @cfg_line;
my $i;
my $out_val = '';

for ($i = 0; $i <= $j; $i++ ){

if ( $cfg_line[$i] =~ /$item_name/ ){
$out_val=substr($cfg_line[$i],$item_len+1);
chop $out_val;
return $out_val
last;
}
}
}

Can you please tell me why the result is not just 90 as expected?

Regards,

Kevin

Re: *main:: added to function result

"news.optusnet.com.au" wrote in message
news:43cc45be$0$26131$afc38c87@news.optusnet.com.au...
> Hi,
>
> I have written a function to lookup a threshold value from a cfg file. For
> some reason, the value returned by the function has *main:: prepended to
> it,
> eg.
>
> C:\toolkit\Scripts\monitor\perl>fs_check.pl
> *main::90
>

I don't get that result, but following would have tipped you off to a number
of problems:

use strict;
use warnings;

> The lines from the code that produce the above output look like:
>
> my $threshold = get_cfg_item($config,"df_threshold:" . @drv_lst[$i]);

I don't think you want to concatenate an array slice, and there's no need to
use double quotes and concentenate:

my $threshold = get_cfg_item($config,"df_threshold:$drv_lst[$i]")

And what is in @drv_lst? It might be relevant to your problem, or it might
not. Without a working script that demonstrates the problem it's impossible
to guess.

> print $threshold;
>
> The function code is:
>
> sub get_cfg_item
> {
> my ($config,$item_name)=@_;

This may be a moot point (assuming your two lines above were taken from a
loop), but if you already have $config at the package level, it's not a good
idea to use the same variable name inside your subroutine. If you change
$config here at some later date, all the subsequent $config variables will
still be valid and will be set instead to the package level value (and won't
generate errors even if you use strictures).

>
> my $item_len=length($item_name);
>
> open(cfg, "$config") or die "\n\nCannot open configuration file:
> $config\n\n";
>

Lowercase filehandles are frowned upon, as is needlessly quoting variables.
See the perlfaqs for more info, but the above would be better written as:

open(CFG, $config) or die "\n\nCannot open configuration file: $config\n\n";

You also aren't helping yourself by checking the reason why the open might
have failed ($!). Better, in my opinion, would be to use the three argument
open:

open(my $cfg, '<', $config) or die "Could not open configuration file
$config : $!";

> my @cfg_line = ;
>
> close(cfg);
>
> my $j = @cfg_line;
> my $i;
> my $out_val = '';
>
> for ($i = 0; $i <= $j; $i++ ){

This above is a long way of writing:

for my $i (0..$#cfg_line) {

>
> if ( $cfg_line[$i] =~ /$item_name/ ){
> $out_val=substr($cfg_line[$i],$item_len+1);
> chop $out_val;
> return $out_val
> last;

Where is the semi-colon after the return? Why are you writing "last" when it
will never be reached?

> }
> }
> }
>
> Can you please tell me why the result is not just 90 as expected?
>

I do get 90. Please post a complete and working example that will
demonstrate your problem.

Matt

Re: *main:: added to function result

Matt,

Thanks for you response. I appreciate your tips especially those relating to
standards. The bug causing the problem was the missing ";" from line 58 as
you pointed out. I was using strict but was not using warnings. When I used
warnings, it still did not detect the missing ";". While my file read loop
is not as code efficient, it is far more maintainable than the solution you
offered (at least given my lack of experience with perl).

I have and revise the code based on your comments. But, I still have one
niggling warning:

Use of uninitialized value in numeric eq (==) at
C:\toolkit\Scripts\monitor\perl\fs_check.pl line 121.

I don't see what the issue is - any ideas?

Following is the (semi) complete script.

# ************************************************************ ******
# *
# * File : fs_check.pl
# *
# * Written By : Kevin Crowley (Pacific DBMS Pty Ltd)
# *
# * Description : Checks for file systems filling up
# *
# * Modification History
# * ------------------------------------------------------------ --
# *
# * Date By Description
# * --------- --- ----------------------------------------------
# * 03-May-00 KJC Original Code
# * 12-Jan-06 KJC Converted to perl
# *
# *
# ************************************************************ ******
#
# Set environment
#
use Config;
use POSIX qw(strftime);
use strict;
no strict "refs";
use warnings;

my $os_name = $Config{osname};
my $server_name = `hostname`;
my $outfile = '';
my $site_name = '';
my $chk_log = '';
my $config = '';

sub get_cfg_item
{
my ($config,$item_name)=@_;

my $item_len=length($item_name);

open(CFG, "$config") or die "\n\nCannot open configuration file:
$config\n\n";

my @cfg_line = ;

close(CFG);

my $j=@cfg_line;
my $i;
my $out_val = '';

for ($i = 0; $i <= $j; $i++ ){

if ( $cfg_line[$i] =~ /$item_name/ ){
$out_val=substr($cfg_line[$i],$item_len+1);
chop $out_val;
return $out_val;
last;
}
}
}

sub get_config
{
my $config_dir = 'c:\temp';
my $config_file = "monitor.cfg";
my $slash = '';
my $date_fmt = strftime('%Y%m%d', localtime);
my $mv_cmd = '';

if ( $os_name eq "MSWin32" ) {
#print "Windows OS \n";
$slash="\\";
$mv_cmd="move";
}
else {
#print "UNIX OS \n";
$slash="/";
$mv_cmd="mv";
}

$config=$config_dir.$slash.$config_file;

$site_name = get_cfg_item($config,'site_name');

$chk_log = get_cfg_item($config,'fs_chk_log');

$outfile=$chk_log.'.'.$date_fmt;
}

sub open_output
{
open(OUT, ">$outfile") or die "Cannot open file: $outfile\n";
}

sub write_output
{
print OUT ($_[0]."\n");
}

sub close_output
{
close (OUT);
}

sub check_diskspace_win32
{
use Win32::DriveInfo;
my @drv_lst = Win32::DriveInfo::DrivesInUse();
my $j = $#drv_lst + 1;
my $i = 0;
for( $i = 0; $i <= $j; $i++){

my $total_space = 0;
my $free_space = 0;
my $pct_used = 0;
my $drive_type = 0;
my $fixed_drive = 3;

$drive_type = Win32::DriveInfo::DriveType($drv_lst[$i]);

if ( $drive_type == $fixed_drive ){
$total_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[5];
$free_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[6];
$pct_used = ($total_space - $free_space) / $total_space * 100;
my $threshold = get_cfg_item($config,"df_threshold:" . $drv_lst[$i]);

#print $threshold;
if ( $threshold == 0 ){
$threshold = 75;
}

if ( $pct_used > $threshold ) {
my $out_ln = sprintf("Drive %s %s %0.2f %s is over %2d",
$drv_lst[$i], '(', $pct_used, '%)', $threshold) . "% full\n";
write_output($out_ln);
}
}
}
}

sub check_diskspace_other
{
}

sub check_diskspace
{
if ( $os_name eq "MSWin32" ) {
check_diskspace_win32;
}
else {
check_diskspace_other;
}
}

#
# Configure the script environment
#
get_config;

#
# Scan todays alert file for ORA-errors.
#
open_output;

check_diskspace;

close_output;

#
# end-of-script
#
"Matt Garrish" wrote in message
news:GzYyf.1598$924.89017@news20.bellglobal.com...
>
> "news.optusnet.com.au" wrote in message
> news:43cc45be$0$26131$afc38c87@news.optusnet.com.au...
> > Hi,
> >
> > I have written a function to lookup a threshold value from a cfg file.
For
> > some reason, the value returned by the function has *main:: prepended to
> > it,
> > eg.
> >
> > C:\toolkit\Scripts\monitor\perl>fs_check.pl
> > *main::90
> >
>
> I don't get that result, but following would have tipped you off to a
number
> of problems:
>
> use strict;
> use warnings;
>
> > The lines from the code that produce the above output look like:
> >
> > my $threshold = get_cfg_item($config,"df_threshold:" .
@drv_lst[$i]);
>
> I don't think you want to concatenate an array slice, and there's no need
to
> use double quotes and concentenate:
>
> my $threshold = get_cfg_item($config,"df_threshold:$drv_lst[$i]")
>
> And what is in @drv_lst? It might be relevant to your problem, or it might
> not. Without a working script that demonstrates the problem it's
impossible
> to guess.
>
> > print $threshold;
> >
> > The function code is:
> >
> > sub get_cfg_item
> > {
> > my ($config,$item_name)=@_;
>
> This may be a moot point (assuming your two lines above were taken from a
> loop), but if you already have $config at the package level, it's not a
good
> idea to use the same variable name inside your subroutine. If you change
> $config here at some later date, all the subsequent $config variables will
> still be valid and will be set instead to the package level value (and
won't
> generate errors even if you use strictures).
>
> >
> > my $item_len=length($item_name);
> >
> > open(cfg, "$config") or die "\n\nCannot open configuration file:
> > $config\n\n";
> >
>
> Lowercase filehandles are frowned upon, as is needlessly quoting
variables.
> See the perlfaqs for more info, but the above would be better written as:
>
> open(CFG, $config) or die "\n\nCannot open configuration file:
$config\n\n";
>
> You also aren't helping yourself by checking the reason why the open might
> have failed ($!). Better, in my opinion, would be to use the three
argument
> open:
>
> open(my $cfg, '<', $config) or die "Could not open configuration file
> $config : $!";
>
> > my @cfg_line = ;
> >
> > close(cfg);
> >
> > my $j = @cfg_line;
> > my $i;
> > my $out_val = '';
> >
> > for ($i = 0; $i <= $j; $i++ ){
>
> This above is a long way of writing:
>
> for my $i (0..$#cfg_line) {
>
> >
> > if ( $cfg_line[$i] =~ /$item_name/ ){
> > $out_val=substr($cfg_line[$i],$item_len+1);
> > chop $out_val;
> > return $out_val
> > last;
>
> Where is the semi-colon after the return? Why are you writing "last" when
it
> will never be reached?
>
> > }
> > }
> > }
> >
> > Can you please tell me why the result is not just 90 as expected?
> >
>
> I do get 90. Please post a complete and working example that will
> demonstrate your problem.
>
> Matt
>
>

Re: *main:: added to function result

news.optusnet.com.au wrote:
> Thanks for you response. I appreciate your tips especially those relating to
> standards.

Then why didn't you follow any of Matt's advice?

> The bug causing the problem was the missing ";" from line 58 as
> you pointed out. I was using strict but was not using warnings. When I used
> warnings, it still did not detect the missing ";". While my file read loop
> is not as code efficient, it is far more maintainable than the solution you
> offered (at least given my lack of experience with perl).

No, it's not. With your solution, you have to look up three or four
lines to figure out when and how the loop is ending. With the standard
idiom Matt provided, it's right there in the loop. Separating out
variables or values from their intended usage makes code *less*
maintainable, not more.

> I have and revise the code based on your comments.

Not enough, you haven't.

> But, I still have one niggling warning:
>
> Use of uninitialized value in numeric eq (==) at
> C:\toolkit\Scripts\monitor\perl\fs_check.pl line 121.

And you didn't think it might be wise to show us what line 121 is? Did
you expect everyone to count lines for you, or copy & paste your code
into a text editor?

The more effort you put into asking a decent question, the better
answers you will receive.

> I don't see what the issue is - any ideas?
>
> Following is the (semi) complete script.

So in other words the code below isn't the code generating the warning.
So then why should anyone make an attempt at figuring out your
problem?

> use Config;
> use POSIX qw(strftime);
> use strict;
> no strict "refs";

Why, why, WHY would you do that?!

> use warnings;
use Carp; #used in re-written get_cfg_item below

> my $os_name = $Config{osname};
> my $server_name = `hostname`;
> my $outfile = '';
> my $site_name = '';
> my $chk_log = '';
> my $config = '';
>
> sub get_cfg_item
> {
> my ($config,$item_name)=@_;
>
> my $item_len=length($item_name);
>
> open(CFG, "$config") or die "\n\nCannot open configuration file:
> $config\n\n";

1) Use the three-argument form of open
2) use Lexical filehandles, not global barewords
3) see: perldoc -q quoting
4) Tell the user *why* the open failed!

open my $cfg_fh, '<', $config or die "Can't open config file '$config':
$!";


> my @cfg_line = ;

Why are you slurping the entire file into memory? What purpose does
that serve?

>
> close(CFG);
>
> my $j=@cfg_line;
> my $i;
> my $out_val = '';
>
> for ($i = 0; $i <= $j; $i++ ){
>
> if ( $cfg_line[$i] =~ /$item_name/ ){

Does $item_name contain a string or a regular expression pattern? The
way you've written it, any *, +, \, [ , etc will all be interpreted by
the regexp engine.

see: perldoc -f quotemeta

> $out_val=substr($cfg_line[$i],$item_len+1);
> chop $out_val;

Why are you using chop instead of chomp? Do you know the difference?
When someone reads your code, you're notifying them that something
unusual is going on here by using a non-standard idiom.

If you did intend chop specifically to remove the last character of
$out_val, why not just change the substr() call to do what you want?

> return $out_val;
> last;

You still have a line that cannot possibly be executed, ever. Why?

> }
> }

Replace all of the above with:
while (my $cfg_line = <$cfg_fh>) {
chomp $cfg_line;
if ($cfg_line =~ /\Q$item_name/) {
return substr($cfg_line, $item_len); #or $item_len + 1, depending
on your needs
}
}
carp "Did not find $item_name in $config";
return undef;

> }
>
> sub get_config
> {
> my $config_dir = 'c:\temp';
> my $config_file = "monitor.cfg";
> my $slash = '';
> my $date_fmt = strftime('%Y%m%d', localtime);
> my $mv_cmd = '';
>
> if ( $os_name eq "MSWin32" ) {
> #print "Windows OS \n";
> $slash="\\";
> $mv_cmd="move";
> }
> else {
> #print "UNIX OS \n";
> $slash="/";
> $mv_cmd="mv";
> }

Why why why would you do that to yourself? For one, you're never using
$mv_cmd, so I have to assume it's used in a piece of code you didn't
show us. But regardless, just use the standard File::Copy module to
get a portable move() function, rather than shelling out to some
OS-dependent system call.

>
> $config=$config_dir.$slash.$config_file;
>
> $site_name = get_cfg_item($config,'site_name');
>
> $chk_log = get_cfg_item($config,'fs_chk_log');
>
> $outfile=$chk_log.'.'.$date_fmt;

Yuck. Learn to love interpolation.
$outfile = "$chk_log.$date_fmt";

> }
>
> sub open_output
> {
> open(OUT, ">$outfile") or die "Cannot open file: $outfile\n";

Again, use the three argument form of open, and tell the user why the
open failed:

open my $out_fh, '>', $outfile or die "Cannot open '$outfile': $!";
return $out_fh;

> }
>
> sub write_output
> {
> print OUT ($_[0]."\n");

What on earth is the point of this function? If you want to print to
the file handle, just print. Why add the overhead of an additional
wrapper subroutine?

Regardless, if you take my advice above about the lexical filehandle,
you'll need to pass that filehandle into this function now.
sub write_output {
my $fh = shift;
print $fh "$_[0]\n";
}

> }
>
> sub close_output
> {
> close (OUT);
> }

Same thing here. Just close it. No need to cause Perl to do more
work.

>
> sub check_diskspace_win32
> {
> use Win32::DriveInfo;
> my @drv_lst = Win32::DriveInfo::DrivesInUse();
> my $j = $#drv_lst + 1;
> my $i = 0;
> for( $i = 0; $i <= $j; $i++){
>
> my $total_space = 0;
> my $free_space = 0;
> my $pct_used = 0;
> my $drive_type = 0;
> my $fixed_drive = 3;
>
> $drive_type = Win32::DriveInfo::DriveType($drv_lst[$i]);
>
> if ( $drive_type == $fixed_drive ){
> $total_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[5];
> $free_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[6];
> $pct_used = ($total_space - $free_space) / $total_space * 100;
> my $threshold = get_cfg_item($config,"df_threshold:" . $drv_lst[$i]);
>
> #print $threshold;
> if ( $threshold == 0 ){
> $threshold = 75;
> }

I'm taking a wild guess that this is the location of your warning.
$threshold comes from get_cfg_item, which in your implementation has no
default value if the configuration option is not found. When I
commented on that subroutine, I added in a warning to notify you when
that happens. If you want to assign a default value when get_cfg_item
returns undef, change your assignment line to:
my $threshold = get_cfg_item($config, "df_threshold:$drv_list[$i]") ||
0;

That will assign 0 to $threshold if get_cfg_item returns undef.

Please take in *all* the advice you've been given in this thread, both
from me and from Matt. Do not simply make the one quick fix that will
solve your immediate problem. You'll be a better Perl programmer for
it.

Paul Lalli

Re: *main:: added to function result

Paul,

Firstly, not by way of an excuse - merely explanation, I have been learning
/ coding perl for 2 working days. I started this Monday just passed to
convert some monitoring scripts previously in production using kornshell.
As such, I have used cut and paste to rapidly convert as much as possible.
Extraneous lines like mv_cmd were just an artifact. As for:

> And you didn't think it might be wise to show us what line 121 is? Did
> you expect everyone to count lines for you, or copy & paste your code
> into a text editor?

Put that down to inexperience with posting to newsgroups. The code is still
semi complete as my unix environment is still being built and so the code /
testing remains incomplete; and yes, it was the code that was being
executed. You then asked:

> > use Config;
> > use POSIX qw(strftime);
> > use strict;
> > no strict "refs";
>
> Why, why, WHY would you do that?!

This is because the addition of 'use warnings;' and commenting out 'no
strict "refs";' leads to:

Can't use string ("CFG") as a symbol ref while "strict refs" in use at
C:\toolkit\Scripts\monitor\perl\fs_check.pl line 44, line 164.

Line 44 now reads: open($cfg_fh, '<', "$config") or die "\n\nCannot open
configuration file: $config: $!\n\n";

$cfg_fh is initialised to "CFG" at the start of the function. Chop vs
chomp - I have no idea of the difference, it was another typo, I meant to
use chomp. Your guess as to the location of the code causing the warning was
reasonably close but it was in fact a few lines earlier the offending line
now reads:

$drive_type = Win32::DriveInfo::DriveType($drv_lst[$i])||0;

Thanks for taking the time to help out, it is very much appreciated. If you
are interested, the current version of the scipt is:

#
# Start script
#
use Config;
use POSIX qw(strftime);
use strict;
no strict "refs";
use warnings;
use Carp;

my $os_name = $Config{osname};
my $server_name = `hostname`;
my $outfile = '';
my $chk_log = '';
my $config = '';
my $out_fh = "OUT";

sub get_cfg_item
{
my $cfg_fh = "CFG";
my ($config,$item_name)=@_;

my $item_len=length($item_name);

open($cfg_fh, '<', "$config") or die "\n\nCannot open configuration file:
$config: $!\n\n";

while (my $cfg_line = <$cfg_fh>) {
chomp $cfg_line;
if ($cfg_line =~ /\Q$item_name/) {
return substr($cfg_line, $item_len+1);
}
}
carp "Did not find $item_name in $config";

close($cfg_fh);

return undef;
}

sub get_config
{
my $config_dir = 'c:\temp';
my $config_file = "monitor.cfg";
my $slash = '';
my $date_fmt = strftime('%Y%m%d', localtime);

if ( $os_name eq "MSWin32" ) {
#print "Windows OS \n";
$slash="\\";
}
else {
#print "UNIX OS \n";
$slash="/";
}

$config="$config_dir$slash$config_file";

$chk_log = get_cfg_item($config,'fs_chk_log');

$outfile="$chk_log.$date_fmt";
}

sub check_diskspace_win32
{
use Win32::DriveInfo;
my @drv_lst = Win32::DriveInfo::DrivesInUse();
my $j = $#drv_lst + 1;
my $i = 0;
for( $i = 0; $i <= $j; $i++){

my $total_space = 0;
my $free_space = 0;
my $pct_used = 0;
my $drive_type = 0;
my $fixed_drive = 3;

$drive_type = Win32::DriveInfo::DriveType($drv_lst[$i])||0;

if ( $drive_type == $fixed_drive ){
$total_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[5];
$free_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[6];
$pct_used = ($total_space - $free_space) / $total_space * 100;
my $threshold = get_cfg_item($config,"df_threshold:$drv_lst[$i]");

#print $threshold;
if ( $threshold == 0 ){
$threshold = 75;
}

if ( $pct_used > $threshold ) {
my $out_ln = sprintf("Drive %s %s %0.2f %s is over %2d",
$drv_lst[$i], '(', $pct_used, '%)', $threshold) . "% full\n";
print $out_fh ("$out_ln\n");
}
}
}
}

sub check_diskspace_other
{
}

sub check_diskspace
{
if ( $os_name eq "MSWin32" ) {
check_diskspace_win32;
}
else {
check_diskspace_other;
}
}

#
# Configure the script environment
#
get_config;

#
# Scan todays alert file for ORA-errors.
#
open($out_fh, '>', $outfile) or die "Cannot open file: $outfile: $!\n";

check_diskspace;

close ($out_fh);

#
# end-of-script
#

Regards,

Kevin

"Paul Lalli" wrote in message
news:1137506393.155247.224050@z14g2000cwz.googlegroups.com.. .
> news.optusnet.com.au wrote:
> > Thanks for you response. I appreciate your tips especially those
relating to
> > standards.
>
> Then why didn't you follow any of Matt's advice?
>
> > The bug causing the problem was the missing ";" from line 58 as
> > you pointed out. I was using strict but was not using warnings. When I
used
> > warnings, it still did not detect the missing ";". While my file read
loop
> > is not as code efficient, it is far more maintainable than the solution
you
> > offered (at least given my lack of experience with perl).
>
> No, it's not. With your solution, you have to look up three or four
> lines to figure out when and how the loop is ending. With the standard
> idiom Matt provided, it's right there in the loop. Separating out
> variables or values from their intended usage makes code *less*
> maintainable, not more.
>
> > I have and revise the code based on your comments.
>
> Not enough, you haven't.
>
> > But, I still have one niggling warning:
> >
> > Use of uninitialized value in numeric eq (==) at
> > C:\toolkit\Scripts\monitor\perl\fs_check.pl line 121.
>
> And you didn't think it might be wise to show us what line 121 is? Did
> you expect everyone to count lines for you, or copy & paste your code
> into a text editor?
>
> The more effort you put into asking a decent question, the better
> answers you will receive.
>
> > I don't see what the issue is - any ideas?
> >
> > Following is the (semi) complete script.
>
> So in other words the code below isn't the code generating the warning.
> So then why should anyone make an attempt at figuring out your
> problem?
>
> > use Config;
> > use POSIX qw(strftime);
> > use strict;
> > no strict "refs";
>
> Why, why, WHY would you do that?!
>
> > use warnings;
> use Carp; #used in re-written get_cfg_item below
>
> > my $os_name = $Config{osname};
> > my $server_name = `hostname`;
> > my $outfile = '';
> > my $site_name = '';
> > my $chk_log = '';
> > my $config = '';
> >
> > sub get_cfg_item
> > {
> > my ($config,$item_name)=@_;
> >
> > my $item_len=length($item_name);
> >
> > open(CFG, "$config") or die "\n\nCannot open configuration file:
> > $config\n\n";
>
> 1) Use the three-argument form of open
> 2) use Lexical filehandles, not global barewords
> 3) see: perldoc -q quoting
> 4) Tell the user *why* the open failed!
>
> open my $cfg_fh, '<', $config or die "Can't open config file '$config':
> $!";
>
>
> > my @cfg_line = ;
>
> Why are you slurping the entire file into memory? What purpose does
> that serve?
>
> >
> > close(CFG);
> >
> > my $j=@cfg_line;
> > my $i;
> > my $out_val = '';
> >
> > for ($i = 0; $i <= $j; $i++ ){
> >
> > if ( $cfg_line[$i] =~ /$item_name/ ){
>
> Does $item_name contain a string or a regular expression pattern? The
> way you've written it, any *, +, \, [ , etc will all be interpreted by
> the regexp engine.
>
> see: perldoc -f quotemeta
>
> > $out_val=substr($cfg_line[$i],$item_len+1);
> > chop $out_val;
>
> Why are you using chop instead of chomp? Do you know the difference?
> When someone reads your code, you're notifying them that something
> unusual is going on here by using a non-standard idiom.
>
> If you did intend chop specifically to remove the last character of
> $out_val, why not just change the substr() call to do what you want?
>
> > return $out_val;
> > last;
>
> You still have a line that cannot possibly be executed, ever. Why?
>
> > }
> > }
>
> Replace all of the above with:
> while (my $cfg_line = <$cfg_fh>) {
> chomp $cfg_line;
> if ($cfg_line =~ /\Q$item_name/) {
> return substr($cfg_line, $item_len); #or $item_len + 1, depending
> on your needs
> }
> }
> carp "Did not find $item_name in $config";
> return undef;
>
> > }
> >
> > sub get_config
> > {
> > my $config_dir = 'c:\temp';
> > my $config_file = "monitor.cfg";
> > my $slash = '';
> > my $date_fmt = strftime('%Y%m%d', localtime);
> > my $mv_cmd = '';
> >
> > if ( $os_name eq "MSWin32" ) {
> > #print "Windows OS \n";
> > $slash="\\";
> > $mv_cmd="move";
> > }
> > else {
> > #print "UNIX OS \n";
> > $slash="/";
> > $mv_cmd="mv";
> > }
>
> Why why why would you do that to yourself? For one, you're never using
> $mv_cmd, so I have to assume it's used in a piece of code you didn't
> show us. But regardless, just use the standard File::Copy module to
> get a portable move() function, rather than shelling out to some
> OS-dependent system call.
>
> >
> > $config=$config_dir.$slash.$config_file;
> >
> > $site_name = get_cfg_item($config,'site_name');
> >
> > $chk_log = get_cfg_item($config,'fs_chk_log');
> >
> > $outfile=$chk_log.'.'.$date_fmt;
>
> Yuck. Learn to love interpolation.
> $outfile = "$chk_log.$date_fmt";
>
> > }
> >
> > sub open_output
> > {
> > open(OUT, ">$outfile") or die "Cannot open file: $outfile\n";
>
> Again, use the three argument form of open, and tell the user why the
> open failed:
>
> open my $out_fh, '>', $outfile or die "Cannot open '$outfile': $!";
> return $out_fh;
>
> > }
> >
> > sub write_output
> > {
> > print OUT ($_[0]."\n");
>
> What on earth is the point of this function? If you want to print to
> the file handle, just print. Why add the overhead of an additional
> wrapper subroutine?
>
> Regardless, if you take my advice above about the lexical filehandle,
> you'll need to pass that filehandle into this function now.
> sub write_output {
> my $fh = shift;
> print $fh "$_[0]\n";
> }
>
> > }
> >
> > sub close_output
> > {
> > close (OUT);
> > }
>
> Same thing here. Just close it. No need to cause Perl to do more
> work.
>
> >
> > sub check_diskspace_win32
> > {
> > use Win32::DriveInfo;
> > my @drv_lst = Win32::DriveInfo::DrivesInUse();
> > my $j = $#drv_lst + 1;
> > my $i = 0;
> > for( $i = 0; $i <= $j; $i++){
> >
> > my $total_space = 0;
> > my $free_space = 0;
> > my $pct_used = 0;
> > my $drive_type = 0;
> > my $fixed_drive = 3;
> >
> > $drive_type = Win32::DriveInfo::DriveType($drv_lst[$i]);
> >
> > if ( $drive_type == $fixed_drive ){
> > $total_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[5];
> > $free_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[6];
> > $pct_used = ($total_space - $free_space) / $total_space * 100;
> > my $threshold = get_cfg_item($config,"df_threshold:" .
$drv_lst[$i]);
> >
> > #print $threshold;
> > if ( $threshold == 0 ){
> > $threshold = 75;
> > }
>
> I'm taking a wild guess that this is the location of your warning.
> $threshold comes from get_cfg_item, which in your implementation has no
> default value if the configuration option is not found. When I
> commented on that subroutine, I added in a warning to notify you when
> that happens. If you want to assign a default value when get_cfg_item
> returns undef, change your assignment line to:
> my $threshold = get_cfg_item($config, "df_threshold:$drv_list[$i]") ||
> 0;
>
> That will assign 0 to $threshold if get_cfg_item returns undef.
>
> Please take in *all* the advice you've been given in this thread, both
> from me and from Matt. Do not simply make the one quick fix that will
> solve your immediate problem. You'll be a better Perl programmer for
> it.
>
> Paul Lalli
>

Re: *main:: added to function result

news.optusnet.com.au wrote:
> > > no strict "refs";
> >
> > Why, why, WHY would you do that?!
>
> This is because the addition of 'use warnings;' and commenting out 'no
> strict "refs";' leads to:
>
> Can't use string ("CFG") as a symbol ref while "strict refs" in use at
> C:\toolkit\Scripts\monitor\perl\fs_check.pl line 44, line 164.

Someone in comp.lang.perl.misc just responded to another person in
almost exactly the same situation: "When your car is making a funny
noise, do you "fix" it by turning up the radio until you can't hear the
funny noise?"

FIX the problem. Do not mask it.

> Line 44 now reads: open($cfg_fh, '<', "$config") or die "\n\nCannot open
> configuration file: $config: $!\n\n";
>
> $cfg_fh is initialised to "CFG" at the start of the function.

*Why* is it initialized at all? Who told you to do anything like that?
You didn't quote my original suggestion, but I'm damned sure I would
never tell you to do that, and instead would have told you to do:

open my $cfg_fg, '<', $config or die "Cannot open '$config': $!";

When a variable is undefined and used in the open() statement, Perl
auto-magically assigns and filehandle reference to that variable. When
it contains a string, Perl attempts to use that string as a symbolic
reference to a filehandle, which 'use strict' rightfully disallows.

(Also note my elimination of the quotes surrounding "$config" here.
Please read:
perldoc -q quoting
)

> #
> # Start script
> #
> use Config;
> use POSIX qw(strftime);
> use strict;
> no strict "refs";
> use warnings;
> use Carp;
>
> my $os_name = $Config{osname};
> my $server_name = `hostname`;
> my $outfile = '';
> my $chk_log = '';
> my $config = '';
> my $out_fh = "OUT";

Stop doing that.

Stop feeling the need to initialize all your variables. This is Perl,
not C. Perl pre-initializes variables to undef for you. This is almost
*always* the value you want an "empty" variable to have.

>
> sub get_cfg_item
> {
> my $cfg_fh = "CFG";

Get rid of this line.

> my ($config,$item_name)=@_;
>
> my $item_len=length($item_name);
>
> open($cfg_fh, '<', "$config") or die "\n\nCannot open configuration file:
> $config: $!\n\n";

Fix as stated above.

> while (my $cfg_line = <$cfg_fh>) {
> chomp $cfg_line;
> if ($cfg_line =~ /\Q$item_name/) {
> return substr($cfg_line, $item_len+1);
> }
> }
> carp "Did not find $item_name in $config";
>
> close($cfg_fh);
>
> return undef;
> }
>
> sub get_config
> {
> my $config_dir = 'c:\temp';
> my $config_file = "monitor.cfg";
> my $slash = '';
> my $date_fmt = strftime('%Y%m%d', localtime);
>
> if ( $os_name eq "MSWin32" ) {
> #print "Windows OS \n";
> $slash="\\";
> }
> else {
> #print "UNIX OS \n";
> $slash="/";
> }
>
> $config="$config_dir$slash$config_file";
>
> $chk_log = get_cfg_item($config,'fs_chk_log');
>
> $outfile="$chk_log.$date_fmt";
> }
>
> sub check_diskspace_win32
> {
> use Win32::DriveInfo;
> my @drv_lst = Win32::DriveInfo::DrivesInUse();
> my $j = $#drv_lst + 1;
> my $i = 0;
> for( $i = 0; $i <= $j; $i++){

Oh good. I'm glad you posted this again. Because I want to point out
that in addition to being inefficient and non-idiomatic, it's also
wrong.

You've assigned $j to be the size of the array. You are looping from 0
to the size of the array. That's one loop more than it should be. If
an array has 3 items, the valid indices are 0, 1, and 2. You're
looping through indices 0, 1, 2, and 3.

Get over your fear of learning real Perl code. Fix this the way we've
been telling you to fix it the entire thread.

>
> my $total_space = 0;
> my $free_space = 0;
> my $pct_used = 0;
> my $drive_type = 0;
> my $fixed_drive = 3;
>
> $drive_type = Win32::DriveInfo::DriveType($drv_lst[$i])||0;
>
> if ( $drive_type == $fixed_drive ){
> $total_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[5];
> $free_space = (Win32::DriveInfo::DriveSpace($drv_lst[$i]))[6];
> $pct_used = ($total_space - $free_space) / $total_space * 100;
> my $threshold = get_cfg_item($config,"df_threshold:$drv_lst[$i]");
>
> #print $threshold;
> if ( $threshold == 0 ){
> $threshold = 75;
> }
>
> if ( $pct_used > $threshold ) {
> my $out_ln = sprintf("Drive %s %s %0.2f %s is over %2d",
> $drv_lst[$i], '(', $pct_used, '%)', $threshold) . "% full\n";
> print $out_fh ("$out_ln\n");
> }
> }
> }
> }
>
> sub check_diskspace_other
> {
> }
>

Well. That's rather pointless...

Paul Lalli