#2981 PR `merged`: Increase USER_INPUT_INTERRUPT_TIMEOUT default from 10 to 30 seconds¶

Labels: enhancement, fixed / solved / done

jsmeix opened issue at 2023-05-05 13:39:¶

Type: Enhancement
Impact: Normal
How was this pull request tested?

In current GitHub master code since DISKS_TO_BE_WIPED="" cf.
https://github.com/rear/rear/pull/2859
there is in layout/recreate/default/120_confirm_wipedisk_disks.sh
a UserInput() dialog like

Disks to be wiped: /dev/sda /dev/sdb /dev/sdd
...
Confirm disks to be wiped and continue 'rear recover'

which has usually (i.e. when not in MIGRATION_MODE)
a timeout of USER_INPUT_INTERRUPT_TIMEOUT.

During my tests with current GitHub master code
I learned that 10 seconds is far too little time
to read and understand that UserInput message and
then some more time to make a decision whether or not
the automated action is actually the right one.

I.e. 10 seconds was far too little time even for me
where I expected that UserInput dialog but even I
could not actually comprehend what disks will be wiped
and I could not at all make a decision whether or not
those are the right disks.

So the same applies to any UserInput dialog
(in prticular to any unexpected UserInput dialog)
where USER_INPUT_INTERRUPT_TIMEOUT is used
i.e. any UserInput() dialog with a predefined input.

In general a default timeout is useless when it is too short
to let the user understand what is going on and let him
make a decision so in practice such a too short timeout
behaves very user unfiendly.

Brief description of the changes in this pull request:

In default.conf increased the default USER_INPUT_INTERRUPT_TIMEOUT
from 10 seconds to 30 seconds because 10 seconds is far too little time
to read and understand a possibly unexpected UserInput() message
and then some more time to make a decision whether or not
the automated action is actually the right one.

jsmeix commented at 2023-05-08 07:16:¶

@schlomo
thank you for your review.
I fully agree with your above comment
https://github.com/rear/rear/pull/2981#pullrequestreview-1414902682

Additionlly
https://stackoverflow.com/questions/4437573/bash-assign-default-value
explains why

VAR="${VAR:-default value}"

is better in practice than

: "${VAR:=default value}"

cf. my comment in
https://github.com/rear/rear/pull/2981/commits/a436ad55e5dc437f173eb3c253341fd4d3c59551
excerpt:

VAR="${VAR:-default value}" is found by 'grep' for 'VAR=' and
with 'set -x' it shows VAR='default value' (instead of : 'default value')
which makes this code easier to understand, debug and maintain,
cf. https://github.com/rear/rear/wiki/Coding-Style

jsmeix commented at 2023-05-08 07:24:¶

I am wondering why we use

VAR="${VAR:-default value}"

and not

VAR="${VAR-default value}"

i.e. why we assign 'defult value' via :-
if VAR is unset or null (i.e. unset or empty)
instead of assigning 'defult value' via -
only if VAR is unset?

In the former case the user cannot set an empty value via

export VAR=""

which he could do (if needed) in the latter case.

As far as I see this does not matter with the current
config variables that are set this way because
for none of those which get a non-empty default set
it makes sense for the user to set them to be empty.

So my question is currently only out of curiosity.

jsmeix commented at 2023-05-08 07:52:¶

I have another question:

What is "the best" way to assign a string of words
via bash parameter expansion?

Currenly I use an unquoted string

VAR="${VAR:-default value}"

in

USER_INPUT_PROMPT="${USER_INPUT_PROMPT:-enter your input}"

because we do it this way already in
rear/output/RAWDISK/Linux-i386/280_create_bootable_disk_image.sh

"${RAWDISK_GPT_PARTITION_NAME:-Rescue System}"
${RAWDISK_BOOT_SYSLINUX_START_INFORMATION:-Starting the rescue system...}

and
rear/output/RAWDISK/Linux-i386/270_create_grub2_efi_bootloader.sh

"${RAWDISK_BOOT_GRUB_MENUENTRY_TITLE:-Recovery System}"

Alternatively

VAR="${VAR:-"default value"}"

would work but the nested double quotes look confusing.
In contrast single quotes like

VAR="${VAR:-'default value'}"

do not work because (at least with bash version 4.4.23)

# unset VAR

# VAR="${VAR:-'default value'}"

# declare -p VAR
declare -- VAR="'default value'"

the single quotes become part of the assigned value.

schlomo commented at 2023-05-08 08:25:¶

@jsmeix the examples in
https://tldp.org/LDP/abs/html/parameter-substitution.html#:~:text=variable%3D%0A%23%20variable%20has%20been%20declared%2C%20but%20is%20set%20to%20null.%0A%0Aecho%20%22%24%7Bvariable%2D0%7D%22%20%20%20%20%23%20(no%20output)%0Aecho%20%22%24%7Bvariable%3A%2D1%7D%22%20%20%20%23%201%0A%23%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%5E
led me (long long time ago and now again) to always use :- syntax as it also handles null set variables.

What is bad with VAR="${VAR:-default value}" if it works as expected?
Even VAR=${VAR:-default value} works the same.

I don't see a need to change anything from our current use of this parameter substitution and it seems to me that there is no need for inner quotes around multi word default values either.

jsmeix commented at 2023-05-08 09:19:¶

By the way regarding ${VAR-value}:

As far as I found we have only one case where this is used:
rear/lib/layout-functions.sh

function get_device_from_partition() {
    ...
    partition_number=${2-$(get_partition_number $partition_block_device )}

It seems to be allowed to call get_device_from_partition
with an empty second argument?

get_device_from_partition is only called in
finalize/Linux-i386/670_run_efibootmgr.sh

@pcahyna
as far as I see from
git log --follow -p usr/share/rear/lib/layout-functions.sh
this ${VAR-value} parameter expansion is from you in
https://github.com/rear/rear/commit/fae98be14d0a7de30128f896c879f19259a96ad7
that belongs to
https://github.com/rear/rear/pull/2608
which has very many comments.
Do you perhaps remember why not ${VAR:-value} is used in this case?

schlomo commented at 2023-05-08 09:20:¶

No memories, most likely I did it wrong. Off the top of my head I'm not aware of any reason to use VAR-value instead of VAR:-value

Personally, I also find :- easier to read and notice

jsmeix commented at 2023-05-08 09:25:¶

@schlomo I guess you are not @pcahyna :-)

schlomo commented at 2023-05-08 09:33:¶

Ah yes, that would explain my lack of memory. Well spotted 😄

jsmeix commented at 2023-05-08 13:25:¶

I did
https://github.com/rear/rear/commit/ad5cac29d948a4a73ee31a47ba515d9f6c305f19
as addednum to this (already merged) pull request.

jsmeix commented at 2023-05-08 13:37:¶

Oh - right now I found in default.conf

OPAL_PBA_DEBUG_PASSWORD=''

jsmeix commented at 2023-05-09 05:34:¶

I will do a new pull request to clean up in default.conf
all cases of config variables for confidential values
i.e. have a generic explanation comment at the beginning
instead of several similar comments at each place:
https://github.com/rear/rear/pull/2982

[Export of Github issue for rear/rear.]

#2981 PR merged: Increase USER_INPUT_INTERRUPT_TIMEOUT default from 10 to 30 seconds¶