#1415 Issue `closed`: USB backup can't work with current code¶

Labels: bug, fixed / solved / done

gozora opened issue at 2017-07-14 16:31:¶

Relax-and-Recover (ReaR) Issue Template¶

rear version (/usr/sbin/rear -V): Relax-and-Recover 2.1 / Git
OS version (cat /etc/rear/os.conf or lsb_release -a): Debian GNU/Linux 8.7 (jessie)
rear configuration files (cat /etc/rear/site.conf or cat /etc/rear/local.conf):

BACKUP=NETFS

# USB testing
OUTPUT=USB
USB_DEVICE=/dev/disk/by-label/REAR-000
BACKUP_URL=usb:///dev/disk/by-label/REAR-000

ONLY_INCLUDE_VG=( "vg00" )
EXCLUDE_BACKUP=( ${EXCLUDE_BACKUP[@]} fs:/crash fs:/usr/sap fs:/oracle )
EXCLUDE_RECREATE=( fs:/data )
BACKUP_PROG_EXCLUDE=( ${BACKUP_PROG_EXCLUDE[@]} '/mnt/*' '/media/*' )

ISO_MKISOFS_BIN=/usr/bin/ebiso

GRUB_RESCUE=n

USE_STATIC_NETWORKING=y
NETWORKING_PREPARATION_COMMANDS=(ifconfig eth1 inet 192.168.0.23)

Are you using legacy BIOS or UEFI boot? UEFI
Brief description of the issue: Backup to USB device can't successfully finish due logical error
Work-around, if any:
in 350_check_usb_disk.sh

- if grep -q "^$REAL_USB_DEVICE " /proc/mounts ; then
-    Error "USB device '$USB_DEVICE' is already mounted on $( grep "^$REAL_USB_DEVICE " /proc/mounts | cut -d' ' -f2 | tail -1 )"
- fi

Well I'm happy that my tradition continues and whenever I try to tackle a bug, I find another one :).

I think that there is a problem with following check in 350_check_usb_disk.sh:

if grep -q "^$REAL_USB_DEVICE " /proc/mounts ; then
    Error "USB device '$USB_DEVICE' is already mounted on $( grep "^$REAL_USB_DEVICE " /proc/mounts | cut -d' ' -f2 | tail -1 )"
fi

First and foremost this condition should always evaluate as TRUE, because USB filesystem is mounted by 060_mount_NETFS_path.sh prior this check.

IMHO check for mounted file systems never actually worked correctly (was well hidden) as until commit (9875ba30288634e3b2c55b26b14a726acce09252) it looked like this:

! grep -q "^$REAL_USB_DEVICE " /proc/mounts
StopIfError "USB device '$USB_DEVICE' is already mounted on $(grep -E "^$REAL_USB_DEVICE\\s" /proc/mounts | cut -d' ' -f2 |tail -1)"

And this is where my head explodes (but at least I realize how bad I'm in reading and understanding code :-))!
The thing is that first grep might set RC != 0, but successive StopIfError will not do any action as it evaluates RC from $(grep -E "^$REAL_USB_DEVICE\\s" /proc/mounts | cut -d' ' -f2 |tail -1)" which will return 0 because filesystem is obviously mounted!

Do you guys think that this check could be completely removed, or replaced by something that will not throw error every time?

V.

gozora commented at 2017-07-14 16:46:¶

Following code works fine for me:

# Check if USB is mounted in other location then $BUILD_DIR
if grep -v $BUILD_DIR /proc/mounts | grep -q "^$REAL_USB_DEVICE"; then
    Error "USB device '$REAL_USB_DEVICE' is already mounted."
fi

If it is OK for everybody I can open PR ...

V.

gdha commented at 2017-07-14 17:57:¶

@gozora Thank you Vlad. Somebody from J&J also reported this directed to me, but I did not find the time to investigate. You have hit the bug ;-) OK to make a PR

jsmeix commented at 2017-07-17 08:16:¶

@gozora
good to hear that also your head (hopefully only almost)
exploded when you tried to understand code like

! grep -q ...
StopIfError "... $( ... )"

because I had exactly the same symptoms when I tried
to convert that into understandable code.

But obviously that code was too complicated for my brain
to correctly convert it into understandable code.
I remember that I had tested it on my system as follows:

# grep '^/dev/sda8 ' /proc/mounts
/dev/sda8 / ext2 rw,relatime,errors=continue,user_xattr,acl 0 0

# ! grep -q '^/dev/sda8 ' /proc/mounts || echo /dev/sda8 is mounted
/dev/sda8 is mounted

# grep -q '^/dev/sda8 ' /proc/mounts && echo /dev/sda8 is mounted
/dev/sda8 is mounted

but that is not what the above code actually does because
I had not tested what that $(...) in StopIfError actually does.

Note that the exit code of

grep -E "^$REAL_USB_DEVICE\\s" /proc/mounts | cut -d' ' -f2 | tail

is always true regardless what is mounted or what grep returns
because the exit code of a pipe is the exit code of its last command
which is here 'tail' e.g.:

# cat qqq | tail
cat: qqq: No such file or directory

# echo $?
0

Only with 'set -o pipefail' the exit code of a pipe is the exit code of
the last(!) command that fails (or zero if none fails):

# set -o pipefail

# cat qqq | tail
cat: qqq: No such file or directory

# echo $?
1

# cat qqq | grep -Q foo | tail
grep: invalid option -- 'Q'
Usage: grep [OPTION]... PATTERN [FILE]...
Try `grep --help' for more information.
cat: qqq: No such file or directory

# echo $?
2

cf. https://github.com/rear/rear/issues/700

This shows that the initial code was just a mess of
overcomplicated weirdness that never worked as intended.

This proves that simple and straightforward (KISS) code
is mandatory in the end because oversophisticated stuff
either is already buggy or will lead to subsequent bugs, cf.
https://github.com/rear/rear/issues/1321#issuecomment-295753687
and see also
https://github.com/rear/rear/wiki/Coding-Style

Make yourself understood to enable others 
to fix and enhance your code properly as needed.

Many thanks for finding the issue and for your fix!

gozora commented at 2017-07-17 18:05:¶

With #1417 merged, this issue can be closed.

jsmeix commented at 2020-07-01 13:45:¶

Via https://github.com/rear/rear/commit/daf35e235d0770c663ff8dba866dddec76586a27
I added an explanatory comment in lib/_input-output-functions.sh
that using the ...IfError functions can result unexpected behaviour in certain cases.

[Export of Github issue for rear/rear.]

#1415 Issue closed: USB backup can't work with current code¶