[PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes

[PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes

[Sami Kerola]

Hi,

As I mentioned in

http://comments.gmane.org/gmane.linux.utilities.util-linux-ng/10390

the previous patch set was not complete.  The script(1) had signals and
io-streams race that is now fixed with changes 0015 to 0018.

The patches below can be also found from my github repository branch
'script'.

https://github.com/kerolasa/lelux-utiliteetit/tree/script


Sami Kerola (22):
  chsh: remove function prototypes
  chfn, chsh: share illegal_passwd_chars() function
  chsh: use getline() to support arbitrarily long lines
  chsh: set few variables read-only and rename one of them
  chsh: allow user to set shell to /bin/sh if none is set
  chsh: clean up parse_argv()
  chsh: rewrite function interacting with user to get path to new shell
  chsh: simplify check_shell()
  chsh: fail get_shell_list() check when /etc/shells cannot be opened
  newgrp: simplify if else clauses
  newgrp: move shell determination closer where it is used
  newgrp: set function arguments read-only when possible
  script: remove function prototypes
  script: add struct script_control and remove global variables
=== end of earlier submission ===
  script: use signalfd() to catch signals
  script: use poll() rather than select()
  script: merge doinput() and output() functions to do_io()
  script: remove io vs signal race
  script: add 'Script started' line always to capture file
  script: move do_io() content to small functions
  script: replace strftime() workaround with CFLAGS = -Wno-format-y2k
  script: use correct input type, move comment, and so on

 login-utils/Makemodule.am |   4 +-
 login-utils/ch-common.c   |  34 ++
 login-utils/ch-common.h   |   6 +
 login-utils/chfn.c        |  16 +-
 login-utils/chsh.c        | 365 +++++++++----------
 login-utils/newgrp.c      |  25 +-
 term-utils/Makemodule.am  |   1 +
 term-utils/script.c       | 866 ++++++++++++++++++++--------------------------
 tests/ts/script/race      |   4 -
 9 files changed, 594 insertions(+), 727 deletions(-)
 create mode 100644 login-utils/ch-common.c
 create mode 100644 login-utils/ch-common.h

-- 
2.2.1

[PATCH 15/22] script: use signalfd() to catch signals

[Sami Kerola]

This is incomplete change.  Working command requires the subsequent
select() to poll() change as well.

Addresses: https://github.com/karelzak/util-linux/pull/62
CC: Wolfgang Richter <wolf@cs.cmu.edu>
CC: Ruediger Meier <ruediger.meier@ga-group.nl>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/script.c | 47 ++++++++++++++---------------------------------
 1 file changed, 14 insertions(+), 33 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index 8f59fe8..315c46b 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -63,6 +63,8 @@
 #include <stddef.h>
 #include <sys/wait.h>
 #include <poll.h>
+#include <sys/signalfd.h>
+#include <assert.h>
 
 #include "closestream.h"
 #include "nls.h"
@@ -106,8 +108,8 @@ struct script_control {
 	 isterm:1,		/* is child process running as terminal */
 	 resized:1,		/* has terminal been resized */
 	 die:1;			/* terminate program */
-	sigset_t block_mask;	/* signals that are blocked */
-	sigset_t unblock_mask;	/* original signal mask */
+	sigset_t sigset;	/* catch SIGCHLD and SIGWINCH with signalfd() */
+	int sigfd;		/* file descriptor for signalfd() */
 };
 struct script_control *gctl;	/* global control structure, used in signal handlers */
 
@@ -245,16 +247,13 @@ static void doinput(struct script_control *ctl)
 
 	FD_ZERO(&readfds);
 
-	/* block SIGCHLD */
-	sigprocmask(SIG_SETMASK, &ctl->block_mask, &ctl->unblock_mask);
-
 	while (!ctl->die) {
 		FD_SET(STDIN_FILENO, &readfds);
 
 		errno = 0;
 		/* wait for input or signal (including SIGCHLD) */
 		if ((cc = pselect(STDIN_FILENO + 1, &readfds, NULL, NULL, NULL,
-				  &ctl->unblock_mask)) > 0) {
+				  NULL)) > 0) {
 
 			if ((cc = read(STDIN_FILENO, ibuf, BUFSIZ)) > 0) {
 				if (write_all(ctl->master, ibuf, cc)) {
@@ -278,9 +277,6 @@ static void doinput(struct script_control *ctl)
 		}
 	}
 
-	/* unblock SIGCHLD */
-	sigprocmask(SIG_SETMASK, &ctl->unblock_mask, NULL);
-
 	/* To be sure that we don't miss any data */
 	wait_for_empty_fd(ctl, ctl->slave);
 	wait_for_empty_fd(ctl, ctl->master);
@@ -349,29 +345,24 @@ static void dooutput(struct script_control *ctl)
 	do {
 		if (ctl->die || errsv == EINTR) {
 			struct pollfd fds[] = {
+				{.fd = ctl->sigfd, .events = POLLIN | POLLERR | POLLHUP},
 				{.fd = ctl->master, .events = POLLIN}
 			};
 			if (poll(fds, 1, 50) <= 0)
 				break;
 		}
 
-		/* block SIGCHLD */
-		sigprocmask(SIG_SETMASK, &ctl->block_mask, &ctl->unblock_mask);
-
 		FD_SET(ctl->master, &readfds);
 		errno = 0;
 
 		/* wait for input or signal (including SIGCHLD) */
 		if ((cc = pselect(ctl->master + 1, &readfds, NULL, NULL, NULL,
-				  &ctl->unblock_mask)) > 0) {
+				  NULL)) > 0) {
 
 			cc = read(ctl->master, obuf, sizeof(obuf));
 		}
 		errsv = errno;
 
-		/* unblock SIGCHLD */
-		sigprocmask(SIG_SETMASK, &ctl->unblock_mask, NULL);
-
 		if (ctl->tflg)
 			gettimeofday(&tv, NULL);
 
@@ -562,7 +553,6 @@ int main(int argc, char **argv)
 		.master = -1,
 		0
 	};
-	struct sigaction sa;
 	int ch;
 
 	enum { FORCE_OPTION = CHAR_MAX + 1 };
@@ -650,19 +640,15 @@ int main(int argc, char **argv)
 #ifdef HAVE_LIBUTEMPTER
 	utempter_add_record(master, NULL);
 #endif
-	/* setup SIGCHLD handler */
-	sigemptyset(&sa.sa_mask);
-	sa.sa_flags = 0;
-	sa.sa_handler = sig_finish;
-	sigaction(SIGCHLD, &sa, NULL);
-
-	/* init mask for SIGCHLD */
-	sigprocmask(SIG_SETMASK, NULL, &ctl.block_mask);
-	sigaddset(&ctl.block_mask, SIGCHLD);
+	/* setup signal handler */
+	assert(sigemptyset(&ctl.sigset) == 0);
+	assert(sigaddset(&ctl.sigset, SIGCHLD) == 0);
+	assert(sigaddset(&ctl.sigset, SIGWINCH) == 0);
+	assert(sigprocmask(SIG_BLOCK, &ctl.sigset, NULL) == 0);
+	if ((ctl.sigfd = signalfd(-1, &ctl.sigset, 0)) < 0)
+		err(EXIT_FAILURE, _("cannot set signal handler"));
 
-	sigprocmask(SIG_SETMASK, &ctl.block_mask, &ctl.unblock_mask);
 	ctl.child = fork();
-	sigprocmask(SIG_SETMASK, &ctl.unblock_mask, NULL);
 
 	if (ctl.child < 0) {
 		warn(_("fork failed"));
@@ -670,9 +656,7 @@ int main(int argc, char **argv)
 	}
 	if (ctl.child == 0) {
 
-		sigprocmask(SIG_SETMASK, &ctl.block_mask, NULL);
 		ctl.subchild = ctl.child = fork();
-		sigprocmask(SIG_SETMASK, &ctl.unblock_mask, NULL);
 
 		if (ctl.child < 0) {
 			warn(_("fork failed"));
@@ -682,9 +666,6 @@ int main(int argc, char **argv)
 			dooutput(&ctl);
 		else
 			doshell(&ctl);
-	} else {
-		sa.sa_handler = resize;
-		sigaction(SIGWINCH, &sa, NULL);
 	}
 	doinput(&ctl);
 
-- 
2.2.1

[PATCH 16/22] script: use poll() rather than select()

[Sami Kerola]

Finalize the signalfd() change by adding file descriptors to poll() loop.

Addresses: https://github.com/karelzak/util-linux/pull/62
CC: Wolfgang Richter <wolf@cs.cmu.edu>
CC: Ruediger Meier <ruediger.meier@ga-group.nl>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/script.c | 202 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 114 insertions(+), 88 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index 315c46b..456355e 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -82,6 +82,8 @@
 
 #define DEFAULT_OUTPUT "typescript"
 
+enum { POLLFDS = 2 };
+
 struct script_control {
 	char *shell;		/* shell to be executed */
 	char *cflg;		/* command to be executed */
@@ -106,12 +108,10 @@ struct script_control {
 	 tflg:1,		/* include timing file */
 	 forceflg:1,		/* write output to links */
 	 isterm:1,		/* is child process running as terminal */
-	 resized:1,		/* has terminal been resized */
 	 die:1;			/* terminate program */
 	sigset_t sigset;	/* catch SIGCHLD and SIGWINCH with signalfd() */
 	int sigfd;		/* file descriptor for signalfd() */
 };
-struct script_control *gctl;	/* global control structure, used in signal handlers */
 
 static void __attribute__((__noreturn__)) usage(FILE *out)
 {
@@ -233,10 +233,10 @@ static void finish(struct script_control *ctl, int wait)
 
 static void doinput(struct script_control *ctl)
 {
-	int errsv = 0;
-	ssize_t cc = 0;
 	char ibuf[BUFSIZ];
-	fd_set readfds;
+	struct pollfd pfd[POLLFDS];
+	int ret, i;
+	ssize_t bytes;
 
 	/* close things irrelevant for this process */
 	if (ctl->typescriptfp)
@@ -245,35 +245,49 @@ static void doinput(struct script_control *ctl)
 		fclose(ctl->timingfp);
 	ctl->typescriptfp = ctl->timingfp = NULL;
 
-	FD_ZERO(&readfds);
+	pfd[0].fd = STDIN_FILENO;
+	pfd[0].events = POLLIN;
+	pfd[1].fd = ctl->sigfd;
+	pfd[1].events = POLLIN | POLLERR | POLLHUP;
 
 	while (!ctl->die) {
-		FD_SET(STDIN_FILENO, &readfds);
-
-		errno = 0;
-		/* wait for input or signal (including SIGCHLD) */
-		if ((cc = pselect(STDIN_FILENO + 1, &readfds, NULL, NULL, NULL,
-				  NULL)) > 0) {
-
-			if ((cc = read(STDIN_FILENO, ibuf, BUFSIZ)) > 0) {
-				if (write_all(ctl->master, ibuf, cc)) {
+		/* wait for input or signal */
+		ret = poll(pfd, POLLFDS, -1);
+		if (ret < 0) {
+			if (errno == EAGAIN)
+				continue;
+			warn(_("poll failed"));
+			fail(ctl);
+		}
+		for (i = 0; i < POLLFDS; i++) {
+			if (pfd[i].revents == 0)
+				continue;
+			if (i == 0 && (bytes = read(pfd[i].fd, ibuf, BUFSIZ)) > 0) {
+				if (write_all(ctl->master, ibuf, bytes)) {
 					warn(_("write failed"));
 					fail(ctl);
 				}
 			}
-		}
-
-		if (cc < 0 && errno == EINTR && ctl->resized) {
-			/* transmit window change information to the child */
-			if (ctl->isterm) {
-				ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&ctl->win);
-				ioctl(ctl->slave, TIOCSWINSZ, (char *)&ctl->win);
+			if (i == 1) {
+				struct signalfd_siginfo info;
+				ssize_t bytes;
+
+				bytes = read(pfd[i].fd, &info, sizeof(info));
+				assert(bytes == sizeof(info));
+				switch (info.ssi_signo) {
+				case SIGCHLD:
+					finish(ctl, 0);
+					break;
+				case SIGWINCH:
+					if (ctl->isterm) {
+						ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&ctl->win);
+						ioctl(ctl->slave, TIOCSWINSZ, (char *)&ctl->win);
+					}
+					break;
+				default:
+					abort();
+				}
 			}
-			ctl->resized = 0;
-
-		} else if (cc <= 0 && errno != EINTR) {
-			errsv = errno;
-			break;
 		}
 	}
 
@@ -281,7 +295,7 @@ static void doinput(struct script_control *ctl)
 	wait_for_empty_fd(ctl, ctl->slave);
 	wait_for_empty_fd(ctl, ctl->master);
 
-	if (ctl->die == 0 && cc == 0 && errsv == 0) {
+	if (ctl->die == 0) {
 		/*
 		 * Forward EOF from stdin (detected by read() above) to slave
 		 * (shell) to correctly terminate the session. It seems we have
@@ -308,24 +322,41 @@ static void doinput(struct script_control *ctl)
 	done(ctl);
 }
 
-static void sig_finish(int dummy __attribute__((__unused__)))
+static void write_output(struct script_control *ctl, char *obuf,
+			    ssize_t bytes, double *oldtime)
 {
-	finish(gctl, 0);
-}
 
-static void resize(int dummy __attribute__((__unused__)))
-{
-	gctl->resized = 1;
+	if (ctl->tflg && ctl->timingfp) {
+		struct timeval tv;
+		double newtime;
+
+		gettimeofday(&tv, NULL);
+		newtime = tv.tv_sec + (double)tv.tv_usec / 1000000;
+		fprintf(ctl->timingfp, "%f %zd\n", newtime - *oldtime, bytes);
+		if (ctl->fflg)
+			fflush(ctl->timingfp);
+		*oldtime = newtime;
+	}
+	if (fwrite_all(obuf, 1, bytes, ctl->typescriptfp)) {
+		warn(_("cannot write script file"));
+		fail(ctl);
+	}
+	if (ctl->fflg)
+		fflush(ctl->typescriptfp);
+	if (write_all(STDOUT_FILENO, obuf, bytes)) {
+		warn(_("write failed"));
+		fail(ctl);
+	}
 }
 
+
 static void dooutput(struct script_control *ctl)
 {
-	ssize_t cc;
 	char obuf[BUFSIZ];
-	struct timeval tv;
-	double oldtime = time(NULL), newtime;
-	int errsv = 0;
-	fd_set readfds;
+	struct pollfd pfd[POLLFDS];
+	int ret, i;
+	ssize_t bytes;
+	double oldtime = time(NULL);
 
 	close(STDIN_FILENO);
 #ifdef HAVE_LIBUTIL
@@ -340,57 +371,53 @@ static void dooutput(struct script_control *ctl)
 		fprintf(ctl->typescriptfp, _("Script started on %s"), obuf);
 	}
 
-	FD_ZERO(&readfds);
-
-	do {
-		if (ctl->die || errsv == EINTR) {
-			struct pollfd fds[] = {
-				{.fd = ctl->sigfd, .events = POLLIN | POLLERR | POLLHUP},
-				{.fd = ctl->master, .events = POLLIN}
-			};
-			if (poll(fds, 1, 50) <= 0)
-				break;
-		}
-
-		FD_SET(ctl->master, &readfds);
-		errno = 0;
-
-		/* wait for input or signal (including SIGCHLD) */
-		if ((cc = pselect(ctl->master + 1, &readfds, NULL, NULL, NULL,
-				  NULL)) > 0) {
-
-			cc = read(ctl->master, obuf, sizeof(obuf));
-		}
-		errsv = errno;
-
-		if (ctl->tflg)
-			gettimeofday(&tv, NULL);
-
-		if (errsv == EINTR && cc <= 0)
-			continue;	/* try it again */
-		if (cc <= 0)
-			break;
-		if (ctl->tflg && ctl->timingfp) {
-			newtime = tv.tv_sec + (double)tv.tv_usec / 1000000;
-			fprintf(ctl->timingfp, "%f %zd\n", newtime - oldtime, cc);
-			oldtime = newtime;
-		}
-		if (fwrite_all(obuf, 1, cc, ctl->typescriptfp)) {
-			warn(_("cannot write script file"));
+	pfd[0].fd = ctl->master;
+	pfd[0].events = POLLIN;
+	pfd[1].fd = ctl->sigfd;
+	pfd[1].events = POLLIN | POLLERR | POLLHUP;
+
+	while (1) {
+		/* wait for input or signal */
+		ret = poll(pfd, POLLFDS, -1);
+		if (ret < 0) {
+			if (errno == EAGAIN)
+				continue;
+			warn(_("poll failed"));
 			fail(ctl);
 		}
-		if (ctl->fflg) {
-			fflush(ctl->typescriptfp);
-			if (ctl->tflg && ctl->timingfp)
-				fflush(ctl->timingfp);
-		}
-		if (write_all(STDOUT_FILENO, obuf, cc)) {
-			warn(_("write failed"));
-			fail(ctl);
+		for (i = 0; i < POLLFDS; i++) {
+			if (pfd[i].revents == 0)
+				continue;
+			if (i == 0) {
+				bytes = read(pfd[i].fd, obuf, BUFSIZ);
+				if (bytes < 0) {
+					if (errno == EAGAIN)
+						continue;
+					fail(ctl);
+				}
+				write_output(ctl, obuf, bytes, &oldtime);
+				continue;
+			}
+			if (i == 1) {
+				struct signalfd_siginfo info;
+				ssize_t bytes;
+
+				bytes = read(pfd[i].fd, &info, sizeof(info));
+				assert(bytes == sizeof(info));
+				switch (info.ssi_signo) {
+				case SIGCHLD:
+					done(ctl);
+					break;
+				case SIGWINCH:
+					/* nothing */
+					break;
+				default:
+					abort();
+				}
+			}
 		}
-	} while (1);
-
-	done(ctl);
+	}
+	abort();
 }
 
 static void getslave(struct script_control *ctl)
@@ -575,7 +602,6 @@ int main(int argc, char **argv)
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 	atexit(close_stdout);
-	gctl = &ctl;
 
 	while ((ch = getopt_long(argc, argv, "ac:efqt::Vh", longopts, NULL)) != -1)
 		switch (ch) {
-- 
2.2.1

[PATCH 17/22] script: merge doinput() and output() functions to do_io()

[Sami Kerola]

One item to poll() more is a lot less work for system than separete input
and output processes.

Addresses: https://github.com/karelzak/util-linux/pull/62
CC: Wolfgang Richter <wolf@cs.cmu.edu>
CC: Ruediger Meier <ruediger.meier@ga-group.nl>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/script.c | 220 ++++++++++++----------------------------------------
 1 file changed, 51 insertions(+), 169 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index 456355e..4a761c6 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -82,7 +82,7 @@
 
 #define DEFAULT_OUTPUT "typescript"
 
-enum { POLLFDS = 2 };
+enum { POLLFDS = 3 };
 
 struct script_control {
 	char *shell;		/* shell to be executed */
@@ -93,7 +93,6 @@ struct script_control {
 	int master;		/* pseudoterminal master file descriptor */
 	int slave;		/* pseudoterminal slave file descriptor */
 	pid_t child;		/* child pid */
-	pid_t subchild;		/* subchild pid */
 	int childstatus;	/* child process exit value */
 	struct termios tt;	/* slave terminal runtime attributes */
 	struct winsize win;	/* terminal window size */
@@ -157,39 +156,15 @@ static void my_strftime(char *buf, size_t len, const char *fmt, const struct tm
 
 static void __attribute__((__noreturn__)) done(struct script_control *ctl)
 {
-	time_t tvec;
-
-	if (ctl->subchild) {
-		/* output process */
-		if (ctl->typescriptfp) {
-			if (!ctl->qflg) {
-				char buf[BUFSIZ];
-				tvec = time((time_t *)NULL);
-				my_strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
-				fprintf(ctl->typescriptfp, _("\nScript done on %s"), buf);
-			}
-			if (close_stream(ctl->typescriptfp) != 0)
-				errx(EXIT_FAILURE, _("write error"));
-			ctl->typescriptfp = NULL;
-		}
-		if (ctl->timingfp && close_stream(ctl->timingfp) != 0)
-			errx(EXIT_FAILURE, _("write error"));
-		ctl->timingfp = NULL;
-
-		close(ctl->master);
-		ctl->master = -1;
-	} else {
-		/* input process */
-		if (ctl->isterm)
-			tcsetattr(STDIN_FILENO, TCSADRAIN, &ctl->tt);
-		if (!ctl->qflg)
-			printf(_("Script done, file is %s\n"), ctl->fname);
+	if (ctl->isterm)
+		tcsetattr(STDIN_FILENO, TCSADRAIN, &ctl->tt);
+	if (!ctl->qflg)
+		printf(_("Script done, file is %s\n"), ctl->fname);
 #ifdef HAVE_LIBUTEMPTER
-		if (ctl->master >= 0)
-			utempter_remove_record(ctl->master);
+	if (ctl->master >= 0)
+		utempter_remove_record(ctl->master);
 #endif
-		kill(ctl->child, SIGTERM);	/* make sure we don't create orphans */
-	}
+	kill(ctl->child, SIGTERM);	/* make sure we don't create orphans */
 
 	if (ctl->eflg) {
 		if (WIFSIGNALED(ctl->childstatus))
@@ -206,15 +181,6 @@ static void fail(struct script_control *ctl)
 	done(ctl);
 }
 
-static void wait_for_empty_fd(struct script_control *ctl, int fd)
-{
-	struct pollfd fds[] = {
-		{.fd = fd, .events = POLLIN}
-	};
-
-	while (ctl->die == 0 && poll(fds, 1, 100) == 1) ;
-}
-
 static void finish(struct script_control *ctl, int wait)
 {
 	int status;
@@ -231,97 +197,6 @@ static void finish(struct script_control *ctl, int wait)
 	errno = errsv;
 }
 
-static void doinput(struct script_control *ctl)
-{
-	char ibuf[BUFSIZ];
-	struct pollfd pfd[POLLFDS];
-	int ret, i;
-	ssize_t bytes;
-
-	/* close things irrelevant for this process */
-	if (ctl->typescriptfp)
-		fclose(ctl->typescriptfp);
-	if (ctl->timingfp)
-		fclose(ctl->timingfp);
-	ctl->typescriptfp = ctl->timingfp = NULL;
-
-	pfd[0].fd = STDIN_FILENO;
-	pfd[0].events = POLLIN;
-	pfd[1].fd = ctl->sigfd;
-	pfd[1].events = POLLIN | POLLERR | POLLHUP;
-
-	while (!ctl->die) {
-		/* wait for input or signal */
-		ret = poll(pfd, POLLFDS, -1);
-		if (ret < 0) {
-			if (errno == EAGAIN)
-				continue;
-			warn(_("poll failed"));
-			fail(ctl);
-		}
-		for (i = 0; i < POLLFDS; i++) {
-			if (pfd[i].revents == 0)
-				continue;
-			if (i == 0 && (bytes = read(pfd[i].fd, ibuf, BUFSIZ)) > 0) {
-				if (write_all(ctl->master, ibuf, bytes)) {
-					warn(_("write failed"));
-					fail(ctl);
-				}
-			}
-			if (i == 1) {
-				struct signalfd_siginfo info;
-				ssize_t bytes;
-
-				bytes = read(pfd[i].fd, &info, sizeof(info));
-				assert(bytes == sizeof(info));
-				switch (info.ssi_signo) {
-				case SIGCHLD:
-					finish(ctl, 0);
-					break;
-				case SIGWINCH:
-					if (ctl->isterm) {
-						ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&ctl->win);
-						ioctl(ctl->slave, TIOCSWINSZ, (char *)&ctl->win);
-					}
-					break;
-				default:
-					abort();
-				}
-			}
-		}
-	}
-
-	/* To be sure that we don't miss any data */
-	wait_for_empty_fd(ctl, ctl->slave);
-	wait_for_empty_fd(ctl, ctl->master);
-
-	if (ctl->die == 0) {
-		/*
-		 * Forward EOF from stdin (detected by read() above) to slave
-		 * (shell) to correctly terminate the session. It seems we have
-		 * to wait for empty terminal FDs otherwise EOF maybe ignored
-		 * (why?) and typescript is incomplete.      -- kzak Dec-2013
-		 *
-		 * We usually use this when stdin is not a tty, for example:
-		 * echo "ps" | script
-		 */
-		int c = DEF_EOF;
-
-		if (write_all(ctl->master, &c, 1)) {
-			warn(_("write failed"));
-			fail(ctl);
-		}
-
-		/* wait for "exit" message from shell before we print "Script
-		 * done" in done() */
-		wait_for_empty_fd(ctl, ctl->master);
-	}
-
-	if (!ctl->die)
-		finish(ctl, 1);	/* wait for children */
-	done(ctl);
-}
-
 static void write_output(struct script_control *ctl, char *obuf,
 			    ssize_t bytes, double *oldtime)
 {
@@ -349,34 +224,31 @@ static void write_output(struct script_control *ctl, char *obuf,
 	}
 }
 
-
-static void dooutput(struct script_control *ctl)
+static void do_io(struct script_control *ctl)
 {
-	char obuf[BUFSIZ];
+	char buf[BUFSIZ];
 	struct pollfd pfd[POLLFDS];
 	int ret, i;
 	ssize_t bytes;
 	double oldtime = time(NULL);
 
-	close(STDIN_FILENO);
-#ifdef HAVE_LIBUTIL
-	close(ctl->slave);
-#endif
 	if (ctl->tflg && !ctl->timingfp)
 		ctl->timingfp = fdopen(STDERR_FILENO, "w");
 
+	pfd[0].fd = STDIN_FILENO;
+	pfd[0].events = POLLIN;
+	pfd[1].fd = ctl->master;
+	pfd[1].events = POLLIN;
+	pfd[2].fd = ctl->sigfd;
+	pfd[2].events = POLLIN | POLLERR | POLLHUP;
+
 	if (!ctl->qflg) {
 		time_t tvec = time((time_t *)NULL);
-		my_strftime(obuf, sizeof obuf, "%c\n", localtime(&tvec));
-		fprintf(ctl->typescriptfp, _("Script started on %s"), obuf);
+		my_strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
+		fprintf(ctl->typescriptfp, _("Script started on %s"), buf);
 	}
 
-	pfd[0].fd = ctl->master;
-	pfd[0].events = POLLIN;
-	pfd[1].fd = ctl->sigfd;
-	pfd[1].events = POLLIN | POLLERR | POLLHUP;
-
-	while (1) {
+	while (!ctl->die) {
 		/* wait for input or signal */
 		ret = poll(pfd, POLLFDS, -1);
 		if (ret < 0) {
@@ -388,17 +260,33 @@ static void dooutput(struct script_control *ctl)
 		for (i = 0; i < POLLFDS; i++) {
 			if (pfd[i].revents == 0)
 				continue;
-			if (i == 0) {
-				bytes = read(pfd[i].fd, obuf, BUFSIZ);
+			if (i < 2) {
+				bytes = read(pfd[i].fd, buf, BUFSIZ);
 				if (bytes < 0) {
 					if (errno == EAGAIN)
 						continue;
 					fail(ctl);
 				}
-				write_output(ctl, obuf, bytes, &oldtime);
+				if (i == 0) {
+					if (write_all(ctl->master, buf, bytes)) {
+						warn(_("write failed"));
+						fail(ctl);
+					} else
+
+						/* without sync write_output()
+						 * will write both input &
+						 * shell output that looks like
+						 * double echoing */
+						fdatasync(ctl->master);
+					if (!ctl->isterm && !feof(stdin)) {
+						int c = DEF_EOF;
+						write_all(ctl->master, &c, 1);
+					}
+				} else
+					write_output(ctl, buf, bytes, &oldtime);
 				continue;
 			}
-			if (i == 1) {
+			if (i == 2) {
 				struct signalfd_siginfo info;
 				ssize_t bytes;
 
@@ -406,10 +294,13 @@ static void dooutput(struct script_control *ctl)
 				assert(bytes == sizeof(info));
 				switch (info.ssi_signo) {
 				case SIGCHLD:
-					done(ctl);
+					finish(ctl, 0);
 					break;
 				case SIGWINCH:
-					/* nothing */
+					if (ctl->isterm) {
+						ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&ctl->win);
+						ioctl(ctl->slave, TIOCSWINSZ, (char *)&ctl->win);
+					}
 					break;
 				default:
 					abort();
@@ -417,7 +308,9 @@ static void dooutput(struct script_control *ctl)
 			}
 		}
 	}
-	abort();
+	if (!ctl->die)
+		finish(ctl, 1); /* wait for children */
+	done(ctl);
 }
 
 static void getslave(struct script_control *ctl)
@@ -680,20 +573,9 @@ int main(int argc, char **argv)
 		warn(_("fork failed"));
 		fail(&ctl);
 	}
-	if (ctl.child == 0) {
-
-		ctl.subchild = ctl.child = fork();
-
-		if (ctl.child < 0) {
-			warn(_("fork failed"));
-			fail(&ctl);
-		}
-		if (ctl.child)
-			dooutput(&ctl);
-		else
-			doshell(&ctl);
-	}
-	doinput(&ctl);
+	if (ctl.child == 0)
+		doshell(&ctl);
+	do_io(&ctl);
 
 	return EXIT_SUCCESS;
 }
-- 
2.2.1

[PATCH 18/22] script: remove io vs signal race

[Sami Kerola]

Make do_io() to run poll() until all streams are empty.  This should
remove the signal from child versus io handling race for good.

Addresses: https://github.com/karelzak/util-linux/pull/62
CC: Wolfgang Richter <wolf@cs.cmu.edu>
CC: Ruediger Meier <ruediger.meier@ga-group.nl>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/script.c  | 20 +++++++++++++-------
 tests/ts/script/race |  4 ----
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index 4a761c6..2def44a 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -92,6 +92,7 @@ struct script_control {
 	FILE *timingfp;		/* timing file pointer */
 	int master;		/* pseudoterminal master file descriptor */
 	int slave;		/* pseudoterminal slave file descriptor */
+	int poll_timeout;	/* poll() timeout, used in end of execution */
 	pid_t child;		/* child pid */
 	int childstatus;	/* child process exit value */
 	struct termios tt;	/* slave terminal runtime attributes */
@@ -185,16 +186,11 @@ static void finish(struct script_control *ctl, int wait)
 {
 	int status;
 	pid_t pid;
-	int errsv = errno;
 	int options = wait ? 0 : WNOHANG;
 
 	while ((pid = wait3(&status, options, 0)) > 0)
-		if (pid == ctl->child) {
+		if (pid == ctl->child)
 			ctl->childstatus = status;
-			ctl->die = 1;
-		}
-
-	errno = errsv;
 }
 
 static void write_output(struct script_control *ctl, char *obuf,
@@ -250,13 +246,15 @@ static void do_io(struct script_control *ctl)
 
 	while (!ctl->die) {
 		/* wait for input or signal */
-		ret = poll(pfd, POLLFDS, -1);
+		ret = poll(pfd, POLLFDS, ctl->poll_timeout);
 		if (ret < 0) {
 			if (errno == EAGAIN)
 				continue;
 			warn(_("poll failed"));
 			fail(ctl);
 		}
+		if (ret == 0)
+			ctl->die = 1;
 		for (i = 0; i < POLLFDS; i++) {
 			if (pfd[i].revents == 0)
 				continue;
@@ -295,6 +293,13 @@ static void do_io(struct script_control *ctl)
 				switch (info.ssi_signo) {
 				case SIGCHLD:
 					finish(ctl, 0);
+					ctl->poll_timeout = 10;
+					if (!ctl->isterm)
+						/* In situation such as 'date' in
+						* $ echo date | ./script
+						* ignore input when shell has
+						* exited.  */
+						pfd[0].fd = -1;
 					break;
 				case SIGWINCH:
 					if (ctl->isterm) {
@@ -471,6 +476,7 @@ int main(int argc, char **argv)
 		.line = "/dev/ptyXX",
 #endif
 		.master = -1,
+		.poll_timeout = -1,
 		0
 	};
 	int ch;
diff --git a/tests/ts/script/race b/tests/ts/script/race
index c947402..103a14a 100755
--- a/tests/ts/script/race
+++ b/tests/ts/script/race
@@ -24,10 +24,6 @@ ts_init "$*"
 
 ts_check_test_command "$TS_CMD_SCRIPT"
 
-# TODO see comments about script design
-# https://github.com/karelzak/util-linux/pull/62
-TS_KNOWN_FAIL="yes"
-
 bingofile="$TS_OUTDIR/${TS_TESTNAME}-bingo"
 
 set -o pipefail
-- 
2.2.1

[PATCH 19/22] script: add 'Script started' line always to capture file

[Sami Kerola]

The scriptreplay(1) will expect the capture file to have a header that is
omited.  Before this change the --quiet option together with timing
caused following replay error.

$ script --quiet --timing=timing
[...]
$ scriptreplay timing typescript
[...]
scriptreplay: unexpected end of file on typescript

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/script.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index 2def44a..6e11fa3 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -227,6 +227,7 @@ static void do_io(struct script_control *ctl)
 	int ret, i;
 	ssize_t bytes;
 	double oldtime = time(NULL);
+	time_t tvec = time((time_t *)NULL);
 
 	if (ctl->tflg && !ctl->timingfp)
 		ctl->timingfp = fdopen(STDERR_FILENO, "w");
@@ -238,11 +239,8 @@ static void do_io(struct script_control *ctl)
 	pfd[2].fd = ctl->sigfd;
 	pfd[2].events = POLLIN | POLLERR | POLLHUP;
 
-	if (!ctl->qflg) {
-		time_t tvec = time((time_t *)NULL);
-		my_strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
-		fprintf(ctl->typescriptfp, _("Script started on %s"), buf);
-	}
+	my_strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
+	fprintf(ctl->typescriptfp, _("Script started on %s"), buf);
 
 	while (!ctl->die) {
 		/* wait for input or signal */
-- 
2.2.1

[PATCH 20/22] script: move do_io() content to small functions

[Sami Kerola]

The do_io() got to be a bit long with relatively deep indentation.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/script.c | 108 ++++++++++++++++++++++++++++------------------------
 1 file changed, 58 insertions(+), 50 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index 6e11fa3..c1a9f5d 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -220,14 +220,63 @@ static void write_output(struct script_control *ctl, char *obuf,
 	}
 }
 
-static void do_io(struct script_control *ctl)
+static void handle_io(struct script_control *ctl, int fd, double *oldtime, int i)
 {
 	char buf[BUFSIZ];
+	ssize_t bytes;
+
+	bytes = read(fd, buf, sizeof(buf));
+	if (bytes < 0) {
+		if (errno == EAGAIN)
+			return;
+		fail(ctl);
+	}
+	if (i == 0) {
+		if (write_all(ctl->master, buf, bytes)) {
+			warn(_("write failed"));
+			fail(ctl);
+		} else
+			/* without sync write_output() will write both input &
+			 * shell output that looks like double echoing */
+			fdatasync(ctl->master);
+		if (!ctl->isterm && !feof(stdin)) {
+			int c = DEF_EOF;
+			write_all(ctl->master, &c, 1);
+		}
+	} else
+		write_output(ctl, buf, bytes, oldtime);
+}
+
+static void handle_signal(struct script_control *ctl, int fd)
+{
+	struct signalfd_siginfo info;
+	ssize_t bytes;
+
+	bytes = read(fd, &info, sizeof(info));
+	assert(bytes == sizeof(info));
+	switch (info.ssi_signo) {
+	case SIGCHLD:
+		finish(ctl, 0);
+		ctl->poll_timeout = 10;
+		return;
+	case SIGWINCH:
+		if (ctl->isterm) {
+			ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&ctl->win);
+			ioctl(ctl->slave, TIOCSWINSZ, (char *)&ctl->win);
+		}
+		break;
+	default:
+		abort();
+	}
+}
+
+static void do_io(struct script_control *ctl)
+{
 	struct pollfd pfd[POLLFDS];
 	int ret, i;
-	ssize_t bytes;
 	double oldtime = time(NULL);
 	time_t tvec = time((time_t *)NULL);
+	char buf[128];
 
 	if (ctl->tflg && !ctl->timingfp)
 		ctl->timingfp = fdopen(STDERR_FILENO, "w");
@@ -257,57 +306,16 @@ static void do_io(struct script_control *ctl)
 			if (pfd[i].revents == 0)
 				continue;
 			if (i < 2) {
-				bytes = read(pfd[i].fd, buf, BUFSIZ);
-				if (bytes < 0) {
-					if (errno == EAGAIN)
-						continue;
-					fail(ctl);
-				}
-				if (i == 0) {
-					if (write_all(ctl->master, buf, bytes)) {
-						warn(_("write failed"));
-						fail(ctl);
-					} else
-
-						/* without sync write_output()
-						 * will write both input &
-						 * shell output that looks like
-						 * double echoing */
-						fdatasync(ctl->master);
-					if (!ctl->isterm && !feof(stdin)) {
-						int c = DEF_EOF;
-						write_all(ctl->master, &c, 1);
-					}
-				} else
-					write_output(ctl, buf, bytes, &oldtime);
+				handle_io(ctl, pfd[i].fd, &oldtime, i);
 				continue;
 			}
 			if (i == 2) {
-				struct signalfd_siginfo info;
-				ssize_t bytes;
-
-				bytes = read(pfd[i].fd, &info, sizeof(info));
-				assert(bytes == sizeof(info));
-				switch (info.ssi_signo) {
-				case SIGCHLD:
-					finish(ctl, 0);
-					ctl->poll_timeout = 10;
-					if (!ctl->isterm)
-						/* In situation such as 'date' in
-						* $ echo date | ./script
-						* ignore input when shell has
-						* exited.  */
-						pfd[0].fd = -1;
-					break;
-				case SIGWINCH:
-					if (ctl->isterm) {
-						ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&ctl->win);
-						ioctl(ctl->slave, TIOCSWINSZ, (char *)&ctl->win);
-					}
-					break;
-				default:
-					abort();
-				}
+				handle_signal(ctl, pfd[i].fd);
+				if (!ctl->isterm && -1 < ctl->poll_timeout)
+					/* In situation such as 'date' in
+					* $ echo date | ./script
+					* ignore input when shell has exited.  */
+					pfd[0].fd = -1;
 			}
 		}
 	}
-- 
2.2.1

[PATCH 21/22] script: replace strftime() workaround with CFLAGS = -Wno-format-y2k

[Sami Kerola]

Nowadays, gcc(1) provides the -Wno-format-y2k option to prevent the
warning, so that the above workaround is no longer required.

Reference: http://man7.org/linux/man-pages/man3/strftime.3.html
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/Makemodule.am |  1 +
 term-utils/script.c      | 11 +----------
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/term-utils/Makemodule.am b/term-utils/Makemodule.am
index e7ac707..2bf916c 100644
--- a/term-utils/Makemodule.am
+++ b/term-utils/Makemodule.am
@@ -2,6 +2,7 @@ if BUILD_SCRIPT
 usrbin_exec_PROGRAMS += script
 dist_man_MANS += term-utils/script.1
 script_SOURCES = term-utils/script.c
+script_CFLAGS = $(AM_CFLAGS) -Wno-format-y2k
 script_LDADD = $(LDADD)
 if HAVE_UTIL
 script_LDADD += -lutil
diff --git a/term-utils/script.c b/term-utils/script.c
index c1a9f5d..7a992da 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -146,15 +146,6 @@ static void die_if_link(const struct script_control *ctl)
 		       "Program not started."), ctl->fname);
 }
 
-/*
- * Stop extremely silly gcc complaint on %c:
- *  warning: `%c' yields only last 2 digits of year in some locales
- */
-static void my_strftime(char *buf, size_t len, const char *fmt, const struct tm *tm)
-{
-	strftime(buf, len, fmt, tm);
-}
-
 static void __attribute__((__noreturn__)) done(struct script_control *ctl)
 {
 	if (ctl->isterm)
@@ -288,7 +279,7 @@ static void do_io(struct script_control *ctl)
 	pfd[2].fd = ctl->sigfd;
 	pfd[2].events = POLLIN | POLLERR | POLLHUP;
 
-	my_strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
+	strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
 	fprintf(ctl->typescriptfp, _("Script started on %s"), buf);
 
 	while (!ctl->die) {
-- 
2.2.1

[PATCH 22/22] script: use correct input type, move comment, and so on

[Sami Kerola]

Minor corrections.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/script.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index 7a992da..c784f8a 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -179,7 +179,7 @@ static void finish(struct script_control *ctl, int wait)
 	pid_t pid;
 	int options = wait ? 0 : WNOHANG;
 
-	while ((pid = wait3(&status, options, 0)) > 0)
+	while ((pid = wait3(&status, options, NULL)) > 0)
 		if (pid == ctl->child)
 			ctl->childstatus = status;
 }
@@ -458,14 +458,6 @@ static void getmaster(struct script_control *ctl)
 #endif				/* not HAVE_LIBUTIL */
 }
 
-/*
- * script -t prints time delays as floating point numbers
- * The example program (scriptreplay) that we provide to handle this
- * timing output is a perl script, and does not handle numbers in
- * locale format (not even when "use locale;" is added).
- * So, since these numbers are not for human consumption, it seems
- * easiest to set LC_NUMERIC here.
- */
 int main(int argc, char **argv)
 {
 	struct script_control ctl = {
@@ -494,7 +486,14 @@ int main(int argc, char **argv)
 	};
 
 	setlocale(LC_ALL, "");
-	setlocale(LC_NUMERIC, "C");	/* see comment above */
+	/*
+	 * script -t prints time delays as floating point numbers.  The example
+	 * program (scriptreplay) that we provide to handle this timing output
+	 * is a perl script, and does not handle numbers in locale format (not
+	 * even when "use locale;" is added).  So, since these numbers are not
+	 * for human consumption, it seems easiest to set LC_NUMERIC here.
+	 */
+	setlocale(LC_NUMERIC, "C");
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 	atexit(close_stdout);
@@ -531,7 +530,6 @@ int main(int argc, char **argv)
 		case 'h':
 			usage(stdout);
 			break;
-		case '?':
 		default:
 			usage(stderr);
 		}
@@ -579,6 +577,6 @@ int main(int argc, char **argv)
 	if (ctl.child == 0)
 		doshell(&ctl);
 	do_io(&ctl);
-
-	return EXIT_SUCCESS;
+	/* should not happen, do_io() calls done() */
+	return EXIT_FAILURE;
 }
-- 
2.2.1

Re: [PATCH 22/22] script: use correct input type, move comment, and so on

[Karel Zak]

On Sat, Dec 27, 2014 at 05:17:35PM +0000, Sami Kerola wrote:
> +	/*
> +	 * script -t prints time delays as floating point numbers.  The example
> +	 * program (scriptreplay) that we provide to handle this timing output

..pretty obsolete information, scriptreplay is in C since 2008:-)

I'll merge the script(1) changes later this week, I need to play with
that little bit more.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com