2011-09-21 18:45:20 +0200, Lukas Czerner:
> Create command provides the functionality of creating a new logical
> volumes including defined file system.
>
> This commit also changes the way how are commands recognised an
> executed. The new approach is more suitable for bigger number of
> commands.
There are so many shell scripting bad practices in there that I
thought I had to reply.
>
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
> scripts/fsadm.sh | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 131 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
> index c8cc5e0..42c7da4 100755
> --- a/scripts/fsadm.sh
> +++ b/scripts/fsadm.sh
> @@ -29,7 +29,7 @@
> # 2 break detected
> # 3 unsupported online filesystem check for given mounted fs
>
> -TOOL=fsadm
> +TOOL=$(basename $0)
Leaving a variable unquoted has a very special meaning in shell.
If we were looking to a perl equivalent of $var that would be:
map glob, split $IFS_regex, $var
With the default value of $IFS:
$ ls
bar foo
$ perl -le '$var="a *"; print for map glob, split " ", $var'
a
bar
foo
$ sh -c 'var="a *"; printf "%s\n" $var'
a
bar
foo
That is it is a special operator that triggers word splitting
(split()) and filename generation (glob()).
Variables should alway be quoted unless you've got a very good
reason not to.
Also,
It's either
cmd "$option_or_argument"
or
cmd -- "$argument"
So above:
TOOL=$(basename -- "$0")
>
> _SAVEPATH=$PATH
Why would you need to save $PATH?
> PATH=/sbin:/usr/sbin:/bin:/usr/sbin:$PATH
> @@ -76,6 +76,8 @@ REMOUNT=
> PROCMOUNTS="/proc/mounts"
> NULL="$DM_DEV_DIR/null"
>
> +MAX_VGS=999
> +
> IFS_OLD=$IFS
doing
IFS_OLD=$IFS
...
IFS=$OLD_IFS
doesn't have the expected behavior if IFS was previously unset
(allowed by LSB and POSIX).
Use subshells or local (LSB but not POSIX).
> # without bash $'\n'
> NL='
> @@ -122,6 +124,14 @@ dry() {
> fi
> verbose "Executing $@"
> $@
$@ doesn't make sense. It should either be
IFS=" "
eval "$*"
(the concatenation of the positional parameters taken as a
command line)
or more likely (without knowing the context)
"$@"
The position parameters are to be interpreted as the arguments
to a simple command.
> + if [ $? -ne 0 ]; then
> + error "FAILED. Exitting!"
> + fi
"$@" || error ...
or
if ! "$@"; then
error ...
fi
> +}
> +
> +is_natural() {
> + test "$1" -ge 0 &> /dev/null && return 1
&> is a bashism.
And depending on the shell, that is not guaranteed to do what
you think it does. for instance, it could say that "1+1", "1 ",
"1.2", are natural numbers.
Also, you seem to have it backward.
It will return 1 (failure/false) if the number is natural.
is_natural()
case "$1" in
("" | *[!0-9]*) return 1;;
(*) return 0;;
esac
> }
>
> cleanup() {
> @@ -365,12 +375,42 @@ resize_xfs() {
> fi
> }
>
> +make_ext() {
> + device=$1
> + fstyp=$2
> + stripe=$3
> + stripesize=$4
> + bsize=4
> +
> + if [ "$YES" ]; then
> + force="-F"
> + fi
> + stride=$(($stripesize/$bsize))
> + stripewidth=$(($stride*$stripe))
> +
> + dry mkfs.$fstyp $force -b$(($bsize*1024)) -E stride=${stride},stripe-width=${stripewidth} $device
Again, I think that should be either:
set --
[ -n "$YES" ] &&
set -- -F
dry "mkfs.$fstyp" "$@" -b "$(($bsize*1024))" -E stride="${stride},stripe-width=${stripewidth}" "$device"
or:
force=
[ -n "$YES" ] &&
force=-F
eval 'dry "mkfs.$fstyp" '"$force"' -b "$(($bsize*1024))" -E stride="${stride},stripe-width=${stripewidth}" "$device"'
[...]
> + is_natural $NEWSIZE
> + [ $? -ne 1 ] && error "$NEWSIZE is not valid number for file system size"
With a fixed is_natural,
is_natural "$NEWSIZE" || error ...
[...]
> + for i in $@; do
for i do
...
> + if [ -b $i ]; then
...and so on.
--
Stephane
_______________________________________________
linux-lvm mailing list
linux-lvm@redhat.com
https://www.redhat.com/mailman/listinfo/linux-lvm
read the LVM HOW-TO at http://tldp.org/HOWTO/LVM-HOWTO/
