[arch-releng] [PATCH] Reworte finddisks and findblockdevices. Renamed findblockdevices to find_usable_devs

[Dieter Plaetinck]

From what I can tell in my mail client, it seems like this will be a nice cleanup of some very old code :)
thanks.


On Sun, 20 Feb 2011 12:30:10 -0500
pyther at pyther.net wrote:

> find_usable_devs will looks for both partitioned and non-partitioned
> block devices. The blockdevice will be excluded from the list if it is
> an extended partition (not logical) or part of a LVM or RAID
> configuration.

typos and not using current tense in commit message.
"look", not looks.
for consistency, i would not try to shorten the function names.
i.e. find_usable_bdevs -> find_usable_blockdevices.
code gets read 95% time, written 5% of the time ;)
it's also not very relevant to know what it looks for. what is relevant is what the function returns.
and it returns partitions themselves as well.

> ---
>  src/core/libs/lib-blockdevices-filesystems.sh |  150 +++++++++++++------------
>  src/core/libs/lib-ui-interactive.sh           |    4 +-
>  2 files changed, 80 insertions(+), 74 deletions(-)
> 
> diff --git a/src/core/libs/lib-blockdevices-filesystems.sh b/src/core/libs/lib-blockdevices-filesystems.sh
> index ac749a3..1b10927 100644
> --- a/src/core/libs/lib-blockdevices-filesystems.sh
> +++ b/src/core/libs/lib-blockdevices-filesystems.sh
> @@ -206,99 +206,105 @@ getlabel() {
>  # find partitionable blockdevices
>  # $1 extra things to echo for each device (optional) (backslash escapes will get interpreted)
>  finddisks() {
> -	workdir="$PWD"
> -	if cd /sys/block 2>/dev/null
> -	then
> -		# ide devices
> -		for dev in $(ls | egrep '^hd')
> -		do
> -			if [ "$(cat $dev/device/media)" = "disk" ]
> -			then
> -				echo -ne "/dev/$dev $1"
> -			fi
> -		done
> -		#scsi/sata devices, and virtio blockdevices (/dev/vd*)
> -		for dev in $(ls | egrep '^[sv]d')
> -		do
> -			# TODO: what is the significance of 5? ASKDEV
> -			if [ "$(cat $dev/device/type)" != "5" ]
> -			then
> -				echo -ne "/dev/$dev $1"
> +	shopt -s nullglob
> +
> +	# Block Devices
> +	if [[ -d /sys/block ]]; then
> +		for dev in /sys/block/*; do
since nullglob is set, I think you can remove all if [[ -d $dir ]] checks. the net result is the same:
$ shopt -s nullglob; for foo in /dev/doesnotexist/*; do echo $foo; done
$
> +			if [[ -f $dev/device/type ]]; then
> +				local type
> +				read type < /sys/block/${dev##*/}/device/type
> +				read size < /sys/block/${dev##*/}/size
> +				# Type 5 is a ROM Device
maybe mention what a ROM device is
> +				if [[ $type != 5 ]] && (( $size > 0 )); then
maybe a comment that size 0 can happen for empty card readers
> +					source "$dev/uevent"
> +					echo -ne "/dev/$DEVNAME $1"
> +					unset DEVNAME
> +				fi
>  			fi
>  		done
>  	fi
> +
>  	# cciss controllers
> -	if cd /dev/cciss 2>/dev/null
> -	then
> -		for dev in $(ls | egrep -v 'p')
> -		do
> -			echo -ne "/dev/cciss/$dev $1"
> +	if [[ -d /dev/cciss ]]; then
> +		for dev in /dev/cciss/*; do
> +			if [[ $dev != *[[:digit:]]p[[:digit:]]* ]]; then
> +				echo "$dev $1"
> +			fi
>  		done
>  	fi
> -	# Smart 2 controllers
> -	if cd /dev/ida 2>/dev/null
> -	then
> -		for dev in $(ls | egrep -v 'p')
> -		do
> -			echo -ne "/dev/ida/$dev $1"
> +	# Smart 2 Controller
> +	if [[ -d /dev/ida ]]; then
> +		for dev in /dev/ida/*; do
> +			if [[ $dev != *[[:digit:]]p[[:digit:]]* ]]; then
> +				echo "$dev $1"
> +			fi
>  		done
>  	fi
> -	cd "$workdir"
> +	shopt -u nullglob
>  }
>  
> +
>  # find block devices, both partionable or not (i.e. partitions themselves)
>  # $1 extra things to echo for each partition (optional) (backslash escapes will get interpreted)
> -findblockdevices() {
> -	workdir="$PWD"
> -	for devpath in $(finddisks)
> -	do
> -		disk=$(basename $devpath)
> -		echo -ne "/dev/$disk $1"
> -		cd /sys/block/$disk
> -		shopt -s nullglob
> -		for part in $disk*
> -		do
> -			# check if not already assembled to a raid device.  TODO: what is the significance of the 5? ASKDEV
> -			if [ -n "$part" ] && ! grep -q $part /proc/mdstat 2>/dev/null && ! fstype 2>/dev/null </dev/$part | grep -q lvm2 && ! sfdisk -c /dev/$disk $(echo $part | sed -e "s#$disk##g") 2>/dev/null | grep -q '5'
> -			then
> -				if [ -d $part ]
> -				then
> -					echo -ne "/dev/$part $1"
> -				fi
> +# find usable block devices
you should merge this comment in the comments above.
> +find_usable_bdevs() {
> +	shopt -s nullglob
> +
> +	local parts
> +
> +	# Cycle through all non-partitional block devices (sda, sdb, etc...)
what's non-partitional? maybe "non-partition" or "block devices which are not partitions"

> +	# Look for partitions and include them only if they are not part of a
> +	# RAID or LVM. Also, exclude the extended partition
- what happens when a non-partition block device is part of RAID/LVM ?
- extended partitionS
- explain *why* we do this: although we strongly encourage users to setup LVM in AIF, we also want to make the life a bit easier for those who set up LVM/softraid before running AIF

> +	for devpath in $(finddisks); do
> +		hidebldev=
how about just "hide"? or "hide_blockdevice" but we already know we're in the context of blockdevices.
> +		unset parts
> +
> +		# Glob allows for following matches:
> +		# /dev/sda -> /dev/sda1
> +		# /dev/cciss/c0d1 -> /dev/cciss/c0d1p1
> +		for dev in ${devpath}*[[:digit:]]*; do
> +			local disk="${dev%%[[:digit:]]*}"
> +			local partnum="${dev##*[[:alpha:]]}"
> +
> +			# Don't display extened partition (not logical parts)
extended
> +			sfdisk -c "$disk" "$partnum" 2>/dev/null | grep -q '5' && continue;
how about:
[ "$(sfdisk -c "$disk" "$partnum" 2>/dev/null)" = 5 ] && continue;
?
> +
> +			# Don't list parts that are part of RAID or LVM volumes
> +			if grep -qsw "${dev##*/}" /proc/mdstat || { pvscan -s 2>/dev/null | grep -q "$dev"; }; then
> +				hidebldev="True"
> +			else
> +				parts+=("$dev")
>  			fi
>  		done
> -		shopt -u nullglob
> +
> +		# If we have no partitions then print the root of the block device
> +		# Otherwise print the partitions
> +		if [[ -z $hidebldev ]] && (( ${#parts[@]} < 1 )); then
> +			echo -ne "$devpath $1"
> +		elif (( ${#parts[@]} )); then
> +			for part in ${parts[@]}; do
> +				echo -ne "$part $1"
> +			done
> +		fi
>  	done
> +
>  	# mapped devices
> -	for devpath in $(ls /dev/mapper 2>/dev/null | grep -v control)
> -	do
> -		echo -ne "/dev/mapper/$devpath $1"
> +	for devpath in /dev/mapper/*; do
> +		# Exclude /control directory and other non-block files
non-block files such as?
> +		if [[ -b $devpath ]]; then
> +			echo -ne "$devpath $1"
> +		fi
>  	done
> +
>  	# raid md devices
> -	for devpath in $(ls -d /dev/md* | grep '[0-9]' 2>/dev/null)
> -	do
> -		if grep -qw $(echo $devpath /proc/mdstat | sed -e 's|/dev/||g')
> -		then
> +	for devpath in /dev/md[0-9]*; do
> +		if grep -qw "${devpath//\/dev\//}" /proc/mdstat; then
>  			echo -ne "$devpath $1"
>  		fi
>  	done
> -	# cciss controllers
> -	if cd /dev/cciss 2>/dev/null
> -	then
> -		for dev in $(ls | egrep 'p')
> -		do
> -			echo -ne "/dev/cciss/$dev $1"
> -		done
> -	fi
> -	# Smart 2 controllers
> -	if cd /dev/ida 2>/dev/null
> -	then
> -		for dev in $(ls | egrep 'p')
> -		do
> -			echo -ne "/dev/ida/$dev $1"
> -		done
> -	fi
> -	cd "$workdir"
> +
> +	shopt -u nullglob
>  }