On Wed, Aug 28, 2013 at 11:48:45AM -0400, Harry Putnam wrote: > I know the format and scripting are probably pretty backwards but I > seem to recall it being important to have a `return' line in a > function. > > The code below was a script by itself but now I need to turn it into a > function inside a larger script... I've done something that sort of > works but fails on a specific file name that looks like `.#somefile'. > > I'll show the code the way I've tried to turn it into a sub routine. > So, I'd like comments both on how to write this more like a proper sub > routine and any other why ideas.
You should have included a more complete program. Since you are
inexperienced with Perl it would be wise for us to be able to
point out all of your bad habits. :) Hopefully you are at least
using strict and warnings pragmas.
> What the sub is supposed to do is plow thru a couple of directories and
> reset the proper owner and group after some other operations have
> changed them.
>
> Its called like this in a getops operation:
>
> if($opt_o ) {
> if (@ARGV) {
> usage();
> print "The \`-o' flag must be used by itself and\n",
> " \`-o' flag requires user to be root .. exiting\n ";
You can use a heredoc string to write multiline text more easily:
print <<EOF;
the \`-o' flag must be used by itself and
\`-o' flag requires user to be root .. exiting.
EOF
You are also wrapping that text at only ~45 characters, which is
a bit early for probably any device you'll use. >:) It may be
wiser to wrap around 72 or 80, or not wrap at all and let the
terminal do it for you.
> exit;
A better behaved program would signal error back to the caller so
they can react appropriately (e.g., if another program called
this program and relies on this program successfully changing the
ownership then it can also exit with an error instead of going
ahead and trying to do something that doesn't apply). Typically 0
signals success, and non-zero error. 1 or -1 are common, but it
can practically be anything in the range supported by your perl
and platform.
exit 1;
> }
> chOwnGrp();
> exit;
This exit isn't strictly necessary since the program will
automatically exit when the end is reached.
> }
>
> sub chOwnGrp {
> use File::Find;
A module cannot actually be loaded within a lexical scope, and
the use will be processed before any other code (despite being
inside of that subroutine) so you should move it to the top of
the program with other use directives so it's easier to spot.
To demonstrate:
#!/usr/bin/perl
use strict;
use warnings;
print join ', ', keys %File::Find::;
sub never_called {
use File::Find;
}
__END__
The reason for this can be learned about in `perldoc -f use' and
`perldoc perlmod'.
> my $buffer = '/merb';
> my $module = '/usr/local/common/merc';
> my $uname = $ENV{'LOGNAME'};
> my $uid = '1000';
> my $gid = '1050';
For a short program that isn't used often there's nothing
particularly wrong here (except for maybe relying on LOGNAME and
those constant uid and gid like that), but for a more robust
program you'd probably want to inject those parameters into the
subroutine through arguments. Or if they really will never
change, perhaps you might declare them using the constant pragma
instead (I am not familiar with those terms so I cannot criticize
the variable names or given them better context in the scope of
the program):
use constant {
BUFFER => '/merb',
GID => 1050,
MODULE => '/usr/local/common/merc',
UID => 1000,
};
> if ($uname =~ /root/) {
> print "Would you like to set owner:group on BUFFER and MODULE?\n",
> "If so, we will run chown -R $uid:$gid $buffer and\n",
> "chown -R $uid:$gid $module\n",
> "[y/n] > ";
Again we could use a heredoc again. Unfortunately, constants are
not interpolated like variables are so that won't work. You can
either assign the constants to lexicals (kind of defeats the
purpose in this case) or use the "magic" [not really a single
operator] turtle-operator: @{[]}.
print <<EOF;
Would you like to set owner:group on BUFFER and MODULE?
If so, we will run chown -R @{[UID()]}:@{[GID()]} @{[BUFFER()]} and
chown -R @{[UID()]}:@{[GID()]} @{[MODULE()]}
EOF
print '[y/n] >';
> my $answer = <STDIN>;
> if ($answer =~ /^y$/) {
> find(
> sub {
> chown $uid, $gid, $_ or warn "could not chown '$_': $!";
> },
> $buffer, $module
> );
> }
> } else {
> print "You must be root to run with this flag (-o) effectively..
> exiting\n";
> print "Not root - tripped at line: <" . __LINE__ . ">\n";
Again we can use a heredoc. Instead though, see my next point.
> exit;
Rather than exit()ing from a subroutine, it is often better to
either return a value indicating failure (e.g., boolean or error
status) or die() instead. This lets the caller decide if they
want to give up or do something else. Not only can die() be
caught and handled with eval(), but it also appends line
information by default so you can leave that second bit off
entirely:
die 'You must be root to run with this flag (-o) effectively.';
> }
> }
As Rob indicated, give us a more specific error if you still need
help with your program (it's hard to speculate what is going
wrong). If you want more code review then post a more complete
program.
Regards,
--
Brandon McCaig <bamcc...@gmail.com> <bamcc...@castopulence.org>
Castopulence Software <https://www.castopulence.org/>
Blog <http://www.bamccaig.com/>
perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'
signature.asc
Description: Digital signature
