Re: [PATCH v4 5/6] test-terminal: introduce --no-stdin-pty

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Rubén

On 03/06/2024 21:38, Rubén Justo wrote:
> In 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
> 2015-08-04), t/test-terminal.perl learned to connect the child process'
> stdin to a pty.  It works well for what was intended: satisfying an
> `isatty(STDIN_FILENO)` check.
> 
> However, the fork introduced, that copies the stdin to the child
> process, does not always manage to send all the information.

I think the problem maybe to do with the use of File::Copy, not with the 
fork. The man page for the copy function says

     Note  that  passing  in  files  as  handles  instead  of  names may
     lead to loss of information on some operating systems; it is
     recommended that you use file names whenever possible.

Rather than adding a new flag to work around a bug in our script it 
might be better to try and fix the bug by using a loop that reads blocks 
of data from the source and writes them to the destination instead of 
calling copy.

Best Wishes

Phillip

> To illustrate this behaviour, we can use a function like this:
> 
>      f ()
>      {
>      	dd if=/dev/zero bs=1 count=10000 status=none |
>      	t/test-terminal.perl cat - 2>/dev/null |
>      	wc -c;
>      }
> 
> We do not obtain the expected results when executing this function
> 100 times:
> 
>      $ for i in $(seq 100); do f; done | sort | uniq -c
>           36 0
>            4 1
>           53 4095
>            7 4159
> 
> If we do the same with a version that does not redirect stdin, a version
> prior to 18d8c26930, the expected result is obtained:
> 
>      $ git checkout 18d8c26930~1
>      $ for i in $(seq 100); do f; done | sort | uniq -c
>          100 10000
> 
> In a subsequent commit, a new test is going to rely on test-terminate,
> and it does not require stdin to be connected to a terminal, but all
> piped data needs to be successfully transmitted to the child process.
> 
> To make this possible, add a new parameter "--no-stdin-pty" to allow
> disabling the stdin redirection though a pty.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   t/test-terminal.perl | 32 ++++++++++++++++++--------------
>   1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/t/test-terminal.perl b/t/test-terminal.perl
> index 3810e9bb43..85edc9e8b9 100755
> --- a/t/test-terminal.perl
> +++ b/t/test-terminal.perl
> @@ -12,10 +12,10 @@ sub start_child {
>   	if (not defined $pid) {
>   		die "fork failed: $!"
>   	} elsif ($pid == 0) {
> -		open STDIN, "<&", $in;
> +		open STDIN, "<&", $in if $in;
>   		open STDOUT, ">&", $out;
>   		open STDERR, ">&", $err;
> -		close $in;
> +		close $in if $in;
>   		close $out;
>   		exec(@$argv) or die "cannot exec '$argv->[0]': $!"
>   	}
> @@ -78,28 +78,32 @@ sub copy_stdio {
>   }
>   
>   if ($#ARGV < 1) {
> -	die "usage: test-terminal program args";
> +	die "usage: test-terminal [--no-stdin-pty] program args";
>   }
> +my $no_stdin_pty = $ARGV[0] eq '--no-stdin-pty';
> +shift @ARGV if $no_stdin_pty;
>   $ENV{TERM} = 'vt100';
> -my $parent_in = new IO::Pty;
> +my $parent_in = $no_stdin_pty ? undef : IO::Pty->new;
>   my $parent_out = new IO::Pty;
>   my $parent_err = new IO::Pty;
> -$parent_in->set_raw();
> +$parent_in->set_raw() if $parent_in;
>   $parent_out->set_raw();
>   $parent_err->set_raw();
> -$parent_in->slave->set_raw();
> +$parent_in->slave->set_raw() if $parent_in;
>   $parent_out->slave->set_raw();
>   $parent_err->slave->set_raw();
> -my $pid = start_child(\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave);
> -close $parent_in->slave;
> +my $pid = start_child(\@ARGV,$parent_in ? $parent_in->slave : undef, $parent_out->slave, $parent_err->slave);
> +close $parent_in->slave if $parent_in;
>   close $parent_out->slave;
>   close $parent_err->slave;
> -my $in_pid = copy_stdin($parent_in);
> +my $in_pid = $no_stdin_pty ? 0 : copy_stdin($parent_in);
>   copy_stdio($parent_out, $parent_err);
>   my $ret = finish_child($pid);
> -# If the child process terminates before our copy_stdin() process is able to
> -# write all of its data to $parent_in, the copy_stdin() process could stall.
> -# Send SIGTERM to it to ensure it terminates.
> -kill 'TERM', $in_pid;
> -finish_child($in_pid);
> +if ($in_pid) {
> +	# If the child process terminates before our copy_stdin() process is able to
> +	# write all of its data to $parent_in, the copy_stdin() process could stall.
> +	# Send SIGTERM to it to ensure it terminates.
> +	kill 'TERM', $in_pid;
> +	finish_child($in_pid);
> +}
>   exit($ret);