patternbashMinor
Function autoloading in bash
Viewed 0 times
functionbashautoloading
Problem
I'm wondering if anyone can code review my code below. That's the core code of my little framework for bash in GitHub.
## ==========================================================================================
## FUNCTION : autoload_functions
## PURPOSE : To check functions that are required, try to autoload them if NOT YET defined
##
## INPUT: array of $req_func
## OUTPUT: exit 1 - if any required function cannot be autoloaded (meaning, not found in standard
## library location.
## ==========================================================================================
autoload_functions()
{
local req_func=$*
local ret_val=0
## check first parameter. If it's a directory, then
## it is where functions should be loaded from
local new_libdir=$1
[[ -d $new_libdir ]] && {
shift
LIB_DIR=$new_libdir
}
req_func=$* ## redeclare
## iterate through all $req_func, set $ret_val to 1
## if any required function cannot be autoloaded
for each_func in $req_func; do
func_name=`basename $each_func`
## if function undefined, try to auto-include (reference from specified location)
type $func_name &> /dev/null || source $LIB_DIR/${each_func}.bash.inc
## recheck, ensure it has been defined.
type $func_name &> /dev/null || (echo "ERROR: Missing required ${each_func}"; ret_val=1)
done
## abort (exit 1) if any required func cannot be autoloaded
[ $ret_val -eq 1 ] && exit $ret_val
} ## END: autoload_functions()Solution
Bug because of subshell
You are losing the value of
Variable assignments in a subshell are local to the subshell, so the variable in your function won't change. Instead of a subshell, you just want to group these commands using
Unnecessary variable
You don't need the
Prefer
The `
You are losing the value of
ret_val in a subshell here:... || (echo "ERROR: Missing required ${each_func}"; ret_val=1)Variable assignments in a subshell are local to the subshell, so the variable in your function won't change. Instead of a subshell, you just want to group these commands using
{ ...; } like this: ... || { echo "ERROR: Missing required ${each_func}"; ret_val=1; }Unnecessary variable
You don't need the
req_func variable. The top of your function could have been written simpler like this:autoload_functions()
{
local ret_val=0
## check first parameter. If it's a directory, then
## it is where functions should be loaded from
local new_libdir=$1
[[ -d $new_libdir ]] && {
shift
LIB_DIR=$new_libdir
}
## iterate through all arguments, set $ret_val to 1
## if any required function cannot be autoloaded
for each_func; do
# ...Prefer
$(...)The `
... form for command substitution is deprecated, use $(...) instead:
func_name=$(basename $each_func)
Quoting variables with paths
This line will not work if LIB_DIR contains spaces:
type $func_name &> /dev/null || source $LIB_DIR/${each_func}.bash.inc
This would be safer:
type $func_name &> /dev/null || source "$LIB_DIR"/${each_func}.bash.inc
Though this is probably a bit paranoid.
The same goes for $func_name and ${each_func} too,
but that would seem even more paranoid.
(Who in his right mind would create bash scripts with spaces in the name?)
Naming
The names you chose for some variables are not great. I'd recommend these alternatives:
- Instead of
each_func: func_path or func_relpath (as in relative path from LIB_DIR)
- Instead of
ret_val: exit_code`, because you may exit with thisCode Snippets
... || (echo "ERROR: Missing required ${each_func}"; ret_val=1)... || { echo "ERROR: Missing required ${each_func}"; ret_val=1; }autoload_functions()
{
local ret_val=0
## check first parameter. If it's a directory, then
## it is where functions should be loaded from
local new_libdir=$1
[[ -d $new_libdir ]] && {
shift
LIB_DIR=$new_libdir
}
## iterate through all arguments, set $ret_val to 1
## if any required function cannot be autoloaded
for each_func; do
# ...func_name=$(basename $each_func)type $func_name &> /dev/null || source $LIB_DIR/${each_func}.bash.incContext
StackExchange Code Review Q#58485, answer score: 8
Revisions (0)
No revisions yet.