#2868 PR `merged`: In the UserInput function drain stdin if stdin is a terminal¶

Labels: enhancement, fixed / solved / done, minor bug

jsmeix opened issue at 2022-09-22 14:29:¶

Type: Minor Bug Fix
Impact: Low
Reference to related issue (URL):
https://github.com/rear/rear/issues/2866
How was this pull request tested?

Works sufficiently well for me
during "rear recover" with MIGRATION_MODE="true"
(where several user dialogs appear).

Brief description of the changes in this pull request:

In the UserInput function
in lib/_input-output-functions.sh
drain stdin if stdin is a terminal (i.e. in interactive mode).

jsmeix commented at 2022-09-23 09:45:¶

FYI
what "help read" shows on SLES11 SP4 with bash version 3.2.57

# help read
read: read [-ers] [-u fd] [-t timeout] [-p prompt] [-a array] [-n nchars] [-d delim] [name ...]
    One line is read from the standard input, or from file descriptor FD if the
    -u option is supplied, and the first word is assigned to the first NAME,
    the second word to the second NAME, and so on, with leftover words assigned
    to the last NAME.  Only the characters found in $IFS are recognized as word
    delimiters.  If no NAMEs are supplied, the line read is stored in the REPLY
    variable.  If the -r option is given, this signifies `raw' input, and
    backslash escaping is disabled.  The -d option causes read to continue
    until the first character of DELIM is read, rather than newline.  If the -p
    option is supplied, the string PROMPT is output without a trailing newline
    before attempting to read.  If -a is supplied, the words read are assigned
    to sequential indices of ARRAY, starting at zero.  If -e is supplied and
    the shell is interactive, readline is used to obtain the line.  If -n is
    supplied with a non-zero NCHARS argument, read returns after NCHARS
    characters have been read.  The -s option causes input coming from a
    terminal to not be echoed.

    The -t option causes read to time out and return failure if a complete line
    of input is not read within TIMEOUT seconds.  If the TMOUT variable is set,
    its value is the default timeout.  The return code is zero, unless end-of-file
    is encountered, read times out, or an invalid file descriptor is supplied as
    the argument to -u.

jsmeix commented at 2022-09-23 11:53:¶

Via my last
https://github.com/rear/rear/pull/2868/commits/6ab2c2a2d4a9fede920fd18a726ac41801cd0930
I simplified it to a single command

test -t 0 && while read -t 0 ; do read -s ; done

pcahyna commented at 2022-09-23 12:12:¶

This does not drain characters not terminated by ENTER, try

sleep 5; while read -t 0 ; do read -s ; done

and press some keys (without ENTER) during the sleep. You will see that the characters remain.

jsmeix commented at 2022-09-23 12:41:¶

Yes this is a known drawback of how read -t 0 is implemented.
It only looks if there is something in stdin.
It does not look if there is something
in the terminal buffer that is not yet in stdin.
See my comment in my last commit.

I tested it right now and it behaves "good enough" for me.

During my test I typed plain '9' without ENTER
(note the 9Marking component... line)
before another UserInput dialog appeared and got
(with MIGRATION_MODE="true" to get those dialogs):

RESCUE localhost:~ # rear -D recover
...
User confirmed disk layout file
9Marking component '/dev/sda' as done in /var/lib/rear/layout/disktodo.conf
Marking component '/dev/sda1' as done in /var/lib/rear/layout/disktodo.conf
...
Running 'layout/recreate' stage ======================
UserInput -I LAYOUT_CODE_CONFIRMATION needed in /usr/share/rear/layout/recreate/default/100_confirm_layout_code.sh line 26
Confirm or edit the disk recreation script
1) Confirm disk recreation script and continue 'rear recover'
2) Edit disk recreation script (/var/lib/rear/layout/diskrestore.sh)
3) View disk recreation script (/var/lib/rear/layout/diskrestore.sh)
4) View original disk space usage (/var/lib/rear/layout/config/df.txt)
5) Use Relax-and-Recover shell and return back to here
6) Abort 'rear recover'
(default '1' timeout 300 seconds)

UserInput: Result is '9'
UserInput -I LAYOUT_CODE_CONFIRMATION needed in /usr/share/rear/layout/recreate/default/100_confirm_layout_code.sh line 26
Confirm or edit the disk recreation script
1) Confirm disk recreation script and continue 'rear recover'
2) Edit disk recreation script (/var/lib/rear/layout/diskrestore.sh)
3) View disk recreation script (/var/lib/rear/layout/diskrestore.sh)
4) View original disk space usage (/var/lib/rear/layout/config/df.txt)
5) Use Relax-and-Recover shell and return back to here
6) Abort 'rear recover'
(default '1' timeout 300 seconds)

when I hit only ENTER in that dialog
because that dialog got '9' and ENTER at its stdin
when ENTER was hit (which triggers to move the '9'
from the terminal buffer into rear's stdin).

By the way:
I hit ENTER many times between dialogs
and typed lots of stuff during backup restore.
All behaved sufficiently well for me.

pcahyna commented at 2022-09-23 12:43:¶

I also wonder why -t in the second read is not needed. I would be afraid that read -s without -t would block forever if there is no more input at this point, but apparently this does not happen.

pcahyna commented at 2022-09-23 12:44:¶

Yes this is a known drawback of how read -t 0 is implemented.

My question is, why not add -n 1000 back? It seemed to get rid of this drawback.

jsmeix commented at 2022-09-23 12:50:¶

I also wondered why -t 1 in the second read is not needed.
I also expected it to wait endlessly for input.
I got that from a colleague and tried it and it works for me.
See also the function clear_stdin() in
https://superuser.com/questions/276531/clear-stdin-before-reading
where while read none; do :; done seems to work.
When I call this on command line it waits for input.

jsmeix commented at 2022-09-23 12:53:¶

Next week I try out how it behaves with re-added -n 1000.
And perhaps I should add -t 1 just as a safety net.

jsmeix commented at 2022-09-26 12:46:¶

Hmpf!
I hate this fragile terminal IO stuff.

I can produce an endless 'read' loop with

while read -t 0 ; do read ... ; done

on comandline with bash 4.4.23
depending on how the process that does the 'read'
is connected with the process that provides read's input
and additionally depending on some (weird) 'sleep':

# ( echo start ; sleep 2 ; echo -en 'foo\nbar' ; sleep 2 ; echo end ) | ( while read -t 0 ; do echo reading ; read -t 1 -n 1000 -d '' ; echo "'$REPLY'" ; sleep 1 ; done )
reading
'start
'
reading
'foo
bar'
reading
'end
'
reading
''
reading
''
...
[continues endlessly]

versus

# ( echo start ; sleep 2 ; echo -en 'foo\nbar' ; sleep 2 ; echo end ) | ( while read -t 0 ; do echo reading ; read -t 1 -n 1000 -d '' ; echo "'$REPLY'" ; done )
bash: echo: write error: Broken pipe
bash: echo: write error: Broken pipe
bash: echo: write error: Broken pipe

or

# { echo start ; sleep 2 ; echo -en 'foo\nbar' ; sleep 2 ; echo end ; } | { while read -t 0 ; do echo reading ; read -t 1 -n 1000 -d '' ; echo "'$REPLY'" ; sleep 1 ; done ; }
bash: echo: write error: Broken pipe
bash: echo: write error: Broken pipe

Because I cannot know how the user will connect
the process that does the 'read' (usr/sbin/rear)
with the process that provides read's input
(the users's normal terminal or something else
where test -t 0 still results zero exit code),
I will not risk any possibility to hang up ReaR
only because to avoid possible bad outcomes when
a user carelessly types things on his keyboard
regardless that he should not type randomly
while a program is running that uses stdin.

So I will not use any kind of unlimited loop.

Meanwhile I think a single

read -s -t 1 -n 1000 -d ''

is sufficient for what is actually needed here in practice
which is primarily to drain some accidental additional ENTER
which can accidentally happen also to experienced users
(in contrast to random typing things on the keyboard).

Of course it happened to me during the sequence of user input
dialogs during "rear recover" with MIGRATION_MODE="true"
where most dialogs are OK so one gets used to hit ENTER
until the one dialog appears where one needs to select
something specific (instead of the default via ENTER).

How read -s -t 1 -n 1000 -d '' behaves for me:

On SLES10 with bash 3.1.17:

# for i in $( seq 10 ) ; do echo $i ; done | ( read -s -t 1 -n 1000 -d '' ; echo "'$REPLY'" | od -a )
0000000   '   1  nl   2  nl   3  nl   4  nl   5  nl   6  nl   7  nl   8
0000020  nl   9  nl   1   0  nl   '  nl

# for i in $( seq 10 ) ; do echo $i ; done | ( read -s -t 1 -n 10 -d '' ; echo "'$REPLY'" | od -a )
0000000   '   1  nl   2  nl   3  nl   4  nl   5  nl   '  nl

# time read -s -t 1 -n 1000 -d ''
real    0m1.002s
user    0m0.000s
sys     0m0.000s

On openSUSE Leap 15.3 with bash 4.4.23:

# for i in $( seq 10 ) ; do echo $i ; done | ( read -s -t 1 -n 1000 -d '' ; echo "'$REPLY'" | od -a )
0000000   '   1  nl   2  nl   3  nl   4  nl   5  nl   6  nl   7  nl   8
0000020  nl   9  nl   1   0  nl   '  nl

# for i in $( seq 10 ) ; do echo $i ; done | ( read -s -t 1 -n 10 -d '' ; echo "'$REPLY'" | od -a )
0000000   '   1  nl   2  nl   3  nl   4  nl   5  nl   '  nl

# time read -s -t 1 -n 1000 -d ''
real    0m1.000s
user    0m0.000s
sys     0m0.000s

This causes again a one second delay when there is no input
but that should not matter in practice in interactive mode.
This one second delay might even be useful in practice
because this way the user may not too easily get used
to carelessly hit ENTER ENTER ENTER... when there is
a sequence of dialogs.

What matters first and foremost is: "It Has To Work."
RFC 1925 item 1 https://datatracker.ietf.org/doc/html/rfc1925

See also the "Maintain backward compatibility" section
in https://github.com/rear/rear/wiki/Coding-Style

Preferably use simple generic functionality
that works on any Linux system.
Better very simple code than oversophisticated
(possibly fragile) constructs.

jsmeix commented at 2022-09-26 13:48:¶

I tested my latest commit
https://github.com/rear/rear/pull/2868/commits/5f28472c018c312f8c6ba2eeee0fa340ceffecaa
during "rear recover" with MIGRATION_MODE="true"
and it behaves perfectly well for me in practice, cf.
https://github.com/rear/rear/pull/2868#issuecomment-1257988466
what it is primarily meant to do in practice.

jsmeix commented at 2022-09-26 13:49:¶

@pcahyna @rear/contributors
if there are no objections I would like to merge it
tomorrow afternoon in its current state.
If later issues appear I will of course fix them.

pcahyna commented at 2022-09-26 16:51:¶

@jsmeix I agree with the reasoning that simple code covering the most problematic case is preferred. One question, why have you added -d ''? That was not in any of the previous attempts and experiments as far as I could see. According to the manual, "The first character of DELIM is used to terminate the input line, rather than newline. If DELIM is the empty string, 'read' will terminate a line when it reads a NUL character." So, are you counting on the fact that the NUL character should never appear in the input and therefore this allows the command to skip multiple ENTERs ?

jsmeix commented at 2022-09-27 07:09:¶

Yes, that's the intent behind -d ''.
I have that from
https://superuser.com/questions/276531/clear-stdin-before-reading
which reads (excerpt):

read -d '' -t 0.1 -n 10000

This reads multiple lines of inputs,
if the user inadvertently pressed enter multiple times

For me -d '' seems to also work with bash 3.x
but neither "man bash" nor "help read" tell about
what 'read' does when the delimiter is the empty string.
This is only described in
https://www.gnu.org/software/bash/manual/html_node/Bash-Builtins.html
which is currtently "for Bash, Version 5.2.", see
https://www.gnu.org/software/bash/manual/html_node/index.html

Previously I had tested -d '~' see
https://github.com/rear/rear/pull/2868#discussion_r978452551

What works for me even with bash 3.x is
using an artificial delimiter character
that is sufficiently unlikely
to be accidentally entered by the user:

# echo -e 'One\nTwo\nThree' | ( read -t 1 -d '~' -n 10 D ; read R1 ; read R2 ; echo D "$D" ; echo R1 "$R1" ; echo R2 "$R2" )
D One
Two
Th
R1 ree
R2

jsmeix commented at 2022-09-27 07:18:¶

I think I can avoid
the one second delay when there is no input
in a simple and fail safe way via

test -t 0 && read -t 0 && read -s -t 1 -n 1000 -d ''

When read -t 0 falsely results non zero exit code
but there is someting in stdin, then
draining stdin would be falsely skipped which is no big issue
because then UserInput() falls back to working as before and
normally there should be nothing accidentally pending in stdin.

When read -t 0 falsely results zero exit code
but there is nothing in stdin, then
the subsequent rear -t 1 times out after one second.

jsmeix commented at 2022-09-27 11:11:¶

read -t 0 may not work sufficiently reliably
when automated input is provided (here with bash 4.4.23):

# for i in $( seq 5 ) ; do echo -n $i ; done | ( read -t 0 && read -s -t 1 -n 100 -d '' ; echo -n "'$REPLY'" | od -a )
0000000   '   '
0000002

# for i in $( seq 5 ) ; do echo -n $i ; done | ( read -t 0 && read -s -t 1 -n 100 -d '' ; echo -n "'$REPLY'" | od -a )
0000000   '   '
0000002
bash: echo: write error: Broken pipe
bash: echo: write error: Broken pipe
bash: echo: write error: Broken pipe
bash: echo: write error: Broken pipe
bash: echo: write error: Broken pipe

I think this happens
when read -t 0 is called in the pipe consumer
before the pipe producer made actual output
and the "Broken pipe" happens when the pipe consumer
already finished so the 'echo' in the pipe producer
writes into a "Broken pipe".

With a sleep in the pipe consumer it works:

# for i in $( seq 5 ) ; do echo -n $i ; done | ( sleep 1 ; read -t 0 && read -s -t 1 -n 100 -d '' ; echo -n "'$REPLY'" | od -a )
0000000   '   1   2   3   4   5   '
0000007

I think in ReaR such a race condition cannot happen
because the stdin producer process
(usually the user's terminal emulation program)
is already running when the usr/sbin/rear process
gets started by the user on his termial.

When usr/sbin/rear is run where stdin is not a terminal
the test -t 0 won't let the 'read' calls happen.

When usr/sbin/rear is run by the user in whatever special
way where stdin is a terminal and somehow automated input
is fed with a little delay into usr/sbin/rear, then
the 'read' calls should happen late enough to work OK.

jsmeix commented at 2022-09-27 12:33:¶

I tested my latest commit
https://github.com/rear/rear/pull/2868/commits/c482b2bdc6cdad5be077026b01ce4e42fd5e2d53
during "rear recover" with MIGRATION_MODE="true"
and it behaves perfectly well for me.

jsmeix commented at 2022-09-27 12:35:¶

@pcahyna @rear/contributors
if there are no objections I would like to merge it
tomorrow afternoon in its last state which is

test -t 0 && read -t 0 && read -s -t 1 -n 1000 -d ''

If issues appear later I will of course fix them.

[Export of Github issue for rear/rear.]

#2868 PR merged: In the UserInput function drain stdin if stdin is a terminal¶