Re: [PATCH 07/10] bash-completion: sys-utils

[Sami Kerola <kerolasa@iki.fi>]

On Fri, Mar 29, 2013 at 05:33:35PM +0100, Karel Zak wrote:
> On Wed, Mar 27, 2013 at 10:07:49PM +0000, Sami Kerola wrote:
> > @@ -0,0 +1,25 @@
> > +_blkdiscard_module()
> > +{
> > +	local cur prev OPTS
> > +	COMPREPLY=()
> > +	cur="${COMP_WORDS[COMP_CWORD]}"
> > +	prev="${COMP_WORDS[COMP_CWORD-1]}"
> > +	case $prev in
> > +		'-o'|'--offset'|'-l'|'--length')
> > +			COMPREPLY=( $(compgen -W "num" -- $cur) )
> > +			return 0
> > +			;;
> > +	esac
> > +	case $cur in
> > +		-*)
> > +			OPTS="-o --offset -l --length -s --secure -v --verbose -h --help -V --version"
> > +			COMPREPLY=( $(compgen -W "${OPTS[*]}" -- $cur) )
> > +			return 0
> > +			;;
> > +	esac
> > +	local DEVS
> > +	DEVS="$(\ls -d /sys/class/block/* | sed 's|/sys/class/block/|/dev/|g')"
> 
> It does not look like the right way how to generate the paths, because
> /sys/class/block/ contains also paths to unassociated loop devices and
> all the paths are based on kernel device names (e.g. dm-X).
> 
> I have add --paths to lsblk(8), so now you can use:
> 
>  lsblk -p -o NAME -n -l
> 
> to list the paths, for exmaple:
>  
>  /dev/sda
>  /dev/sda1
>  /dev/sda2
>  /dev/sda3
>  /dev/sda4
>  /dev/sda5
>  /dev/sda6
>  /dev/mapper/luks-10d813de-fa82-4f67-a86c-23d5d0e7c30e
>  /dev/sdb
>  /dev/sr0
> 
> it would be nice to use this solution everywhere.

I think I found all references to block devices, and replaced them with
lsblk which is using Dave's while loop.

> > +_eject_module()
> > +{
>  ....
> > +	DEVS="$(for I in /sys/class/block/*/removable; do
> > +		if [ $(cat $I) -ne 0 ]; then
> > +			OLD_IFS=$IFS
> > +			IFS='/';
> > +			ARR=($I)
> > +			echo "/dev/${ARR[4]}"
> > +			IFS=$OLD_IFS
> > +		fi
> > +	done)"
> 
>  lsblk -rpn -o RM,NAME | awk '/^1/ { print $2 }'
> 
>  The common rule is to not read data from /sys if possible.
> 
> > diff --git a/shell-completion/fstrim b/shell-completion/fstrim
> > new file mode 100644
> > index 0000000..87cb050
> > --- /dev/null
> > +++ b/shell-completion/fstrim
> > @@ -0,0 +1,25 @@
> > +_fstrim_module()
> > +{
> > +	local cur prev OPTS
> > +	COMPREPLY=()
> > +	cur="${COMP_WORDS[COMP_CWORD]}"
> > +	prev="${COMP_WORDS[COMP_CWORD-1]}"
> > +	case $prev in
> > +		'-o'|'--offset'|'-l'|'--length'|'-m'|'--minimum')
> > +			COMPREPLY=( $(compgen -W "num" -- $cur) )
> > +			return 0
> > +			;;
> > +	esac
> > +	case $cur in
> > +		-*)
> > +			OPTS="-o --offset -l --length -m --minimum -v --verbose -h --help -V --version"
> > +			COMPREPLY=( $(compgen -W "${OPTS[*]}" -- $cur) )
> > +			return 0
> > +			;;
> > +	esac
> > +	local MPOINTS
> > +	MPOINTS=$(awk '{if ($1 ~ /^\//){print $2}}' /etc/mtab 2>/dev/null)
> 
>  The same problem, don't read mtab directly (for example because
>  some chars could be escaped...). Use:
> 
>    findmnt -rno SOURCE | grep '/dev'

With this I had a small whoops.  The diff I sent moment ago did not have
findmnt v.s. /etc/mtab fixes, which are now in my git.

What comes to principle of using helper commands rather than /sys or
/proc files I fully agree this is better.

-- 
   Sami Kerola
   http://www.iki.fi/kerolasa/