[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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"


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]