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

Sami Kerola
2013-04-01T16:33:04Z

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