Increase USER_INPUT_INTERRUPT_TIMEOUT default from 10 to 30 seconds

[jsmeix]

• Type: Enhancement

• Impact: Normal

• How was this pull request tested?

In current GitHub master code since DISKS_TO_BE_WIPED="" cf.
#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.

[schlomo]

Good change.
In general I feel that we don't need to explain how export works for every variable that can be set from the outside.

If we use the :- syntax consistently then it becomes very simple to quickly check for such variables via grep:

$ grep :- usr/share/rear/conf/default.conf 
KERNEL_VERSION="${KERNEL_VERSION:-$( uname -r )}"
GALAXY11_BACKUPSET=${GALAXY11_BACKUPSET:-}
GALAXY11_PIT_RECOVERY=${GALAXY11_PIT_RECOVERY:-}
GALAXY11_USER=${GALAXY11_USER:-}
GALAXY11_PASSWORD=${GALAXY11_PASSWORD:-}

Whereas grepping for test yields way too many false positives.

[jsmeix]

@schlomo
thank you for your review.
I fully agree with your above comment
#2981 (review)

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
a436ad5
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]

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]

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]

@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]

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
fae98be
that belongs to
#2608
which has very many comments.
Do you perhaps remember why not ${VAR:-value} is used in this case?

[schlomo]

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]

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

[schlomo]

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

[jsmeix]

I did
ad5cac2
as addednum to this (already merged) pull request.

[jsmeix]

Oh - right now I found in default.conf

OPAL_PBA_DEBUG_PASSWORD=''

[jsmeix]

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:
#2982