#1542 PR `merged`: Some code simplification plus explicit 'return 0' where needed¶

Labels: documentation, cleanup, fixed / solved / done, critical / security / legal

jsmeix opened issue at 2017-10-24 09:13:¶

Some simplification in code that is related to issue 1541
plus explicit 'return 0' when return is intended after
a failed command i.e. when return is actually 'success'
because otherwise return results the exit code of the
failed command e.g. as in

# function testfunc1 () { test -f qqq || return ; }
# testfunc1
# echo $?
1

# function testfunc2 () { grep -Q foo bar && echo found ; return ; }
# testfunc2
grep: invalid option -- 'Q'
Usage: grep [OPTION]... PATTERN [FILE]...
Try `grep --help' for more information.
# echo $?
2

jsmeix commented at 2017-10-24 09:20:¶

@gdha
in usr/share/rear/prep/GNU/Linux/300_include_grub_tools.sh
I added a 'FIXME' for you because via
https://github.com/rear/rear/commit/2de5b1b9f3f1cc6e59da860bfa91d391790fe82c
you added

grubdir=$(ls -d /boot/grub*)

which is not fail-safe because
usr/sbin/rear sets 'shopt -s nullglob'
so that the 'ls' command will list all files in the current
working directory if nothing matches the bash globbing
pattern '/boot/grub*' which results in this case '.' in 'grubdir'
(the plain 'ls -d' output in the current working directory).

jsmeix commented at 2017-10-24 10:29:¶

@gdha
do you have a good idea how one could make things like

grubdir=$( ls -d /boot/grub* )

work fail-safe even with 'shopt -s nullglob'?

I would like to get the 'FIXME' fixed.

What I did in such cases was to not use 'ls' but 'find'
e.g. as in prep/NETFS/default/070_set_backup_archive.sh

# Here things like 'find /path/to/dir -name '*.tar.gz' | sort' are used because
# one cannot use bash globbing via commands like 'ls /path/to/dir/*.tar.gz'
# because /usr/sbin/rear sets the nullglob bash option which leads to plain 'ls'
# when '/path/to/dir/*.tar.gz' matches nothing (i.e. when no backup file exists)
# so that then plain 'ls' would result nonsense.
local latest_full_backup=$( find $backup_directory -name "$full_backup_glob_regex" | sort | tail -n1 )

jsmeix commented at 2017-10-24 15:07:¶

I think I found another possible way
how to be fail-safe for 'shopt -s nullglob':

Do not use

ls globbing_pattern

but (where possible)

echo -n globbing_pattern

because with 'shopt -s nullglob' the 'echo -n' command
outputs nothing if nothing matches the globbing pattern
so that one can test for an empty variable like:

local grubdir="$( echo -n /boot/grub* )"
# Set fallback if nothing matches '/boot/grub*'
test -d "$grubdir" || grubdir='/boot/grub'

I implemented that in
https://github.com/rear/rear/pull/1542/commits/6ee5c6d9c98ca49eb58001ea39073ab4ff62bd5c

gdha commented at 2017-10-24 15:25:¶

@jsmeix excellent work - thank you for that

jsmeix commented at 2017-10-25 10:31:¶

In a ReaR git checkout directory I did things like

# find usr/sbin/rear usr/share/rear/ | xargs grep '=$(ls '

# find usr/sbin/rear usr/share/rear/ | xargs grep '=$( ls '

# find usr/sbin/rear usr/share/rear/ | xargs grep '="$(ls '

# find usr/sbin/rear usr/share/rear/ | xargs grep '="$( ls '

and found

var=$( ls globbing_pattern )

usage that needs to be fixed only in
output/ISO/Linux-i386/810_prepare_multiple_iso.sh

Other such usage in
lib/bootloader-functions.sh
and
output/USB/Linux-i386/300_create_extlinux.sh
looks to be already prepared for 'shopt -s nullglob'
so that I only added an explanatory comment there.

See
https://github.com/rear/rear/pull/1542/commits/5dac6f84c27254208b9134318823496e9a763090

jsmeix commented at 2017-10-25 14:39:¶

Because all still works for me I "just merge" it, cf.
https://github.com/rear/rear/issues/1545#issuecomment-339352069

[Export of Github issue for rear/rear.]

#1542 PR merged: Some code simplification plus explicit 'return 0' where needed¶