Re: bin/55531: [PATCH] script(1): Check if stdin is a terminal + other error handling

[Soumendra Ganguly <soumendra%tamu.edu@localhost>]

In case there was whitespace substitution in the previous message, the
attached file contains the updated patch. Thank you.

On 8/2/20, Soumendra Ganguly <soumendra%tamu.edu@localhost> wrote:
> --- src/usr.bin/script/script.c	2020-08-02 02:06:56.406214345 -0500
> +++ script.c	2020-08-02 04:46:28.843706209 -0500
> @@ -155,17 +155,28 @@
>  	if (pflg)
>  		playback(fscript);
>
> -	(void)tcgetattr(STDIN_FILENO, &tt);
> -	(void)ioctl(STDIN_FILENO, TIOCGWINSZ, &win);
> -	if (openpty(&master, &slave, NULL, &tt, &win) == -1)
> -		err(1, "openpty");
> +	isterm = isatty(STDIN_FILENO);
> +	if (isterm) {
> +		if (tcgetattr(STDIN_FILENO, &tt) == -1)
> +			err(1, "tcgetattr");
> +		if (ioctl(STDIN_FILENO, TIOCGWINSZ, &win) == -1)
> +			err(1, "ioctl");
> +		if (openpty(&master, &slave, NULL, &tt, &win) == -1)
> +			err(1, "openpty");
> +	} else {
> +		if (openpty(&master, &slave, NULL, NULL, NULL) == -1)
> +			err(1, "openpty");		
> +	}
>
>  	if (!quiet)
>  		(void)printf("Script started, output file is %s\n", fname);
> -	rtt = tt;
> -	cfmakeraw(&rtt);
> -	rtt.c_lflag &= ~ECHO;
> -	(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &rtt);
> +
> +	if (isterm) {
> +		rtt = tt;
> +		cfmakeraw(&rtt);
> +		rtt.c_lflag &= ~ECHO;
> +		(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &rtt);
> +	}
>
>  	(void)signal(SIGCHLD, finish);
>  	child = fork();
> @@ -301,7 +312,8 @@
>  		(void)fclose(fscript);
>  		(void)close(master);
>  	} else {
> -		(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &tt);
> +		if (isterm)
> +			(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &tt);
>  		if (!quiet)
>  			(void)printf("Script done, output file is %s\n", fname);
>  	}
> @@ -365,13 +377,13 @@
>  	if (!isterm)
>  		return;
>
> -	if (tcgetattr(STDOUT_FILENO, &tt) != 0)
> +	if (tcgetattr(STDOUT_FILENO, &tt) == -1)
>  		err(1, "tcgetattr");
>
>  	traw = tt;
>  	cfmakeraw(&traw);
>  	traw.c_lflag |= ISIG;
> -	if (tcsetattr(STDOUT_FILENO, TCSANOW, &traw) != 0)
> +	if (tcsetattr(STDOUT_FILENO, TCSANOW, &traw) == -1)
>  		err(1, "tcsetattr");
>  }
>
> @@ -396,8 +408,10 @@
>  	time_t tclock;
>  	int reg;
>
> +	isterm = 0;
> +
>  	if (fstat(fileno(fp), &pst) == -1)
> -		err(1, "fstat failed");	
> +		err(1, "fstat failed");
>
>  	reg = S_ISREG(pst.st_mode);
>
> @@ -431,6 +445,7 @@
>  			atexit(termreset);
>  			break;
>  		case 'e':
> +			termreset();
>  			if (!quiet)
>  				(void)printf("\nScript done on %s",
>  				    ctime(&tclock));
>
>
> On 8/2/20, Soumendra Ganguly <soumendra%tamu.edu@localhost> wrote:
>> Hypothesis: reintroduction of the extra termreset() makes it necessary
>> to have "isterm = 0" at the beginning of "playback()" so that if a
>> malformed recording file forces case 'e' before case 's', then
>> tcsetattr will not be called by termreset(). In that case, an updated
>> patch follows. Also, this patch removes an extra whitespace after
>> err(1, "fstat failed").
>>
>> On 8/2/20, Soumendra Ganguly <soumendra%tamu.edu@localhost> wrote:
>>> The following reply was made to PR bin/55531; it has been noted by
>>> GNATS.
>>>
>>> From: Soumendra Ganguly <soumendra%tamu.edu@localhost>
>>> To: gnats-bugs%netbsd.org@localhost
>>> Cc:
>>> Subject: Re: bin/55531: [PATCH] script(1): Check if stdin is a terminal
>>> +
>>>  other error handling
>>> Date: Sun, 2 Aug 2020 03:44:03 -0500
>>>
>>>  This follows #55529
>>>
>>>  On 8/2/20, gnats-admin%netbsd.org@localhost <gnats-admin%netbsd.org@localhost> wrote:
>>>  > Thank you very much for your problem report.
>>>  > It has the internal identification `bin/55531'.
>>>  > The individual assigned to look at your
>>>  > report is: bin-bug-people.
>>>  >
>>>  >>Category:       bin
>>>  >>Responsible:    bin-bug-people
>>>  >>Synopsis:       [PATCH] script(1): Check if stdin is a terminal +
>>> other
>>>  >> error handling
>>>  >>Arrival-Date:   Sun Aug 02 08:30:00 +0000 2020
>>>  >
>>>  >
>>>
>>>
>>
>

--- src/usr.bin/script/script.c	2020-08-02 02:06:56.406214345 -0500
+++ script.c	2020-08-02 04:46:28.843706209 -0500
@@ -155,17 +155,28 @@
 	if (pflg)
 		playback(fscript);
 
-	(void)tcgetattr(STDIN_FILENO, &tt);
-	(void)ioctl(STDIN_FILENO, TIOCGWINSZ, &win);
-	if (openpty(&master, &slave, NULL, &tt, &win) == -1)
-		err(1, "openpty");
+	isterm = isatty(STDIN_FILENO);
+	if (isterm) {
+		if (tcgetattr(STDIN_FILENO, &tt) == -1)
+			err(1, "tcgetattr");
+		if (ioctl(STDIN_FILENO, TIOCGWINSZ, &win) == -1)
+			err(1, "ioctl");
+		if (openpty(&master, &slave, NULL, &tt, &win) == -1)
+			err(1, "openpty");
+	} else {
+		if (openpty(&master, &slave, NULL, NULL, NULL) == -1)
+			err(1, "openpty");		
+	}
 
 	if (!quiet)
 		(void)printf("Script started, output file is %s\n", fname);
-	rtt = tt;
-	cfmakeraw(&rtt);
-	rtt.c_lflag &= ~ECHO;
-	(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &rtt);
+
+	if (isterm) {
+		rtt = tt;
+		cfmakeraw(&rtt);
+		rtt.c_lflag &= ~ECHO;
+		(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &rtt);
+	}
 
 	(void)signal(SIGCHLD, finish);
 	child = fork();
@@ -301,7 +312,8 @@
 		(void)fclose(fscript);
 		(void)close(master);
 	} else {
-		(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &tt);
+		if (isterm)
+			(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &tt);
 		if (!quiet)
 			(void)printf("Script done, output file is %s\n", fname);
 	}
@@ -365,13 +377,13 @@
 	if (!isterm)
 		return;
 
-	if (tcgetattr(STDOUT_FILENO, &tt) != 0)
+	if (tcgetattr(STDOUT_FILENO, &tt) == -1)
 		err(1, "tcgetattr");
 
 	traw = tt;
 	cfmakeraw(&traw);
 	traw.c_lflag |= ISIG;
-	if (tcsetattr(STDOUT_FILENO, TCSANOW, &traw) != 0)
+	if (tcsetattr(STDOUT_FILENO, TCSANOW, &traw) == -1)
 		err(1, "tcsetattr");
 }
 
@@ -396,8 +408,10 @@
 	time_t tclock;
 	int reg;
 
+	isterm = 0;
+
 	if (fstat(fileno(fp), &pst) == -1)
-		err(1, "fstat failed");	
+		err(1, "fstat failed");
 
 	reg = S_ISREG(pst.st_mode);
 
@@ -431,6 +445,7 @@
 			atexit(termreset);
 			break;
 		case 'e':
+			termreset();
 			if (!quiet)
 				(void)printf("\nScript done on %s",
 				    ctime(&tclock));