[RFC][PATCH v3 00/16] fsadm update

[RFC][PATCH v3 00/16] fsadm update

[Lukas Czerner]

Hello again,

this is third update of the fsadm.

Changes:

version 2:
The main difference is that all the big features are collapsed into one
patch for better review (since some of the features depends on each other
and has been developed in that way). Also one new test is added to exercise
"remove" command.

version 3:
In version 2 some patches was unintentionally squashed together, which is
fixed in this version.

Here is a link to the first version of the patch set:
http://www.redhat.com/archives/linux-lvm/2011-September/msg00046.html

Comments are welcomed.

Thanks!
-Lukas

-- 
[PATCH v3 01/18] fsadm: Add new commands create,list,add and remove
[PATCH v3 02/18] fsadm: Make all internal math in kilobytes
[PATCH v3 03/18] fsadm: Add simple configuration file
[PATCH v3 04/18] fsadm: Use DEFAULT_DEVICE_POOL when creating volume
[PATCH v3 05/18] fsadm: Add LVOL_PREFIX configuration option
[PATCH v3 06/18] fsadm: Fsck extN before resize only if it is not
[PATCH v3 07/18] fsadm: Only use readlink if link is provided
[PATCH v3 08/18] fsadm: Remove unnecessary modification of PATH
[PATCH v3 09/18] fsadm: Allow to specify lv in vg/lv format
[PATCH v3 10/18] fsadm: Umount ext2 file system prior resize
[PATCH v3 11/18] fsadm: Add help for new commands and update man
[PATCH v3 12/18] lvresize: Specify --resize-fs-only when going to
[PATCH v3 13/18] fsadm: remove -y (YES) option
[PATCH v3 14/18] test: add helper to compute aligned lv size
[PATCH v3 15/18] test: Add test for fsadm add command
[PATCH v3 16/18] test: Add test for fsadm create command
[PATCH v3 17/18] test: Add test for fsadm resize command
[PATCH v3 18/18] test: Add test for fsadm remove command

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Lukas Czerner]

This commit adds new functionality in the form of new commands. Namely
it is create, list, add and remove. This commit also changes the way how
are commands recognised an executed. The new approach is more suitable
for bigger number of commands.

Resize command is also significantly reworked. Unlike in the old
approach fsadm will always attempt to resize the logical volume as well,
since this is what it is expected. Leave the --lvresize option for
backwards compatibility, but remove it from the help. If no file system
resides on the logical volume, only the volume will be resized.

* Create command provides the functionality of creating a new logical
  volumes including defined file system.
* List command provides the functionality of listing useful information
  about the logical volumes, file systems and devices.
* Add command allows to add devices into volume groups (pool).
* Remove command allows to remove the volumes or volume groups from the
  system.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |  978 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 908 insertions(+), 70 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index c8cc5e0..f0da586 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -14,10 +14,16 @@
 #
 # Author: Zdenek Kabelac <zkabelac@redhat.com>
 #
-# Script for resizing devices (usable for LVM resize)
+# Author: Lukas Czerner <lczerner@redhat.com>
+#    - Significantly extended fsadm functionality
+#    - Implemented new commands "add", "create", "list", "remove"
+#    - Rework of "resize" command
+#
+# Utility to manage logical volumes
 #
 # Needed utilities:
-#   mount, umount, grep, readlink, blockdev, blkid, fsck, xfs_check
+#   mount, umount, grep, readlink, blockdev, blkid, fsck, xfs_check, bc, df
+#   xfs_db, mktemp
 #
 # ext2/ext3/ext4: resize2fs, tune2fs
 # reiserfs: resize_reiserfs, reiserfstune
@@ -29,7 +35,7 @@
 #   2 break detected
 #   3 unsupported online filesystem check for given mounted fs
 
-TOOL=fsadm
+TOOL=$(basename "$0")
 
 _SAVEPATH=$PATH
 PATH=/sbin:/usr/sbin:/bin:/usr/sbin:$PATH
@@ -54,6 +60,8 @@ READLINK=readlink
 READLINK_E="-e"
 FSCK=fsck
 XFS_CHECK=xfs_check
+BC=bc
+DF=df
 
 # user may override lvm location by setting LVM_BINARY
 LVM=${LVM_BINARY:-lvm}
@@ -63,7 +71,6 @@ DRY=0
 VERB=
 FORCE=
 EXTOFF=0
-DO_LVRESIZE=0
 FSTYPE=unknown
 VOLUME=unknown
 TEMPDIR="${TMPDIR:-/tmp}/${TOOL}_${RANDOM}$$/m"
@@ -73,10 +80,16 @@ BLOCKCOUNT=
 MOUNTPOINT=
 MOUNTED=
 REMOUNT=
+IN_GROUP=
+NOT_IN_GROUP=
+GROUP=
 PROCMOUNTS="/proc/mounts"
+PROCDEVICES="/proc/devices"
+PROCPARTITIONS="/proc/partitions"
 NULL="$DM_DEV_DIR/null"
 
-IFS_OLD=$IFS
+MAX_VGS=999
+
 # without bash $'\n'
 NL='
 '
@@ -96,7 +109,6 @@ tool_usage() {
 	echo "    -e | --ext-offline  unmount filesystem before ext2/ext3/ext4 resize"
 	echo "    -f | --force        Bypass sanity checks"
 	echo "    -n | --dry-run      Print commands without running them"
-	echo "    -l | --lvresize     Resize given device (if it is LVM device)"
 	echo "    -y | --yes          Answer \"yes\" at any prompts"
 	echo
 	echo "  new_size - Absolute number of filesystem blocks to be in the filesystem,"
@@ -115,13 +127,37 @@ error() {
 	cleanup 1
 }
 
-dry() {
+warn() {
+	echo "$TOOL: $@" >&2
+}
+
+dry_nofail() {
 	if [ "$DRY" -ne 0 ]; then
 		verbose "Dry execution $@"
 		return 0
 	fi
 	verbose "Executing $@"
-	$@
+	local IFS=" "
+	eval "$*"
+}
+
+dry() {
+	dry_nofail "$@" || error "FAILED. Exitting!"
+}
+
+float_math() {
+	if [ $# -le 0 ]; then
+		return
+	fi
+	result=$(LANG=C echo "scale=2; $@" | "$BC" -q 2> /dev/null)
+	echo "$result"
+}
+
+is_natural() {
+	case "$1" in
+		"" | *[!0-9]*) return 1;;
+		*) return 0;;
+	esac
 }
 
 cleanup() {
@@ -132,54 +168,140 @@ cleanup() {
 		verbose "Remounting unmounted filesystem back"
 		dry "$MOUNT" "$VOLUME" "$MOUNTED"
 	fi
-	IFS=$IFS_OLD
 	trap 2
 
 	test "$1" -eq 2 && verbose "Break detected"
 
-	if [ "$DO_LVRESIZE" -eq 2 ]; then
-		# start LVRESIZE with the filesystem modification flag
-		# and allow recursive call of fsadm
-		_FSADM_YES=$YES
-		export _FSADM_YES
-		unset FSADM_RUNNING
-		test -n "$LVM_BINARY" && PATH=$_SAVEPATH
-		dry exec "$LVM" lvresize $VERB $FORCE -r -L${NEWSIZE}b "$VOLUME_ORIG"
-	fi
-
 	# error exit status for break
 	exit ${1:-1}
 }
 
+#####################################
+# Convet the size into human readable
+# form. size in Bytes expected
+#####################################
+humanize_size() {
+	size="$1"
+	count=0
+	while [ ${size%%.*} -ge 1024 ] && [ $count -lt 7 ]; do
+		size=$(float_math $size/1024)
+		count=$(($count+1))
+	done
+	case $count in
+		0) unit="B" ;;
+		1) unit="KiB" ;;
+		2) unit="MiB" ;;
+		3) unit="GiB" ;;
+		4) unit="TiB" ;;
+		5) unit="PiB" ;;
+		6) unit="EiB" ;;
+		7) unit="ZiB" ;;
+		8) unit="YiB" ;;
+		*) unit="???" ;;
+	esac
+	echo "$size $unit"
+}
+
+#############################
+# Get size of entN filesystem
+# by reading tune2fs output
+#############################
+get_ext_size() {
+	local IFS=$NL
+	for i in $(LANG=C $TUNE_EXT -l "$VOLUME"); do
+		case "$i" in
+			"Block size"*) bsize=${i##*  } ;;
+			"Block count"*) bcount=${i##*  } ;;
+			"Reserved block count"*) rbcount=${i##*  } ;;
+			"Free blocks"*) fbcount=${i##*  } ;;
+		esac
+	done
+
+	total=$(($bcount*$bsize))
+	TOTAL=$(humanize_size $total)
+	used=$((($bcount-$fbcount)*$bsize))
+	USED=$(humanize_size $used)
+	free=$((($fbcount-$rbcount)*$bsize))
+	FREE=$(humanize_size $free)
+}
+
+############################
+# Get size of xfs file system
+# by reading the df output or
+# examine file system with
+# xfs_db tool
+#############################
+get_xfs_size() {
+	local IFS=$NL
+	if [ -z "$MOUNTED" ]; then
+
+		for i in $(LANG=C xfs_db -c 'sb' -c 'print blocksize fdblocks dblocks logblocks agcount' $VOLUME); do
+			case "$i" in
+				"blocksize ="*) bsize=${i##* } ;;
+				"fdblocks ="*) fbcount=${i##* } ;;
+				"dblocks ="*) bcount=${i##* } ;;
+				"logblocks ="*) lbcount=${i##* } ;;
+				"agcount ="*) agcount=${i##* } ;;
+			esac
+		done
+		bcount=$(($bcount-$lbcount))
+		fbcount=$(($fbcount-(4+(4*$agcount))))
+
+		total=$(($bcount*$bsize))
+		TOTAL=$(humanize_size $total)
+		used=$((($bcount-$fbcount)*$bsize))
+		USED=$(humanize_size $used)
+		free=$(($fbcount*$bsize))
+		FREE=$(humanize_size $free)
+		return
+	fi
+	line=$("$DF" -k "$VOLUME" | "$GREP" -e "$MOUNTED$" | sed -e 's/  */ /g')
+	line=${line#* }
+	total=$(echo $line | cut -d' ' -f1)
+	TOTAL=$(humanize_size $total)
+	free=$(echo $line | cut -d' ' -f3)
+	FREE=$(humanize_size $free)
+	used=$(echo $line | cut -d' ' -f2)
+	USED=$(humanize_size $used)
+}
+
+detect_fs_size() {
+	if [ -z "$FSTYPE" ]; then
+		return
+	fi
+	case "$FSTYPE" in
+		ext[234]) get_ext_size ;;
+		xfs) get_xfs_size ;;
+		*) return 1 ;;
+	esac
+	return 0
+}
+
 # convert parameter from Exa/Peta/Tera/Giga/Mega/Kilo/Bytes and blocks
 # (2^(60/50/40/30/20/10/0))
 decode_size() {
 	case "$1" in
-	 *[eE]) NEWSIZE=$(( ${1%[eE]} * 1152921504606846976 )) ;;
-	 *[pP]) NEWSIZE=$(( ${1%[pP]} * 1125899906842624 )) ;;
-	 *[tT]) NEWSIZE=$(( ${1%[tT]} * 1099511627776 )) ;;
-	 *[gG]) NEWSIZE=$(( ${1%[gG]} * 1073741824 )) ;;
-	 *[mM]) NEWSIZE=$(( ${1%[mM]} * 1048576 )) ;;
-	 *[kK]) NEWSIZE=$(( ${1%[kK]} * 1024 )) ;;
+	 *[eE]) NEWSIZE=$(float_math "${1%[eE]} * 1152921504606846976") ;;
+	 *[pP]) NEWSIZE=$(float_math "${1%[pP]} * 1125899906842624") ;;
+	 *[tT]) NEWSIZE=$(float_math "${1%[tT]} * 1099511627776") ;;
+	 *[gG]) NEWSIZE=$(float_math "${1%[gG]} * 1073741824") ;;
+	 *[mM]) NEWSIZE=$(float_math "${1%[mM]} * 1048576") ;;
+	 *[kK]) NEWSIZE=$(float_math "${1%[kK]} * 1024") ;;
 	 *[bB]) NEWSIZE=${1%[bB]} ;;
 	     *) NEWSIZE=$(( $1 * $2 )) ;;
 	esac
-	#NEWBLOCKCOUNT=$(round_block_size $NEWSIZE $2)
-	NEWBLOCKCOUNT=$(( $NEWSIZE / $2 ))
 
-	if [ $DO_LVRESIZE -eq 1 ]; then
-		# start lvresize, but first cleanup mounted dirs
-		DO_LVRESIZE=2
-		cleanup 0
-	fi
+	NEWSIZE=${NEWSIZE%%.*}
+	NEWBLOCKCOUNT=$(float_math "$NEWSIZE / $2")
+	NEWBLOCKCOUNT=${NEWBLOCKCOUNT%%.*}
 }
 
 # detect filesystem on the given device
 # dereference device name if it is symbolic link
 detect_fs() {
-	VOLUME_ORIG=$1
+	VOLUME_ORIG="$1"
 	VOLUME=${1/#"${DM_DEV_DIR}/"/}
-	VOLUME=$("$READLINK" $READLINK_E "$DM_DEV_DIR/$VOLUME") || error "Cannot get readlink \"$1\""
+	VOLUME=$("$READLINK" "$READLINK_E" "$DM_DEV_DIR/$VOLUME") || error "Cannot get readlink \"$1\""
 	RVOLUME=$VOLUME
 	case "$RVOLUME" in
           # hardcoded /dev  since udev does not create these entries elsewhere
@@ -189,10 +311,10 @@ detect_fs() {
 	esac
 	# use null device as cache file to be sure about the result
 	# not using option '-o value' to be compatible with older version of blkid
-	FSTYPE=$("$BLKID" -c "$NULL" -s TYPE "$VOLUME") || error "Cannot get FSTYPE of \"$VOLUME\""
+	FSTYPE=$("$BLKID" -c "$NULL" -s TYPE "$VOLUME") || return 1
 	FSTYPE=${FSTYPE##*TYPE=\"} # cut quotation marks
 	FSTYPE=${FSTYPE%%\"*}
-	verbose "\"$FSTYPE\" filesystem found on \"$VOLUME\""
+	return 0
 }
 
 # check if the given device is already mounted and where
@@ -200,10 +322,10 @@ detect_fs() {
 detect_mounted()  {
 	test -e "$PROCMOUNTS" || error "Cannot detect mounted device \"$VOLUME\""
 
-	MOUNTED=$("$GREP" ^"$VOLUME" "$PROCMOUNTS")
+	MOUNTED=$("$GREP" -e "^$VOLUME " "$PROCMOUNTS")
 
 	# for empty string try again with real volume name
-	test -z "$MOUNTED" && MOUNTED=$("$GREP" ^"$RVOLUME" "$PROCMOUNTS")
+	test -z "$MOUNTED" && MOUNTED=$("$GREP" -e "^$RVOLUME " "$PROCMOUNTS")
 
 	# cut device name prefix and trim everything past mountpoint
 	# echo translates \040 to spaces
@@ -212,8 +334,8 @@ detect_mounted()  {
 
 	# for systems with different device names - check also mount output
 	if test -z "$MOUNTED" ; then
-		MOUNTED=$(LANG=C "$MOUNT" | "$GREP" ^"$VOLUME")
-		test -z "$MOUNTED" && MOUNTED=$(LANG=C "$MOUNT" | "$GREP" ^"$RVOLUME")
+		MOUNTED=$(LANG=C "$MOUNT" | "$GREP" -e "^$VOLUME ")
+		test -z "$MOUNTED" && MOUNTED=$(LANG=C "$MOUNT" | "$GREP" -e "^$RVOLUME ")
 		MOUNTED=${MOUNTED##* on }
 		MOUNTED=${MOUNTED% type *} # allow type in the mount name
 	fi
@@ -295,17 +417,19 @@ resize_ext() {
 	if [ "$NEWBLOCKCOUNT" -lt "$BLOCKCOUNT" -o "$EXTOFF" -eq 1 ]; then
 		detect_mounted && verbose "$RESIZE_EXT needs unmounted filesystem" && try_umount
 		REMOUNT=$MOUNTED
-		if test -n "$MOUNTED" ; then
-			# Forced fsck -f for umounted extX filesystem.
-			case "$-" in
-			  *i*) dry "$FSCK" $YES -f "$VOLUME" ;;
-			  *) dry "$FSCK" -f -p "$VOLUME" ;;
-			esac
-		fi
+	fi
+
+	# We should really do the fsck before every resize.
+	if [ -z "$MOUNTED" ] || [ "$REMOUNT" ]; then
+		# Forced fsck -f for umounted extX filesystem.
+		case "$-" in
+		  *i*) dry "$FSCK" "$YES" -f "$VOLUME" ;;
+		  *) dry "$FSCK" -f -p "$VOLUME" ;;
+		esac
 	fi
 
 	verbose "Resizing filesystem on device \"$VOLUME\" to $NEWSIZE bytes ($BLOCKCOUNT -> $NEWBLOCKCOUNT blocks of $BLOCKSIZE bytes)"
-	dry "$RESIZE_EXT" $FSFORCE "$VOLUME" $NEWBLOCKCOUNT
+	dry "$RESIZE_EXT" "$FSFORCE" "$VOLUME" "$NEWBLOCKCOUNT"
 }
 
 #############################
@@ -327,9 +451,9 @@ resize_reiser() {
 	decode_size $1 $BLOCKSIZE
 	verbose "Resizing \"$VOLUME\" $BLOCKCOUNT -> $NEWBLOCKCOUNT blocks ($NEWSIZE bytes, bs: $NEWBLOCKCOUNT)"
 	if [ -n "$YES" ]; then
-		echo y | dry "$RESIZE_REISER" -s $NEWSIZE "$VOLUME"
+		echo y | dry "$RESIZE_REISER" -s "$NEWSIZE" "$VOLUME"
 	else
-		dry "$RESIZE_REISER" -s $NEWSIZE "$VOLUME"
+		dry "$RESIZE_REISER" -s "$NEWSIZE" "$VOLUME"
 	fi
 }
 
@@ -357,7 +481,7 @@ resize_xfs() {
 	decode_size $1 $BLOCKSIZE
 	if [ $NEWBLOCKCOUNT -gt $BLOCKCOUNT ]; then
 		verbose "Resizing Xfs mounted on \"$MOUNTPOINT\" to fill device \"$VOLUME\""
-		dry "$RESIZE_XFS" $MOUNTPOINT
+		dry "$RESIZE_XFS" "$MOUNTPOINT"
 	elif [ $NEWBLOCKCOUNT -eq $BLOCKCOUNT ]; then
 		verbose "Xfs filesystem already has the right size"
 	else
@@ -365,25 +489,736 @@ resize_xfs() {
 	fi
 }
 
+#####################################
+# Create extN filesystem with respect
+# to the striped volume
+#####################################
+make_ext() {
+	device="$1"
+	fstyp="$2"
+	bsize=4
+
+	if [ "$YES" ]; then
+		force="-F"
+	fi
+
+	dry "mkfs.$fstyp" "$force" -b "$(($bsize*1024))" $device
+}
+
+############################################
+# Create a file system just using mkfs.fstyp
+############################################
+generic_make_fs() {
+	device=$1
+	fstyp=$2
+	bsize=4096
+
+	if [ "$YES" ]; then
+		force="-f"
+	fi
+
+	dry "mkfs.$fstyp" $force "$device"
+}
+
 ####################
 # Resize filesystem
 ####################
-resize() {
+resize_fs() {
 	NEWSIZE=$2
-	detect_fs "$1"
+	detect_fs "$1" ||  error "Cannot get FSTYPE of \"$VOLUME\""
+	verbose "\"$FSTYPE\" filesystem found on \"$VOLUME\""
+	if [ "$NEWSIZE" ]; then
+		is_natural $NEWSIZE ||  error "$NEWSIZE is not valid number for file system size"
+	fi
 	detect_device_size
 	verbose "Device \"$VOLUME\" size is $DEVSIZE bytes"
 	# if the size parameter is missing use device size
 	#if [ -n "$NEWSIZE" -a $NEWSIZE <
 	test -z "$NEWSIZE" && NEWSIZE=${DEVSIZE}b
-	IFS=$NL
+	local IFS=$NL
 	case "$FSTYPE" in
 	  "ext3"|"ext2"|"ext4") resize_ext $NEWSIZE ;;
 	  "reiserfs") resize_reiser $NEWSIZE ;;
 	  "xfs") resize_xfs $NEWSIZE ;;
 	  *) error "Filesystem \"$FSTYPE\" on device \"$VOLUME\" is not supported by this tool" ;;
 	esac || error "Resize $FSTYPE failed"
-	cleanup 0
+}
+
+resize_lvolume() {
+	lvname=$1
+	newsize=$2
+	lvsize=$(LANG=C "$LVM" lvs -o lv_size --separator ' ' --noheadings --nosuffix --units b $lvname 2> /dev/null | sed -e 's/^ *//')
+	if [ $lvsize != $newsize ]; then
+		dry "$LVM" lvresize "$VERB" "$FORCE" -L"${newsize}"b "$lvname"
+	else
+		verbose "Logical volume already is of the size you're trying to resize it to"
+	fi
+}
+
+resize() {
+	local size=
+
+	# Special case for the situation we have been called from within the lvresize code.
+	if [ "$RESIZE_FS_ONLY" ]; then
+		resize_fs "$@"
+		return
+	fi
+
+	for i in "$@"; do
+		if [ -b "$i" ]; then
+			if [ -z "$devcount" ]; then
+				"$LVM" lvs "$i" &> /dev/null || error "$i is not valid logical volume"
+				lvname=$i
+				devcount=0
+				continue
+			fi
+			devices="$devices $i"
+			devcount=$(($devcount+1))
+			continue
+		fi
+		case $i in
+			"size="*)		[ -z "$size" ] && size=${i##*=};;
+			[[:digit:]]*)		[ -z "$size" ] && size=${i##*=};;
+			+[[:digit:]]*)		[ -z "$size" ] && size=${i##*=};;
+			-[[:digit:]]*)		[ -z "$size" ] && size=${i##*=};;
+			*) error "Wrong option $i. (see: $TOOL --help)";;
+		esac
+	done
+
+	if [ -z "$size" ]; then
+		error "Please provide the size argument. (see: $TOOL --help )"
+	fi
+
+	[ -z $lvname ] && error "Logical volume to resize was not specified"
+	[ $devcount -gt 0 ] && [ "$shrink" ] && warn "While shrinking the file system we "\
+						     "do not need additional devices. "\
+						     "Those provided will be ignored"
+
+	# We need to add provided devices into particular group
+	if [ $devcount -gt 0 ] && [ -z $shrink ]; then
+		# Get the volume group of the logical volume
+		vgname=$(detect_volume_group $lvname)
+		[ -z $vgname ] && error "Wrong argument $lvname. (see: $TOOL --help)"
+
+		# Determine whether the devices are in the group
+		detect_device_group $devices ||
+			error "Devices are in different groups. Please run "\
+				"\"$TOOL list\" to get more information on layout."
+
+		[ "$GROUP" ] && [ "$GROUP" != "$vgname" ] && error "Some devices are in different"\
+								   "group than the logical volume"\
+								   "($lvname). Please provide unused"\
+								   "devices"
+		# Add missing devices into the group
+		if [ "$NOT_IN_GROUP" ]; then
+			dry "$LVM" vgextend "$VERB" $vgname $NOT_IN_GROUP
+		fi
+	fi
+
+	resizefs=
+	detect_fs $lvname
+	[ "$FSTYPE" ] && resizefs="-r"
+
+	# Avoid callinf lvresize in recurse
+	if [ "$FSADM_RESIZE_RECURSE" ]; then
+		error "Recursive lvresize call detected! Exiting."
+	fi
+
+	# We are going to call lvresize with -r parameter which will not only
+	# resize the logical volume, but also the file system (if present)
+	# by calling fsadm recursively with --resize-fs-only argument.
+	# To be able to do this we should set some appropriate variables.
+	export _FSADM_YES=$YES
+	export FSADM_RESIZE_RECURSE=1
+	unset FSADM_RUNNING
+
+	dry exec "$LVM" lvresize $VERB $FORCE $resizefs -L${size} "$lvname"
+
+}
+
+#################################
+# Check the device list to detect
+# if there is not multiple groups
+#################################
+detect_device_group() {
+	ret=0
+	prev_vgname=""
+	vgs=""
+	tmp=$(mktemp)
+
+	LANG=C "$LVM" vgs -o vg_name,pv_name --separator ' ' --noheadings --nosuffix 2> /dev/null | sed -e 's/^ *//' | sort | uniq > $tmp
+	NOT_IN_GROUP=
+	IN_GROUP=
+	for i in "$@"; do
+		cur_vgname=$("$GREP" -e "$i$" "$tmp" | cut -d' ' -f1)
+
+		if [ -z "$cur_vgname" ]; then
+			NOT_IN_GROUP+="$i "
+			continue
+		else
+			IN_GROUP+="$i "
+		fi
+
+		if [ -z "$prev_vgname" ]; then
+			prev_vgname=$cur_vgname
+			vgs=$cur_vgname
+			continue;
+		fi
+
+		if [ "$cur_vgname" != "$prev_vgname" ]; then
+			ret=1
+			vgs+=" $cur_vgname"
+		fi
+	done
+	rm -f $tmp
+	GROUP=$vgs
+	return $ret
+}
+
+#############################
+# Create a new logical volume
+#############################
+create() {
+	devcount=0
+	vg_create=0
+
+	for i in "$@"; do
+		if [ -b $i ]; then
+			devices="$devices $i"
+			devcount=$(($devcount+1))
+			continue
+		fi
+		case $i in
+			"stripesize"*) stripesize=${i##*=} ;;
+			"name"*) name=${i##*=} ;;
+			"fstyp"* | "fs"*) fstyp=${i##*=} ;;
+			"size"*) size=${i##*=} ;;
+			"stripes"*) stripes=${i##*=} ;;
+			*) if [ -z $vgname ]; then vgname=$i; else
+				error "Wrong option $i. (see: $TOOL --help)"
+			   fi ;;
+		esac
+	done
+
+	detect_device_group $devices ||
+		error "Devices are in different groups. Please run "\
+			"\"$TOOL list\" to get more information on layout."
+
+	if [ "$GROUP" ] && [ "$vgname" ];then
+		if [ "$vgname" != "$GROUP" ]; then
+			error "You specified group ($vgname) and devices which does "\
+				"belong to a different group ($GROUP). Either remove "\
+				"devices first, or specify devices which are free."
+		fi
+	elif [ "$GROUP" ]; then
+		vgname=$GROUP
+	fi
+
+	# Devices are not in any group so we should find some.
+	# Either create a new one, or use the existing one, if
+	# there is only one vgroup, or use the one with enough
+	# free space in it
+	if [ -z "$vgname" ]; then
+		vgroups=$("$LVM" vgs -o vg_name,vg_free --separator ' ' --noheadings --units b 2> /dev/null)
+		lines=$("$LVM" vgs -o vg_name,vg_free --separator ' ' --noheadings --units b 2> /dev/null | wc -l)
+
+		if [ $lines -eq 1 ]; then
+			vgname=$(echo $vgroups | sed -e 's/^ *//' | cut -d' ' -f1)
+		elif [ $lines -gt 1 ]; then
+			if [ -z $size ]; then
+				error "Not enough information to create" \
+					"logical volume"
+			fi
+			decode_size $size 1
+			new_size=$NEWBLOCKCOUNT
+			IFS=$NL
+			for i in $vgroups; do
+				vgsize=$(echo $i | sed -e 's/^ *//' | cut -d' ' -f2)
+				vgsize=${vgsize%%B}
+				if [ $vgsize -ge $new_size ]; then
+					vgname=$(echo $i | sed -e 's/^ *//' | cut -d' ' -f1)
+					break;
+				fi
+			done
+		fi
+	fi
+	IFS=$IFS_OLD
+
+	if [ -z "$vgname" ]; then
+		[ $devcount -eq 0 ] && error "No suitable device specified."
+		for i in $(seq -w $MAX_VGS); do
+			"$LVM" vgs vg${i} &> /dev/null
+			if [ $? -ne 0 ]; then
+				vgname="vg${i}"
+				break;
+			fi
+		done
+		vg_create=1
+	fi
+
+	[ -z "$vgname" ] && error "No suitable name for volume group found."
+
+	if [ "$stripesize" ] || [ "$stripes" ]; then
+		if [ -z "$stripes" ] && [ $devcount -eq 0 ]; then
+			error "Stripe size specified ($stripesize), but " \
+			      "neither devices, nor number of stripes " \
+			      "has been provided"
+		fi
+		[ $devcount -gt 0 ] && [ -z "$stripes" ] && stripes=$devcount
+		if [ "$stripesize" ]; then
+			striped="-i $stripes -I $stripesize"
+		else
+			striped="-i $stripes"
+		fi
+	fi
+
+	if [ "$name" ]; then
+		lvname="--name $name"
+	else
+		tmp=$(mktemp)
+		"$LVM" lvs "$vgname" --separator ' ' --noheadings > $tmp
+		for i in $(seq -w $MAX_VGS); do
+			"$GREP" -e " lvol${i} " $tmp &> /dev/null
+			if [ $? -ne 0 ]; then
+				name="lvol${i}"
+				lvname="--name $name"
+				break;
+			fi
+		done
+		rm -f $tmp
+	fi
+
+	[ -z "$lvname" ] && error "No suitable name for the logical volume."
+
+	if [ $vg_create -eq 1 ]; then
+		dry "$LVM" vgcreate "$VERB" $vgname $devices
+	elif [ ! -z "$NOT_IN_GROUP" ]; then
+		dry "$LVM" vgextend "$VERB" $vgname $NOT_IN_GROUP
+	fi
+
+
+	if [ -z $size ] && [ -n "$devices" ]; then
+		size="-l 100%PVS"
+	elif [ -z $size ]; then
+		size="-l 100%FREE"
+	else
+		size="-L $size"
+	fi
+	dry "$LVM" lvcreate "$VERB" $lvname $size $striped "$vgname" $devices
+	device="/dev/${vgname}/${name}"
+
+	guess=0
+	if [ -z $fstyp ]; then
+		fstyp=$(echo $TOOL | sed 's/^.*\.//g')
+		guess=1
+	fi
+
+	case $fstyp in
+		ext[234]) make_ext $device $fstyp;;
+		xfs|reiserfs) generic_make_fs $device $fstyp;;
+		*)	if [ $guess -eq 1 ]; then
+				return 0
+			else
+				error "Filesystem $fstyp is not supported"
+			fi
+			;;
+	esac
+}
+
+###############################
+# Remove device form the group
+###############################
+remove_device() {
+	vgname=$("$LVM" pvs "$1" -o vg_name --separator ' ' --noheadings) 2> /dev/null || return 1
+	vgname=$(echo $vgname | sed 's/[ \t]*//')
+	if [ "$vgname" ]; then
+		dry_nofail "$LVM" vgreduce "$vgname" "$device"
+	else
+		warn "Device $1 is not in any pool."
+	fi
+	return 0
+}
+
+#############################
+# Remove the logical volume
+# volume group, or the device
+# from the group
+#############################
+do_remove() {
+	item="${1%% }"
+	device=
+	MOUNTED=
+
+	# Block device has been given
+	if [ -b "$item" ]; then
+		device=$item
+	fi
+
+	# Mount point has been given
+	if [ -z "$device" ] && [ -d "$item" ]; then
+		mp=$(echo "$item" | sed -e 's/\/$//')
+
+		count=$("$GREP" " $mp " "$PROCMOUNTS" | wc -l)
+
+		if [ $count -eq 1 ]; then
+			device=$("$GREP" " $mp " "$PROCMOUNTS" | cut -d' ' -f1)
+		else
+			count=$(LANG=C "$MOUNT" | "$GREP" " $mp " | wc -l)
+			[ $count -ne 1 ] && warn "Could not find device mounted at $mp" && return
+			device=$(LANG=C "$MOUNT" | "$GREP" " $mp " | cut -d' ' -f1)
+		fi
+		VOLUME=$device
+	fi
+
+	if [ -z "$device" ]; then
+		"$LVM" vgs "$item" &> /dev/null
+		# Volume group has been given
+		if [ $? -eq 0 ]; then
+			dry "$LVM" vgremove "$FORCE" "$item"
+			return
+		fi
+	fi
+
+	[ -z "$device" ] && warn "$item is not a valid mount point, dm device nor volume group name."&& return
+
+	# Try to remove device from the group
+	remove_device "$device" && return 0
+
+	detect_mounted && try_umount
+	dry "$LVM" lvremove "$FORCE" "$device"
+	return
+}
+
+###############################
+# Iterate through the arguments
+# and do_remove on them
+###############################
+remove() {
+	if [ $# -eq 0 ]; then
+		error "Please specify what do you want to remove"\
+		      "(see $TOOL remove --help)."
+	fi
+	# help
+	if [ "$1" == "help" ]; then
+		echo "Usage: $TOOL remove [mount point | dm device | voulume group | device | --all]"
+		exit 0
+	elif [ "$1" == "--all" ]; then
+		list="$(LANG=C $LVM vgs -o vg_name --separator ' ' --noheadings --nosuffix --units b 2> /dev/null)"
+	else
+		list="$@"
+	fi
+
+	for item in $list; do
+		do_remove "$item"
+	done
+}
+
+#############################
+# List all file systems built
+# on top of DM device
+#############################
+list_filesystems() {
+	local IFS=$NL
+	local c=0
+	for line in $(LANG=C "$LVM" lvs -o lv_path,lv_size,segtype --noheadings --separator ' ' --nosuffix --units b 2> /dev/null); do
+		line=$(echo $line | sed -e 's/^ *\//\//')
+		volume=$(echo $line | cut -d' ' -f1)
+		[ "$volume" == "$last_volume" ] && continue
+		c=$((c+1))
+		local volumes[$c]=$volume
+		local segtype[$c]=$(echo $line | cut -d' ' -f3)
+		local lvsize[$c]=$(humanize_size $(echo $line | cut -d' ' -f2))
+		detect_fs "$volume"
+		detect_mounted
+		detect_fs_size
+		local total[$c]=$TOTAL
+		local fstype[$c]=$FSTYPE
+		local free[$c]=$FREE
+		local used[$c]=$USED
+		local mounted[$c]=$MOUNTED
+		FSTYPE=
+		FREE=
+		USED=
+		TOTAL=
+		MOUNTED=
+		last_volume=$volume
+	done
+
+	if [ $c -eq 0 ]; then
+		warn " No file systems suitable for managing by $TOOL found."
+		return
+	fi
+
+	len_volume=6
+	len_fstype=2
+	len_free=4
+	len_used=4
+	len_total=5
+	len_mounted=11
+	len_segtype=4
+	len_lvsize=4
+	for i in $(seq $c); do
+		local _volume=${volumes[$i]}
+		local _fstype=${fstype[$i]}
+		local _total=${total[$i]}
+		local _free=${free[$i]}
+		local _used=${used[$i]}
+		local _segtype=${segtype[$i]}
+		local _lvsize=${lvsize[$i]}
+		local _mounted=${mounted[$i]}
+		[ ${#_volume} -gt "0$len_volume" ] && len_volume=${#_volume}
+		[ ${#_fstype} -gt "0$len_fstype" ] && len_fstype=${#_fstype}
+		[ ${#_total} -gt "0$len_total" ] && len_total=${#_total}
+		[ ${#_free} -gt "0$len_free" ] && len_free=${#_free}
+		[ ${#_used} -gt "0$len_used" ] && len_used=${#_used}
+		[ ${#_segtype} -gt "0$len_segtype" ] && len_segtype=${#_segtype}
+		[ ${#_lvsize} -gt "0$len_lvsize" ] && len_lvsize=${#_lvsize}
+		[ ${#_mounted} -gt "0$len_mounted" ] && len_mounted=${#_mounted}
+	done
+
+	format="%-$(($len_volume+2))s %$(($len_lvsize))s  %-$(($len_fstype+2))s%$(($len_free))s  %$(($len_used))s  %$(($len_total))s  %-$(($len_segtype+2))s%-$(($len_mounted))s\n"
+	header=$(printf "$format" "Volume" "LV size" "FS" "Free" "Used" "Total" "Type" "Mount point")
+	separator=""
+	for i in $(seq ${#header}); do
+		separator+="-"
+	done
+	echo $separator
+	printf "$format" "Volume" "LV size" "FS" "Free" "Used" "Total" "Type" "Mount point"
+	echo $separator
+
+	for i in $(seq $c); do
+		printf "$format" "${volumes[$i]}" "${lvsize[$i]}" "${fstype[$i]}" "${free[$i]}" "${used[$i]}" "${total[$i]}" "${segtype[$i]}" "${mounted[$i]}"
+	done
+
+	echo $separator
+}
+
+###########################
+# List all non DM devices
+###########################
+list_devices() {
+	local IFS=$NL
+	tmp=$(mktemp)
+
+	c=0
+	dmnumber=$(cat "$PROCDEVICES" | "$GREP" device-mapper | sed -e 's/^  *//')
+	dmnumber=${dmnumber%% *}
+	LANG=C "$LVM" pvs -o pv_name,vg_name,pv_size,pv_free,pv_used --separator ' ' --noheadings --nosuffix --units b > $tmp
+	for line in $(cat "$PROCPARTITIONS" | tail -n +3 | sed -e 's/^  *//' | "$GREP" -v -e "^$dmnumber"); do
+		c=$((c+1))
+		line=$(echo $line | sed -e 's/  */ /g')
+		_total=$(($(echo $line | cut -d' ' -f3)*1024))
+		local total[$c]=$(humanize_size ${_total%%.*})
+		VOLUME=$(echo $line | cut -d' ' -f4)
+		RVOLUME="/dev/$(echo $line | cut -d' ' -f4)"
+		line=$("$GREP" -e " $RVOLUME " $tmp)
+		detect_mounted
+		if [ -z $MOUNTED ]; then
+			count=$("$GREP" "$VOLUME" "$PROCPARTITIONS" | wc -l)
+			[ $count -gt 1 ] && MOUNTED="PARTITIONED"
+		fi
+
+		if [ ! -z $line ]; then
+			line=$(echo $line | sed -e 's/^ *\//\//')
+			local group[$c]=$(echo $line | cut -d' ' -f2)
+			_total=$(echo $line | cut -d' ' -f3)
+			local total[$c]=$(humanize_size ${_total%%.*})
+			_free=$(echo $line | cut -d' ' -f4)
+			local free[$c]=$(humanize_size ${_free%%.*})
+			_used=$(echo $line | cut -d' ' -f5)
+			local used[$c]=$(humanize_size ${_used%%.*})
+		fi
+		local volumes[$c]=$RVOLUME
+		local mounted[$c]=$MOUNTED
+		free=
+		used=
+		total=
+		MOUNTED=
+	done
+	rm -f $tmp
+
+	if [ $c -eq 0 ]; then
+		warn " No devices found."
+		return
+	fi
+
+	len_volume=6
+	len_free=4
+	len_used=4
+	len_total=5
+	len_group=5
+	len_mounted=11
+	for i in $(seq $c); do
+		local _volume=${volumes[$i]}
+		local _free=${free[$i]}
+		local _used=${used[$i]}
+		local _total=${total[$i]}
+		local _group=${group[$i]}
+		local _mounted=${mounted[$i]}
+		[ ${#_volume} -gt "0$len_volume" ] && len_volume=${#_volume}
+		[ ${#_free} -gt "0$len_free" ] && len_free=${#_free}
+		[ ${#_used} -gt "0$len_used" ] && len_used=${#_used}
+		[ ${#_total} -gt "0$len_total" ] && len_total=${#_total}
+		[ ${#_group} -gt "0$len_group" ] && len_group=${#_group}
+		[ ${#_mounted} -gt "0$len_mounted" ] && len_mounted=${#_mounted}
+	done
+
+	format="%-$(($len_volume+2))s%$(($len_free))s  %$(($len_used))s  %$(($len_total))s  %-$(($len_group+2))s%-$(($len_mounted))s\n"
+	header=$(printf "$format" "Device" "Free" "Used" "Total" "Group" "Mount point")
+	separator=""
+	for i in $(seq ${#header}); do
+		separator+="-"
+	done
+	echo $separator
+	printf "$format" "Device" "Free" "Used" "Total" "Group" "Mount point"
+	echo $separator
+
+	for i in $(seq $c); do
+		printf "$format" "${volumes[$i]}" "${free[$i]}" "${used[$i]}" "${total[$i]}" "${group[$i]}" "${mounted[$i]}"
+	done
+
+	echo $separator
+}
+
+################################
+# List all pools (volume groups)
+################################
+list_pool() {
+	local IFS=$NL
+	c=0
+	for line in $(LANG=C "$LVM" vgs -o vg_name,pv_count,vg_size,vg_free --separator ' ' --noheadings --nosuffix --units b 2> /dev/null); do
+		c=$((c+1))
+		line=$(echo $line | sed -e 's/^ *//')
+		local group[$c]=$(echo $line | cut -d' ' -f1)
+		local devices[$c]=$(echo $line | cut -d' ' -f2)
+		_total=$(echo $line | cut -d' ' -f3)
+		_free=$(echo $line | cut -d' ' -f4)
+		_used=$((${_total%%.*}-${_free%%.*}))
+		local used[$c]=$(humanize_size ${_used%%.*})
+		local total[$c]=$(humanize_size ${_total%%.*})
+		local free[$c]=$(humanize_size ${_free%%.*})
+	done
+
+	if [ $c -eq 0 ]; then
+		warn " No pools found on the system."
+		return
+	fi
+
+	len_group=5
+	len_devices=6
+	len_free=4
+	len_used=4
+	len_total=5
+	for i in $(seq $c); do
+		local _group=${group[$i]}
+		local _devices=${devices[$i]}
+		local _free=${free[$i]}
+		local _used=${used[$i]}
+		local _total=${total[$i]}
+		[ ${#_group} -gt "0$len_group" ] && len_group=${#_group}
+		[ ${#_devices} -gt "0$len_devices" ] && len_devices=${#_devices}
+		[ ${#_free} -gt "0$len_free" ] && len_free=${#_free}
+		[ ${#_used} -gt "0$len_used" ] && len_used=${#_used}
+		[ ${#_total} -gt "0$len_total" ] && len_total=${#_total}
+	done
+
+	format="%-$(($len_group+2))s%-$(($len_devices+2))s%$(($len_free))s  %$(($len_used))s  %$(($len_total))s\n"
+	header=$(printf "$format" "Group" "Devices" "Free" "Used" "Total")
+	separator=""
+	for i in $(seq ${#header}); do
+		separator+="-"
+	done
+	echo $separator
+	printf "$format" "Group" "Devices" "Free" "Used" "Total"
+	echo $separator
+
+	for i in $(seq $c); do
+		printf "$format" "${group[$i]}" "${devices[$i]}" "${free[$i]}" "${used[$i]}" "${total[$i]}"
+	done
+	echo $separator
+}
+
+#############################
+# List the available storage
+# and file systems
+#############################
+list() {
+	for i in "$@"; do
+		case "$i" in
+			"filesystems" | "fs") list_filesystems ;;
+			"devices" | "dev") list_devices ;;
+			"pool") list_pool ;;
+			*) error "Wrong option $i. (see: $TOOL --help)"
+		esac
+	done
+
+	if [ $# -eq 0 ]; then
+		list_devices
+		echo ""
+		list_pool
+		echo ""
+		list_filesystems
+	fi
+}
+
+detect_volume_group() {
+	_vg=$("$LVM" lvs -o vg_name --separator ' ' --noheadings $1 2> /dev/null)
+	ret=$?
+	echo "$_vg" | sed -e 's/^ *//'
+	return $ret
+}
+
+############################
+# Add devices into the group
+############################
+add() {
+	vg_create=0
+	devcount=0
+	tmp=$(mktemp)
+
+	for i in "$@"; do
+		if [ -b "$i" ]; then
+			devices="$devices $i"
+			devcount=$(($devcount+1))
+			continue
+		elif [ -z "$vgname" ]; then vgname=$i; else
+			error "Wrong argument $i. (see: $TOOL --help)"
+		fi
+	done
+
+	[ $devcount -eq 0 ] && error "No suitable device specified."
+	"$LVM" vgs -o vg_name --separator ' ' --noheadings | sed -e 's/^ *//' > $tmp
+
+	if [ -z "$vgname" ]; then
+		lines=$(wc -l $tmp)
+		lines=${lines%% *}
+
+		if [ $lines -eq 1 ]; then
+			vgname=$(cut -d' ' -f1 $tmp)
+		elif [ $lines -gt 1 ]; then
+			list_pool
+			rm -f $tmp
+			error "Please, specify group you want to add the device into"
+		elif [ $lines -eq 0 ]; then
+			vgname="vg001"
+			vg_create=1
+		fi
+	else
+		"$GREP" -e "$vgname" $tmp &> /dev/null
+		vg_create=$?
+	fi
+	rm -f $tmp
+
+	[ -z "$vgname" ] && error "No suitable name for volume group found."
+	detect_device_group $devices
+
+	if [ $vg_create -eq 1 ]; then
+		dry "$LVM" vgcreate "$VERB" "$vgname" $devices
+	elif [ ! -z "$NOT_IN_GROUP" ]; then
+		dry "$LVM" vgextend "$VERB" "$vgname" $NOT_IN_GROUP
+	else
+		warn "Nothing to do"
+	fi
 }
 
 ####################################
@@ -399,7 +1234,8 @@ diff_dates() {
 # Check filesystem
 ###################
 check() {
-	detect_fs "$1"
+	detect_fs "$1" || error "Cannot get FSTYPE of \"$VOLUME\""
+	verbose "\"$FSTYPE\" filesystem found on \"$VOLUME\""
 	if detect_mounted ; then
 		verbose "Skipping filesystem check for device \"$VOLUME\" as the filesystem is mounted on $MOUNTED";
 		cleanup 3
@@ -407,8 +1243,7 @@ check() {
 
 	case "$FSTYPE" in
 	  "ext2"|"ext3"|"ext4")
-		IFS_CHECK=$IFS
-		IFS=$NL
+		local IFS=$NL
 		for i in $(LANG=C "$TUNE_EXT" -l "$VOLUME"); do
 			case "$i" in
 			  "Last mount"*) LASTMOUNT=${i##*: } ;;
@@ -425,15 +1260,14 @@ check() {
 			fi
 			;;
 		esac
-		IFS=$IFS_CHECK
 	esac
 
 	case "$FSTYPE" in
 	  "xfs") dry "$XFS_CHECK" "$VOLUME" ;;
 	  *)    # check if executed from interactive shell environment
 		case "$-" in
-		  *i*) dry "$FSCK" $YES $FORCE "$VOLUME" ;;
-		  *) dry "$FSCK" $FORCE -p "$VOLUME" ;;
+		  *i*) dry "$FSCK" "$YES" "$FORCE" "$VOLUME" ;;
+		  *) dry "$FSCK" "$FORCE" -p "$VOLUME" ;;
 		esac
 	esac
 }
@@ -475,20 +1309,24 @@ do
 	  "-n"|"--dry-run") DRY=1 ;;
 	  "-f"|"--force") FORCE="-f" ;;
 	  "-e"|"--ext-offline") EXTOFF=1 ;;
+	  "-o"|"--resize-fs-only") RESIZE_FS_ONLY=1 ;;
 	  "-y"|"--yes") YES="-y" ;;
-	  "-l"|"--lvresize") DO_LVRESIZE=1 ;;
-	  "check") CHECK="$2" ; shift ;;
-	  "resize") RESIZE="$2"; NEWSIZE="$3" ; shift 2 ;;
+	  "-l"|"--lvresize") ;;
+	  "check") COMMAND=$1; shift; ARGS="$@"; break ;;
+	  "resize") COMMAND=$1; shift; ARGS="$@"; break ;;
+	  "create") COMMAND=$1; shift; ARGS="$@"; break ;;
+	  "list") COMMAND=$1; shift; ARGS="$@"; break ;;
+	  "add") COMMAND=$1; shift; ARGS="$@"; break ;;
+	  "remove") COMMAND=$1; shift; ARGS="$@"; break ;;
 	  *) error "Wrong argument \"$1\". (see: $TOOL --help)"
 	esac
 	shift
 done
 
-if [ -n "$CHECK" ]; then
-	check "$CHECK"
-elif [ -n "$RESIZE" ]; then
-	export FSADM_RUNNING="fsadm"
-	resize "$RESIZE" "$NEWSIZE"
-else
+if [ -z "$COMMAND" ]; then
 	error "Missing command. (see: $TOOL --help)"
 fi
+
+export FSADM_RUNNING="fsadm"
+$COMMAND $ARGS
+cleanup 0
-- 
1.7.4.4

[PATCH v3 02/18] fsadm: Make all internal math in kilobytes

[Lukas Czerner]

We should make all math in fsadm to count with kilobytes (if possible)
because it will bypass the problem of having too big numbers. Also the
smaller granularity might have troubles with 512B alignment.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |   79 ++++++++++++++++++++++++++---------------------------
 1 files changed, 39 insertions(+), 40 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index f0da586..58ff0ab 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -188,15 +188,14 @@ humanize_size() {
 		count=$(($count+1))
 	done
 	case $count in
-		0) unit="B" ;;
-		1) unit="KiB" ;;
-		2) unit="MiB" ;;
-		3) unit="GiB" ;;
-		4) unit="TiB" ;;
-		5) unit="PiB" ;;
-		6) unit="EiB" ;;
-		7) unit="ZiB" ;;
-		8) unit="YiB" ;;
+		0) unit="KiB" ;;
+		1) unit="MiB" ;;
+		2) unit="GiB" ;;
+		3) unit="TiB" ;;
+		4) unit="PiB" ;;
+		5) unit="EiB" ;;
+		6) unit="ZiB" ;;
+		7) unit="YiB" ;;
 		*) unit="???" ;;
 	esac
 	echo "$size $unit"
@@ -217,11 +216,11 @@ get_ext_size() {
 		esac
 	done
 
-	total=$(($bcount*$bsize))
+	total=$(($bcount*$bsize/1024))
 	TOTAL=$(humanize_size $total)
-	used=$((($bcount-$fbcount)*$bsize))
+	used=$((($bcount-$fbcount)*$bsize/1024))
 	USED=$(humanize_size $used)
-	free=$((($fbcount-$rbcount)*$bsize))
+	free=$((($fbcount-$rbcount)*$bsize/1024))
 	FREE=$(humanize_size $free)
 }
 
@@ -247,11 +246,11 @@ get_xfs_size() {
 		bcount=$(($bcount-$lbcount))
 		fbcount=$(($fbcount-(4+(4*$agcount))))
 
-		total=$(($bcount*$bsize))
+		total=$(($bcount*$bsize/1024))
 		TOTAL=$(humanize_size $total)
-		used=$((($bcount-$fbcount)*$bsize))
+		used=$((($bcount-$fbcount)*$bsize/1024))
 		USED=$(humanize_size $used)
-		free=$(($fbcount*$bsize))
+		free=$(($fbcount*$bsize/1024))
 		FREE=$(humanize_size $free)
 		return
 	fi
@@ -278,21 +277,20 @@ detect_fs_size() {
 }
 
 # convert parameter from Exa/Peta/Tera/Giga/Mega/Kilo/Bytes and blocks
-# (2^(60/50/40/30/20/10/0))
+# (2^(60/50/40/30/20/10/0)) into Kilobytes (KiB)
 decode_size() {
 	case "$1" in
-	 *[eE]) NEWSIZE=$(float_math "${1%[eE]} * 1152921504606846976") ;;
-	 *[pP]) NEWSIZE=$(float_math "${1%[pP]} * 1125899906842624") ;;
-	 *[tT]) NEWSIZE=$(float_math "${1%[tT]} * 1099511627776") ;;
-	 *[gG]) NEWSIZE=$(float_math "${1%[gG]} * 1073741824") ;;
-	 *[mM]) NEWSIZE=$(float_math "${1%[mM]} * 1048576") ;;
-	 *[kK]) NEWSIZE=$(float_math "${1%[kK]} * 1024") ;;
-	 *[bB]) NEWSIZE=${1%[bB]} ;;
-	     *) NEWSIZE=$(( $1 * $2 )) ;;
+	 *[eE]) NEWSIZE=$(float_math "${1%[eE]} * 1125899906842624") ;;
+	 *[pP]) NEWSIZE=$(float_math "${1%[pP]} * 1099511627776") ;;
+	 *[tT]) NEWSIZE=$(float_math "${1%[tT]} * 1073741824") ;;
+	 *[gG]) NEWSIZE=$(float_math "${1%[gG]} * 1048576") ;;
+	 *[mM]) NEWSIZE=$(float_math "${1%[mM]} * 1024") ;;
+	 *[kK]) NEWSIZE=${1%[kK]} ;;
+	 *[bB]) NEWSIZE=$(float_math "${1%[bB]} / 1024") ;;
 	esac
 
 	NEWSIZE=${NEWSIZE%%.*}
-	NEWBLOCKCOUNT=$(float_math "$NEWSIZE / $2")
+	NEWBLOCKCOUNT=$(float_math "($NEWSIZE / $2) * 1024")
 	NEWBLOCKCOUNT=${NEWBLOCKCOUNT%%.*}
 }
 
@@ -428,8 +426,8 @@ resize_ext() {
 		esac
 	fi
 
-	verbose "Resizing filesystem on device \"$VOLUME\" to $NEWSIZE bytes ($BLOCKCOUNT -> $NEWBLOCKCOUNT blocks of $BLOCKSIZE bytes)"
-	dry "$RESIZE_EXT" "$FSFORCE" "$VOLUME" "$NEWBLOCKCOUNT"
+	verbose "Resizing filesystem on device \"$VOLUME\" to $NEWSIZE KiB ($BLOCKCOUNT -> $NEWBLOCKCOUNT blocks of $BLOCKSIZE bytes)"
+	dry "$RESIZE_EXT" $FSFORCE "$VOLUME" "$NEWBLOCKCOUNT"
 }
 
 #############################
@@ -449,11 +447,11 @@ resize_reiser() {
 	done
 	validate_parsing "$TUNE_REISER"
 	decode_size $1 $BLOCKSIZE
-	verbose "Resizing \"$VOLUME\" $BLOCKCOUNT -> $NEWBLOCKCOUNT blocks ($NEWSIZE bytes, bs: $NEWBLOCKCOUNT)"
+	verbose "Resizing \"$VOLUME\" $BLOCKCOUNT -> $NEWBLOCKCOUNT blocks ($NEWSIZE KiB, bs: $NEWBLOCKCOUNT)"
 	if [ -n "$YES" ]; then
-		echo y | dry "$RESIZE_REISER" -s "$NEWSIZE" "$VOLUME"
+		echo y | dry "$RESIZE_REISER" -s "${NEWSIZE}K" "$VOLUME"
 	else
-		dry "$RESIZE_REISER" -s "$NEWSIZE" "$VOLUME"
+		dry "$RESIZE_REISER" -s "${NEWSIZE}K" "$VOLUME"
 	fi
 }
 
@@ -547,9 +545,10 @@ resize_fs() {
 resize_lvolume() {
 	lvname=$1
 	newsize=$2
-	lvsize=$(LANG=C "$LVM" lvs -o lv_size --separator ' ' --noheadings --nosuffix --units b $lvname 2> /dev/null | sed -e 's/^ *//')
+
+	lvsize=$(LANG=C "$LVM" lvs -o lv_size --separator ' ' --noheadings --nosuffix --units k $lvname 2> /dev/null | sed -e 's/^ *//')
 	if [ $lvsize != $newsize ]; then
-		dry "$LVM" lvresize "$VERB" "$FORCE" -L"${newsize}"b "$lvname"
+		dry "$LVM" lvresize "$VERB" "$FORCE" -L "${newsize}k" "$lvname"
 	else
 		verbose "Logical volume already is of the size you're trying to resize it to"
 	fi
@@ -719,8 +718,8 @@ create() {
 	# there is only one vgroup, or use the one with enough
 	# free space in it
 	if [ -z "$vgname" ]; then
-		vgroups=$("$LVM" vgs -o vg_name,vg_free --separator ' ' --noheadings --units b 2> /dev/null)
-		lines=$("$LVM" vgs -o vg_name,vg_free --separator ' ' --noheadings --units b 2> /dev/null | wc -l)
+		vgroups=$("$LVM" vgs -o vg_name,vg_free --separator ' ' --noheadings --units k 2> /dev/null)
+		lines=$("$LVM" vgs -o vg_name,vg_free --separator ' ' --noheadings --units k 2> /dev/null | wc -l)
 
 		if [ $lines -eq 1 ]; then
 			vgname=$(echo $vgroups | sed -e 's/^ *//' | cut -d' ' -f1)
@@ -903,7 +902,7 @@ remove() {
 		echo "Usage: $TOOL remove [mount point | dm device | voulume group | device | --all]"
 		exit 0
 	elif [ "$1" == "--all" ]; then
-		list="$(LANG=C $LVM vgs -o vg_name --separator ' ' --noheadings --nosuffix --units b 2> /dev/null)"
+		list="$(LANG=C $LVM vgs -o vg_name --separator ' ' --noheadings --nosuffix --units k 2> /dev/null)"
 	else
 		list="$@"
 	fi
@@ -920,7 +919,7 @@ remove() {
 list_filesystems() {
 	local IFS=$NL
 	local c=0
-	for line in $(LANG=C "$LVM" lvs -o lv_path,lv_size,segtype --noheadings --separator ' ' --nosuffix --units b 2> /dev/null); do
+	for line in $(LANG=C "$LVM" lvs -o lv_path,lv_size,segtype --noheadings --separator ' ' --nosuffix --units k 2> /dev/null); do
 		line=$(echo $line | sed -e 's/^ *\//\//')
 		volume=$(echo $line | cut -d' ' -f1)
 		[ "$volume" == "$last_volume" ] && continue
@@ -1003,11 +1002,11 @@ list_devices() {
 	c=0
 	dmnumber=$(cat "$PROCDEVICES" | "$GREP" device-mapper | sed -e 's/^  *//')
 	dmnumber=${dmnumber%% *}
-	LANG=C "$LVM" pvs -o pv_name,vg_name,pv_size,pv_free,pv_used --separator ' ' --noheadings --nosuffix --units b > $tmp
-	for line in $(cat "$PROCPARTITIONS" | tail -n +3 | sed -e 's/^  *//' | "$GREP" -v -e "^$dmnumber"); do
+	LANG=C "$LVM" pvs -o pv_name,vg_name,pv_size,pv_free,pv_used --separator ' ' --noheadings --nosuffix --units k > $tmp
+	for line in $(cat "$PROCPARTITIONS" | tail -n +3 | sed -e 's/^  *//' | grep -v -e "^$dmnumber"); do
 		c=$((c+1))
 		line=$(echo $line | sed -e 's/  */ /g')
-		_total=$(($(echo $line | cut -d' ' -f3)*1024))
+		_total=$(echo $line | cut -d' ' -f3)
 		local total[$c]=$(humanize_size ${_total%%.*})
 		VOLUME=$(echo $line | cut -d' ' -f4)
 		RVOLUME="/dev/$(echo $line | cut -d' ' -f4)"
@@ -1086,7 +1085,7 @@ list_devices() {
 list_pool() {
 	local IFS=$NL
 	c=0
-	for line in $(LANG=C "$LVM" vgs -o vg_name,pv_count,vg_size,vg_free --separator ' ' --noheadings --nosuffix --units b 2> /dev/null); do
+	for line in $(LANG=C "$LVM" vgs -o vg_name,pv_count,vg_size,vg_free --separator ' ' --noheadings --nosuffix --units k 2> /dev/null); do
 		c=$((c+1))
 		line=$(echo $line | sed -e 's/^ *//')
 		local group[$c]=$(echo $line | cut -d' ' -f1)
-- 
1.7.4.4

[PATCH v3 03/18] fsadm: Add simple configuration file

[Lukas Czerner]

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index 58ff0ab..98921d3 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -88,6 +88,9 @@ PROCDEVICES="/proc/devices"
 PROCPARTITIONS="/proc/partitions"
 NULL="$DM_DEV_DIR/null"
 
+CONFIG_PATHS="/etc/fsadm.conf ~/fsadm.conf"
+CONFIG=
+
 MAX_VGS=999
 
 # without bash $'\n'
@@ -1271,6 +1274,22 @@ check() {
 	esac
 }
 
+set_default_config() {
+	[ -z "$DEFAULT_DEVICE_POOL" ] && DEFAULT_DEVICE_POOL="device_pool"
+	[ -z "$LVOL_PREFIX" ] && LVOL_PREFIX="lvol"
+}
+
+parse_config() {
+	for line in $(cat "$1"); do
+		case "$line" in
+		  "DEFAULT_DEVICE_POOL"*) DEFAULT_DEVICE_POOL=${line##*=} ;;
+		  "LVOL_PREFIX"*) LVOL_PREFIX=${line##*=} ;;
+		  "#"*) continue ;;
+		  *) error "Unknown value \"$line\" in configuration file"
+		esac
+	done
+}
+
 #############################
 # start point of this script
 # - parsing parameters
@@ -1311,6 +1330,7 @@ do
 	  "-o"|"--resize-fs-only") RESIZE_FS_ONLY=1 ;;
 	  "-y"|"--yes") YES="-y" ;;
 	  "-l"|"--lvresize") ;;
+	  "-c"|"--config") CONFIG=$2; shift 1 ;;
 	  "check") COMMAND=$1; shift; ARGS="$@"; break ;;
 	  "resize") COMMAND=$1; shift; ARGS="$@"; break ;;
 	  "create") COMMAND=$1; shift; ARGS="$@"; break ;;
@@ -1326,6 +1346,16 @@ if [ -z "$COMMAND" ]; then
 	error "Missing command. (see: $TOOL --help)"
 fi
 
+set_default_config
+if [ "$CONFIG" ]; then
+	[ ! -f "$CONFIG" ] && error "\"$CONFIG\" is not proper configuration file"
+	CONFIG_PATHS+=" $CONFIG"
+fi
+for i in "$CONFIG_PATHS"; do
+	[ ! -f "$i" ] && continue
+	parse_config "$i"
+done
+
 export FSADM_RUNNING="fsadm"
 $COMMAND $ARGS
 cleanup 0
-- 
1.7.4.4

[PATCH v3 04/18] fsadm: Use DEFAULT_DEVICE_POOL when creating volume group

[Lukas Czerner]

I user did not specified group, use DEFAULT_DEVICE_POOL from configuration
file.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |   58 +++--------------------------------------------------
 1 files changed, 4 insertions(+), 54 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index 98921d3..d1b17e5 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -716,49 +716,10 @@ create() {
 		vgname=$GROUP
 	fi
 
-	# Devices are not in any group so we should find some.
-	# Either create a new one, or use the existing one, if
-	# there is only one vgroup, or use the one with enough
-	# free space in it
 	if [ -z "$vgname" ]; then
-		vgroups=$("$LVM" vgs -o vg_name,vg_free --separator ' ' --noheadings --units k 2> /dev/null)
-		lines=$("$LVM" vgs -o vg_name,vg_free --separator ' ' --noheadings --units k 2> /dev/null | wc -l)
-
-		if [ $lines -eq 1 ]; then
-			vgname=$(echo $vgroups | sed -e 's/^ *//' | cut -d' ' -f1)
-		elif [ $lines -gt 1 ]; then
-			if [ -z $size ]; then
-				error "Not enough information to create" \
-					"logical volume"
-			fi
-			decode_size $size 1
-			new_size=$NEWBLOCKCOUNT
-			IFS=$NL
-			for i in $vgroups; do
-				vgsize=$(echo $i | sed -e 's/^ *//' | cut -d' ' -f2)
-				vgsize=${vgsize%%B}
-				if [ $vgsize -ge $new_size ]; then
-					vgname=$(echo $i | sed -e 's/^ *//' | cut -d' ' -f1)
-					break;
-				fi
-			done
-		fi
+		vgname=$DEFAULT_DEVICE_POOL
+		"$LVM" vgs "$vgname" &> /dev/null || vg_create=1
 	fi
-	IFS=$IFS_OLD
-
-	if [ -z "$vgname" ]; then
-		[ $devcount -eq 0 ] && error "No suitable device specified."
-		for i in $(seq -w $MAX_VGS); do
-			"$LVM" vgs vg${i} &> /dev/null
-			if [ $? -ne 0 ]; then
-				vgname="vg${i}"
-				break;
-			fi
-		done
-		vg_create=1
-	fi
-
-	[ -z "$vgname" ] && error "No suitable name for volume group found."
 
 	if [ "$stripesize" ] || [ "$stripes" ]; then
 		if [ -z "$stripes" ] && [ $devcount -eq 0 ]; then
@@ -1192,19 +1153,8 @@ add() {
 	"$LVM" vgs -o vg_name --separator ' ' --noheadings | sed -e 's/^ *//' > $tmp
 
 	if [ -z "$vgname" ]; then
-		lines=$(wc -l $tmp)
-		lines=${lines%% *}
-
-		if [ $lines -eq 1 ]; then
-			vgname=$(cut -d' ' -f1 $tmp)
-		elif [ $lines -gt 1 ]; then
-			list_pool
-			rm -f $tmp
-			error "Please, specify group you want to add the device into"
-		elif [ $lines -eq 0 ]; then
-			vgname="vg001"
-			vg_create=1
-		fi
+		vgname=$DEFAULT_DEVICE_POOL
+		"$GREP" "$vgname" $tmp &> /dev/null || vg_create=1
 	else
 		"$GREP" -e "$vgname" $tmp &> /dev/null
 		vg_create=$?
-- 
1.7.4.4

[PATCH v3 05/18] fsadm: Add LVOL_PREFIX configuration option

[Lukas Czerner]

This commit adds new configuration option LVOL_PREFIX which defines a
prefix for new logical volumes. The prefix will be concatenated with a
sequential number of the logical volume.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index d1b17e5..d4901d1 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -741,12 +741,10 @@ create() {
 		tmp=$(mktemp)
 		"$LVM" lvs "$vgname" --separator ' ' --noheadings > $tmp
 		for i in $(seq -w $MAX_VGS); do
-			"$GREP" -e " lvol${i} " $tmp &> /dev/null
-			if [ $? -ne 0 ]; then
-				name="lvol${i}"
-				lvname="--name $name"
-				break;
-			fi
+			"$GREP" -e " ${LVOL_PREFIX}${i} " $tmp &> /dev/null && continue
+			name="${LVOL_PREFIX}${i}"
+			lvname="--name $name"
+			break;
 		done
 		rm -f $tmp
 	fi
-- 
1.7.4.4

[PATCH v3 06/18] fsadm: Fsck extN before resize only if it is not mounted

[Lukas Czerner]

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index d4901d1..f43d72e 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -415,8 +415,9 @@ resize_ext() {
 	decode_size $1 $BLOCKSIZE
 	FSFORCE=$FORCE
 
+	detect_mounted
 	if [ "$NEWBLOCKCOUNT" -lt "$BLOCKCOUNT" -o "$EXTOFF" -eq 1 ]; then
-		detect_mounted && verbose "$RESIZE_EXT needs unmounted filesystem" && try_umount
+		[ "$MOUNTED" ] && verbose "$RESIZE_EXT needs unmounted filesystem" && try_umount
 		REMOUNT=$MOUNTED
 	fi
 
-- 
1.7.4.4

[PATCH v3 07/18] fsadm: Only use readlink if link is provided

[Lukas Czerner]

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index f43d72e..2d5c6f9 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -301,8 +301,10 @@ decode_size() {
 # dereference device name if it is symbolic link
 detect_fs() {
 	VOLUME_ORIG="$1"
-	VOLUME=${1/#"${DM_DEV_DIR}/"/}
-	VOLUME=$("$READLINK" "$READLINK_E" "$DM_DEV_DIR/$VOLUME") || error "Cannot get readlink \"$1\""
+	VOLUME=${DM_DEV_DIR}/${1/#"${DM_DEV_DIR}/"/}
+	if [ -h "$VOLUME" ]; then
+		VOLUME=$("$READLINK" "$READLINK_E" "$VOLUME") || error "Cannot get readlink \"$VOLUME\""
+	fi
 	RVOLUME=$VOLUME
 	case "$RVOLUME" in
           # hardcoded /dev  since udev does not create these entries elsewhere
-- 
1.7.4.4

[PATCH v3 08/18] fsadm: Remove unnecessary modification of PATH variable

[Lukas Czerner]

User (or the system) should be set properly so there is no need to
modify PATH variable. It also prevent us from setting appropriate PATH
before calling the fsadm.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index 2d5c6f9..a1044f7 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -37,9 +37,6 @@
 
 TOOL=$(basename "$0")
 
-_SAVEPATH=$PATH
-PATH=/sbin:/usr/sbin:/bin:/usr/sbin:$PATH
-
 # utilities
 TUNE_EXT=tune2fs
 RESIZE_EXT=resize2fs
-- 
1.7.4.4

[PATCH v3 09/18] fsadm: Allow to specify lv in vg/lv format

[Lukas Czerner]

Various lvm tools does allow to specify logical volume in vg/lv
format, which meand no real path is provided.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index a1044f7..0a2c8cc 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -294,10 +294,7 @@ decode_size() {
 	NEWBLOCKCOUNT=${NEWBLOCKCOUNT%%.*}
 }
 
-# detect filesystem on the given device
-# dereference device name if it is symbolic link
-detect_fs() {
-	VOLUME_ORIG="$1"
+get_volume() {
 	VOLUME=${DM_DEV_DIR}/${1/#"${DM_DEV_DIR}/"/}
 	if [ -h "$VOLUME" ]; then
 		VOLUME=$("$READLINK" "$READLINK_E" "$VOLUME") || error "Cannot get readlink \"$VOLUME\""
@@ -309,6 +306,12 @@ detect_fs() {
 		read </sys/block/${RVOLUME#/dev/}/dm/name SYSVOLUME 2>&1 && VOLUME="$DM_DEV_DIR/mapper/$SYSVOLUME"
 		;;
 	esac
+}
+
+# detect filesystem on the given device
+# dereference device name if it is symbolic link
+detect_fs() {
+	get_volume "$1"
 	# use null device as cache file to be sure about the result
 	# not using option '-o value' to be compatible with older version of blkid
 	FSTYPE=$("$BLKID" -c "$NULL" -s TYPE "$VOLUME") || return 1
@@ -529,6 +532,7 @@ resize_fs() {
 	detect_fs "$1" ||  error "Cannot get FSTYPE of \"$VOLUME\""
 	verbose "\"$FSTYPE\" filesystem found on \"$VOLUME\""
 	if [ "$NEWSIZE" ]; then
+		decode_size $NEWSIZE
 		is_natural $NEWSIZE ||  error "$NEWSIZE is not valid number for file system size"
 	fi
 	detect_device_size
@@ -559,6 +563,7 @@ resize_lvolume() {
 
 resize() {
 	local size=
+	devcount=
 
 	# Special case for the situation we have been called from within the lvresize code.
 	if [ "$RESIZE_FS_ONLY" ]; then
@@ -567,7 +572,8 @@ resize() {
 	fi
 
 	for i in "$@"; do
-		if [ -b "$i" ]; then
+		get_volume "$i"
+		if [ -b "$VOLUME" ]; then
 			if [ -z "$devcount" ]; then
 				"$LVM" lvs "$i" &> /dev/null || error "$i is not valid logical volume"
 				lvname=$i
@@ -810,9 +816,10 @@ do_remove() {
 	device=
 	MOUNTED=
 
+	get_volume "$item"
 	# Block device has been given
-	if [ -b "$item" ]; then
-		device=$item
+	if [ -b "$VOLUME" ]; then
+		device=$VOLUME
 	fi
 
 	# Mount point has been given
-- 
1.7.4.4

[PATCH v3 10/18] fsadm: Umount ext2 file system prior resize

[Lukas Czerner]

ext2 does NOT support online resize, so we should attempt to umount the
file system first.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 scripts/fsadm.sh |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index 0a2c8cc..7220f98 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -417,6 +417,11 @@ resize_ext() {
 	decode_size $1 $BLOCKSIZE
 	FSFORCE=$FORCE
 
+	# ext2 does NOT support online resize
+	if [ "$FSTYPE" == "ext2" ]; then
+		EXTOFF=1
+	fi
+
 	detect_mounted
 	if [ "$NEWBLOCKCOUNT" -lt "$BLOCKCOUNT" -o "$EXTOFF" -eq 1 ]; then
 		[ "$MOUNTED" ] && verbose "$RESIZE_EXT needs unmounted filesystem" && try_umount
-- 
1.7.4.4

[PATCH v3 11/18] fsadm: Add help for new commands and update man page

[Lukas Czerner]

This commit updates fsadm manual pages with new fsadm commands and
functionality it provides. It also adds --help for the new commands
directly into fsadm, so we can specify --help for each command
specifically.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 man/fsadm.8.in   |  323 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 scripts/fsadm.sh |  134 +++++++++++++++++++---
 2 files changed, 417 insertions(+), 40 deletions(-)

diff --git a/man/fsadm.8.in b/man/fsadm.8.in
index b7bd0a0..0de2c9e 100644
--- a/man/fsadm.8.in
+++ b/man/fsadm.8.in
@@ -1,24 +1,119 @@
 .TH "FSADM" "8" "LVM TOOLS #VERSION#" "Red Hat, Inc" "\""
 .SH "NAME"
-fsadm \- utility to resize or check filesystem on a device
+fsadm \- utility to manage logical volumes
 .SH SYNOPSIS
 .B fsadm
-.RI [ options ]
+.RI "[ " "OPTIONS" " ] " "COMMAND" " [ " "COMMAND_OPTIONS" " ] "
+.R [ ... ]
+.PP
+.B fsadm
+.RI "[ " "OPTIONS" " ]"
 .B check
+[
+.B --help
+]
 .I device
-.sp
+.PP
 .B fsadm
-.RI [ options ]
+.RI "[ " "OPTIONS" " ]"
 .B resize
+[
+.B --help
+]
+[
+.B size=\fISize\fP
+|
+.B size=+\fISize\fP
+|
+.B size=-\fISize\fP
+]
+[
+.I device...
+]
+.I volume
+.PP
+.B fsadm
+.RI "[ " "OPTIONS" " ]
+.B create
+[
+.B --help
+]
+[
+.B stripesize=\fIStripeSize\fP
+]
+[
+.B name=\fIName\fP
+]
+[
+.B fs=\fIType\fP
+|
+.B fstyp=\fIType\fP
+]
+[
+.B size=\fISize\fP
+]
+[
+.B stripes\fIStripes\fP
+]
+[
+.I device...
+]
+[
+.I pool
+]
+.PP
+.B fsadm
+.RI "[ " "OPTIONS" " ]
+.B list
+[
+.B --help
+]
+[
+.B filesystems
+|
+.B fs
+]
+[
+.B device
+|
+.B dev
+]
+[
+.B pool
+]
+.PP
+.B fsadm
+.RI "[ " "OPTIONS" " ]
+.B add
+[
+.B --help
+]
 .I device
-.RI [ new_size [ BKMGTEP ]]
-.sp
+[
+.I device...
+]
+[
+.I pool
+]
+.PP
+.B fsadm
+.RI "[ " "OPTIONS" " ]
+.B remove
+[
+.B --help
+]
+[
+.B --all
+]
+.I item
+[
+.I item...
+]
+.PP
 .SH DESCRIPTION
 .B fsadm
-utility checks or resizes the filesystem on a device.
-It tries to use the same API for 
-.IR ext2 , ext3 , ext4 , ReiserFS
-and \fIXFS\fP filesystem.
+utility to manage logical volumes and pools (volume groups).
+It tries to use the same API for ext3, ext4 and xfs file system.
 .SH OPTIONS
 .TP
 .BR \-h ", " \-\-help
@@ -39,11 +134,117 @@ Print commands without running them.
 .BR \-y ", " \-\-yes
 Answer "yes" at any prompts.
 .TP
-.I new_size
-Absolute number of filesystem blocks to be in the filesystem,
-or an absolute size using a suffix (in powers of 1024).
-If new_size is not supplied, the whole device is used.
-
+.B \-c ", " \-\-config\fP  \fIpath\fP
+Specify the path to the configuration file. If not provided, default
+location will be used. The default locations are
+.B /etc/fsadm.conf
+and
+.B ~/fsadm.conf
+, but options in the second one will override options in the first one.
+.SH COMMANDS
+.TP
+.B check
+Check the file system on
+.I device
+using fsck.
+.TP
+.B resize
+Change the size of the logical
+.I volume
+and file system on it. You can specify
+.B size=\fP\fISize
+to resize to given
+.IR Size ,
+or
+.B size=+\fP\fISize
+to extend the volume by the given
+.IR Size ,
+or
+.B size=-\fP\fISize
+to shrink the volume by the given
+.IR Size .
+If no
+.I Size
+is provided the volume will be resized to its maximum size.
+You can also specify one or more
+.I devices
+to use for extending the
+.IR volume .
+If the device is not in any pool, it will be added the volume's
+pool prior the resize. Note that some file system (namely
+.BR xfs )
+does not support shrinking. Also some file system does not support online
+resize, it means that it will be unmounted before the resize and then mounted
+back afterwards. It will also happen when you specify
+.BR -e ", " --ext-offline
+for extN file systems, or if you're trying to shrink the file system since none
+of supported file system supports online shrinking. And finally, some file
+systems (namely
+.BR xfs )
+does not support offline resize, it means, that the file system will be mounted
+prior to resize and then unmounted afterwards.
+.TP
+.B create
+Create a new logical volume from the
+.IR pool ,
+optionally with the defined file system. You can specify
+.I Type
+of the file system which will be automatically created on the new logical
+volume. Currently only
+.BR ext3 ", " ext4 " and " xfs
+file systems are supported. You can create striped volume by defining the
+.IR StripeSize .
+In that case, if
+.IR Stripes
+is not defined, then number of provided
+.I devices
+will be used. Either
+.I devices
+or
+.I Stripes
+has to be defined if
+.I StripeSize
+is provided, otherwise
+.B fsadm
+does not have enough information to proceed in creating new logical volume.
+Note that if no pool is specified the default pool will be used.
+.TP
+.B list
+List devices, file systems and pools in your system. You can select to list
+all logical volumes by specifying
+.BR filesystems ,
+or
+.BR fs.
+Or you can select to list all pools in the system by specifying
+.BR pool.
+Or you can also select to list all devices in the system by specifying
+.BR devices ,
+or
+.BR dev ,
+however note that this will not list any DM device.
+Optionally you can specify any combination of the above options to list whatever
+you desire, or you can simply omit the option. In that case it will list
+everything as if all options has been specified.
+.TP
+.B add
+Add one, or more
+.I devices
+into the
+.IR pool .
+If no
+.I pool
+is specified, provided
+.I devices
+will be added into the default pool. Note that, if any device is already part
+of the same, or different pool, it will be skipped.
+.TP
+.B remove
+Remove one, or more logical volumes, devices, or pools defined
+by
+.IR items .
+You can also specify
+.B --all
+to remove all pools corresponding volumes from your system.
 .SH DIAGNOSTICS
 On successful completion, the status code is 0.
 A status code of 2 indicates the operation was interrupted by the user.
@@ -51,14 +252,76 @@ A status code of 3 indicates the requested check operation could not be performe
 because the filesystem is mounted and does not support an online 
 .BR fsck (8).
 A status code of 1 is used for other failures.
-
 .SH EXAMPLES
-Resize the filesystem on logical volume /dev/vg/test to 1000 megabytes.
-If /dev/vg/test contains ext2/ext3/ext4
-filesystem it will be unmounted prior the resize.
-All [y|n] questions will be answered 'y'.
-.sp
-.B fsadm \-e \-y resize /dev/vg/test 1000M
+To
+.B add
+device
+.I /dev/sdb
+into the default pool run this command:
+.PP
+    \fB fsadm add /dev/sdb
+.PP
+You can also add mode devices into another pool called
+.I mypool
+.PP
+    \fB fsadm add /dev/sdc /dev/sdd /dev/sde mypool
+.PP
+To create a 300GB linear logical volume with
+.B ext4
+file system using devices
+.B /dev/sda
+and
+.B /dev/sdb
+you can use the following command:
+.PP
+    \fB fsadm create fs=ext4 size=300G /dev/sda /dev/sdb
+.PP
+Of course, we are assuming that /dev/sda and /dev/sdb does have at least 300GB
+of space when combined, otherwise the volume creation would fail.
+.PP
+Now let's create 500GB striped volume with stripe size of 16KB and
+.B xfs
+file system using four devices. It means that
+.I Stripes
+will be equal to 4, however note that we do not need to define
+.I Stripes
+manually if we are listing devices as follows:
+.PP
+    \fB fsadm create fs=xfs size=500G stripesize=16 /dev/sda /dev/sdb /dev/sdc /dev/sdd
+.PP
+Now, if we assume that we already have at least four devices in the default
+pool, we can achieve the same result by calling:
+.PP
+    \fB fsadm create fs=xfs size=500G stripesize=16 stripes=4
+.PP
+To
+.B shrink
+the
+.B device_pool/lvol001
+logical volume by 10G we can simply call
+.PP
+    \fB fsadm resize size=-10G device_pool/lvol001
+.PP
+Or we can
+.B extend
+it by 1T using more devices which are not in the pool just yet:
+.PP
+    \fB fsadm resize size=+1T device_pool/lvol001 /dev/sde /dev/sdf
+.PP
+To
+.B remove
+the above logical volume we can use the following command:
+.PP
+    \fB fsadm remove device_pool/lvol001
+.PP
+or we can simply remove the whole pool, wiping all logical volumes in it:
+.PP
+    \fB fsadm remove device_pool
+.PP
+Alternatively we can remove all pools in the system by calling:
+.PP
+    \fB fsadm remove --all
+.PP
 .SH ENVIRONMENT VARIABLES
 .TP
 .B TMPDIR
@@ -67,6 +330,22 @@ The temporary directory name for mount points. Defaults to "/tmp".
 .B DM_DEV_DIR
 The device directory name.
 Defaults to "/dev" and must be an absolute path.
+.TP
+.B DEFAULT_DEVICE_POOL
+Default pool that will be used in
+.B fsadm
+command if not pool is specified. It defaults to "device_pool" and can
+also be set in configuration file (see
+.B \-c ", " \-\-config\fP  \fIpath\fP
+).
+.TP
+.B LVOL_PREFIX
+Defines the name of the newly created volume if no
+.B name=
+is provided. Defaults to "lvol" and will be concatenated with the number
+of the volume. It can also be set in configuration file (see
+.B \-c ", " \-\-config\fP  \fIpath\fP
+).
 
 .SH SEE ALSO
 .BR lvm (8),
diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index 7220f98..eb2be44 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -94,27 +94,107 @@ MAX_VGS=999
 NL='
 '
 
-tool_usage() {
-	echo "${TOOL}: Utility to resize or check the filesystem on a device"
-	echo
+check_usage() {
 	echo "  ${TOOL} [options] check device"
 	echo "    - Check the filesystem on device using fsck"
 	echo
-	echo "  ${TOOL} [options] resize device [new_size[BKMGTPE]]"
-	echo "    - Change the size of the filesystem on device to new_size"
+}
+
+resize_usage() {
+	echo "  ${TOOL} [options] resize [resize options] [device...] volume"
+	echo "    - Change the size of the logical volume and file system on it"
 	echo
-	echo "  Options:"
-	echo "    -h | --help         Show this help message"
-	echo "    -v | --verbose      Be verbose"
-	echo "    -e | --ext-offline  unmount filesystem before ext2/ext3/ext4 resize"
-	echo "    -f | --force        Bypass sanity checks"
-	echo "    -n | --dry-run      Print commands without running them"
-	echo "    -y | --yes          Answer \"yes\" at any prompts"
+	echo "  Resize options:"
+	echo "    size=n        Resize the volume to the given size."
+	echo "    size=+n       Extend the volume by the given size."
+	echo "    size=-n       Shrink the volume by the given size."
+	echo "    [device...]   Devices to use when extending the volume. If the"
+	echo "                  device is not in any pool, it is added to the"
+	echo "                  volume's pool prior to the extension."
+	echo "    A suffix K, M, G, T, P, E can be used to define size units."
 	echo
-	echo "  new_size - Absolute number of filesystem blocks to be in the filesystem,"
-	echo "             or an absolute size using a suffix (in powers of 1024)."
-	echo "             If new_size is not supplied, the whole device is used."
+}
 
+create_usage() {
+	echo "  ${TOOL} [options] create [create options] [device...] [pool]"
+	echo "    - Create a new logical volume from the pool"
+	echo
+	echo "  Create options:"
+	echo "    stripesize=n     Gives the number of kilobytes for the granularity"
+	echo "                     of stripes (see man 8 fsadm for more information)."
+	echo "                     This is optional and if not given, linear logical"
+	echo "                     volume will be created."
+	echo "    name=n           The name for the new logical volume. Without this"
+	echo "                     option the default names of "lvol#" will be generated"
+	echo "                     where # is internal number of the logical volume"
+	echo "                     The default can be changed by setting LVOL_PREFIX"
+	echo "                     fsadm configuration file."
+	echo "    fstyp=n | fs=n   Gives the file system type to create on the new"
+	echo "                     logical volume. Supported file systems are (ext3,"
+	echo "                     ext4, xfs). This is optional and if not given"
+	echo "                     file system will not be created."
+	echo "    size=n           Gives the size to allocate for the new logical volume"
+	echo "                     A size suffix K, M, G, T, P, E can be used to define"
+	echo "                     units (see man 8 fsadm for more information). This"
+	echo "                     is optional if if not given maximum size will be"
+	echo "                     used."
+	echo "    stripes=n        Gives the number of stripes. This is equal to the"
+	echo "                     number of physical volumes to scatter the logical"
+	echo "                     volume. This is optional and if stripesize is set"
+	echo "                     and multiple devices are provided stripes is"
+	echo "                     determined automatically."
+	echo
+}
+
+list_usage() {
+	echo "  ${TOOL} [options] list [filesystems|fs|devices|dev|pool]"
+	echo "    - List devices, file systems and pools in your system"
+	echo
+	echo "  List options:"
+	echo "    filesystems | fs   List all logical volumes."
+	echo "    devices | dev      List all devices except the ones create by DM."
+	echo "    pool               List all available pools to create new logical"
+	echo "                       volumes from."
+	echo
+}
+
+add_usage() {
+	echo "  ${TOOL} [options] add device [device...] [pool]"
+	echo "    - Add one, or more devices into the defined, or default pool"
+	echo
+}
+
+remove_usage() {
+	echo "  ${TOOL} [options] remove [remove options] item [item...]"
+	echo "    - Remove one, or more logical volumes, devices, or pools defined"
+	echo "      by item"
+	echo
+	echo "  Remove options:"
+	echo "    item   Can be logical volume, mount point, device in the pool"
+	echo "             or name of the pool to remove."
+	echo "    --all    Removes all pools and corresponding logical volumes"
+	echo
+
+}
+
+tool_usage() {
+	echo "${TOOL}: Utility to manage logical volumes"
+	echo
+	check_usage
+	resize_usage
+	create_usage
+	list_usage
+	add_usage
+	remove_usage
+	echo "  Options:"
+	echo "    -h | --help           Show this help message"
+	echo "    -v | --verbose        Be verbose"
+	echo "    -e | --ext-offline    unmount filesystem before ext2/ext3/ext4 resize"
+	echo "    -f | --force          Bypass sanity checks"
+	echo "    -n | --dry-run        Print commands without running them"
+	echo "    -y | --yes            Answer \"yes\" at any prompts"
+	echo "    -c | --config path    Specify the path to the configuration file"
+	echo
 	exit
 }
 
@@ -567,6 +647,10 @@ resize_lvolume() {
 }
 
 resize() {
+	if [ "$1" == "--help" ]; then
+		resize_usage
+		exit
+	fi
 	local size=
 	devcount=
 
@@ -707,6 +791,9 @@ create() {
 			"fstyp"* | "fs"*) fstyp=${i##*=} ;;
 			"size"*) size=${i##*=} ;;
 			"stripes"*) stripes=${i##*=} ;;
+			"--help") create_usage
+				  exit
+				  ;;
 			*) if [ -z $vgname ]; then vgname=$i; else
 				error "Wrong option $i. (see: $TOOL --help)"
 			   fi ;;
@@ -872,9 +959,9 @@ remove() {
 		      "(see $TOOL remove --help)."
 	fi
 	# help
-	if [ "$1" == "help" ]; then
-		echo "Usage: $TOOL remove [mount point | dm device | voulume group | device | --all]"
-		exit 0
+	if [ "$1" == "--help" ]; then
+		remove_usage
+		exit
 	elif [ "$1" == "--all" ]; then
 		list="$(LANG=C $LVM vgs -o vg_name --separator ' ' --noheadings --nosuffix --units k 2> /dev/null)"
 	else
@@ -1121,6 +1208,9 @@ list() {
 			"filesystems" | "fs") list_filesystems ;;
 			"devices" | "dev") list_devices ;;
 			"pool") list_pool ;;
+			"--help") list_usage
+				  exit
+				  ;;
 			*) error "Wrong option $i. (see: $TOOL --help)"
 		esac
 	done
@@ -1145,6 +1235,10 @@ detect_volume_group() {
 # Add devices into the group
 ############################
 add() {
+	if [ "$1" == "--help" ]; then
+		add_usage
+		exit
+	fi
 	vg_create=0
 	devcount=0
 	tmp=$(mktemp)
@@ -1196,6 +1290,10 @@ diff_dates() {
 # Check filesystem
 ###################
 check() {
+	if [ "$1" == "--help" ]; then
+		check_usage
+		exit
+	fi
 	detect_fs "$1" || error "Cannot get FSTYPE of \"$VOLUME\""
 	verbose "\"$FSTYPE\" filesystem found on \"$VOLUME\""
 	if detect_mounted ; then
-- 
1.7.4.4

[PATCH v3 12/18] lvresize: Specify --resize-fs-only when going to use fsadm resize

[Lukas Czerner]

When lvresize is calling fsadm to resize the file system, originally
it was the default action. However now fsadm support much more
features and it is doing the lvresize by itself. Luckily there is
still a way to tell fsadm to resize only the file system ane leave
the rest up to the lvresize - provide "--resize-fs-only" argument.

This commit adds "--resize-fs-only" argument when we are going to
use fsadm to resize the fs from within the lvresize.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 tools/lvresize.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/lvresize.c b/tools/lvresize.c
index 96e623e..44f934e 100644
--- a/tools/lvresize.c
+++ b/tools/lvresize.c
@@ -128,7 +128,7 @@ static int _request_confirmation(struct cmd_context *cmd,
 
 enum fsadm_cmd_e { FSADM_CMD_CHECK, FSADM_CMD_RESIZE };
 #define FSADM_CMD "fsadm"
-#define FSADM_CMD_MAX_ARGS 6
+#define FSADM_CMD_MAX_ARGS 7
 #define FSADM_CHECK_FAILS_FOR_MOUNTED 3 /* shell exist status code */
 
 /*
@@ -157,7 +157,15 @@ static int _fsadm_cmd(struct cmd_context *cmd,
 	if (arg_count(cmd, force_ARG))
 		argv[i++] = "--force";
 
-	argv[i++] = (fcmd == FSADM_CMD_RESIZE) ? "resize" : "check";
+	switch (fcmd) {
+		case FSADM_CMD_RESIZE:
+			argv[i++] = "--resize-fs-only";
+			argv[i++] = "resize";
+			break;
+		case FSADM_CMD_CHECK:
+			argv[i++] = "check";
+			break;
+	}
 
 	if (dm_snprintf(lv_path, PATH_MAX, "%s%s/%s", cmd->dev_dir, lp->vg_name,
 			lp->lv_name) < 0) {
-- 
1.7.4.4

[PATCH v3 13/18] fsadm: remove -y (YES) option

[Lukas Czerner]

There is some confusion in using -y (YES) and -f (FORCE) options in
fsadm. In some cases we are asked for yes/no question which can be
override by -f option, but not by -y option. Usually most of the questions
tools ask for are yes/no and it can be overridden by forcing it with -f
(e.g. fsck.(extN|xfs), lvm and others...) so it make sense to get rid of
-y option and use only -f instead.

Also I do not think it is wise to use -y option in fsck.extN since
people using fsadm would probably not know how it works, so we should
NOT provide them with that option, but rather let them use "real" fsck
instead (and let them read man page if needed). Also running fsck with
-y when you have corrupted file system is probably not a good idea from
multiple reasons. This is also fixed by this commit.

This commit removes '-y' option and use '-f' instead. With exception of
fsck.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 man/fsadm.8.in   |    5 +----
 scripts/fsadm.sh |   23 +++++++++++------------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/man/fsadm.8.in b/man/fsadm.8.in
index 0de2c9e..4f187ae 100644
--- a/man/fsadm.8.in
+++ b/man/fsadm.8.in
@@ -126,14 +126,11 @@ Be more verbose.
 Unmount ext2/ext3/ext4 filesystem before doing resize.
 .TP
 .BR \-f ", " \-\-force
-Bypass some sanity checks.
+Force all commands without asking questions.
 .TP
 .BR \-n ", " \-\-dry\-run
 Print commands without running them.
 .TP
-.BR \-y ", " \-\-yes
-Answer "yes" at any prompts.
-.TP
 .B \-c ", " \-\-config\fP  \fIpath\fP
 Specify the path to the configuration file. If not provided, default
 location will be used. The default locations are
diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
index eb2be44..c8f6999 100755
--- a/scripts/fsadm.sh
+++ b/scripts/fsadm.sh
@@ -63,7 +63,7 @@ DF=df
 # user may override lvm location by setting LVM_BINARY
 LVM=${LVM_BINARY:-lvm}
 
-YES=${_FSADM_YES}
+FORCE=${_FSADM_FORCE}
 DRY=0
 VERB=
 FORCE=
@@ -190,9 +190,8 @@ tool_usage() {
 	echo "    -h | --help           Show this help message"
 	echo "    -v | --verbose        Be verbose"
 	echo "    -e | --ext-offline    unmount filesystem before ext2/ext3/ext4 resize"
-	echo "    -f | --force          Bypass sanity checks"
+	echo "    -f | --force          Force all commands without asking questions"
 	echo "    -n | --dry-run        Print commands without running them"
-	echo "    -y | --yes            Answer \"yes\" at any prompts"
 	echo "    -c | --config path    Specify the path to the configuration file"
 	echo
 	exit
@@ -460,8 +459,9 @@ temp_umount() {
 yes_no() {
 	echo -n "$@? [Y|n] "
 
-	if [ -n "$YES" ]; then
-		echo y ; return 0
+	if [ -n "$FORCE" ]; then
+		echo y
+		return 0
 	fi
 
 	while read -r -s -n 1 ANS ; do
@@ -512,7 +512,7 @@ resize_ext() {
 	if [ -z "$MOUNTED" ] || [ "$REMOUNT" ]; then
 		# Forced fsck -f for umounted extX filesystem.
 		case "$-" in
-		  *i*) dry "$FSCK" "$YES" -f "$VOLUME" ;;
+		  *i*) dry "$FSCK" -f "$VOLUME" ;;
 		  *) dry "$FSCK" -f -p "$VOLUME" ;;
 		esac
 	fi
@@ -539,7 +539,7 @@ resize_reiser() {
 	validate_parsing "$TUNE_REISER"
 	decode_size $1 $BLOCKSIZE
 	verbose "Resizing \"$VOLUME\" $BLOCKCOUNT -> $NEWBLOCKCOUNT blocks ($NEWSIZE KiB, bs: $NEWBLOCKCOUNT)"
-	if [ -n "$YES" ]; then
+	if [ -n "$FORCE" ]; then
 		echo y | dry "$RESIZE_REISER" -s "${NEWSIZE}K" "$VOLUME"
 	else
 		dry "$RESIZE_REISER" -s "${NEWSIZE}K" "$VOLUME"
@@ -587,7 +587,7 @@ make_ext() {
 	fstyp="$2"
 	bsize=4
 
-	if [ "$YES" ]; then
+	if [ "$FORCE" ]; then
 		force="-F"
 	fi
 
@@ -602,7 +602,7 @@ generic_make_fs() {
 	fstyp=$2
 	bsize=4096
 
-	if [ "$YES" ]; then
+	if [ "$FORCE" ]; then
 		force="-f"
 	fi
 
@@ -725,7 +725,7 @@ resize() {
 	# resize the logical volume, but also the file system (if present)
 	# by calling fsadm recursively with --resize-fs-only argument.
 	# To be able to do this we should set some appropriate variables.
-	export _FSADM_YES=$YES
+	export _FSADM_FORCE=$FORCE
 	export FSADM_RESIZE_RECURSE=1
 	unset FSADM_RUNNING
 
@@ -1326,7 +1326,7 @@ check() {
 	  "xfs") dry "$XFS_CHECK" "$VOLUME" ;;
 	  *)    # check if executed from interactive shell environment
 		case "$-" in
-		  *i*) dry "$FSCK" "$YES" "$FORCE" "$VOLUME" ;;
+		  *i*) dry "$FSCK" "$FORCE" "$VOLUME" ;;
 		  *) dry "$FSCK" "$FORCE" -p "$VOLUME" ;;
 		esac
 	esac
@@ -1386,7 +1386,6 @@ do
 	  "-f"|"--force") FORCE="-f" ;;
 	  "-e"|"--ext-offline") EXTOFF=1 ;;
 	  "-o"|"--resize-fs-only") RESIZE_FS_ONLY=1 ;;
-	  "-y"|"--yes") YES="-y" ;;
 	  "-l"|"--lvresize") ;;
 	  "-c"|"--config") CONFIG=$2; shift 1 ;;
 	  "check") COMMAND=$1; shift; ARGS="$@"; break ;;
-- 
1.7.4.4

[PATCH v3 14/18] test: add helper to compute aligned lv size

[Lukas Czerner]

For test purposes add a helper function to compute lv size as it
would be done by lvm tools.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 test/lib/utils.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/test/lib/utils.sh b/test/lib/utils.sh
index 4c3cd93..c555e45 100644
--- a/test/lib/utils.sh
+++ b/test/lib/utils.sh
@@ -177,6 +177,29 @@ kernel_at_least() {
     return 1
 }
 
+align_size_up() {
+    size=$1
+    [ -z $2 ] && stripes=0
+    [ -z $3 ] && extent=4
+
+    [ -z $size ] || [ -z $stripes ] || [ -z $extent ] && exit 1
+
+    tmp=$((size%extent))
+    if [ $tmp -ne 0 ]; then
+        size=$(($size+($extent-$tmp)))
+    fi
+    if [ $stripes -eq 0 ]; then
+        echo "$size"
+        return 0
+    fi
+    extents=$(($size/$extent))
+    tmp=$(($extents%$stripes))
+    if [ $tmp -ne 0 ]; then
+        extents=$(($extents-$tmp+$stripes))
+    fi
+    echo "$(($extents*$extent))"
+}
+
 . lib/paths || { echo >&2 you must run make first; exit 1; }
 
 PATH=$abs_top_builddir/test/lib:$PATH
-- 
1.7.4.4

[PATCH v3 15/18] test: Add test for fsadm add command

[Lukas Czerner]

The tests exercises the fsadm "add" functionality, by trying various
combinations of arguments and checking the desired results.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 test/t-fsadm-add.sh |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)
 create mode 100644 test/t-fsadm-add.sh

diff --git a/test/t-fsadm-add.sh b/test/t-fsadm-add.sh
new file mode 100644
index 0000000..4c4eb2e
--- /dev/null
+++ b/test/t-fsadm-add.sh
@@ -0,0 +1,53 @@
+#!/bin/bash
+# Copyright 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions
+# of the GNU General Public License v.2.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+
+test_description='Exercise fsadm add'
+
+. lib/test
+
+DEV_COUNT=10
+aux prepare_devs $DEV_COUNT 10
+TEST_DEVS=$(cat DEVICES)
+export DEFAULT_DEVICE_POOL=$vg1
+
+pool1=$vg2
+pool2=$vg3
+
+# Create default pool with all devices at once
+fsadm add $TEST_DEVS
+check vg_field $DEFAULT_DEVICE_POOL pv_count $DEV_COUNT
+fsadm -f remove $DEFAULT_DEVICE_POOL
+
+# Create default pool by adding devices one per a call
+for i in $TEST_DEVS; do
+        fsadm add $i
+done
+check vg_field $DEFAULT_DEVICE_POOL pv_count $DEV_COUNT
+fsadm -f remove $DEFAULT_DEVICE_POOL
+
+# Create different groups from different devices
+fsadm add $dev1 $dev2 $dev3 $pool1
+fsadm add $dev4 $dev5 $dev6
+fsadm add $dev7 $dev8 $dev9 $pool2
+fsadm add $dev10 $pool2
+not fsadm add $dev10 $pool2 $pool1
+check vg_field $DEFAULT_DEVICE_POOL pv_count 3
+check vg_field $pool1 pv_count 3
+check vg_field $pool2 pv_count 4
+fsadm -f remove --all
+
+fsadm add --help
+
+# Some cases which should fail
+not fsadm _garbage_
+not fsadm add
+not fsadm add _somepool
+not fsadm add $dev1 $dev2 $dev3 $pool1 _otherpool
-- 
1.7.4.4

[PATCH v3 16/18] test: Add test for fsadm create command

[Lukas Czerner]

The tests exercises the fsadm "create" functionality, by trying various
combinations of arguments and checking the desired results. It also
tries to create volumes with various supported file systems.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 test/t-fsadm-create.sh |  128 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 128 insertions(+), 0 deletions(-)
 create mode 100644 test/t-fsadm-create.sh

diff --git a/test/t-fsadm-create.sh b/test/t-fsadm-create.sh
new file mode 100644
index 0000000..4b735f8
--- /dev/null
+++ b/test/t-fsadm-create.sh
@@ -0,0 +1,128 @@
+#!/bin/bash
+# Copyright 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions
+# of the GNU General Public License v.2.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+
+test_description='Exercise fsadm create'
+
+. lib/test
+
+DEV_COUNT=10
+DEV_SIZE=10
+TEST_MAX_SIZE=$(($DEV_COUNT*$DEV_SIZE))
+aux prepare_devs $DEV_COUNT $DEV_SIZE
+TEST_DEVS=$(cat DEVICES)
+export DEFAULT_DEVICE_POOL=$vg1
+export LVOL_PREFIX="lvol"
+lvol1=${LVOL_PREFIX}001
+lvol2=${LVOL_PREFIX}002
+lvol3=${LVOL_PREFIX}003
+
+pool1=$vg2
+pool2=$vg3
+
+TEST_FS=
+which mkfs.ext2 && TEST_FS+="ext2 "
+which mkfs.ext3 && TEST_FS+="ext3 "
+which mkfs.ext4 && TEST_FS+="ext4 "
+which mkfs.xfs  && TEST_FS+="xfs"
+
+_do_fsck() {
+	case $1 in
+		ext[234]) call="fsck.$1 -F -n" ;;
+		xfs) call="xfs_check" ;;
+		*) return 1 ;;
+	esac
+	$call $DM_DEV_DIR/$2
+}
+
+# Create volume with all devices at once
+fsadm create $TEST_DEVS
+not fsadm create $TEST_DEVS
+not fsadm create $TEST_DEVS $pool1
+not fsadm create size=$DEV_SIZE $TEST_DEVS
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 pv_count $DEV_COUNT
+fsadm -f remove $DEFAULT_DEVICE_POOL
+
+# Create the group first and then create volume using the whole group
+fsadm add $TEST_DEVS
+fsadm create
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 pv_count $DEV_COUNT
+fsadm -f remove $DEFAULT_DEVICE_POOL
+
+# Create a logical volume of fixed size
+size=$(($DEV_SIZE*6))
+fsadm create size=${size}M $TEST_DEVS
+not fsadm create size=$TEST_MAX_SIZE
+size=$(align_size_up $size)
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 lv_size ${size}.00m
+fsadm -f remove $DEFAULT_DEVICE_POOL
+
+# Create a striped logical volume
+fsadm create stripesize=32 $TEST_DEVS
+not fsadm create stripesize=32 size=$TEST_MAX_SIZE
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 stripesize 32.00k
+fsadm  -f remove $DEFAULT_DEVICE_POOL
+
+# Create several volumes with different parameters
+fsadm  add $TEST_DEVS
+fsadm create stripesize=8 stripes=$(($DEV_COUNT/2)) size=$(($DEV_SIZE*2))M
+fsadm create stripes=$(($DEV_COUNT)) size=$(($DEV_SIZE))M
+not fsadm create stripesize=32 size=$(($DEV_SIZE*2))M
+fsadm list
+fsadm create
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 stripesize 8.00k
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 stripes $(($DEV_COUNT/2))
+check lv_field $DEFAULT_DEVICE_POOL/$lvol2 stripes $DEV_COUNT
+check lv_field $DEFAULT_DEVICE_POOL/$lvol3 segtype linear
+fsadm  -f remove $DEFAULT_DEVICE_POOL
+
+# Create several volumes with different parameters from different groups
+fsadm add $dev1 $dev2 $dev3 $pool1
+fsadm create $dev1 $dev2
+fsadm add $dev4 $dev5 $dev6 $pool2
+fsadm create stripesize=32 stripes=3 size=$(($DEV_SIZE*2))M $pool2
+fsadm create $dev7 $dev8 $dev9 stripesize=8
+not fsadm create size=$DEV_SIZE $pool1
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 stripesize 8.00k
+check lv_field $pool1/$lvol1 seg_count 2
+check lv_field $pool2/$lvol1 stripesize 32.00k
+check lv_field $pool2/$lvol1 stripes 3
+fsadm  -f remove --all
+
+# Create logical volumes with file system
+for fs in $TEST_FS; do
+	fsadm create fs=$fs size=$(($DEV_SIZE*6))M $TEST_DEVS
+	check lv_field $DEFAULT_DEVICE_POOL/$lvol1 pv_count $DEV_COUNT
+	fsadm -f check ${DEFAULT_DEVICE_POOL}/$lvol1
+	_do_fsck $fs ${DEFAULT_DEVICE_POOL}/$lvol1
+	fsadm  -f remove $DEFAULT_DEVICE_POOL
+
+	fsadm create fs=$fs stripesize=32 size=$(($DEV_SIZE*6))M $TEST_DEVS
+	check lv_field $DEFAULT_DEVICE_POOL/$lvol1 pv_count $DEV_COUNT
+	check lv_field $DEFAULT_DEVICE_POOL/$lvol1 stripesize 32.00k
+	_do_fsck $fs ${DEFAULT_DEVICE_POOL}/$lvol1
+	fsadm  -f remove $DEFAULT_DEVICE_POOL
+
+	fsadm add $TEST_DEVS
+	fsadm create fs=$fs stripesize=8 stripes=$((DEV_COUNT/5)) size=$(($DEV_SIZE*2))M
+	check lv_field $DEFAULT_DEVICE_POOL/$lvol1 stripes $(($DEV_COUNT/5))
+	check lv_field $DEFAULT_DEVICE_POOL/$lvol1 stripesize 8.00k
+	_do_fsck $fs ${DEFAULT_DEVICE_POOL}/$lvol1
+	fsadm  -f remove $DEFAULT_DEVICE_POOL
+done
+
+fsadm create --help
+
+# Some cases which should fail
+not fsadm create
+fsadm add $TEST_DEVS
+not fsadm create $pool1
+not fsadm create stripesize=16 stripes=3 $dev1 $dev2
+fsadm  -f remove --all
-- 
1.7.4.4

[PATCH v3 17/18] test: Add test for fsadm resize command

[Lukas Czerner]

The tests exercises the fsadm "resize" functionality, by trying various
combinations of arguments and checking the desired results. It also
tries to create and resize volumes with various supported file systems,
checking the resulting size.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 test/t-fsadm-resize.sh |  125 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 125 insertions(+), 0 deletions(-)
 create mode 100644 test/t-fsadm-resize.sh

diff --git a/test/t-fsadm-resize.sh b/test/t-fsadm-resize.sh
new file mode 100644
index 0000000..130bd54
--- /dev/null
+++ b/test/t-fsadm-resize.sh
@@ -0,0 +1,125 @@
+#!/bin/bash
+# Copyright 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions
+# of the GNU General Public License v.2.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+
+test_description='Exercise fsadm create'
+
+. lib/test
+
+DEV_COUNT=10
+DEV_SIZE=10
+TEST_MAX_SIZE=$(($DEV_COUNT*$DEV_SIZE))
+aux prepare_devs $DEV_COUNT $DEV_SIZE
+TEST_DEVS=$(cat DEVICES)
+export DEFAULT_DEVICE_POOL=$vg1
+export LVOL_PREFIX="lvol"
+lvol1=${LVOL_PREFIX}001
+lvol2=${LVOL_PREFIX}002
+lvol3=${LVOL_PREFIX}003
+
+pool1=$vg2
+pool2=$vg3
+DEFAULT_VOLUME=${DEFAULT_DEVICE_POOL}/$lvol1
+
+TEST_FS=
+which mkfs.ext2 && TEST_FS+="ext2 "
+which mkfs.ext3 && TEST_FS+="ext3 "
+which mkfs.ext4 && TEST_FS+="ext4 "
+which mkfs.xfs  && TEST_FS+="xfs"
+
+TEST_MNT=$TESTDIR/mnt
+
+_test_resize()
+{
+	size=$((TEST_MAX_SIZE/2))
+	fsadm -f resize size=${size}M ${DM_DEV_DIR}/$DEFAULT_VOLUME
+	size=$(align_size_up $size)
+	check lv_field $DEFAULT_DEVICE_POOL/$lvol1 lv_size ${size}.00m
+
+	# xfs does not support shrinking (xfs only grows big!! :))
+	if [ "$fs" != "xfs" ]; then
+		fsadm -f -v resize size=-$(($TEST_MAX_SIZE/4))M $DEFAULT_VOLUME
+		size=$(align_size_up $(($size-($TEST_MAX_SIZE/4))))
+		check lv_field $DEFAULT_DEVICE_POOL/$lvol1 lv_size ${size}.00m
+	fi
+	fsadm -f -v resize size=+$(($TEST_MAX_SIZE/5))M $DEFAULT_VOLUME
+	size=$(align_size_up $(($size+($TEST_MAX_SIZE/5))))
+	check lv_field $DEFAULT_DEVICE_POOL/$lvol1 lv_size ${size}.00m
+}
+
+fsadm add $TEST_DEVS
+size=$((TEST_MAX_SIZE/3))
+fsadm create size=${size}M $TEST_DEVS
+size=$(align_size_up $size)
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 lv_size ${size}.00m
+
+fsadm -f resize size=+$(($TEST_MAX_SIZE/3))M ${DM_DEV_DIR}/$DEFAULT_VOLUME
+size=$(align_size_up $(($size+($TEST_MAX_SIZE/3))))
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 lv_size ${size}.00m
+
+fsadm -f resize size=-$(($TEST_MAX_SIZE/2))M $DEFAULT_VOLUME
+size=$(align_size_up $(($size-($TEST_MAX_SIZE/2))))
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 lv_size ${size}.00m
+
+fsadm -f resize size=$(($TEST_MAX_SIZE/2))M $DEFAULT_VOLUME
+size=$(align_size_up $(($TEST_MAX_SIZE/2)))
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 lv_size ${size}.00m
+fsadm -f remove $DEFAULT_DEVICE_POOL
+
+fsadm create $dev1
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 pv_count 1
+devs=${TEST_DEVS##*$dev1 }
+fsadm resize size=+$((TEST_MAX_SIZE/2))M $DEFAULT_VOLUME $devs
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 pv_count $DEV_COUNT
+fsadm -f remove $DEFAULT_DEVICE_POOL
+
+fsadm create $dev1
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 pv_count 1
+devs=${TEST_DEVS##*$dev1 }
+fsadm resize size=$((TEST_MAX_SIZE/2))M ${DM_DEV_DIR}/$DEFAULT_VOLUME $devs
+check lv_field $DEFAULT_DEVICE_POOL/$lvol1 pv_count $DEV_COUNT
+fsadm -f remove $DEFAULT_DEVICE_POOL
+
+[ ! -d $TEST_MNT ] && mkdir $TEST_MNT &> /dev/null
+for fs in $TEST_FS; do
+	# umounted test
+	fsadm add $TEST_DEVS
+	size=$((TEST_MAX_SIZE/4))
+	fsadm create fs=$fs size=${size}M $TEST_DEVS
+
+	_test_resize
+	fsadm -f check $DEFAULT_VOLUME
+	fsadm -f remove $DEFAULT_DEVICE_POOL
+
+	# mounted test
+	fsadm add $TEST_DEVS
+	size=$((TEST_MAX_SIZE/4))
+	fsadm create fs=$fs size=${size}M $TEST_DEVS
+
+	mount ${DM_DEV_DIR}/$DEFAULT_VOLUME $TEST_MNT
+
+	_test_resize
+
+	umount $TEST_MNT
+	fsadm -f check $DEFAULT_VOLUME
+	fsadm -f remove $DEFAULT_DEVICE_POOL
+done
+fsadm  -f remove --all
+
+fsadm resize --help
+
+# Some cases which should fail
+not fsadm resize
+not fsadm resize _garbage_
+not fsadm resize $dev1
+fsadm create $TEST_DEVS
+not fsadm resize $DEFAULT_VOLUME
+not fsadm -f resize size=+10G $DEFAULT_VOLUME
+not fsadm -f resize size=-10G $DEFAULT_VOLUME
-- 
1.7.4.4

[PATCH v3 18/18] test: Add test for fsadm remove command

[Lukas Czerner]

The tests exercises the fsadm "remove" functionality, by trying various
combinations of arguments and checking the desired results.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 test/t-fsadm-remove.sh |  123 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 123 insertions(+), 0 deletions(-)
 create mode 100644 test/t-fsadm-remove.sh

diff --git a/test/t-fsadm-remove.sh b/test/t-fsadm-remove.sh
new file mode 100644
index 0000000..c7f71fb
--- /dev/null
+++ b/test/t-fsadm-remove.sh
@@ -0,0 +1,123 @@
+#!/bin/bash
+# Copyright 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions
+# of the GNU General Public License v.2.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+
+test_description='Exercise fsadm remove'
+
+. lib/test
+
+DEV_COUNT=10
+DEV_SIZE=10
+TEST_MAX_SIZE=$(($DEV_COUNT*$DEV_SIZE))
+aux prepare_devs $DEV_COUNT $DEV_SIZE
+TEST_DEVS=$(cat DEVICES)
+export DEFAULT_DEVICE_POOL=$vg1
+export LVOL_PREFIX="lvol"
+lvol1=${LVOL_PREFIX}001
+lvol2=${LVOL_PREFIX}002
+lvol3=${LVOL_PREFIX}003
+
+pool1=$vg2
+pool2=$vg3
+DEFAULT_VOLUME=${DEFAULT_DEVICE_POOL}/$lvol1
+
+_FS=
+which mkfs.ext2 && _FS="ext2"
+which mkfs.ext3 && _FS="ext3"
+which mkfs.ext4 && _FS="ext4"
+which mkfs.xfs  && _FS="xfs"
+
+
+TEST_MNT=$TESTDIR/mnt
+
+# Remove logical volume
+fsadm create $TEST_DEVS
+check lv_field $DEFAULT_VOLUME lv_name $lvol1
+fsadm -f remove $DEFAULT_VOLUME
+not check lv_field $DEFAULT_VOLUME lv_name $lvol1
+
+# Remove volume group
+fsadm create $TEST_DEVS
+check vg_field $DEFAULT_DEVICE_POOL vg_name $DEFAULT_DEVICE_POOL
+fsadm -f remove $DEFAULT_DEVICE_POOL
+not check vg_field $DEFAULT_DEVICE_POOL vg_name $DEFAULT_DEVICE_POOL
+
+if [ "$_FS" ]; then
+	[ ! -d $TEST_MNT ] && mkdir $TEST_MNT &> /dev/null
+	# Remove mounted logical volume by mount point
+	fsadm create fs=$_FS $TEST_DEVS
+	mount ${DM_DEV_DIR}/$DEFAULT_VOLUME $TEST_MNT
+	check lv_field $DEFAULT_VOLUME lv_name $lvol1
+	fsadm -f remove $TEST_MNT
+	not check lv_field $DEFAULT_VOLUME lv_name $lvol1
+	fsadm -f remove $DEFAULT_DEVICE_POOL
+
+	# Remove mounted logical volume by lv
+	fsadm create fs=$_FS $TEST_DEVS
+	mount ${DM_DEV_DIR}/$DEFAULT_VOLUME $TEST_MNT
+	check lv_field $DEFAULT_VOLUME lv_name $lvol1
+	fsadm -f remove $DEFAULT_VOLUME
+	not check lv_field $DEFAULT_VOLUME lv_name $lvol1
+	fsadm -f remove $DEFAULT_DEVICE_POOL
+fi
+
+# Remove unused devices from the pool
+fsadm create $dev1 $dev2 $dev3
+fsadm add $TEST_DEVS
+check vg_field $DEFAULT_DEVICE_POOL pv_count $DEV_COUNT
+fsadm -f remove $TEST_DEVS
+check vg_field $DEFAULT_DEVICE_POOL pv_count 3
+fsadm -f remove --all
+
+# Remove multiple things
+fsadm add $dev1 $dev2 $pool1
+fsadm add $dev3 $dev4 $pool2
+fsadm create $pool2
+fsadm create $dev5 $dev6
+fsadm create $dev7 $dev8
+fsadm add $dev9
+check vg_field $pool1 pv_count 2
+check vg_field $pool2 pv_count 2
+check vg_field $pool2 lv_count 1
+check vg_field $DEFAULT_DEVICE_POOL pv_count 5
+check vg_field $DEFAULT_DEVICE_POOL lv_count 2
+check vg_field $pool1 vg_name $pool1
+check lv_field ${pool2}/$lvol1 lv_name $lvol1
+fsadm -f remove $pool1 ${pool2}/$lvol1 $DEFAULT_VOLUME $dev9
+not check vg_field $pool1 vg_name $pool1
+not check lv_field ${pool2}/$lvol1 lv_name $lvol1
+not check lv_field $DEFAULT_VOLUME lv_name $lvol1
+check vg_field $DEFAULT_DEVICE_POOL pv_count 4
+fsadm -f remove --all
+
+# Remove all
+fsadm add $dev1 $dev2 $pool1
+fsadm add $dev3 $dev4 $pool2
+fsadm create $pool2
+fsadm create $dev5 $dev6
+fsadm create $dev7 $dev8
+fsadm add $dev9
+check vg_field $pool1 pv_count 2
+check vg_field $pool2 pv_count 2
+check vg_field $pool2 lv_count 1
+check vg_field $DEFAULT_DEVICE_POOL pv_count 5
+check vg_field $DEFAULT_DEVICE_POOL lv_count 2
+check vg_field $pool1 vg_name $pool1
+check vg_field $pool2 vg_name $pool2
+check vg_field $DEFAULT_DEVICE_POOL vg_name $DEFAULT_DEVICE_POOL
+fsadm -f remove --all
+not check vg_field $pool1 vg_name $pool1
+not check vg_field $pool2 vg_name $pool2
+not check vg_field $DEFAULT_DEVICE_POOL vg_name $DEFAULT_DEVICE_POOL
+
+fsadm remove --help
+
+# Some cases which should fail
+not fsadm remove
-- 
1.7.4.4

[RFC][PATCH v3 00/16] fsadm update

[Lukas Czerner]

On Tue, 27 Sep 2011, Lukas Czerner wrote:

> Hello again,
> 
> this is third update of the fsadm.
> 
> Changes:
> 
> version 2:
> The main difference is that all the big features are collapsed into one
> patch for better review (since some of the features depends on each other
> and has been developed in that way). Also one new test is added to exercise
> "remove" command.
> 
> version 3:
> In version 2 some patches was unintentionally squashed together, which is
> fixed in this version.
> 
> Here is a link to the first version of the patch set:
> http://www.redhat.com/archives/linux-lvm/2011-September/msg00046.html
> 
> Comments are welcomed.

Optionally you can get the whole patched fsadm script from:

http://people.redhat.com/lczerner/fsadm/fsadm.sh

-Lukas

> 
> Thanks!
> -Lukas
> 
> 

[PATCH v3 13/18] fsadm: remove -y (YES) option

[Zdenek Kabelac]

Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> There is some confusion in using -y (YES) and -f (FORCE) options in
> fsadm. In some cases we are asked for yes/no question which can be
> override by -f option, but not by -y option. Usually most of the questions
> tools ask for are yes/no and it can be overridden by forcing it with -f
> (e.g. fsck.(extN|xfs), lvm and others...) so it make sense to get rid of
> -y option and use only -f instead.
> 
> Also I do not think it is wise to use -y option in fsck.extN since
> people using fsadm would probably not know how it works, so we should
> NOT provide them with that option, but rather let them use "real" fsck
> instead (and let them read man page if needed). Also running fsck with
> -y when you have corrupted file system is probably not a good idea from
> multiple reasons. This is also fixed by this commit.
> 
> This commit removes '-y' option and use '-f' instead. With exception of
> fsck.
> 

NACK

-f  and  -y  are different.

While  fsck will proceed with -f  on mounted file system (leading to certain
damage) -y  option will stop here and just answer -y  to  question about umount.

And also you would change command line syntax and change the behavior of
already written scripts.

Zdenek

[PATCH v3 03/18] fsadm: Add simple configuration file

[Zdenek Kabelac]

Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  scripts/fsadm.sh |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
> index 58ff0ab..98921d3 100755
> --- a/scripts/fsadm.sh
> +++ b/scripts/fsadm.sh
> @@ -88,6 +88,9 @@ PROCDEVICES="/proc/devices"
>  PROCPARTITIONS="/proc/partitions"
>  NULL="$DM_DEV_DIR/null"
>  
> +CONFIG_PATHS="/etc/fsadm.conf ~/fsadm.conf"
> +CONFIG=
> +


Looks too complicated -  sourcing config file with some defines looks like
better approach here  if you really think it's useful.


Zdenek

[PATCH v3 02/18] fsadm: Make all internal math in kilobytes

[Zdenek Kabelac]

Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> We should make all math in fsadm to count with kilobytes (if possible)
> because it will bypass the problem of having too big numbers. Also the
> smaller granularity might have troubles with 512B alignment.


I don't like this one -  if you have problems with big numbers - bugreport
bash. You are probably covering up different bug here.


Zdenek

[PATCH v3 12/18] lvresize: Specify --resize-fs-only when going to use fsadm resize

[Zdenek Kabelac]

Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> When lvresize is calling fsadm to resize the file system, originally
> it was the default action. However now fsadm support much more
> features and it is doing the lvresize by itself. Luckily there is
> still a way to tell fsadm to resize only the file system ane leave
> the rest up to the lvresize - provide "--resize-fs-only" argument.
> 
> This commit adds "--resize-fs-only" argument when we are going to
> use fsadm to resize the fs from within the lvresize.


There is a big reason to call lvresize for this operation - it's the cluster
atomicity, so the resize operation (while using lvm tools) provide consistent
results - this breaks it - so NACK for now.

Zdenek

[PATCH v3 02/18] fsadm: Make all internal math in kilobytes

[Lukas Czerner]

On Tue, 27 Sep 2011, Zdenek Kabelac wrote:

> Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> > We should make all math in fsadm to count with kilobytes (if possible)
> > because it will bypass the problem of having too big numbers. Also the
> > smaller granularity might have troubles with 512B alignment.
> 
> 
> I don't like this one -  if you have problems with big numbers - bugreport
> bash. You are probably covering up different bug here.

I am not sure you do not "like" this one, it does not seem like a real
reason, could you be more specific ? This patch just forces internal
math to be done in kilobytes, since there is no real reason to do it in
bytes.

It has no implications for the user interface.


Also, please when you reply to my patches, could you cc me as well ?
This is not the only list I am sending patches to so I can not just
check all lists for replies.

Thanks!
-Lukas

> 
> 
> Zdenek
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel
> 

[PATCH v3 12/18] lvresize: Specify --resize-fs-only when going to use fsadm resize

[Lukas Czerner]

On Tue, 27 Sep 2011, Zdenek Kabelac wrote:

> Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> > When lvresize is calling fsadm to resize the file system, originally
> > it was the default action. However now fsadm support much more
> > features and it is doing the lvresize by itself. Luckily there is
> > still a way to tell fsadm to resize only the file system ane leave
> > the rest up to the lvresize - provide "--resize-fs-only" argument.
> > 
> > This commit adds "--resize-fs-only" argument when we are going to
> > use fsadm to resize the fs from within the lvresize.
> 
> 
> There is a big reason to call lvresize for this operation - it's the cluster
> atomicity, so the resize operation (while using lvm tools) provide consistent
> results - this breaks it - so NACK for now.
> 
> Zdenek

Sure, the patch actually does not change the atomicity, however the
description might be a bit badly worded, I'll change that.

Thanks!
-Lukas

> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel
> 

[PATCH v3 13/18] fsadm: remove -y (YES) option

[Lukas Czerner]

On Tue, 27 Sep 2011, Zdenek Kabelac wrote:

> Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> > There is some confusion in using -y (YES) and -f (FORCE) options in
> > fsadm. In some cases we are asked for yes/no question which can be
> > override by -f option, but not by -y option. Usually most of the questions
> > tools ask for are yes/no and it can be overridden by forcing it with -f
> > (e.g. fsck.(extN|xfs), lvm and others...) so it make sense to get rid of
> > -y option and use only -f instead.
> > 
> > Also I do not think it is wise to use -y option in fsck.extN since
> > people using fsadm would probably not know how it works, so we should
> > NOT provide them with that option, but rather let them use "real" fsck
> > instead (and let them read man page if needed). Also running fsck with
> > -y when you have corrupted file system is probably not a good idea from
> > multiple reasons. This is also fixed by this commit.
> > 
> > This commit removes '-y' option and use '-f' instead. With exception of
> > fsck.
> > 
> 
> NACK
> 
> -f  and  -y  are different.
> 
> While  fsck will proceed with -f  on mounted file system (leading to certain
> damage) -y  option will stop here and just answer -y  to  question about umount.

I am not saying that -f should be used instead of -y.

That is why having both options for the fsadm does not make sense,
because it is not just fsck which fsadm is using internally. Force means
force, we are trying to simplify things here not map every argument of
every tool into fsadm.

When user force things, he should know what is he doing, since he really
is using "force" :). -f option is quite generic and most of the tools
does have it, however -y options is specific, that is why fsadm does not
have -n option, or even -p option.

Generally I am in favour of changing check command to only check for
file system consistency and if problems are found report to the user
that he should run proper fsck by itself, since it is a bit delicate
situation and the user should really know what is he doing if he does
not want to lose data.

Finally I think that having both '-f' and '-y' option which are really
inconsistent among the tools does not make sense.

Thanks!
-Lukas

> 
> And also you would change command line syntax and change the behavior of
> already written scripts.
> 
> Zdenek
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel
> 

-- 

[PATCH v3 03/18] fsadm: Add simple configuration file

[Lukas Czerner]

On Tue, 27 Sep 2011, Zdenek Kabelac wrote:

> Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  scripts/fsadm.sh |   30 ++++++++++++++++++++++++++++++
> >  1 files changed, 30 insertions(+), 0 deletions(-)
> > 
> > diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
> > index 58ff0ab..98921d3 100755
> > --- a/scripts/fsadm.sh
> > +++ b/scripts/fsadm.sh
> > @@ -88,6 +88,9 @@ PROCDEVICES="/proc/devices"
> >  PROCPARTITIONS="/proc/partitions"
> >  NULL="$DM_DEV_DIR/null"
> >  
> > +CONFIG_PATHS="/etc/fsadm.conf ~/fsadm.conf"
> > +CONFIG=
> > +
> 
> 
> Looks too complicated -  sourcing config file with some defines looks like
> better approach here  if you really think it's useful.
> 
> 
> Zdenek

I do not think we should or can support setting all the different
variables we are using in fsadm via configuration file. That is why I
choose to only have strictly defined options in configuration file. This
way we have absolute control over what is configured via the
configuration file, which is the right approach.

Thanks!
-Lukas

> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel
> 

[PATCH v3 13/18] fsadm: remove -y (YES) option

[Zdenek Kabelac]

Dne 3.10.2011 18:39, Lukas Czerner napsal(a):
> On Tue, 27 Sep 2011, Zdenek Kabelac wrote:
>
>> Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
>>> There is some confusion in using -y (YES) and -f (FORCE) options in
>>> fsadm. In some cases we are asked for yes/no question which can be
>>> override by -f option, but not by -y option. Usually most of the questions
>>> tools ask for are yes/no and it can be overridden by forcing it with -f
>>> (e.g. fsck.(extN|xfs), lvm and others...) so it make sense to get rid of
>>> -y option and use only -f instead.
>>>
>>> Also I do not think it is wise to use -y option in fsck.extN since
>>> people using fsadm would probably not know how it works, so we should
>>> NOT provide them with that option, but rather let them use "real" fsck
>>> instead (and let them read man page if needed). Also running fsck with
>>> -y when you have corrupted file system is probably not a good idea from
>>> multiple reasons. This is also fixed by this commit.
>>>
>>> This commit removes '-y' option and use '-f' instead. With exception of
>>> fsck.
>>>
>>
>> NACK
>>
>> -f  and  -y  are different.
>>
>> While  fsck will proceed with -f  on mounted file system (leading to certain
>> damage) -y  option will stop here and just answer -y  to  question about umount.
>
> I am not saying that -f should be used instead of -y.
>
> That is why having both options for the fsadm does not make sense,
> because it is not just fsck which fsadm is using internally. Force means
> force, we are trying to simplify things here not map every argument of
> every tool into fsadm.
>
> When user force things, he should know what is he doing, since he really
> is using "force" :). -f option is quite generic and most of the tools
> does have it, however -y options is specific, that is why fsadm does not
> have -n option, or even -p option.
>
> Generally I am in favour of changing check command to only check for
> file system consistency and if problems are found report to the user
> that he should run proper fsck by itself, since it is a bit delicate
> situation and the user should really know what is he doing if he does
> not want to lose data.
>
> Finally I think that having both '-f' and '-y' option which are really
> inconsistent among the tools does not make sense.

Well think more about the difference between -y and -f.

If something does not make sense to you, the first solution is to remove it, 
the second (harder) is to try to think about it deeply and try to understand 
for multiple reasons of this NACK - and from your other thread replies, it 
also applies to your other  'does not make sense to me' comments...

Zdenek

[PATCH v3 13/18] fsadm: remove -y (YES) option

[Lukas Czerner]

On Mon, 3 Oct 2011, Zdenek Kabelac wrote:

> Dne 3.10.2011 18:39, Lukas Czerner napsal(a):
> > On Tue, 27 Sep 2011, Zdenek Kabelac wrote:
> > 
> > > Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> > > > There is some confusion in using -y (YES) and -f (FORCE) options in
> > > > fsadm. In some cases we are asked for yes/no question which can be
> > > > override by -f option, but not by -y option. Usually most of the
> > > > questions
> > > > tools ask for are yes/no and it can be overridden by forcing it with -f
> > > > (e.g. fsck.(extN|xfs), lvm and others...) so it make sense to get rid of
> > > > -y option and use only -f instead.
> > > > 
> > > > Also I do not think it is wise to use -y option in fsck.extN since
> > > > people using fsadm would probably not know how it works, so we should
> > > > NOT provide them with that option, but rather let them use "real" fsck
> > > > instead (and let them read man page if needed). Also running fsck with
> > > > -y when you have corrupted file system is probably not a good idea from
> > > > multiple reasons. This is also fixed by this commit.
> > > > 
> > > > This commit removes '-y' option and use '-f' instead. With exception of
> > > > fsck.
> > > > 
> > > 
> > > NACK
> > > 
> > > -f  and  -y  are different.
> > > 
> > > While  fsck will proceed with -f  on mounted file system (leading to
> > > certain
> > > damage) -y  option will stop here and just answer -y  to  question about
> > > umount.
> > 
> > I am not saying that -f should be used instead of -y.
> > 
> > That is why having both options for the fsadm does not make sense,
> > because it is not just fsck which fsadm is using internally. Force means
> > force, we are trying to simplify things here not map every argument of
> > every tool into fsadm.
> > 
> > When user force things, he should know what is he doing, since he really
> > is using "force" :). -f option is quite generic and most of the tools
> > does have it, however -y options is specific, that is why fsadm does not
> > have -n option, or even -p option.
> > 
> > Generally I am in favour of changing check command to only check for
> > file system consistency and if problems are found report to the user
> > that he should run proper fsck by itself, since it is a bit delicate
> > situation and the user should really know what is he doing if he does
> > not want to lose data.
> > 
> > Finally I think that having both '-f' and '-y' option which are really
> > inconsistent among the tools does not make sense.
> 
> Well think more about the difference between -y and -f.
> 
> If something does not make sense to you, the first solution is to remove it,
> the second (harder) is to try to think about it deeply and try to understand
> for multiple reasons of this NACK - and from your other thread replies, it
> also applies to your other  'does not make sense to me' comments...
> 
> Zdenek
> 

I think I gave you multiple reasons why it does not make sense, if you
do not want to listen, I do not really care :) but it does to change the
fact that it is not consistent and it is confusing. No talking about the
fact that doing fsck with -y might be really bad idea if you do not know
what you're doing, or if you at least do not have metadata backup.

-Lukas

[PATCH v3 13/18] fsadm: remove -y (YES) option

[Zdenek Kabelac]

Dne 4.10.2011 08:29, Lukas Czerner napsal(a):
> On Mon, 3 Oct 2011, Zdenek Kabelac wrote:
>
>> Dne 3.10.2011 18:39, Lukas Czerner napsal(a):
>>> On Tue, 27 Sep 2011, Zdenek Kabelac wrote:
>>>
>>>> Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
>>>>> There is some confusion in using -y (YES) and -f (FORCE) options in
>>>>> fsadm. In some cases we are asked for yes/no question which can be
>>>>> override by -f option, but not by -y option. Usually most of the
>>>>> questions
>>>>> tools ask for are yes/no and it can be overridden by forcing it with -f
>>>>> (e.g. fsck.(extN|xfs), lvm and others...) so it make sense to get rid of
>>>>> -y option and use only -f instead.
>>>>>
>>>>> Also I do not think it is wise to use -y option in fsck.extN since
>>>>> people using fsadm would probably not know how it works, so we should
>>>>> NOT provide them with that option, but rather let them use "real" fsck
>>>>> instead (and let them read man page if needed). Also running fsck with
>>>>> -y when you have corrupted file system is probably not a good idea from
>>>>> multiple reasons. This is also fixed by this commit.
>>>>>
>>>>> This commit removes '-y' option and use '-f' instead. With exception of
>>>>> fsck.
>>>>>
>>>>
>>>> NACK
>>>>
>>>> -f  and  -y  are different.
>>>>
>>>> While  fsck will proceed with -f  on mounted file system (leading to
>>>> certain
>>>> damage) -y  option will stop here and just answer -y  to  question about
>>>> umount.
>>>
>>> I am not saying that -f should be used instead of -y.
>>>
>>> That is why having both options for the fsadm does not make sense,
>>> because it is not just fsck which fsadm is using internally. Force means
>>> force, we are trying to simplify things here not map every argument of
>>> every tool into fsadm.
>>>
>>> When user force things, he should know what is he doing, since he really
>>> is using "force" :). -f option is quite generic and most of the tools
>>> does have it, however -y options is specific, that is why fsadm does not
>>> have -n option, or even -p option.
>>>
>>> Generally I am in favour of changing check command to only check for
>>> file system consistency and if problems are found report to the user
>>> that he should run proper fsck by itself, since it is a bit delicate
>>> situation and the user should really know what is he doing if he does
>>> not want to lose data.
>>>
>>> Finally I think that having both '-f' and '-y' option which are really
>>> inconsistent among the tools does not make sense.
>>
>> Well think more about the difference between -y and -f.
>>
>> If something does not make sense to you, the first solution is to remove it,
>> the second (harder) is to try to think about it deeply and try to understand
>> for multiple reasons of this NACK - and from your other thread replies, it
>> also applies to your other  'does not make sense to me' comments...
>>
>> Zdenek
>>
>
> I think I gave you multiple reasons why it does not make sense, if you
> do not want to listen, I do not really care :) but it does to change the
> fact that it is not consistent and it is confusing. No talking about the
> fact that doing fsck with -y might be really bad idea if you do not know
> what you're doing, or if you at least do not have metadata backup.
>

Well simply take it as a fact -y  and -f  will stay.

And please try to read what has been written (or even said) about the reasons 
these options have to stay supported - like all other options which are 
already part of fsadm.

Zdenek

[PATCH v3 06/18] fsadm: Fsck extN before resize only if it is not mounted

[Zdenek Kabelac]

Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> Signed-off-by: Lukas Czerner<lczerner@redhat.com>
> ---
>   scripts/fsadm.sh |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
> index d4901d1..f43d72e 100755
> --- a/scripts/fsadm.sh
> +++ b/scripts/fsadm.sh
> @@ -415,8 +415,9 @@ resize_ext() {
>   	decode_size $1 $BLOCKSIZE
>   	FSFORCE=$FORCE
>
> +	detect_mounted
>   	if [ "$NEWBLOCKCOUNT" -lt "$BLOCKCOUNT" -o "$EXTOFF" -eq 1 ]; then
> -		detect_mounted&&  verbose "$RESIZE_EXT needs unmounted filesystem"&&  try_umount
> +		[ "$MOUNTED" ]&&  verbose "$RESIZE_EXT needs unmounted filesystem"&&  try_umount
>   		REMOUNT=$MOUNTED
>   	fi
>


Patches like these need to go in the front of the patch set  so they could be 
eaily applied upstream.


Zdenek

[PATCH v3 08/18] fsadm: Remove unnecessary modification of PATH variable

[Zdenek Kabelac]

Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> User (or the system) should be set properly so there is no need to
> modify PATH variable. It also prevent us from setting appropriate PATH
> before calling the fsadm.
>
> Signed-off-by: Lukas Czerner<lczerner@redhat.com>
> ---
>   scripts/fsadm.sh |    3 ---
>   1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
> index 2d5c6f9..a1044f7 100755
> --- a/scripts/fsadm.sh
> +++ b/scripts/fsadm.sh
> @@ -37,9 +37,6 @@
>
>   TOOL=$(basename "$0")
>
> -_SAVEPATH=$PATH
> -PATH=/sbin:/usr/sbin:/bin:/usr/sbin:$PATH
> -
>   # utilities
>   TUNE_EXT=tune2fs
>   RESIZE_EXT=resize2fs


Move this patch out of the future set - it's for separate discussion.

Zdenek

[PATCH v3 08/18] fsadm: Remove unnecessary modification of PATH variable

[Lukas Czerner]

On Tue, 4 Oct 2011, Zdenek Kabelac wrote:

> Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> > User (or the system) should be set properly so there is no need to
> > modify PATH variable. It also prevent us from setting appropriate PATH
> > before calling the fsadm.
> > 
> > Signed-off-by: Lukas Czerner<lczerner@redhat.com>
> > ---
> >   scripts/fsadm.sh |    3 ---
> >   1 files changed, 0 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
> > index 2d5c6f9..a1044f7 100755
> > --- a/scripts/fsadm.sh
> > +++ b/scripts/fsadm.sh
> > @@ -37,9 +37,6 @@
> > 
> >   TOOL=$(basename "$0")
> > 
> > -_SAVEPATH=$PATH
> > -PATH=/sbin:/usr/sbin:/bin:/usr/sbin:$PATH
> > -
> >   # utilities
> >   TUNE_EXT=tune2fs
> >   RESIZE_EXT=resize2fs
> 
> 
> Move this patch out of the future set - it's for separate discussion.
> 
> Zdenek

So let the separate discussion begin!

-Lukas

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Zdenek Kabelac]

Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> This commit adds new functionality in the form of new commands. Namely
> it is create, list, add and remove. This commit also changes the way how
> are commands recognised an executed. The new approach is more suitable
> for bigger number of commands.
>
> Resize command is also significantly reworked. Unlike in the old
> approach fsadm will always attempt to resize the logical volume as well,
> since this is what it is expected. Leave the --lvresize option for
> backwards compatibility, but remove it from the help. If no file system
> resides on the logical volume, only the volume will be resized.
>
> * Create command provides the functionality of creating a new logical
>    volumes including defined file system.
> * List command provides the functionality of listing useful information
>    about the logical volumes, file systems and devices.
> * Add command allows to add devices into volume groups (pool).
> * Remove command allows to remove the volumes or volume groups from the
>    system.
>
> Signed-off-by: Lukas Czerner<lczerner@redhat.com>

> @@ -96,7 +109,6 @@ tool_usage() {
>   	echo "    -e | --ext-offline  unmount filesystem before ext2/ext3/ext4 resize"
>   	echo "    -f | --force        Bypass sanity checks"
>   	echo "    -n | --dry-run      Print commands without running them"
> -	echo "    -l | --lvresize     Resize given device (if it is LVM device)"
>   	echo "    -y | --yes          Answer \"yes\" at any prompts"
>   	echo
>   	echo "  new_size - Absolute number of filesystem blocks to be in the filesystem,"


Again - no way to remove already supported option.



> @@ -115,13 +127,37 @@ error() {
>   	cleanup 1
>   }
>
> -dry() {
> +warn() {
> +	echo "$TOOL: $@">&2
> +}
> +
> +dry_nofail() {
>   	if [ "$DRY" -ne 0 ]; then
>   		verbose "Dry execution $@"
>   		return 0
>   	fi
>   	verbose "Executing $@"
> -	$@
> +	local IFS=" "
> +	eval "$*"
> +}


That looks weird ?
Dry execution has it's limits - if something fails - it cannot be be 
simulated. Going with _nofail seems to be dangerous.

So for now skip it.



> +float_math() {
> +	if [ $# -le 0 ]; then
> +		return
> +	fi
> +	result=$(LANG=C echo "scale=2; $@" | "$BC" -q 2>  /dev/null)
> +	echo "$result"
> +}
> +
> +is_natural() {
> +	case "$1" in
> +		"" | *[!0-9]*) return 1;;
> +		*) return 0;;
> +	esac
>   }


Please - split whole float_math in separate patch - I think this one could be 
easily upstreamed. Though it's adding new deps - however 'bc' is very common 
command available probably everywhere - so it should not present a problem.

(Though needs new packaging deps to be properly handled - wondering how we 
could make this easier for package maintainers)





>
>   cleanup() {
> @@ -132,54 +168,140 @@ cleanup() {
>   		verbose "Remounting unmounted filesystem back"
>   		dry "$MOUNT" "$VOLUME" "$MOUNTED"
>   	fi
> -	IFS=$IFS_OLD
>   	trap 2
>
>   	test "$1" -eq 2&&  verbose "Break detected"
>
> -	if [ "$DO_LVRESIZE" -eq 2 ]; then
> -		# start LVRESIZE with the filesystem modification flag
> -		# and allow recursive call of fsadm
> -		_FSADM_YES=$YES
> -		export _FSADM_YES
> -		unset FSADM_RUNNING
> -		test -n "$LVM_BINARY"&&  PATH=$_SAVEPATH
> -		dry exec "$LVM" lvresize $VERB $FORCE -r -L${NEWSIZE}b "$VOLUME_ORIG"
> -	fi
> -

Again - please do not remove things you do not understand.

And also thing like this needs to be in a separate patch - not bundled in 
hundreds patch lines.




>   	# error exit status for break
>   	exit ${1:-1}
>   }
>
> +#####################################
> +# Convet the size into human readable
> +# form. size in Bytes expected
> +#####################################
> +humanize_size() {
> +	size="$1"
> +	count=0
> +	while [ ${size%%.*} -ge 1024 ]&&  [ $count -lt 7 ]; do
> +		size=$(float_math $size/1024)
> +		count=$(($count+1))
> +	done
> +	case $count in
> +		0) unit="B" ;;
> +		1) unit="KiB" ;;
> +		2) unit="MiB" ;;
> +		3) unit="GiB" ;;
> +		4) unit="TiB" ;;
> +		5) unit="PiB" ;;
> +		6) unit="EiB" ;;
> +		7) unit="ZiB" ;;
> +		8) unit="YiB" ;;
> +		*) unit="???" ;;
> +	esac
> +	echo "$size $unit"
> +}
> +

We need to make sure - using are handled in the very same way by both commands
(lvm & fsadm -  might probably require to parse lvm.conf  for 
'si_unit_consistency' option - so the user will not be confused.
(As fsadm is part of lvm2 package and this nice extensions might bring some 
unwanted confusion to users with differently configure lvm2).



> +#############################
> +# Get size of entN filesystem
> +# by reading tune2fs output
> +#############################
> +get_ext_size() {
> +	local IFS=$NL
> +	for i in $(LANG=C $TUNE_EXT -l "$VOLUME"); do
> +		case "$i" in
> +			"Block size"*) bsize=${i##*  } ;;
> +			"Block count"*) bcount=${i##*  } ;;
> +			"Reserved block count"*) rbcount=${i##*  } ;;
> +			"Free blocks"*) fbcount=${i##*  } ;;
> +		esac
> +	done
> +
> +	total=$(($bcount*$bsize))
> +	TOTAL=$(humanize_size $total)
> +	used=$((($bcount-$fbcount)*$bsize))
> +	USED=$(humanize_size $used)
> +	free=$((($fbcount-$rbcount)*$bsize))
> +	FREE=$(humanize_size $free)
> +}
> +
> +############################
> +# Get size of xfs file system
> +# by reading the df output or
> +# examine file system with
> +# xfs_db tool
> +#############################
> +get_xfs_size() {
> +	local IFS=$NL
> +	if [ -z "$MOUNTED" ]; then
> +
> +		for i in $(LANG=C xfs_db -c 'sb' -c 'print blocksize fdblocks dblocks logblocks agcount' $VOLUME); do

"$XFS_DB"

"$VOLUME"


> +	line=$("$DF" -k "$VOLUME" | "$GREP" -e "$MOUNTED$" | sed -e 's/  */ /g')

"$SED"


> +	line=${line#* }
> +	total=$(echo $line | cut -d' ' -f1)

"$CUT"

Though maybe there is proably native shell way to extract things like this 
without extra execution command.


> +detect_fs_size() {
> +	if [ -z "$FSTYPE" ]; then
> +		return
> +	fi

Why ?
(Imho it's handled by case below via return 1


> +	case "$FSTYPE" in
> +		ext[234]) get_ext_size ;;
> +		xfs) get_xfs_size ;;
> +		*) return 1 ;;
> +	esac
> +	return 0
> +}
> +
>   # convert parameter from Exa/Peta/Tera/Giga/Mega/Kilo/Bytes and blocks
>   # (2^(60/50/40/30/20/10/0))
>   decode_size() {
>   	case "$1" in
> -	 *[eE]) NEWSIZE=$(( ${1%[eE]} * 1152921504606846976 )) ;;
> -	 *[pP]) NEWSIZE=$(( ${1%[pP]} * 1125899906842624 )) ;;
> -	 *[tT]) NEWSIZE=$(( ${1%[tT]} * 1099511627776 )) ;;
> -	 *[gG]) NEWSIZE=$(( ${1%[gG]} * 1073741824 )) ;;
> -	 *[mM]) NEWSIZE=$(( ${1%[mM]} * 1048576 )) ;;
> -	 *[kK]) NEWSIZE=$(( ${1%[kK]} * 1024 )) ;;
> +	 *[eE]) NEWSIZE=$(float_math "${1%[eE]} * 1152921504606846976") ;;
> +	 *[pP]) NEWSIZE=$(float_math "${1%[pP]} * 1125899906842624") ;;
> +	 *[tT]) NEWSIZE=$(float_math "${1%[tT]} * 1099511627776") ;;
> +	 *[gG]) NEWSIZE=$(float_math "${1%[gG]} * 1073741824") ;;
> +	 *[mM]) NEWSIZE=$(float_math "${1%[mM]} * 1048576") ;;
> +	 *[kK]) NEWSIZE=$(float_math "${1%[kK]} * 1024") ;;
>   	 *[bB]) NEWSIZE=${1%[bB]} ;;
>   	     *) NEWSIZE=$(( $1 * $2 )) ;;

Yep - separate patch for math operation.


>   	esac
> -	#NEWBLOCKCOUNT=$(round_block_size $NEWSIZE $2)
> -	NEWBLOCKCOUNT=$(( $NEWSIZE / $2 ))
>
> -	if [ $DO_LVRESIZE -eq 1 ]; then
> -		# start lvresize, but first cleanup mounted dirs
> -		DO_LVRESIZE=2
> -		cleanup 0
> -	fi

Recursion stays for now.


> +	NEWSIZE=${NEWSIZE%%.*}
> +	NEWBLOCKCOUNT=$(float_math "$NEWSIZE / $2")
> +	NEWBLOCKCOUNT=${NEWBLOCKCOUNT%%.*}
>   }
>
>   # detect filesystem on the given device
>   # dereference device name if it is symbolic link
>   detect_fs() {
> -	VOLUME_ORIG=$1
> +	VOLUME_ORIG="$1"
>   	VOLUME=${1/#"${DM_DEV_DIR}/"/}
> -	VOLUME=$("$READLINK" $READLINK_E "$DM_DEV_DIR/$VOLUME") || error "Cannot get readlink \"$1\""
> +	VOLUME=$("$READLINK" "$READLINK_E" "$DM_DEV_DIR/$VOLUME") || error "Cannot get readlink \"$1\""

Do not modify same thing in multiple patches
(The advantage of shuffling these things in front of the whole set.)




> -	MOUNTED=$("$GREP" ^"$VOLUME" "$PROCMOUNTS")
> +	MOUNTED=$("$GREP" -e "^$VOLUME " "$PROCMOUNTS")

Candidate for separate patch in front.
Assuming we currently do not handle well VGs starting with letter '-'.
So it's hidden bugfix (grep -e)


> @@ -295,17 +417,19 @@ resize_ext() {
>   	if [ "$NEWBLOCKCOUNT" -lt "$BLOCKCOUNT" -o "$EXTOFF" -eq 1 ]; then
>   		detect_mounted&&  verbose "$RESIZE_EXT needs unmounted filesystem"&&  try_umount
>   		REMOUNT=$MOUNTED
> -		if test -n "$MOUNTED" ; then
> -			# Forced fsck -f for umounted extX filesystem.
> -			case "$-" in
> -			  *i*) dry "$FSCK" $YES -f "$VOLUME" ;;
> -			  *) dry "$FSCK" -f -p "$VOLUME" ;;
> -			esac
> -		fi
> +	fi
> +
> +	# We should really do the fsck before every resize.

Well user is always the master - if he is brave to go without it, let him do 
what he wants (i.e. destroy his fs).


> -		dry "$RESIZE_REISER" -s $NEWSIZE "$VOLUME"
> +		dry "$RESIZE_REISER" -s "$NEWSIZE" "$VOLUME"

If a variable is under our full control, and we know the content could never 
have a space inside - we do not need to put all of them to "".
Though I do not really mind about this one.


> +make_ext() {
> +	device="$1"
> +	fstyp="$2"
> +	bsize=4
> +
> +	if [ "$YES" ]; then
> +		force="-F"
> +	fi
> +
> +	dry "mkfs.$fstyp" "$force" -b "$(($bsize*1024))" $device

I'd prefer to stay with upper case shell variable names.


>   ####################
>   # Resize filesystem
>   ####################
> -resize() {
> +resize_fs() {
>   	NEWSIZE=$2
> -	detect_fs "$1"
> +	detect_fs "$1" ||  error "Cannot get FSTYPE of \"$VOLUME\""
> +	verbose "\"$FSTYPE\" filesystem found on \"$VOLUME\""
> +	if [ "$NEWSIZE" ]; then
> +		is_natural $NEWSIZE ||  error "$NEWSIZE is not valid number for file system size"
> +	fi
>   	detect_device_size
>   	verbose "Device \"$VOLUME\" size is $DEVSIZE bytes"
>   	# if the size parameter is missing use device size
>   	#if [ -n "$NEWSIZE" -a $NEWSIZE<
>   	test -z "$NEWSIZE"&&  NEWSIZE=${DEVSIZE}b
> -	IFS=$NL
> +	local IFS=$NL
>   	case "$FSTYPE" in
>   	  "ext3"|"ext2"|"ext4") resize_ext $NEWSIZE ;;
>   	  "reiserfs") resize_reiser $NEWSIZE ;;
>   	  "xfs") resize_xfs $NEWSIZE ;;
>   	  *) error "Filesystem \"$FSTYPE\" on device \"$VOLUME\" is not supported by this tool" ;;
>   	esac || error "Resize $FSTYPE failed"
> -	cleanup 0


Your recursion removal patch must be separate patch proposal.


> +		case $i in
> +			"size="*)		[ -z "$size" ]&&  size=${i##*=};;
> +			[[:digit:]]*)		[ -z "$size" ]&&  size=${i##*=};;
> +			+[[:digit:]]*)		[ -z "$size" ]&&  size=${i##*=};;
> +			-[[:digit:]]*)		[ -z "$size" ]&&  size=${i##*=};;
> +			*) error "Wrong option $i. (see: $TOOL --help)";;
> +		esac

Another separate patch - to add support  for  +-


> +
> +		[ "$GROUP" ]&&  [ "$GROUP" != "$vgname" ]&&  error "Some devices are in different"\
> +								   "group than the logical volume"\
> +								   "($lvname). Please provide unused"\

test -n  -a   !=  &&


> +	# Avoid callinf lvresize in recurse
> +	if [ "$FSADM_RESIZE_RECURSE" ]; then
> +		error "Recursive lvresize call detected! Exiting."
> +	fi

Recursion - separate patch...



> +#################################
> +# Check the device list to detect
> +# if there is not multiple groups

are

> +#################################
> +detect_device_group() {
> +	ret=0
> +	prev_vgname=""
> +	vgs=""
> +	tmp=$(mktemp)
> +
> +	LANG=C "$LVM" vgs -o vg_name,pv_name --separator ' ' --noheadings --nosuffix 2>  /dev/null | sed -e 's/^ *//' | sort | uniq>  $tmp
> +	NOT_IN_GROUP=
> +	IN_GROUP=
> +	for i in "$@"; do
> +		cur_vgname=$("$GREP" -e "$i$" "$tmp" | cut -d' ' -f1)
> +
> +		if [ -z "$cur_vgname" ]; then
> +			NOT_IN_GROUP+="$i "
> +			continue
> +		else
> +			IN_GROUP+="$i "
> +		fi
> +
> +		if [ -z "$prev_vgname" ]; then
> +			prev_vgname=$cur_vgname
> +			vgs=$cur_vgname
> +			continue;
> +		fi
> +
> +		if [ "$cur_vgname" != "$prev_vgname" ]; then
> +			ret=1
> +			vgs+=" $cur_vgname"

Avoid using bashisms as much as you could - I think we could make the shell 
script usable with traditional posix shell.

(look for checkbashism perl script)
i.e.   vgs="$vgs $cur_name"  - like you do elsewhere.



> +		if [ $lines -eq 1 ]; then
> +			vgname=$(echo $vgroups | sed -e 's/^ *//' | cut -d' ' -f1)

Assuming shell has enough power to do this on its own.

> +	dmnumber=$(cat "$PROCDEVICES" | "$GREP" device-mapper | sed -e 's/^  *//')

"$CAT"

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Lukas Czerner]

On Tue, 4 Oct 2011, Zdenek Kabelac wrote:

> Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> > This commit adds new functionality in the form of new commands. Namely
> > it is create, list, add and remove. This commit also changes the way how
> > are commands recognised an executed. The new approach is more suitable
> > for bigger number of commands.
> > 
> > Resize command is also significantly reworked. Unlike in the old
> > approach fsadm will always attempt to resize the logical volume as well,
> > since this is what it is expected. Leave the --lvresize option for
> > backwards compatibility, but remove it from the help. If no file system
> > resides on the logical volume, only the volume will be resized.
> > 
> > * Create command provides the functionality of creating a new logical
> >    volumes including defined file system.
> > * List command provides the functionality of listing useful information
> >    about the logical volumes, file systems and devices.
> > * Add command allows to add devices into volume groups (pool).
> > * Remove command allows to remove the volumes or volume groups from the
> >    system.
> > 
> > Signed-off-by: Lukas Czerner<lczerner@redhat.com>
> 
> > @@ -96,7 +109,6 @@ tool_usage() {
> >   	echo "    -e | --ext-offline  unmount filesystem before ext2/ext3/ext4
> > resize"
> >   	echo "    -f | --force        Bypass sanity checks"
> >   	echo "    -n | --dry-run      Print commands without running them"
> > -	echo "    -l | --lvresize     Resize given device (if it is LVM
> > device)"
> >   	echo "    -y | --yes          Answer \"yes\" at any prompts"
> >   	echo
> >   	echo "  new_size - Absolute number of filesystem blocks to be in the
> > filesystem,"
> 
> 
> Again - no way to remove already supported option.

This is only removed from the tool_usage(), as you can see. I am not
removing it from the code, but only from the documentation since it is
not needed in the new code.

> 
> 
> 
> > @@ -115,13 +127,37 @@ error() {
> >   	cleanup 1
> >   }
> > 
> > -dry() {
> > +warn() {
> > +	echo "$TOOL: $@">&2
> > +}
> > +
> > +dry_nofail() {
> >   	if [ "$DRY" -ne 0 ]; then
> >   		verbose "Dry execution $@"
> >   		return 0
> >   	fi
> >   	verbose "Executing $@"
> > -	$@
> > +	local IFS=" "
> > +	eval "$*"
> > +}
> 
> 
> That looks weird ?
> Dry execution has it's limits - if something fails - it cannot be be
> simulated. Going with _nofail seems to be dangerous.

Actually what looks weird that we call function we are executing
commands with "dry()" because it really is dry only if --dry-run has
been specified. I only coped with that, but I would be really in favour
of changing it to run() or similar.

dry_nofail() is different from dry() only in the way it handles failures
of invoked command. In dry() we are calling error() in that case, but
dry_nofail() is not calling error, but rather report the return value,
so its user can handle the failure by himself. Please see where it is
used.

> 
> So for now skip it.
> 
> 
> 
> > +float_math() {
> > +	if [ $# -le 0 ]; then
> > +		return
> > +	fi
> > +	result=$(LANG=C echo "scale=2; $@" | "$BC" -q 2>  /dev/null)
> > +	echo "$result"
> > +}
> > +
> > +is_natural() {
> > +	case "$1" in
> > +		"" | *[!0-9]*) return 1;;
> > +		*) return 0;;
> > +	esac
> >   }
> 
> 
> Please - split whole float_math in separate patch - I think this one could be
> easily upstreamed. Though it's adding new deps - however 'bc' is very common
> command available probably everywhere - so it should not present a problem.

I can do that, however it is just one step in allowing float math in
fsadm. I have done that so we have easier work in the future and because
it is useful, but I have no intention of making fsadm fully float number
aware, at least for now.

> 
> (Though needs new packaging deps to be properly handled - wondering how we
> could make this easier for package maintainers)
> 
> 
> 
> 
> 
> > 
> >   cleanup() {
> > @@ -132,54 +168,140 @@ cleanup() {
> >   		verbose "Remounting unmounted filesystem back"
> >   		dry "$MOUNT" "$VOLUME" "$MOUNTED"
> >   	fi
> > -	IFS=$IFS_OLD
> >   	trap 2
> > 
> >   	test "$1" -eq 2&&  verbose "Break detected"
> > 
> > -	if [ "$DO_LVRESIZE" -eq 2 ]; then
> > -		# start LVRESIZE with the filesystem modification flag
> > -		# and allow recursive call of fsadm
> > -		_FSADM_YES=$YES
> > -		export _FSADM_YES
> > -		unset FSADM_RUNNING
> > -		test -n "$LVM_BINARY"&&  PATH=$_SAVEPATH
> > -		dry exec "$LVM" lvresize $VERB $FORCE -r -L${NEWSIZE}b
> > "$VOLUME_ORIG"
> > -	fi
> > -
> 
> Again - please do not remove things you do not understand.

Oh, but I do understand. Please read the new code, before trying to
insult me. But maybe it would be better to separate resize command
changes into separate patch.

> 
> And also thing like this needs to be in a separate patch - not bundled in
> hundreds patch lines.
> 
> 
> 
> 
> >   	# error exit status for break
> >   	exit ${1:-1}
> >   }
> > 
> > +#####################################
> > +# Convet the size into human readable
> > +# form. size in Bytes expected
> > +#####################################
> > +humanize_size() {
> > +	size="$1"
> > +	count=0
> > +	while [ ${size%%.*} -ge 1024 ]&&  [ $count -lt 7 ]; do
> > +		size=$(float_math $size/1024)
> > +		count=$(($count+1))
> > +	done
> > +	case $count in
> > +		0) unit="B" ;;
> > +		1) unit="KiB" ;;
> > +		2) unit="MiB" ;;
> > +		3) unit="GiB" ;;
> > +		4) unit="TiB" ;;
> > +		5) unit="PiB" ;;
> > +		6) unit="EiB" ;;
> > +		7) unit="ZiB" ;;
> > +		8) unit="YiB" ;;
> > +		*) unit="???" ;;
> > +	esac
> > +	echo "$size $unit"
> > +}
> > +
> 
> We need to make sure - using are handled in the very same way by both commands
> (lvm & fsadm -  might probably require to parse lvm.conf  for
> 'si_unit_consistency' option - so the user will not be confused.
> (As fsadm is part of lvm2 package and this nice extensions might bring some
> unwanted confusion to users with differently configure lvm2).

Regardless on what I think of this "option" I'll try to look at this.

> 
> 
> 
> > +#############################
> > +# Get size of entN filesystem
> > +# by reading tune2fs output
> > +#############################
> > +get_ext_size() {
> > +	local IFS=$NL
> > +	for i in $(LANG=C $TUNE_EXT -l "$VOLUME"); do
> > +		case "$i" in
> > +			"Block size"*) bsize=${i##*  } ;;
> > +			"Block count"*) bcount=${i##*  } ;;
> > +			"Reserved block count"*) rbcount=${i##*  } ;;
> > +			"Free blocks"*) fbcount=${i##*  } ;;
> > +		esac
> > +	done
> > +
> > +	total=$(($bcount*$bsize))
> > +	TOTAL=$(humanize_size $total)
> > +	used=$((($bcount-$fbcount)*$bsize))
> > +	USED=$(humanize_size $used)
> > +	free=$((($fbcount-$rbcount)*$bsize))
> > +	FREE=$(humanize_size $free)
> > +}
> > +
> > +############################
> > +# Get size of xfs file system
> > +# by reading the df output or
> > +# examine file system with
> > +# xfs_db tool
> > +#############################
> > +get_xfs_size() {
> > +	local IFS=$NL
> > +	if [ -z "$MOUNTED" ]; then
> > +
> > +		for i in $(LANG=C xfs_db -c 'sb' -c 'print blocksize fdblocks
> > dblocks logblocks agcount' $VOLUME); do
> 
> "$XFS_DB"
> 
> "$VOLUME"
> 
> 
> > +	line=$("$DF" -k "$VOLUME" | "$GREP" -e "$MOUNTED$" | sed -e 's/  */
> > /g')
> 
> "$SED"
> 
> 
> > +	line=${line#* }
> > +	total=$(echo $line | cut -d' ' -f1)
> 
> "$CUT"
> 
> Though maybe there is proably native shell way to extract things like this
> without extra execution command.
> 
> 
> > +detect_fs_size() {
> > +	if [ -z "$FSTYPE" ]; then
> > +		return
> > +	fi
> 
> Why ?
> (Imho it's handled by case below via return 1
> 
> 
> > +	case "$FSTYPE" in
> > +		ext[234]) get_ext_size ;;
> > +		xfs) get_xfs_size ;;
> > +		*) return 1 ;;
> > +	esac
> > +	return 0
> > +}
> > +
> >   # convert parameter from Exa/Peta/Tera/Giga/Mega/Kilo/Bytes and blocks
> >   # (2^(60/50/40/30/20/10/0))
> >   decode_size() {
> >   	case "$1" in
> > -	 *[eE]) NEWSIZE=$(( ${1%[eE]} * 1152921504606846976 )) ;;
> > -	 *[pP]) NEWSIZE=$(( ${1%[pP]} * 1125899906842624 )) ;;
> > -	 *[tT]) NEWSIZE=$(( ${1%[tT]} * 1099511627776 )) ;;
> > -	 *[gG]) NEWSIZE=$(( ${1%[gG]} * 1073741824 )) ;;
> > -	 *[mM]) NEWSIZE=$(( ${1%[mM]} * 1048576 )) ;;
> > -	 *[kK]) NEWSIZE=$(( ${1%[kK]} * 1024 )) ;;
> > +	 *[eE]) NEWSIZE=$(float_math "${1%[eE]} * 1152921504606846976") ;;
> > +	 *[pP]) NEWSIZE=$(float_math "${1%[pP]} * 1125899906842624") ;;
> > +	 *[tT]) NEWSIZE=$(float_math "${1%[tT]} * 1099511627776") ;;
> > +	 *[gG]) NEWSIZE=$(float_math "${1%[gG]} * 1073741824") ;;
> > +	 *[mM]) NEWSIZE=$(float_math "${1%[mM]} * 1048576") ;;
> > +	 *[kK]) NEWSIZE=$(float_math "${1%[kK]} * 1024") ;;
> >   	 *[bB]) NEWSIZE=${1%[bB]} ;;
> >   	     *) NEWSIZE=$(( $1 * $2 )) ;;
> 
> Yep - separate patch for math operation.
> 
> 
> >   	esac
> > -	#NEWBLOCKCOUNT=$(round_block_size $NEWSIZE $2)
> > -	NEWBLOCKCOUNT=$(( $NEWSIZE / $2 ))
> > 
> > -	if [ $DO_LVRESIZE -eq 1 ]; then
> > -		# start lvresize, but first cleanup mounted dirs
> > -		DO_LVRESIZE=2
> > -		cleanup 0
> > -	fi
> 
> Recursion stays for now.
> 
> 
> > +	NEWSIZE=${NEWSIZE%%.*}
> > +	NEWBLOCKCOUNT=$(float_math "$NEWSIZE / $2")
> > +	NEWBLOCKCOUNT=${NEWBLOCKCOUNT%%.*}
> >   }
> > 
> >   # detect filesystem on the given device
> >   # dereference device name if it is symbolic link
> >   detect_fs() {
> > -	VOLUME_ORIG=$1
> > +	VOLUME_ORIG="$1"
> >   	VOLUME=${1/#"${DM_DEV_DIR}/"/}
> > -	VOLUME=$("$READLINK" $READLINK_E "$DM_DEV_DIR/$VOLUME") || error
> > "Cannot get readlink \"$1\""
> > +	VOLUME=$("$READLINK" "$READLINK_E" "$DM_DEV_DIR/$VOLUME") || error
> > "Cannot get readlink \"$1\""
> 
> Do not modify same thing in multiple patches
> (The advantage of shuffling these things in front of the whole set.)
> 
> 
> 
> 
> > -	MOUNTED=$("$GREP" ^"$VOLUME" "$PROCMOUNTS")
> > +	MOUNTED=$("$GREP" -e "^$VOLUME " "$PROCMOUNTS")
> 
> Candidate for separate patch in front.
> Assuming we currently do not handle well VGs starting with letter '-'.
> So it's hidden bugfix (grep -e)
> 
> 
> > @@ -295,17 +417,19 @@ resize_ext() {
> >   	if [ "$NEWBLOCKCOUNT" -lt "$BLOCKCOUNT" -o "$EXTOFF" -eq 1 ]; then
> >   		detect_mounted&&  verbose "$RESIZE_EXT needs unmounted
> > filesystem"&&  try_umount
> >   		REMOUNT=$MOUNTED
> > -		if test -n "$MOUNTED" ; then
> > -			# Forced fsck -f for umounted extX filesystem.
> > -			case "$-" in
> > -			  *i*) dry "$FSCK" $YES -f "$VOLUME" ;;
> > -			  *) dry "$FSCK" -f -p "$VOLUME" ;;
> > -			esac
> > -		fi
> > +	fi
> > +
> > +	# We should really do the fsck before every resize.
> 
> Well user is always the master - if he is brave to go without it, let him do
> what he wants (i.e. destroy his fs).

You're missing the point, resize2fs requires that the file system is
checked with e2fsck -f prior the resize, if on of the conditions happen

1. file system was not cleanly umounted
2. file system contain errors, or is invalid
3. it was not checked since the last time it was mounted

It is just to make sure that the file system is consistent and we will
not make everything worse by messing with metadata further.

Thanks!
-Lukas

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Zdenek Kabelac]

Dne 4.10.2011 14:13, Lukas Czerner napsal(a):
> On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
>
>> Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
>>> This commit adds new functionality in the form of new commands. Namely
>>> it is create, list, add and remove. This commit also changes the way how
>>> are commands recognised an executed. The new approach is more suitable
>>> for bigger number of commands.
>>>
>>> Resize command is also significantly reworked. Unlike in the old
>>> approach fsadm will always attempt to resize the logical volume as well,
>>> since this is what it is expected. Leave the --lvresize option for
>>> backwards compatibility, but remove it from the help. If no file system
>>> resides on the logical volume, only the volume will be resized.
>>>
>>> * Create command provides the functionality of creating a new logical
>>>     volumes including defined file system.
>>> * List command provides the functionality of listing useful information
>>>     about the logical volumes, file systems and devices.
>>> * Add command allows to add devices into volume groups (pool).
>>> * Remove command allows to remove the volumes or volume groups from the
>>>     system.
>>>
>>> Signed-off-by: Lukas Czerner<lczerner@redhat.com>
>>
>>> @@ -96,7 +109,6 @@ tool_usage() {
>>>    	echo "    -e | --ext-offline  unmount filesystem before ext2/ext3/ext4
>>> resize"
>>>    	echo "    -f | --force        Bypass sanity checks"
>>>    	echo "    -n | --dry-run      Print commands without running them"
>>> -	echo "    -l | --lvresize     Resize given device (if it is LVM
>>> device)"
>>>    	echo "    -y | --yes          Answer \"yes\" at any prompts"
>>>    	echo
>>>    	echo "  new_size - Absolute number of filesystem blocks to be in the
>>> filesystem,"
>>
>>
>> Again - no way to remove already supported option.
>
> This is only removed from the tool_usage(), as you can see. I am not
> removing it from the code, but only from the documentation since it is
> not needed in the new code.
>

Ok this made it clear that such functional change than needs a separate patch, 
which will demonstrate how it's going to be used and how it is matching 
current behavior.



>>> -	if [ "$DO_LVRESIZE" -eq 2 ]; then
>>> -		# start LVRESIZE with the filesystem modification flag
>>> -		# and allow recursive call of fsadm
>>> -		_FSADM_YES=$YES
>>> -		export _FSADM_YES
>>> -		unset FSADM_RUNNING
>>> -		test -n "$LVM_BINARY"&&   PATH=$_SAVEPATH
>>> -		dry exec "$LVM" lvresize $VERB $FORCE -r -L${NEWSIZE}b
>>> "$VOLUME_ORIG"
>>> -	fi
>>> -
>>
>> Again - please do not remove things you do not understand.
>
> Oh, but I do understand. Please read the new code, before trying to
> insult me. But maybe it would be better to separate resize command
> changes into separate patch.
>
>>
>> And also thing like this needs to be in a separate patch - not bundled in
>> hundreds patch lines.


It's nothing about insulting - this is very tricky change - so unless this is 
extracted to separate patch to be properly reviewed for all combination and 
whether all cases keep all VG locks properly.
(Thus patch needs to explain how it will resize  filesystem and LV when user 
calls fsadm resize and this operation could not be broken with another 
parallel lvm operation in both directions - upsize/downsize while current 
syntax is still usable)


>> Well user is always the master - if he is brave to go without it, let him do
>> what he wants (i.e. destroy his fs).
>
> You're missing the point, resize2fs requires that the file system is
> checked with e2fsck -f prior the resize, if on of the conditions happen
>
> 1. file system was not cleanly umounted
> 2. file system contain errors, or is invalid
> 3. it was not checked since the last time it was mounted
>
> It is just to make sure that the file system is consistent and we will
> not make everything worse by messing with metadata further.
>

If the user wants to risk  resize2fs without fsck - it's his decision (and 
current -f with fsadm allows it) - and since we are not like gnome3 - we allow 
users to kill their systems in number of ways already, so no reason to 
eliminate this one.

i.e. mkfs, mount, umount, resize LV, resize2fs -f   works without fsck and 
makes errorless result - so we support.

Zdenek

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Lukas Czerner]

On Tue, 4 Oct 2011, Zdenek Kabelac wrote:

> Dne 4.10.2011 14:13, Lukas Czerner napsal(a):
> > On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
> > 
> > > Dne 27.9.2011 15:42, Lukas Czerner napsal(a):
> > > > This commit adds new functionality in the form of new commands. Namely
> > > > it is create, list, add and remove. This commit also changes the way how
> > > > are commands recognised an executed. The new approach is more suitable
> > > > for bigger number of commands.
> > > > 
> > > > Resize command is also significantly reworked. Unlike in the old
> > > > approach fsadm will always attempt to resize the logical volume as well,
> > > > since this is what it is expected. Leave the --lvresize option for
> > > > backwards compatibility, but remove it from the help. If no file system
> > > > resides on the logical volume, only the volume will be resized.
> > > > 
> > > > * Create command provides the functionality of creating a new logical
> > > >     volumes including defined file system.
> > > > * List command provides the functionality of listing useful information
> > > >     about the logical volumes, file systems and devices.
> > > > * Add command allows to add devices into volume groups (pool).
> > > > * Remove command allows to remove the volumes or volume groups from the
> > > >     system.
> > > > 
> > > > Signed-off-by: Lukas Czerner<lczerner@redhat.com>
> > > 
> > > > @@ -96,7 +109,6 @@ tool_usage() {
> > > >    	echo "    -e | --ext-offline  unmount filesystem before
> > > > ext2/ext3/ext4
> > > > resize"
> > > >    	echo "    -f | --force        Bypass sanity checks"
> > > >    	echo "    -n | --dry-run      Print commands without running
> > > > them"
> > > > -	echo "    -l | --lvresize     Resize given device (if it is LVM
> > > > device)"
> > > >    	echo "    -y | --yes          Answer \"yes\" at any prompts"
> > > >    	echo
> > > >    	echo "  new_size - Absolute number of filesystem blocks to be
> > > > in the
> > > > filesystem,"
> > > 
> > > 
> > > Again - no way to remove already supported option.
> > 
> > This is only removed from the tool_usage(), as you can see. I am not
> > removing it from the code, but only from the documentation since it is
> > not needed in the new code.
> > 
> 
> Ok this made it clear that such functional change than needs a separate patch,
> which will demonstrate how it's going to be used and how it is matching
> current behavior.
> 

Agreed, as I said I'll separate resize update into separate patch.

> 
> 
> > > > -	if [ "$DO_LVRESIZE" -eq 2 ]; then
> > > > -		# start LVRESIZE with the filesystem modification flag
> > > > -		# and allow recursive call of fsadm
> > > > -		_FSADM_YES=$YES
> > > > -		export _FSADM_YES
> > > > -		unset FSADM_RUNNING
> > > > -		test -n "$LVM_BINARY"&&   PATH=$_SAVEPATH
> > > > -		dry exec "$LVM" lvresize $VERB $FORCE -r -L${NEWSIZE}b
> > > > "$VOLUME_ORIG"
> > > > -	fi
> > > > -
> > > 
> > > Again - please do not remove things you do not understand.
> > 
> > Oh, but I do understand. Please read the new code, before trying to
> > insult me. But maybe it would be better to separate resize command
> > changes into separate patch.
> > 
> > > 
> > > And also thing like this needs to be in a separate patch - not bundled in
> > > hundreds patch lines.
> 
> 
> It's nothing about insulting - this is very tricky change - so unless this is
> extracted to separate patch to be properly reviewed for all combination and
> whether all cases keep all VG locks properly.
> (Thus patch needs to explain how it will resize  filesystem and LV when user
> calls fsadm resize and this operation could not be broken with another
> parallel lvm operation in both directions - upsize/downsize while current
> syntax is still usable)
> 
> 
> > > Well user is always the master - if he is brave to go without it, let him
> > > do
> > > what he wants (i.e. destroy his fs).
> > 
> > You're missing the point, resize2fs requires that the file system is
> > checked with e2fsck -f prior the resize, if on of the conditions happen
> > 
> > 1. file system was not cleanly umounted
> > 2. file system contain errors, or is invalid
> > 3. it was not checked since the last time it was mounted
> > 
> > It is just to make sure that the file system is consistent and we will
> > not make everything worse by messing with metadata further.
> > 
> 
> If the user wants to risk  resize2fs without fsck - it's his decision (and
> current -f with fsadm allows it) - and since we are not like gnome3 - we allow
> users to kill their systems in number of ways already, so no reason to
> eliminate this one.

Well, this is just wrong in so many ways. Running fsck.ext4 -f to force
the full check is not the same as skipping the fsck before resize in the
tool which is suppose to do-it-all. We have to pick the safe way here.

> 
> i.e. mkfs, mount, umount, resize LV, resize2fs -f   works without fsck and
> makes errorless result - so we support.

Also it makes me think that you do not really see what is in the code.
There is nothing about about 'force', fsck is done anyway (in the old
code - your code) in the case that 

[ "$NEWBLOCKCOUNT" -lt "$BLOCKCOUNT" -o "$EXTOFF" -eq 1 ]

and 

if test -n "$MOUNTED"

so please, read before you write.

Also your arguments are out of the line. There are different levels of
granularity of force, and if user force things he does not expect things
will be done in wrong way. That said, when I use force I do not want to
be asked about stupid questions, but I certaily do not want fsck before
resize2fs to be skipped.

You just have to understand that we are using multiple tools in fsadm
and just passing force to all of them would be incredibly stupid, even
if you properly document it.

User is just not able to guess what different sort of tools we are using
in the path he is about to invoke, and guess where we are going to use
his 'force' and what consequences will that have. We just HAVE to pick
the most logical and most safe way of doing things. But your way of
doing thing is just to get as many option as you can aggregate and throw
it to the user while screaming RTFM, that is easy, but it is also wrong.

-Lukas


> 
> Zdenek
> 
> 
> 

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Zdenek Kabelac]

Dne 5.10.2011 10:02, Lukas Czerner napsal(a):
> On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
>
>> Dne 4.10.2011 14:13, Lukas Czerner napsal(a):
>>> On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
>>>


>>>> Well user is always the master - if he is brave to go without it, let him
>>>> do
>>>> what he wants (i.e. destroy his fs).
>>>
>>> You're missing the point, resize2fs requires that the file system is
>>> checked with e2fsck -f prior the resize, if on of the conditions happen
>>>
>>> 1. file system was not cleanly umounted
>>> 2. file system contain errors, or is invalid
>>> 3. it was not checked since the last time it was mounted
>>>
>>> It is just to make sure that the file system is consistent and we will
>>> not make everything worse by messing with metadata further.
>>>
>>
>> If the user wants to risk  resize2fs without fsck - it's his decision (and
>> current -f with fsadm allows it) - and since we are not like gnome3 - we allow
>> users to kill their systems in number of ways already, so no reason to
>> eliminate this one.
>
> Well, this is just wrong in so many ways. Running fsck.ext4 -f to force
> the full check is not the same as skipping the fsck before resize in the
> tool which is suppose to do-it-all. We have to pick the safe way here.
>

Tool is not meant to be used only in a 'safe' way - tool is supposed to fully 
usable even various force scenarios - i.e. when fsck support options '-y' we 
should be able to use - even if some extX developers think it's very 
dangerous option and should never ever be used - it's there and it has its 
use. (I've used it already many times, and nothing unexpectedly bad happened).

What we could do here better is to draw some state diagram and eventually add 
few more options - but we will not strip down the usefulness of fsadm only for 
clean filesystem and leave the user  to run  fsck -y on its - as we can do 
that for him.


>>
>> i.e. mkfs, mount, umount, resize LV, resize2fs -f   works without fsck and
>> makes errorless result - so we support.
>
> Also it makes me think that you do not really see what is in the code.
> There is nothing about about 'force', fsck is done anyway (in the old
> code - your code) in the case that
>
> [ "$NEWBLOCKCOUNT" -lt "$BLOCKCOUNT" -o "$EXTOFF" -eq 1 ]
>
> and
>
> if test -n "$MOUNTED"
>
> so please, read before you write.

man lvresize  --nofsck


> Also your arguments are out of the line. There are different levels of
> granularity of force, and if user force things he does not expect things
> will be done in wrong way. That said, when I use force I do not want to
> be asked about stupid questions, but I certaily do not want fsck before
> resize2fs to be skipped.

We already allow that - since there are situation you may need to use it.

> You just have to understand that we are using multiple tools in fsadm
> and just passing force to all of them would be incredibly stupid, even
> if you properly document it.

Certainly personal opinion.

As already said - provide state diagram and suggest more options
(in lvm we use  -ff for some situations -  so maybe more levels of force)

But we need to support what we already can do with fsadm tool.

If the user puts --force anywhere on the command line - he surely needs to 
expect, that some things may cause damage.

> his 'force' and what consequences will that have. We just HAVE to pick
> the most logical and most safe way of doing things. But your way of

There is no 'safe' way with --force - that's the purpose of force.
However there could be multiple force levels.

Zdenek

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Lukas Czerner]

On Wed, 5 Oct 2011, Zdenek Kabelac wrote:

> Dne 5.10.2011 10:02, Lukas Czerner napsal(a):
> > On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
> > 
> > > Dne 4.10.2011 14:13, Lukas Czerner napsal(a):
> > > > On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
> > > > 
> 
> 
> > > > > Well user is always the master - if he is brave to go without it, let
> > > > > him
> > > > > do
> > > > > what he wants (i.e. destroy his fs).
> > > > 
> > > > You're missing the point, resize2fs requires that the file system is
> > > > checked with e2fsck -f prior the resize, if on of the conditions happen
> > > > 
> > > > 1. file system was not cleanly umounted
> > > > 2. file system contain errors, or is invalid
> > > > 3. it was not checked since the last time it was mounted
> > > > 
> > > > It is just to make sure that the file system is consistent and we will
> > > > not make everything worse by messing with metadata further.
> > > > 
> > > 
> > > If the user wants to risk  resize2fs without fsck - it's his decision (and
> > > current -f with fsadm allows it) - and since we are not like gnome3 - we
> > > allow
> > > users to kill their systems in number of ways already, so no reason to
> > > eliminate this one.
> > 
> > Well, this is just wrong in so many ways. Running fsck.ext4 -f to force
> > the full check is not the same as skipping the fsck before resize in the
> > tool which is suppose to do-it-all. We have to pick the safe way here.
> > 
> 
> Tool is not meant to be used only in a 'safe' way - tool is supposed to fully
> usable even various force scenarios - i.e. when fsck support options '-y' we
> should be able to use - even if some extX developers think it's very dangerous
> option and should never ever be used - it's there and it has its use. (I've
> used it already many times, and nothing unexpectedly bad happened).

No you're wrong, we are aggregating option from multiple tools, and
using them only in the way it is useful for that tool. We certainly do
not want to map every possible options, and this is even more obvious if
this option is not safe, or needed for that matter.

I have never said that -y should never be used, I just said that user
needs to know what is he doing, which is not the case when you're using
-y to allow automatic umount and doing fsck -y .

> 
> What we could do here better is to draw some state diagram and eventually add
> few more options - but we will not strip down the usefulness of fsadm only for
> clean filesystem and leave the user  to run  fsck -y on its - as we can do
> that for him.

Yeah, doing fsck -y on your laptop, where you probably does not care
about your data wary much is fine if you're ok with it. But if you're in
enterprise environment, it definitely is not a smart thing to do,
because you do not want to lose your data and ideally you want to find
the root cause of the problem to avoid the corruption on future, hence
fsck -y is just dumbest thing to do. Not talking about the fact that
'-y' does not mean "Please fix my file system', but rather 'Answer yes
to everything', which does not make sense if you do not know what
happened. On the other hand '-p' means 'Please, fix my file system in
the bast way you can'.

> 
> 
> > > 
> > > i.e. mkfs, mount, umount, resize LV, resize2fs -f   works without fsck and
> > > makes errorless result - so we support.
> > 
> > Also it makes me think that you do not really see what is in the code.
> > There is nothing about about 'force', fsck is done anyway (in the old
> > code - your code) in the case that
> > 
> > [ "$NEWBLOCKCOUNT" -lt "$BLOCKCOUNT" -o "$EXTOFF" -eq 1 ]
> > 
> > and
> > 
> > if test -n "$MOUNTED"
> > 
> > so please, read before you write.
> 
> man lvresize  --nofsck

That is so funny, it just proves how little you know about your own
code, this is the second time I am saying you that it just does not work
that way, and fsck will be run if the above conditions are met,
regardless on whether you do run lvresize --nofsck of fsadm -f.

> 
> 
> > Also your arguments are out of the line. There are different levels of
> > granularity of force, and if user force things he does not expect things
> > will be done in wrong way. That said, when I use force I do not want to
> > be asked about stupid questions, but I certaily do not want fsck before
> > resize2fs to be skipped.
> 
> We already allow that - since there are situation you may need to use it.

No you do not. You allow umount without interaction, but that will imply
insane fsck -y, hence using that in script is really bad thing.

> 
> > You just have to understand that we are using multiple tools in fsadm
> > and just passing force to all of them would be incredibly stupid, even
> > if you properly document it.
> 
> Certainly personal opinion.
> 
> As already said - provide state diagram and suggest more options
> (in lvm we use  -ff for some situations -  so maybe more levels of force)
> 
> But we need to support what we already can do with fsadm tool.
> 
> If the user puts --force anywhere on the command line - he surely needs to
> expect, that some things may cause damage.

nope, that is not true for all the tools generally. fsck.extN -f fill not do
anything bad.

> 
> > his 'force' and what consequences will that have. We just HAVE to pick
> > the most logical and most safe way of doing things. But your way of
> 
> There is no 'safe' way with --force - that's the purpose of force.
> However there could be multiple force levels.

Please do not spread your absolutely undocumented '-f' and '-ff' lvm
options which no user knows what is actually does, any further.

And I do not have to remind you that force does not usually imply "skip
fsck".

> 
> Zdenek
> 

-Lukas

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Alasdair G Kergon]

On Wed, Oct 05, 2011 at 11:46:45AM +0200, Lukas Czerner wrote:
> On Wed, 5 Oct 2011, Zdenek Kabelac wrote:
> > Dne 5.10.2011 10:02, Lukas Czerner napsal(a):
> > > On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
> > > > Dne 4.10.2011 14:13, Lukas Czerner napsal(a):
> > > > > On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
 
You both lost me several messages ago!

For *changes* to functionality, what I'm looking for is:

  Description of how it works today in *all* relevant code paths/option
combinations/invocation methods;

  Description/explanation of how it will work *after* the change has been made
in all the same cases as now plus any new ones

Explanation of/justification for the difference.

Currently in lvm:

  --yes is meant to mean 'answer yes to all questions'
These are generally informative questions telling the user what the tool will do
and seeking confirmation.  On its own, they should not be dangerous or risk
corruption.  Example: removing an LV that is active but not currently being used.
The idea of such questions is to point out to the user the actual consequences
of the command to help to make sure they understand what will happen.

  --force is meant to mean 'permit operations that might not always be wise but
sometimes do need to be done - assume the user knows what they are doing'.
Normally a warning of what will happen is issued and confirmation is required,
but --yes can be used to avoid that confirmation.

For dangerous things that wouldn't normally make sense but occasionally can be
justified, like destroying PVs while they are in (inactive) VGs, --force can be
required to be repeated twice.


Now, let's try to understand:
  How these options are currently used in fsadm-related tools;
  What's good/bad here and perhaps ought to change.

Alasdair

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Lukas Czerner]

On Wed, 5 Oct 2011, Alasdair G Kergon wrote:

> On Wed, Oct 05, 2011 at 11:46:45AM +0200, Lukas Czerner wrote:
> > On Wed, 5 Oct 2011, Zdenek Kabelac wrote:
> > > Dne 5.10.2011 10:02, Lukas Czerner napsal(a):
> > > > On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
> > > > > Dne 4.10.2011 14:13, Lukas Czerner napsal(a):
> > > > > > On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
>  

Hi Alasdair,

> You both lost me several messages ago!
> 
> For *changes* to functionality, what I'm looking for is:
> 
>   Description of how it works today in *all* relevant code paths/option
> combinations/invocation methods;
> 
>   Description/explanation of how it will work *after* the change has been made
> in all the same cases as now plus any new ones
> 
> Explanation of/justification for the difference.
> 
> Currently in lvm:
> 
>   --yes is meant to mean 'answer yes to all questions'
> These are generally informative questions telling the user what the tool will do
> and seeking confirmation.  On its own, they should not be dangerous or risk
> corruption.  Example: removing an LV that is active but not currently being used.
> The idea of such questions is to point out to the user the actual consequences
> of the command to help to make sure they understand what will happen.
> 
>   --force is meant to mean 'permit operations that might not always be wise but
> sometimes do need to be done - assume the user knows what they are doing'.
> Normally a warning of what will happen is issued and confirmation is required,
> but --yes can be used to avoid that confirmation.

Do you mean --force in the last sentence ?

> 
> For dangerous things that wouldn't normally make sense but occasionally can be
> justified, like destroying PVs while they are in (inactive) VGs, --force can be
> required to be repeated twice.
> 
> 
> Now, let's try to understand:
>   How these options are currently used in fsadm-related tools;
>   What's good/bad here and perhaps ought to change.
> 
> Alasdair
> 

The first misunderstanding was the use of -f in lvm toos. Because I can
not see -y|--yes in the documentation I simply used force for not asking
questions about active volumes to be removed. This can be fixed, if '-y'
really works.

The other is, that using -y option to answer yes to umount mounted file
system to be checked is ok, but then running fsck with that '-y' option
does not make sense and it is also dangerous. But Zdenek is convinced
that it has to stay there (I say it is a bug).

And lastly, the problem with using --force to skip the fsck before
attempt to resize the file system. Using force in my opinion does not
imply skipping fsck, and I believe that is how it is implemented in
lvresize as well, where you have to use "--nofsck" (which is broken btw).

I believe that fscking file system before the resize is the right thing
to do and I do not see any reason to allow it to be overridden in fsadm,
although we probably should check for 'last mount' and 'last check' fs
attributes to see if it was checked after the last mount and fsck it
otherwise.

Generally the problem is that we could not agree on almost anything and
due to 'it stays as it is, deal with it' altitude it seems to be that
even the discussion is not allowed.

Honestly, when I started to poke the fsadm I quickly realized that if I
am going to improve the functionality in the bigger scale, it will be
more and more painful to do this in Bash. So maybe it will be easier for
both sides to just forget about fsadm and make a separate project for
example in Python. Because after all I would like this tool to be able
to manage btrfs as well someday and since that does not have anything to
do with lvm, I fear that it will be a major problem to get this into
upstream fsadm. So what people think ?

Thanks!
-Lukas

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Lukas Czerner]

On Wed, 5 Oct 2011, Lukas Czerner wrote:

> On Wed, 5 Oct 2011, Alasdair G Kergon wrote:
> 
> > On Wed, Oct 05, 2011 at 11:46:45AM +0200, Lukas Czerner wrote:
> > > On Wed, 5 Oct 2011, Zdenek Kabelac wrote:
> > > > Dne 5.10.2011 10:02, Lukas Czerner napsal(a):
> > > > > On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
> > > > > > Dne 4.10.2011 14:13, Lukas Czerner napsal(a):
> > > > > > > On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
> >  
> 
> Hi Alasdair,
> 
> > You both lost me several messages ago!
> > 
> > For *changes* to functionality, what I'm looking for is:
> > 
> >   Description of how it works today in *all* relevant code paths/option
> > combinations/invocation methods;
> > 
> >   Description/explanation of how it will work *after* the change has been made
> > in all the same cases as now plus any new ones
> > 
> > Explanation of/justification for the difference.
> > 
> > Currently in lvm:
> > 
> >   --yes is meant to mean 'answer yes to all questions'
> > These are generally informative questions telling the user what the tool will do
> > and seeking confirmation.  On its own, they should not be dangerous or risk
> > corruption.  Example: removing an LV that is active but not currently being used.
> > The idea of such questions is to point out to the user the actual consequences
> > of the command to help to make sure they understand what will happen.

Btw, there is no such option in lvm. It is not documented and it does
not work, at least for vgremove and lvremove. However it does work in
pvremove, also it is documented there.

> > 
> >   --force is meant to mean 'permit operations that might not always be wise but
> > sometimes do need to be done - assume the user knows what they are doing'.
> > Normally a warning of what will happen is issued and confirmation is required,
> > but --yes can be used to avoid that confirmation.
> 
> Do you mean --force in the last sentence ?
> 
> > 
> > For dangerous things that wouldn't normally make sense but occasionally can be
> > justified, like destroying PVs while they are in (inactive) VGs, --force can be
> > required to be repeated twice.
> > 
> > 
> > Now, let's try to understand:
> >   How these options are currently used in fsadm-related tools;
> >   What's good/bad here and perhaps ought to change.
> > 
> > Alasdair
> > 
> 
> The first misunderstanding was the use of -f in lvm toos. Because I can
> not see -y|--yes in the documentation I simply used force for not asking
> questions about active volumes to be removed. This can be fixed, if '-y'
> really works.
> 
> The other is, that using -y option to answer yes to umount mounted file
> system to be checked is ok, but then running fsck with that '-y' option
> does not make sense and it is also dangerous. But Zdenek is convinced
> that it has to stay there (I say it is a bug).
> 
> And lastly, the problem with using --force to skip the fsck before
> attempt to resize the file system. Using force in my opinion does not
> imply skipping fsck, and I believe that is how it is implemented in
> lvresize as well, where you have to use "--nofsck" (which is broken btw).
> 
> I believe that fscking file system before the resize is the right thing
> to do and I do not see any reason to allow it to be overridden in fsadm,
> although we probably should check for 'last mount' and 'last check' fs
> attributes to see if it was checked after the last mount and fsck it
> otherwise.
> 
> Generally the problem is that we could not agree on almost anything and
> due to 'it stays as it is, deal with it' altitude it seems to be that
> even the discussion is not allowed.
> 
> Honestly, when I started to poke the fsadm I quickly realized that if I
> am going to improve the functionality in the bigger scale, it will be
> more and more painful to do this in Bash. So maybe it will be easier for
> both sides to just forget about fsadm and make a separate project for
> example in Python. Because after all I would like this tool to be able
> to manage btrfs as well someday and since that does not have anything to
> do with lvm, I fear that it will be a major problem to get this into
> upstream fsadm. So what people think ?
> 
> Thanks!
> -Lukas
> 

-- 

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Ric Wheeler]

On 10/05/2011 07:21 AM, Lukas Czerner wrote:
> On Wed, 5 Oct 2011, Alasdair G Kergon wrote:
>
>> On Wed, Oct 05, 2011 at 11:46:45AM +0200, Lukas Czerner wrote:
>>> On Wed, 5 Oct 2011, Zdenek Kabelac wrote:
>>>> Dne 5.10.2011 10:02, Lukas Czerner napsal(a):
>>>>> On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
>>>>>> Dne 4.10.2011 14:13, Lukas Czerner napsal(a):
>>>>>>> On Tue, 4 Oct 2011, Zdenek Kabelac wrote:
>>
> Hi Alasdair,
>
>> You both lost me several messages ago!
>>
>> For *changes* to functionality, what I'm looking for is:
>>
>>    Description of how it works today in *all* relevant code paths/option
>> combinations/invocation methods;
>>
>>    Description/explanation of how it will work *after* the change has been made
>> in all the same cases as now plus any new ones
>>
>> Explanation of/justification for the difference.
>>
>> Currently in lvm:
>>
>>    --yes is meant to mean 'answer yes to all questions'
>> These are generally informative questions telling the user what the tool will do
>> and seeking confirmation.  On its own, they should not be dangerous or risk
>> corruption.  Example: removing an LV that is active but not currently being used.
>> The idea of such questions is to point out to the user the actual consequences
>> of the command to help to make sure they understand what will happen.
>>
>>    --force is meant to mean 'permit operations that might not always be wise but
>> sometimes do need to be done - assume the user knows what they are doing'.
>> Normally a warning of what will happen is issued and confirmation is required,
>> but --yes can be used to avoid that confirmation.
> Do you mean --force in the last sentence ?
>
>> For dangerous things that wouldn't normally make sense but occasionally can be
>> justified, like destroying PVs while they are in (inactive) VGs, --force can be
>> required to be repeated twice.
>>
>>
>> Now, let's try to understand:
>>    How these options are currently used in fsadm-related tools;
>>    What's good/bad here and perhaps ought to change.
>>
>> Alasdair
>>
> The first misunderstanding was the use of -f in lvm toos. Because I can
> not see -y|--yes in the documentation I simply used force for not asking
> questions about active volumes to be removed. This can be fixed, if '-y'
> really works.
>
> The other is, that using -y option to answer yes to umount mounted file
> system to be checked is ok, but then running fsck with that '-y' option
> does not make sense and it is also dangerous. But Zdenek is convinced
> that it has to stay there (I say it is a bug).

fsck should *never* be run on a mounted file system. If that is what you are 
saying happens, that is 100% a bug and will cause data loss.

>
> And lastly, the problem with using --force to skip the fsck before
> attempt to resize the file system. Using force in my opinion does not
> imply skipping fsck, and I believe that is how it is implemented in
> lvresize as well, where you have to use "--nofsck" (which is broken btw).
>
> I believe that fscking file system before the resize is the right thing
> to do and I do not see any reason to allow it to be overridden in fsadm,
> although we probably should check for 'last mount' and 'last check' fs
> attributes to see if it was checked after the last mount and fsck it
> otherwise.

We should definitely follow the rules of the file system for when it is safe to 
resize. Having a clean file system verified by fsck or the checks you mention 
would both be fine, over riding those checks is not.

I think in general our tools should "do the right thing" and fall back to having 
a human do manual fsck checks and so on. Running fsck in automatic "yes" mode in 
a script seems quite iffy to me.
>
> Generally the problem is that we could not agree on almost anything and
> due to 'it stays as it is, deal with it' altitude it seems to be that
> even the discussion is not allowed.
>
> Honestly, when I started to poke the fsadm I quickly realized that if I
> am going to improve the functionality in the bigger scale, it will be
> more and more painful to do this in Bash. So maybe it will be easier for
> both sides to just forget about fsadm and make a separate project for
> example in Python. Because after all I would like this tool to be able
> to manage btrfs as well someday and since that does not have anything to
> do with lvm, I fear that it will be a major problem to get this into
> upstream fsadm. So what people think ?
>
> Thanks!
> -Lukas

If we do need to expand beyond the boundaries of fsadm, that is fine with me. I 
think that the goal should be to present a pluggable & flexible user command 
that will work for all local file systems including btrfs so we should not 
require lvm to be there.

thanks!

ric

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Alasdair G Kergon]

On Wed, Oct 05, 2011 at 01:21:17PM +0200, Lukas Czerner wrote:
> On Wed, 5 Oct 2011, Alasdair G Kergon wrote:
> >   --force is meant to mean 'permit operations that might not always be wise but
> > sometimes do need to be done - assume the user knows what they are doing'.
> > Normally a warning of what will happen is issued and confirmation is required,
> > but --yes can be used to avoid that confirmation.
 
> Do you mean --force in the last sentence ?
 
No, I meant --yes.

It works like this:

  If you use --yes, you will not be asked any questions.
  Where the code would otherwise have issued a question, it proceeds as if you
answered 'yes, do it'.

If you use --force, it can open up *extra* things the code allows to happen.
Without it, the code would just give an error 'I'm not going to let you do that'.


So, the number of --force options supplied controls *what* the code lets you do.

--yes just controls whether or not you get interactive prompts advising you
what will happen and giving you chances to bail out.

> The first misunderstanding was the use of -f in lvm toos. Because I can
> not see -y|--yes in the documentation I simply used force for not asking
> questions about active volumes to be removed. This can be fixed, if '-y'
> really works.
 
I can see things missing from the man pages...

> The other is, that using -y option to answer yes to umount mounted file
> system to be checked is ok, but then running fsck with that '-y' option
> does not make sense and it is also dangerous. But Zdenek is convinced
> that it has to stay there (I say it is a bug).

Well what we could do there is:
  --yes  only:  we do NOT supply -y to fsck.  (Ideally we'd like to choose
non-interactive options though i.e. if it hits something that would
normally require an interactive response, then instead of prompting,
fail the operation.  Perhaps -y should map to 'fsck -n' OR 'fsck -p'?
It should be non-interactive AND safe.)

  --force --yes: This could perhaps map to -y, if there's a case
for that.  Or perhaps '--force --force --yes'

> lvresize as well, where you have to use "--nofsck" (which is broken btw).
 
The reason for --nofsck is if you *know* the filesystem is ok because you
already ran a check (for example).  But if the fs is indeed ok, won't a basic
fsck be fast anyway, (just checking fs state in superblock rather than
performing a full check) such that it doesn't matter if it gets run again?

Alasdair

[PATCH v3 01/18] fsadm: Add new commands create, list, add and remove

[Lukas Czerner]

On Wed, 5 Oct 2011, Alasdair G Kergon wrote:

> On Wed, Oct 05, 2011 at 01:21:17PM +0200, Lukas Czerner wrote:
> > On Wed, 5 Oct 2011, Alasdair G Kergon wrote:
> > >   --force is meant to mean 'permit operations that might not always be wise but
> > > sometimes do need to be done - assume the user knows what they are doing'.
> > > Normally a warning of what will happen is issued and confirmation is required,
> > > but --yes can be used to avoid that confirmation.
>  
> > Do you mean --force in the last sentence ?
>  
> No, I meant --yes.
> 
> It works like this:
> 
>   If you use --yes, you will not be asked any questions.
>   Where the code would otherwise have issued a question, it proceeds as if you
> answered 'yes, do it'.
> 
> If you use --force, it can open up *extra* things the code allows to happen.
> Without it, the code would just give an error 'I'm not going to let you do that'.
> 
> 
> So, the number of --force options supplied controls *what* the code lets you do.
> 
> --yes just controls whether or not you get interactive prompts advising you
> what will happen and giving you chances to bail out.

Ok, now it is clear to me. However --yes option does not exist in some
lvm tolls (e.g. lvremove, vgremove)

> 
> > The first misunderstanding was the use of -f in lvm toos. Because I can
> > not see -y|--yes in the documentation I simply used force for not asking
> > questions about active volumes to be removed. This can be fixed, if '-y'
> > really works.
>  
> I can see things missing from the man pages...
> 
> > The other is, that using -y option to answer yes to umount mounted file
> > system to be checked is ok, but then running fsck with that '-y' option
> > does not make sense and it is also dangerous. But Zdenek is convinced
> > that it has to stay there (I say it is a bug).
> 
> Well what we could do there is:
>   --yes  only:  we do NOT supply -y to fsck.  (Ideally we'd like to choose
> non-interactive options though i.e. if it hits something that would
> normally require an interactive response, then instead of prompting,
> fail the operation.  Perhaps -y should map to 'fsck -n' OR 'fsck -p'?
> It should be non-interactive AND safe.)

Non interactive would be fsck -p which actually will try to fix the file
system, not answer yes to all questions, however I am not sure if we
want it to map it to --yes option.

> 
>   --force --yes: This could perhaps map to -y, if there's a case
> for that.  Or perhaps '--force --force --yes'

The problem here is, that --yes is also used to umount the file system
prior to the fsck. I would be in favor of just not supporting fsck -y in
fsadm, but having fsck -p for non interactive use.

> 
> > lvresize as well, where you have to use "--nofsck" (which is broken btw).
>  
> The reason for --nofsck is if you *know* the filesystem is ok because you
> already ran a check (for example).  But if the fs is indeed ok, won't a basic
> fsck be fast anyway, (just checking fs state in superblock rather than
> performing a full check) such that it doesn't matter if it gets run again?

We can easily get the information whether the file system has been
checked since the last umount and skip the check automatically, since
resize2fs will not require it. However basic check is not very useful,
since as you said it'll only check the state of the super block, which
is not enough and resize2fs will refuse to run in the case that the file
system has not received full check from the last mount.

So as you can see, --nofsck is actually not needed for the case you've
described and I would prefer not allowing user to override that
behaviour and always do fsck prior to the resize2fs, but I can not
insist on it - it just can not be done with --force, but rather with
--nofsck.

> 
> Alasdair

-Lukas