#3503 PR open: Fix is_disk_valid in layout-functions.sh

Labels: bug

jsmeix opened issue at 2025-08-04 14:32:

Not yet tested, see
https://github.com/rear/rear/pull/3047#discussion_r2251699696

  • Description of the changes in this pull request:

In lib/layout-functions.sh
tried to fix function is_disk_valid()
according to what I described in
https://github.com/rear/rear/pull/3047#discussion_r2238624740

jsmeix commented at 2025-08-04 14:36:

@pcahyna
could you please have a look here (as time permits) ?

jsmeix commented at 2025-08-04 14:56:

My test commands on bash command line to simulate how a command like

blockdev_stdout_stderr="$( blockdev --getsize64 "$disk" 2>&1 )"

behaves in general (in particular with set -x):

# ( set -x ; if stdout_stderr="$( grep 'ext4' /etc/fstab 2>&1 )" ; then echo "grep exit code $? : '$stdout_stderr'" ; else echo "grep exit code $? : '$stdout_stderr'" ; fi )
++ grep --color=auto ext4 /etc/fstab
+ stdout_stderr='/dev/mapper/cr_root  /          ext4  defaults  0  1'
+ echo 'grep exit code 0 : '\''/dev/mapper/cr_root  /          ext4  defaults  0  1'\'''
grep exit code 0 : '/dev/mapper/cr_root  /          ext4  defaults  0  1'

# ( set -x ; if stdout_stderr="$( grep 'qqq' /etc/fstab 2>&1 )" ; then echo "grep exit code $? : '$stdout_stderr'" ; else echo "grep exit code $? : '$stdout_stderr'" ; fi )
++ grep --color=auto qqq /etc/fstab
+ stdout_stderr=
+ echo 'grep exit code 1 : '\'''\'''
grep exit code 1 : ''

# ( set -x ; if stdout_stderr="$( grep 'ext4' /etc/qqq 2>&1 )" ; then echo "grep exit code $? : '$stdout_stderr'" ; else echo "grep exit code $? : '$stdout_stderr'" ; fi )
++ grep --color=auto ext4 /etc/qqq
+ stdout_stderr='grep: /etc/qqq: No such file or directory'
+ echo 'grep exit code 2 : '\''grep: /etc/qqq: No such file or directory'\'''
grep exit code 2 : 'grep: /etc/qqq: No such file or directory'

jsmeix commented at 2025-08-06 13:28:

@pcahyna
no worries!
I am happy that my attempt moved things forward
towards a proper solution.

When

{ size=$( /sbin/blockdev --getsize64 "$disk" 2>&3- ) ; } 3>&1

is really a proper solution, could you then provide
an explanatory comment in your code which tells WHY
here RFC1925 6a "add another level of indirection"
is really the right solution in this particular case
so that at any time later others still understand your code
so they can properly fix and enhance your code as needed
without false optimizations or false simplifications
because they cannot see or do not understand the
reasoning behind.

jsmeix commented at 2025-08-06 13:57:

@pcahyna
what does the trailing hyphen in 2>&3- do?
I only know about >&- and <&- to close a file descriptor as in
https://github.com/rear/rear/blob/rear-2.9/usr/share/rear/lib/_input-output-functions.sh#L346
and subsequent lines.
As far as I see nowhere in ReaR is &...- currently used:

# find usr/sbin/rear usr/share/rear/ -type f | xargs grep -ho "&[0-9]." | sort -u
&1 
&1'
&1)
&1;
&2 
&2"
&2;
&6 
&7 
&8 
&8"
&8'
&8)
&9

This output also shows that fd 3 is currently nowhere used
so fd 3 should be safe to use.

pcahyna commented at 2025-08-06 14:02:

@jsmeix

the trailing hyphen causes fd 3 to be closed, i.e. fd 3 is moved to fd 2. See https://www.gnu.org/software/bash/manual/html_node/Redirections.html#Moving-File-Descriptors . The hyphen is not strictly necessary, but I wanted to avoid passing an unnecessary open descriptor to blockdev.

pcahyna commented at 2025-08-06 14:50:

By the way, I checked that this redirection works already even in Bash in RHEL 6 (which we don't support anymore).

jsmeix commented at 2025-08-07 06:02:

Strange - I must have been blind - because somehow
I did not recognize the "Moving File Descriptors"
section in "man bash" of my GNU bash version 4.4.23.
So I was wondering if the trailing hyphen in 2>&3-
is perhaps a new bash feature which might be not
supported in earlier bash 4 versions.

jsmeix commented at 2025-08-07 06:29:

@pcahyna
perhaps I misunderstand something but
I fail to make your proposal working for me:

# ( disk=/dev/sdq ; set -x ; if { size=$( /sbin/blockdev --getsize64 "$disk" 2>&3- ) ; } 3>&1 ; then echo OK size=$size ; else echo failed size=$size ; fi )

++ /sbin/blockdev --getsize64 /dev/sdq
blockdev: cannot open /dev/sdq: No such file or directory
+ size=
+ echo failed size=
failed size=

so when blockdev fails its stderr message does not
appear in the size variable in contrast to my proposal

# ( disk=/dev/sdq ; set -x ; if size=$( /sbin/blockdev --getsize64 "$disk" 2>&1 ) ; then echo OK size=$size ; else echo failed size=$size ; fi )

++ /sbin/blockdev --getsize64 /dev/sdq
+ size='blockdev: cannot open /dev/sdq: No such file or directory'
+ echo failed size=blockdev: cannot open /dev/sdq: No such file or directory
failed size=blockdev: cannot open /dev/sdq: No such file or directory

Because the resulting if condition in your proposal

if { size=$( /sbin/blockdev --getsize64 "$disk" 2>&3- ) ; } 3>&1 ; then

looks rather complicated to decipher what it is about
I am wondering if using a temporary file instead of fd 3
could make the code easier to read and understand
for example something like

# ( disk=/dev/sdq ; set -x ; if size=$( /sbin/blockdev --getsize64 "$disk" 2>/tmp/blockdev_stderr ) ; then echo $size ; else cat /tmp/blockdev_stderr ; fi )

++ /sbin/blockdev --getsize64 /dev/sdq
+ size=
+ cat /tmp/blockdev_stderr
blockdev: cannot open /dev/sdq: No such file or directory

# ( disk=/dev/nvme0n1 ; set -x ; if size=$( /sbin/blockdev --getsize64 "$disk" 2>/tmp/blockdev_stderr ) ; then echo $size ; else cat /tmp/blockdev_stderr ; fi )

++ /sbin/blockdev --getsize64 /dev/nvme0n1
+ size=500107862016
+ echo 500107862016
500107862016

which seems to "just work" at least for me.

In ReaR we would use ReaR's TMP_DIR like

size="$( /sbin/blockdev --getsize64 "$disk" 2>"$TMP_DIR/blockdev_stderr" )"

pcahyna commented at 2025-08-07 09:10:

when blockdev fails its stderr message does not
appear in the size variable

That's the point! size holds the stdout of blockdev, not stderr. Stderr of blockdev gets redirected to stdout of the whole is_disk_valid function, which is intended - look at its usage example.

[Export of Github issue for rear/rear.]