[PATCH 00/12] pull: script(1) changes

[PATCH 00/12] pull: script(1) changes

[Sami Kerola]

Hello,

The following changes are clean ups and improvements to script(1)
command.  These changes are also available from my github remote
repository.

 git://github.com/kerolasa/lelux-utiliteetit.git script

Sami Kerola (12):
  script: remove function prototypes
  script: add struct script_control and remove global variables
  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
  script: use gettime_monotonic() to get timing file timestamps
  script: add noreturn function attributes

 term-utils/Makemodule.am |   5 +-
 term-utils/script.c      | 872 ++++++++++++++++++++---------------------------
 tests/ts/script/race     |   4 -
 3 files changed, 380 insertions(+), 501 deletions(-)

-- 
2.3.0

[PATCH 01/12] script: remove function prototypes

[Sami Kerola]

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

diff --git a/term-utils/script.c b/term-utils/script.c
index 02ba596..41beecd 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -80,18 +80,6 @@
 
 #define DEFAULT_OUTPUT "typescript"
 
-void sig_finish(int);
-void finish(int);
-void done(void);
-void fail(void);
-void resize(int);
-void fixtty(void);
-void getmaster(void);
-void getslave(void);
-void doinput(void);
-void dooutput(void);
-void doshell(void);
-
 char	*shell;
 FILE	*fscript;
 FILE	*timingfd;
@@ -123,21 +111,7 @@ sigset_t block_mask, unblock_mask;
 int die;
 int resized;
 
-static void
-die_if_link(char *fn) {
-	struct stat s;
-
-	if (forceflg)
-		return;
-	if (lstat(fn, &s) == 0 && (S_ISLNK(s.st_mode) || s.st_nlink > 1))
-		errx(EXIT_FAILURE,
-		     _("output file `%s' is a link\n"
-		       "Use --force if you really want to use it.\n"
-		       "Program not started."), fn);
-}
-
-static void __attribute__((__noreturn__))
-usage(FILE *out)
+static void __attribute__((__noreturn__)) usage(FILE *out)
 {
 	fputs(USAGE_HEADER, out);
 	fprintf(out,
@@ -161,146 +135,81 @@ usage(FILE *out)
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-/*
- * 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 sigaction sa;
-	int ch;
+static void die_if_link(char *fn)
+{
+	struct stat s;
 
-	enum { FORCE_OPTION = CHAR_MAX + 1 };
+	if (forceflg)
+		return;
+	if (lstat(fn, &s) == 0 && (S_ISLNK(s.st_mode) || s.st_nlink > 1))
+		errx(EXIT_FAILURE,
+		     _("output file `%s' is a link\n"
+		       "Use --force if you really want to use it.\n"
+		       "Program not started."), fn);
+}
 
-	static const struct option longopts[] = {
-		{ "append",	no_argument,	   NULL, 'a' },
-		{ "command",	required_argument, NULL, 'c' },
-		{ "return",	no_argument,	   NULL, 'e' },
-		{ "flush",	no_argument,	   NULL, 'f' },
-		{ "force",	no_argument,	   NULL, FORCE_OPTION, },
-		{ "quiet",	no_argument,	   NULL, 'q' },
-		{ "timing",	optional_argument, NULL, 't' },
-		{ "version",	no_argument,	   NULL, 'V' },
-		{ "help",	no_argument,	   NULL, 'h' },
-		{ NULL,		0, NULL, 0 }
-	};
+/*
+ * 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);
+}
 
-	setlocale(LC_ALL, "");
-	setlocale(LC_NUMERIC, "C");	/* see comment above */
-	bindtextdomain(PACKAGE, LOCALEDIR);
-	textdomain(PACKAGE);
-	atexit(close_stdout);
+static void __attribute__((__noreturn__)) done(void)
+{
+	time_t tvec;
 
-	while ((ch = getopt_long(argc, argv, "ac:efqt::Vh", longopts, NULL)) != -1)
-		switch(ch) {
-		case 'a':
-			aflg = 1;
-			break;
-		case 'c':
-			cflg = optarg;
-			break;
-		case 'e':
-			eflg = 1;
-			break;
-		case 'f':
-			fflg = 1;
-			break;
-		case FORCE_OPTION:
-			forceflg = 1;
-			break;
-		case 'q':
-			qflg = 1;
-			break;
-		case 't':
-			if (optarg && !(timingfd = fopen(optarg, "w")))
-				err(EXIT_FAILURE, _("cannot open %s"), optarg);
-			tflg = 1;
-			break;
-		case 'V':
-			printf(UTIL_LINUX_VERSION);
-			exit(EXIT_SUCCESS);
-			break;
-		case 'h':
-			usage(stdout);
-			break;
-		case '?':
-		default:
-			usage(stderr);
+	if (subchild) {
+		/* output process */
+		if (fscript) {
+			if (!qflg) {
+				char buf[BUFSIZ];
+				tvec = time((time_t *)NULL);
+				my_strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
+				fprintf(fscript, _("\nScript done on %s"), buf);
+			}
+			if (close_stream(fscript) != 0)
+				errx(EXIT_FAILURE, _("write error"));
+			fscript = NULL;
 		}
-	argc -= optind;
-	argv += optind;
-
-	if (argc > 0)
-		fname = argv[0];
-	else {
-		fname = DEFAULT_OUTPUT;
-		die_if_link(fname);
-	}
-
-	if ((fscript = fopen(fname, aflg ? "a" : "w")) == NULL) {
-		warn(_("cannot open %s"), fname);
-		fail();
-	}
-
-	shell = getenv("SHELL");
-	if (shell == NULL)
-		shell = _PATH_BSHELL;
-
-	getmaster();
-	if (!qflg)
-		printf(_("Script started, file is %s\n"), fname);
-	fixtty();
+		if (timingfd && close_stream(timingfd) != 0)
+			errx(EXIT_FAILURE, _("write error"));
+		timingfd = NULL;
 
+		close(master);
+		master = -1;
+	} else {
+		/* input process */
+		if (isterm)
+			tcsetattr(STDIN_FILENO, TCSADRAIN, &tt);
+		if (!qflg)
+			printf(_("Script done, file is %s\n"), fname);
 #ifdef HAVE_LIBUTEMPTER
-	utempter_add_record(master, NULL);
+		if (master >= 0)
+			utempter_remove_record(master);
 #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, &block_mask);
-	sigaddset(&block_mask, SIGCHLD);
-
-	fflush(stdout);
-	sigprocmask(SIG_SETMASK, &block_mask, &unblock_mask);
-	child = fork();
-	sigprocmask(SIG_SETMASK, &unblock_mask, NULL);
-
-	if (child < 0) {
-		warn(_("fork failed"));
-		fail();
+		kill(child, SIGTERM);	/* make sure we don't create orphans */
 	}
-	if (child == 0) {
-
-		sigprocmask(SIG_SETMASK, &block_mask, NULL);
-		subchild = child = fork();
-		sigprocmask(SIG_SETMASK, &unblock_mask, NULL);
 
-		if (child < 0) {
-			warn(_("fork failed"));
-			fail();
-		}
-		if (child)
-			dooutput();
+	if(eflg) {
+		if (WIFSIGNALED(childstatus))
+			exit(WTERMSIG(childstatus) + 0x80);
 		else
-			doshell();
-	} else {
-		sa.sa_handler = resize;
-		sigaction(SIGWINCH, &sa, NULL);
+			exit(WEXITSTATUS(childstatus));
 	}
-	doinput();
+	exit(EXIT_SUCCESS);
+}
 
-	return EXIT_SUCCESS;
+static void
+fail(void) {
+
+	kill(0, SIGTERM);
+	done();
 }
 
+
 static void wait_for_empty_fd(int fd)
 {
 	struct pollfd fds[] = {
@@ -310,8 +219,24 @@ static void wait_for_empty_fd(int fd)
 	while (die == 0 && poll(fds, 1, 100) == 1);
 }
 
-void
-doinput(void) {
+static void finish(int wait)
+{
+	int status;
+	pid_t pid;
+	int errsv = errno;
+	int options = wait ? 0 : WNOHANG;
+
+	while ((pid = wait3(&status, options, 0)) > 0)
+		if (pid == child) {
+			childstatus = status;
+			die = 1;
+		}
+
+	errno = errsv;
+}
+
+static void doinput(void)
+{
 	int errsv = 0;
 	ssize_t cc = 0;
 	char ibuf[BUFSIZ];
@@ -394,43 +319,18 @@ doinput(void) {
 	done();
 }
 
-void
-finish(int wait) {
-	int status;
-	pid_t pid;
-	int errsv = errno;
-	int options = wait ? 0 : WNOHANG;
-
-	while ((pid = wait3(&status, options, 0)) > 0)
-		if (pid == child) {
-			childstatus = status;
-			die = 1;
-		}
-
-	errno = errsv;
-}
-
-void
-sig_finish(int dummy __attribute__ ((__unused__))) {
+static void sig_finish(int dummy __attribute__ ((__unused__)))
+{
 	finish(0);
 }
 
-void
-resize(int dummy __attribute__ ((__unused__))) {
+static void resize(int dummy __attribute__ ((__unused__)))
+{
 	resized = 1;
 }
 
-/*
- * 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);
-}
-
-void
-dooutput(void) {
+static void dooutput(void)
+{
 	ssize_t cc;
 	char obuf[BUFSIZ];
 	struct timeval tv;
@@ -507,8 +407,26 @@ dooutput(void) {
 	done();
 }
 
-void
-doshell(void) {
+static void getslave(void)
+{
+#ifndef HAVE_LIBUTIL
+	line[strlen("/dev/")] = 't';
+	slave = open(line, O_RDWR);
+	if (slave < 0) {
+		warn(_("cannot open %s"), line);
+		fail();
+	}
+	if (isterm) {
+		tcsetattr(slave, TCSANOW, &tt);
+		ioctl(slave, TIOCSWINSZ, (char *)&win);
+	}
+#endif
+	setsid();
+	ioctl(slave, TIOCSCTTY, 0);
+}
+
+static void doshell(void)
+{
 	char *shname;
 
 	getslave();
@@ -559,8 +477,8 @@ doshell(void) {
 	fail();
 }
 
-void
-fixtty(void) {
+static void fixtty(void)
+{
 	struct termios rtt;
 
 	if (!isterm)
@@ -572,60 +490,8 @@ fixtty(void) {
 	tcsetattr(STDIN_FILENO, TCSANOW, &rtt);
 }
 
-void
-fail(void) {
-
-	kill(0, SIGTERM);
-	done();
-}
-
-void __attribute__((__noreturn__))
-done(void) {
-	time_t tvec;
-
-	if (subchild) {
-		/* output process */
-		if (fscript) {
-			if (!qflg) {
-				char buf[BUFSIZ];
-				tvec = time((time_t *)NULL);
-				my_strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
-				fprintf(fscript, _("\nScript done on %s"), buf);
-			}
-			if (close_stream(fscript) != 0)
-				errx(EXIT_FAILURE, _("write error"));
-			fscript = NULL;
-		}
-		if (timingfd && close_stream(timingfd) != 0)
-			errx(EXIT_FAILURE, _("write error"));
-		timingfd = NULL;
-
-		close(master);
-		master = -1;
-	} else {
-		/* input process */
-		if (isterm)
-			tcsetattr(STDIN_FILENO, TCSADRAIN, &tt);
-		if (!qflg)
-			printf(_("Script done, file is %s\n"), fname);
-#ifdef HAVE_LIBUTEMPTER
-		if (master >= 0)
-			utempter_remove_record(master);
-#endif
-		kill(child, SIGTERM);	/* make sure we don't create orphans */
-	}
-
-	if(eflg) {
-		if (WIFSIGNALED(childstatus))
-			exit(WTERMSIG(childstatus) + 0x80);
-		else
-			exit(WEXITSTATUS(childstatus));
-	}
-	exit(EXIT_SUCCESS);
-}
-
-void
-getmaster(void) {
+static void getmaster(void)
+{
 #if defined(HAVE_LIBUTIL) && defined(HAVE_PTY_H)
 	int rc;
 
@@ -685,20 +551,141 @@ getmaster(void) {
 #endif /* not HAVE_LIBUTIL */
 }
 
-void
-getslave(void) {
-#ifndef HAVE_LIBUTIL
-	line[strlen("/dev/")] = 't';
-	slave = open(line, O_RDWR);
-	if (slave < 0) {
-		warn(_("cannot open %s"), line);
-		fail();
+/*
+ * 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 sigaction sa;
+	int ch;
+
+	enum { FORCE_OPTION = CHAR_MAX + 1 };
+
+	static const struct option longopts[] = {
+		{ "append",	no_argument,	   NULL, 'a' },
+		{ "command",	required_argument, NULL, 'c' },
+		{ "return",	no_argument,	   NULL, 'e' },
+		{ "flush",	no_argument,	   NULL, 'f' },
+		{ "force",	no_argument,	   NULL, FORCE_OPTION, },
+		{ "quiet",	no_argument,	   NULL, 'q' },
+		{ "timing",	optional_argument, NULL, 't' },
+		{ "version",	no_argument,	   NULL, 'V' },
+		{ "help",	no_argument,	   NULL, 'h' },
+		{ NULL,		0, NULL, 0 }
+	};
+
+	setlocale(LC_ALL, "");
+	setlocale(LC_NUMERIC, "C");	/* see comment above */
+	bindtextdomain(PACKAGE, LOCALEDIR);
+	textdomain(PACKAGE);
+	atexit(close_stdout);
+
+	while ((ch = getopt_long(argc, argv, "ac:efqt::Vh", longopts, NULL)) != -1)
+		switch(ch) {
+		case 'a':
+			aflg = 1;
+			break;
+		case 'c':
+			cflg = optarg;
+			break;
+		case 'e':
+			eflg = 1;
+			break;
+		case 'f':
+			fflg = 1;
+			break;
+		case FORCE_OPTION:
+			forceflg = 1;
+			break;
+		case 'q':
+			qflg = 1;
+			break;
+		case 't':
+			if (optarg && !(timingfd = fopen(optarg, "w")))
+				err(EXIT_FAILURE, _("cannot open %s"), optarg);
+			tflg = 1;
+			break;
+		case 'V':
+			printf(UTIL_LINUX_VERSION);
+			exit(EXIT_SUCCESS);
+			break;
+		case 'h':
+			usage(stdout);
+			break;
+		case '?':
+		default:
+			usage(stderr);
+		}
+	argc -= optind;
+	argv += optind;
+
+	if (argc > 0)
+		fname = argv[0];
+	else {
+		fname = DEFAULT_OUTPUT;
+		die_if_link(fname);
 	}
-	if (isterm) {
-		tcsetattr(slave, TCSANOW, &tt);
-		ioctl(slave, TIOCSWINSZ, (char *)&win);
+
+	if ((fscript = fopen(fname, aflg ? "a" : "w")) == NULL) {
+		warn(_("cannot open %s"), fname);
+		fail();
 	}
+
+	shell = getenv("SHELL");
+	if (shell == NULL)
+		shell = _PATH_BSHELL;
+
+	getmaster();
+	if (!qflg)
+		printf(_("Script started, file is %s\n"), fname);
+	fixtty();
+
+#ifdef HAVE_LIBUTEMPTER
+	utempter_add_record(master, NULL);
 #endif
-	setsid();
-	ioctl(slave, TIOCSCTTY, 0);
+	/* 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, &block_mask);
+	sigaddset(&block_mask, SIGCHLD);
+
+	fflush(stdout);
+	sigprocmask(SIG_SETMASK, &block_mask, &unblock_mask);
+	child = fork();
+	sigprocmask(SIG_SETMASK, &unblock_mask, NULL);
+
+	if (child < 0) {
+		warn(_("fork failed"));
+		fail();
+	}
+	if (child == 0) {
+
+		sigprocmask(SIG_SETMASK, &block_mask, NULL);
+		subchild = child = fork();
+		sigprocmask(SIG_SETMASK, &unblock_mask, NULL);
+
+		if (child < 0) {
+			warn(_("fork failed"));
+			fail();
+		}
+		if (child)
+			dooutput();
+		else
+			doshell();
+	} else {
+		sa.sa_handler = resize;
+		sigaction(SIGWINCH, &sa, NULL);
+	}
+	doinput();
+
+	return EXIT_SUCCESS;
 }
-- 
2.3.0

[PATCH 02/12] script: add struct script_control and remove global variables

[Sami Kerola]

Fix also couple indentation issues.

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

diff --git a/term-utils/script.c b/term-utils/script.c
index 41beecd..3d47362 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -12,8 +12,8 @@
  *    documentation and/or other materials provided with the distribution.
  * 3. All advertising materials mentioning features or use of this software
  *    must display the following acknowledgement:
- *	This product includes software developed by the University of
- *	California, Berkeley and its contributors.
+ *      This product includes software developed by the University of
+ *      California, Berkeley and its contributors.
  * 4. Neither the name of the University nor the names of its contributors
  *    may be used to endorse or promote products derived from this software
  *    without specific prior written permission.
@@ -80,42 +80,41 @@
 
 #define DEFAULT_OUTPUT "typescript"
 
-char	*shell;
-FILE	*fscript;
-FILE	*timingfd;
-int	master = -1;
-int	slave;
-pid_t	child;
-pid_t	subchild;
-int	childstatus;
-char	*fname;
-
-struct	termios tt;
-struct	winsize win;
-int	lb;
-int	l;
+struct script_control {
+	char *shell;		/* shell to be executed */
+	char *cflg;		/* command to be executed */
+	char *fname;		/* output file path */
+	FILE *typescriptfp;	/* output file pointer */
+	FILE *timingfp;		/* timing file pointer */
+	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 */
 #if !HAVE_LIBUTIL || !HAVE_PTY_H
-char	line[] = "/dev/ptyXX";
+	char line *;		/* terminal line */
 #endif
-int	aflg = 0;
-char	*cflg = NULL;
-int	eflg = 0;
-int	fflg = 0;
-int	qflg = 0;
-int	tflg = 0;
-int	forceflg = 0;
-int	isterm;
-
-sigset_t block_mask, unblock_mask;
-
-int die;
-int resized;
+	unsigned int
+	 aflg:1,		/* append output */
+	 eflg:1,		/* return child exit value */
+	 fflg:1,		/* flush after each write */
+	 qflg:1,		/* suppress most output */
+	 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 block_mask;	/* signals that are blocked */
+	sigset_t unblock_mask;	/* original signal mask */
+};
+struct script_control *gctl;	/* global control structure, used in signal handlers */
 
 static void __attribute__((__noreturn__)) usage(FILE *out)
 {
 	fputs(USAGE_HEADER, out);
-	fprintf(out,
-	      _(" %s [options] [file]\n"), program_invocation_short_name);
+	fprintf(out, _(" %s [options] [file]\n"), program_invocation_short_name);
 
 	fputs(USAGE_SEPARATOR, out);
 	fputs(_("Make a typescript of a terminal session.\n"), out);
@@ -135,17 +134,17 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-static void die_if_link(char *fn)
+static void die_if_link(const struct script_control *ctl)
 {
 	struct stat s;
 
-	if (forceflg)
+	if (ctl->forceflg)
 		return;
-	if (lstat(fn, &s) == 0 && (S_ISLNK(s.st_mode) || s.st_nlink > 1))
+	if (lstat(ctl->fname, &s) == 0 && (S_ISLNK(s.st_mode) || s.st_nlink > 1))
 		errx(EXIT_FAILURE,
 		     _("output file `%s' is a link\n"
 		       "Use --force if you really want to use it.\n"
-		       "Program not started."), fn);
+		       "Program not started."), ctl->fname);
 }
 
 /*
@@ -157,69 +156,67 @@ static void my_strftime(char *buf, size_t len, const char *fmt, const struct tm
 	strftime(buf, len, fmt, tm);
 }
 
-static void __attribute__((__noreturn__)) done(void)
+static void __attribute__((__noreturn__)) done(struct script_control *ctl)
 {
 	time_t tvec;
 
-	if (subchild) {
+	if (ctl->subchild) {
 		/* output process */
-		if (fscript) {
-			if (!qflg) {
+		if (ctl->typescriptfp) {
+			if (!ctl->qflg) {
 				char buf[BUFSIZ];
 				tvec = time((time_t *)NULL);
 				my_strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
-				fprintf(fscript, _("\nScript done on %s"), buf);
+				fprintf(ctl->typescriptfp, _("\nScript done on %s"), buf);
 			}
-			if (close_stream(fscript) != 0)
+			if (close_stream(ctl->typescriptfp) != 0)
 				errx(EXIT_FAILURE, _("write error"));
-			fscript = NULL;
+			ctl->typescriptfp = NULL;
 		}
-		if (timingfd && close_stream(timingfd) != 0)
+		if (ctl->timingfp && close_stream(ctl->timingfp) != 0)
 			errx(EXIT_FAILURE, _("write error"));
-		timingfd = NULL;
+		ctl->timingfp = NULL;
 
-		close(master);
-		master = -1;
+		close(ctl->master);
+		ctl->master = -1;
 	} else {
 		/* input process */
-		if (isterm)
-			tcsetattr(STDIN_FILENO, TCSADRAIN, &tt);
-		if (!qflg)
-			printf(_("Script done, file is %s\n"), 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 (master >= 0)
-			utempter_remove_record(master);
+		if (ctl->master >= 0)
+			utempter_remove_record(ctl->master);
 #endif
-		kill(child, SIGTERM);	/* make sure we don't create orphans */
+		kill(ctl->child, SIGTERM);	/* make sure we don't create orphans */
 	}
 
-	if(eflg) {
-		if (WIFSIGNALED(childstatus))
-			exit(WTERMSIG(childstatus) + 0x80);
+	if (ctl->eflg) {
+		if (WIFSIGNALED(ctl->childstatus))
+			exit(WTERMSIG(ctl->childstatus) + 0x80);
 		else
-			exit(WEXITSTATUS(childstatus));
+			exit(WEXITSTATUS(ctl->childstatus));
 	}
 	exit(EXIT_SUCCESS);
 }
 
-static void
-fail(void) {
-
+static void fail(struct script_control *ctl)
+{
 	kill(0, SIGTERM);
-	done();
+	done(ctl);
 }
 
-
-static void wait_for_empty_fd(int fd)
+static void wait_for_empty_fd(struct script_control *ctl, int fd)
 {
 	struct pollfd fds[] = {
-		{ .fd = fd, .events = POLLIN }
+		{.fd = fd, .events = POLLIN}
 	};
 
-	while (die == 0 && poll(fds, 1, 100) == 1);
+	while (ctl->die == 0 && poll(fds, 1, 100) == 1) ;
 }
 
-static void finish(int wait)
+static void finish(struct script_control *ctl, int wait)
 {
 	int status;
 	pid_t pid;
@@ -227,15 +224,15 @@ static void finish(int wait)
 	int options = wait ? 0 : WNOHANG;
 
 	while ((pid = wait3(&status, options, 0)) > 0)
-		if (pid == child) {
-			childstatus = status;
-			die = 1;
+		if (pid == ctl->child) {
+			ctl->childstatus = status;
+			ctl->die = 1;
 		}
 
 	errno = errsv;
 }
 
-static void doinput(void)
+static void doinput(struct script_control *ctl)
 {
 	int errsv = 0;
 	ssize_t cc = 0;
@@ -243,41 +240,40 @@ static void doinput(void)
 	fd_set readfds;
 
 	/* close things irrelevant for this process */
-	if (fscript)
-		fclose(fscript);
-	if (timingfd)
-		fclose(timingfd);
-	fscript = timingfd = NULL;
+	if (ctl->typescriptfp)
+		fclose(ctl->typescriptfp);
+	if (ctl->timingfp)
+		fclose(ctl->timingfp);
+	ctl->typescriptfp = ctl->timingfp = NULL;
 
 	FD_ZERO(&readfds);
 
 	/* block SIGCHLD */
-	sigprocmask(SIG_SETMASK, &block_mask, &unblock_mask);
+	sigprocmask(SIG_SETMASK, &ctl->block_mask, &ctl->unblock_mask);
 
-	while (die == 0) {
+	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,
-			&unblock_mask)) > 0) {
+				  &ctl->unblock_mask)) > 0) {
 
 			if ((cc = read(STDIN_FILENO, ibuf, BUFSIZ)) > 0) {
-				if (write_all(master, ibuf, cc)) {
-					warn (_("write failed"));
-					fail();
+				if (write_all(ctl->master, ibuf, cc)) {
+					warn(_("write failed"));
+					fail(ctl);
 				}
 			}
 		}
 
-		if (cc < 0 && errno == EINTR && resized)
-		{
+		if (cc < 0 && errno == EINTR && ctl->resized) {
 			/* transmit window change information to the child */
-			if (isterm) {
-				ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&win);
-				ioctl(slave, TIOCSWINSZ, (char *)&win);
+			if (ctl->isterm) {
+				ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&ctl->win);
+				ioctl(ctl->slave, TIOCSWINSZ, (char *)&ctl->win);
 			}
-			resized = 0;
+			ctl->resized = 0;
 
 		} else if (cc <= 0 && errno != EINTR) {
 			errsv = errno;
@@ -286,13 +282,13 @@ static void doinput(void)
 	}
 
 	/* unblock SIGCHLD */
-	sigprocmask(SIG_SETMASK, &unblock_mask, NULL);
+	sigprocmask(SIG_SETMASK, &ctl->unblock_mask, NULL);
 
 	/* To be sure that we don't miss any data */
-	wait_for_empty_fd(slave);
-	wait_for_empty_fd(master);
+	wait_for_empty_fd(ctl, ctl->slave);
+	wait_for_empty_fd(ctl, ctl->master);
 
-	if (die == 0 && cc == 0 && errsv == 0) {
+	if (ctl->die == 0 && cc == 0 && errsv == 0) {
 		/*
 		 * Forward EOF from stdin (detected by read() above) to slave
 		 * (shell) to correctly terminate the session. It seems we have
@@ -304,153 +300,155 @@ static void doinput(void)
 		 */
 		int c = DEF_EOF;
 
-		if (write_all(master, &c, 1)) {
-			warn (_("write failed"));
-			fail();
+		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(master);
+		wait_for_empty_fd(ctl, ctl->master);
 	}
 
-	if (!die)
-		finish(1);	/* wait for children */
-	done();
+	if (!ctl->die)
+		finish(ctl, 1);	/* wait for children */
+	done(ctl);
 }
 
-static void sig_finish(int dummy __attribute__ ((__unused__)))
+static void sig_finish(int dummy __attribute__((__unused__)))
 {
-	finish(0);
+	finish(gctl, 0);
 }
 
-static void resize(int dummy __attribute__ ((__unused__)))
+static void resize(int dummy __attribute__((__unused__)))
 {
-	resized = 1;
+	gctl->resized = 1;
 }
 
-static void dooutput(void)
+static void dooutput(struct script_control *ctl)
 {
 	ssize_t cc;
 	char obuf[BUFSIZ];
 	struct timeval tv;
-	double oldtime=time(NULL), newtime;
+	double oldtime = time(NULL), newtime;
 	int errsv = 0;
 	fd_set readfds;
 
 	close(STDIN_FILENO);
 #ifdef HAVE_LIBUTIL
-	close(slave);
+	close(ctl->slave);
 #endif
-	if (tflg && !timingfd)
-		timingfd = fdopen(STDERR_FILENO, "w");
+	if (ctl->tflg && !ctl->timingfp)
+		ctl->timingfp = fdopen(STDERR_FILENO, "w");
 
-	if (!qflg) {
+	if (!ctl->qflg) {
 		time_t tvec = time((time_t *)NULL);
 		my_strftime(obuf, sizeof obuf, "%c\n", localtime(&tvec));
-		fprintf(fscript, _("Script started on %s"), obuf);
+		fprintf(ctl->typescriptfp, _("Script started on %s"), obuf);
 	}
 
 	FD_ZERO(&readfds);
 
 	do {
-		if (die || errsv == EINTR) {
-			struct pollfd fds[] = {{ .fd = master, .events = POLLIN }};
+		if (ctl->die || errsv == EINTR) {
+			struct pollfd fds[] = {
+				{.fd = ctl->master, .events = POLLIN}
+			};
 			if (poll(fds, 1, 50) <= 0)
 				break;
 		}
 
 		/* block SIGCHLD */
-		sigprocmask(SIG_SETMASK, &block_mask, &unblock_mask);
+		sigprocmask(SIG_SETMASK, &ctl->block_mask, &ctl->unblock_mask);
 
-		FD_SET(master, &readfds);
+		FD_SET(ctl->master, &readfds);
 		errno = 0;
 
 		/* wait for input or signal (including SIGCHLD) */
-		if ((cc = pselect(master+1, &readfds, NULL, NULL, NULL,
-			&unblock_mask)) > 0) {
+		if ((cc = pselect(ctl->master + 1, &readfds, NULL, NULL, NULL,
+				  &ctl->unblock_mask)) > 0) {
 
-			cc = read(master, obuf, sizeof (obuf));
+			cc = read(ctl->master, obuf, sizeof(obuf));
 		}
 		errsv = errno;
 
 		/* unblock SIGCHLD */
-		sigprocmask(SIG_SETMASK, &unblock_mask, NULL);
+		sigprocmask(SIG_SETMASK, &ctl->unblock_mask, NULL);
 
-		if (tflg)
+		if (ctl->tflg)
 			gettimeofday(&tv, NULL);
 
 		if (errsv == EINTR && cc <= 0)
 			continue;	/* try it again */
 		if (cc <= 0)
 			break;
-		if (tflg && timingfd) {
-			newtime = tv.tv_sec + (double) tv.tv_usec / 1000000;
-			fprintf(timingfd, "%f %zd\n", newtime - oldtime, cc);
+		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, fscript)) {
-			warn (_("cannot write script file"));
-			fail();
+		if (fwrite_all(obuf, 1, cc, ctl->typescriptfp)) {
+			warn(_("cannot write script file"));
+			fail(ctl);
 		}
-		if (fflg) {
-			fflush(fscript);
-			if (tflg && timingfd)
-				fflush(timingfd);
+		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();
+			warn(_("write failed"));
+			fail(ctl);
 		}
-	} while(1);
+	} while (1);
 
-	done();
+	done(ctl);
 }
 
-static void getslave(void)
+static void getslave(struct script_control *ctl)
 {
 #ifndef HAVE_LIBUTIL
-	line[strlen("/dev/")] = 't';
-	slave = open(line, O_RDWR);
-	if (slave < 0) {
-		warn(_("cannot open %s"), line);
-		fail();
+	ctl->line[strlen("/dev/")] = 't';
+	ctl->slave = open(ctl->line, O_RDWR);
+	if (ctl->slave < 0) {
+		warn(_("cannot open %s"), ctl->line);
+		fail(ctl);
 	}
-	if (isterm) {
-		tcsetattr(slave, TCSANOW, &tt);
-		ioctl(slave, TIOCSWINSZ, (char *)&win);
+	if (ctl->isterm) {
+		tcsetattr(ctl->slave, TCSANOW, &ctl->tt);
+		ioctl(ctl->slave, TIOCSWINSZ, (char *)&ctl->win);
 	}
 #endif
 	setsid();
-	ioctl(slave, TIOCSCTTY, 0);
+	ioctl(ctl->slave, TIOCSCTTY, 0);
 }
 
-static void doshell(void)
+static void doshell(struct script_control *ctl)
 {
 	char *shname;
 
-	getslave();
+	getslave(ctl);
 
 	/* close things irrelevant for this process */
-	close(master);
-	if (fscript)
-		fclose(fscript);
-	if (timingfd)
-		fclose(timingfd);
-	fscript = timingfd = NULL;
+	close(ctl->master);
+	if (ctl->typescriptfp)
+		fclose(ctl->typescriptfp);
+	if (ctl->timingfp)
+		fclose(ctl->timingfp);
+	ctl->typescriptfp = ctl->timingfp = NULL;
 
-	dup2(slave, STDIN_FILENO);
-	dup2(slave, STDOUT_FILENO);
-	dup2(slave, STDERR_FILENO);
-	close(slave);
+	dup2(ctl->slave, STDIN_FILENO);
+	dup2(ctl->slave, STDOUT_FILENO);
+	dup2(ctl->slave, STDERR_FILENO);
+	close(ctl->slave);
 
-	master = -1;
+	ctl->master = -1;
 
-	shname = strrchr(shell, '/');
+	shname = strrchr(ctl->shell, '/');
 	if (shname)
 		shname++;
 	else
-		shname = shell;
+		shname = ctl->shell;
 
 	/*
 	 * When invoked from within /etc/csh.login, script spawns a csh shell
@@ -462,93 +460,92 @@ static void doshell(void)
 	 */
 	signal(SIGTERM, SIG_DFL);
 
-	if (access(shell, X_OK) == 0) {
-		if (cflg)
-			execl(shell, shname, "-c", cflg, NULL);
+	if (access(ctl->shell, X_OK) == 0) {
+		if (ctl->cflg)
+			execl(ctl->shell, shname, "-c", ctl->cflg, NULL);
 		else
-			execl(shell, shname, "-i", NULL);
+			execl(ctl->shell, shname, "-i", NULL);
 	} else {
-		if (cflg)
-			execlp(shname, "-c", cflg, NULL);
+		if (ctl->cflg)
+			execlp(shname, "-c", ctl->cflg, NULL);
 		else
 			execlp(shname, "-i", NULL);
 	}
-	warn(_("failed to execute %s"), shell);
-	fail();
+	warn(_("failed to execute %s"), ctl->shell);
+	fail(ctl);
 }
 
-static void fixtty(void)
+static void fixtty(struct script_control *ctl)
 {
 	struct termios rtt;
 
-	if (!isterm)
+	if (!ctl->isterm)
 		return;
 
-	rtt = tt;
+	rtt = ctl->tt;
 	cfmakeraw(&rtt);
 	rtt.c_lflag &= ~ECHO;
 	tcsetattr(STDIN_FILENO, TCSANOW, &rtt);
 }
 
-static void getmaster(void)
+static void getmaster(struct script_control *ctl)
 {
 #if defined(HAVE_LIBUTIL) && defined(HAVE_PTY_H)
 	int rc;
 
-	isterm = isatty(STDIN_FILENO);
+	ctl->isterm = isatty(STDIN_FILENO);
 
-	if (isterm) {
-		if (tcgetattr(STDIN_FILENO, &tt) != 0)
+	if (ctl->isterm) {
+		if (tcgetattr(STDIN_FILENO, &ctl->tt) != 0)
 			err(EXIT_FAILURE, _("failed to get terminal attributes"));
-		ioctl(STDIN_FILENO, TIOCGWINSZ, (char *) &win);
-		rc = openpty(&master, &slave, NULL, &tt, &win);
+		ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&ctl->win);
+		rc = openpty(&ctl->master, &ctl->slave, NULL, &ctl->tt, &ctl->win);
 	} else
-		rc = openpty(&master, &slave, NULL, NULL, NULL);
+		rc = openpty(&ctl->master, &ctl->slave, NULL, NULL, NULL);
 
 	if (rc < 0) {
 		warn(_("openpty failed"));
-		fail();
+		fail(ctl);
 	}
 #else
 	char *pty, *bank, *cp;
 	struct stat stb;
 
-	isterm = isatty(STDIN_FILENO);
+	ctl->isterm = isatty(STDIN_FILENO);
 
-	pty = &line[strlen("/dev/ptyp")];
+	pty = &ctl->line[strlen("/dev/ptyp")];
 	for (bank = "pqrs"; *bank; bank++) {
-		line[strlen("/dev/pty")] = *bank;
+		ctl->line[strlen("/dev/pty")] = *bank;
 		*pty = '0';
-		if (stat(line, &stb) < 0)
+		if (stat(ctl->line, &stb) < 0)
 			break;
 		for (cp = "0123456789abcdef"; *cp; cp++) {
 			*pty = *cp;
-			master = open(line, O_RDWR);
-			if (master >= 0) {
-				char *tp = &line[strlen("/dev/")];
+			ctl->master = open(ctl->line, O_RDWR);
+			if (ctl->master >= 0) {
+				char *tp = &ctl->line[strlen("/dev/")];
 				int ok;
 
 				/* verify slave side is usable */
 				*tp = 't';
-				ok = access(line, R_OK|W_OK) == 0;
+				ok = access(ctl->line, R_OK | W_OK) == 0;
 				*tp = 'p';
 				if (ok) {
-					if (isterm) {
-						tcgetattr(STDIN_FILENO, &tt);
-						ioctl(STDIN_FILENO, TIOCGWINSZ,
-								(char *)&win);
+					if (ctl->isterm) {
+						tcgetattr(STDIN_FILENO, &ctl->tt);
+						ioctl(STDIN_FILENO, TIOCGWINSZ, (char *)&ctl->win);
 					}
 					return;
 				}
-				close(master);
-				master = -1;
+				close(ctl->master);
+				ctl->master = -1;
 			}
 		}
 	}
-	master = -1;
+	ctl->master = -1;
 	warn(_("out of pty's"));
-	fail();
-#endif /* not HAVE_LIBUTIL */
+	fail(ctl);
+#endif				/* not HAVE_LIBUTIL */
 }
 
 /*
@@ -561,22 +558,29 @@ static void getmaster(void)
  */
 int main(int argc, char **argv)
 {
+	struct script_control ctl = {
+#if !HAVE_LIBUTIL || !HAVE_PTY_H
+		.line = "/dev/ptyXX",
+#endif
+		.master = -1,
+		0
+	};
 	struct sigaction sa;
 	int ch;
 
 	enum { FORCE_OPTION = CHAR_MAX + 1 };
 
 	static const struct option longopts[] = {
-		{ "append",	no_argument,	   NULL, 'a' },
-		{ "command",	required_argument, NULL, 'c' },
-		{ "return",	no_argument,	   NULL, 'e' },
-		{ "flush",	no_argument,	   NULL, 'f' },
-		{ "force",	no_argument,	   NULL, FORCE_OPTION, },
-		{ "quiet",	no_argument,	   NULL, 'q' },
-		{ "timing",	optional_argument, NULL, 't' },
-		{ "version",	no_argument,	   NULL, 'V' },
-		{ "help",	no_argument,	   NULL, 'h' },
-		{ NULL,		0, NULL, 0 }
+		{"append", no_argument, NULL, 'a'},
+		{"command", required_argument, NULL, 'c'},
+		{"return", no_argument, NULL, 'e'},
+		{"flush", no_argument, NULL, 'f'},
+		{"force", no_argument, NULL, FORCE_OPTION,},
+		{"quiet", no_argument, NULL, 'q'},
+		{"timing", optional_argument, NULL, 't'},
+		{"version", no_argument, NULL, 'V'},
+		{"help", no_argument, NULL, 'h'},
+		{NULL, 0, NULL, 0}
 	};
 
 	setlocale(LC_ALL, "");
@@ -584,31 +588,32 @@ 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) {
+		switch (ch) {
 		case 'a':
-			aflg = 1;
+			ctl.aflg = 1;
 			break;
 		case 'c':
-			cflg = optarg;
+			ctl.cflg = optarg;
 			break;
 		case 'e':
-			eflg = 1;
+			ctl.eflg = 1;
 			break;
 		case 'f':
-			fflg = 1;
+			ctl.fflg = 1;
 			break;
 		case FORCE_OPTION:
-			forceflg = 1;
+			ctl.forceflg = 1;
 			break;
 		case 'q':
-			qflg = 1;
+			ctl.qflg = 1;
 			break;
 		case 't':
-			if (optarg && !(timingfd = fopen(optarg, "w")))
+			if (optarg && !(ctl.timingfp = fopen(optarg, "w")))
 				err(EXIT_FAILURE, _("cannot open %s"), optarg);
-			tflg = 1;
+			ctl.tflg = 1;
 			break;
 		case 'V':
 			printf(UTIL_LINUX_VERSION);
@@ -625,25 +630,25 @@ int main(int argc, char **argv)
 	argv += optind;
 
 	if (argc > 0)
-		fname = argv[0];
+		ctl.fname = argv[0];
 	else {
-		fname = DEFAULT_OUTPUT;
-		die_if_link(fname);
+		ctl.fname = DEFAULT_OUTPUT;
+		die_if_link(&ctl);
 	}
 
-	if ((fscript = fopen(fname, aflg ? "a" : "w")) == NULL) {
-		warn(_("cannot open %s"), fname);
-		fail();
+	if ((ctl.typescriptfp = fopen(ctl.fname, ctl.aflg ? "a" : "w")) == NULL) {
+		warn(_("cannot open %s"), ctl.fname);
+		fail(&ctl);
 	}
 
-	shell = getenv("SHELL");
-	if (shell == NULL)
-		shell = _PATH_BSHELL;
+	ctl.shell = getenv("SHELL");
+	if (ctl.shell == NULL)
+		ctl.shell = _PATH_BSHELL;
 
-	getmaster();
-	if (!qflg)
-		printf(_("Script started, file is %s\n"), fname);
-	fixtty();
+	getmaster(&ctl);
+	if (!ctl.qflg)
+		printf(_("Script started, file is %s\n"), ctl.fname);
+	fixtty(&ctl);
 
 #ifdef HAVE_LIBUTEMPTER
 	utempter_add_record(master, NULL);
@@ -655,37 +660,37 @@ int main(int argc, char **argv)
 	sigaction(SIGCHLD, &sa, NULL);
 
 	/* init mask for SIGCHLD */
-	sigprocmask(SIG_SETMASK, NULL, &block_mask);
-	sigaddset(&block_mask, SIGCHLD);
+	sigprocmask(SIG_SETMASK, NULL, &ctl.block_mask);
+	sigaddset(&ctl.block_mask, SIGCHLD);
 
 	fflush(stdout);
-	sigprocmask(SIG_SETMASK, &block_mask, &unblock_mask);
-	child = fork();
-	sigprocmask(SIG_SETMASK, &unblock_mask, NULL);
+	sigprocmask(SIG_SETMASK, &ctl.block_mask, &ctl.unblock_mask);
+	ctl.child = fork();
+	sigprocmask(SIG_SETMASK, &ctl.unblock_mask, NULL);
 
-	if (child < 0) {
+	if (ctl.child < 0) {
 		warn(_("fork failed"));
-		fail();
+		fail(&ctl);
 	}
-	if (child == 0) {
+	if (ctl.child == 0) {
 
-		sigprocmask(SIG_SETMASK, &block_mask, NULL);
-		subchild = child = fork();
-		sigprocmask(SIG_SETMASK, &unblock_mask, NULL);
+		sigprocmask(SIG_SETMASK, &ctl.block_mask, NULL);
+		ctl.subchild = ctl.child = fork();
+		sigprocmask(SIG_SETMASK, &ctl.unblock_mask, NULL);
 
-		if (child < 0) {
+		if (ctl.child < 0) {
 			warn(_("fork failed"));
-			fail();
+			fail(&ctl);
 		}
-		if (child)
-			dooutput();
+		if (ctl.child)
+			dooutput(&ctl);
 		else
-			doshell();
+			doshell(&ctl);
 	} else {
 		sa.sa_handler = resize;
 		sigaction(SIGWINCH, &sa, NULL);
 	}
-	doinput();
+	doinput(&ctl);
 
 	return EXIT_SUCCESS;
 }
-- 
2.3.0

Re: [PATCH 02/12] script: add struct script_control and remove global variables

[Karel Zak]

On Sun, Feb 22, 2015 at 02:42:39PM +0000, Sami Kerola wrote:
>  int main(int argc, char **argv)
...
> +	if (optarg && !(ctl.timingfp = fopen(optarg, "w")))
...

> +	if ((ctl.typescriptfp = fopen(ctl.fname, ctl.aflg ? "a" : "w")) == NULL) {

 Does it make sense to open the files in main() and then later close
 it in doshell()? What about add "typescrip" and "timing" filenames
 to the control structs and open the files later in do_io() ?

    Karel

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

[PATCH 03/12] 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 3d47362..0f27163 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 */
 
@@ -248,16 +250,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)) {
@@ -281,9 +280,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);
@@ -352,29 +348,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);
 
@@ -565,7 +556,6 @@ int main(int argc, char **argv)
 		.master = -1,
 		0
 	};
-	struct sigaction sa;
 	int ch;
 
 	enum { FORCE_OPTION = CHAR_MAX + 1 };
@@ -653,20 +643,16 @@ 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"));
 
 	fflush(stdout);
-	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"));
@@ -674,9 +660,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"));
@@ -686,9 +670,6 @@ int main(int argc, char **argv)
 			dooutput(&ctl);
 		else
 			doshell(&ctl);
-	} else {
-		sa.sa_handler = resize;
-		sigaction(SIGWINCH, &sa, NULL);
 	}
 	doinput(&ctl);
 
-- 
2.3.0

[PATCH 04/12] 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 0f27163..672c610 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)
 {
@@ -236,10 +236,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)
@@ -248,35 +248,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;
 		}
 	}
 
@@ -284,7 +298,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
@@ -311,24 +325,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
@@ -343,57 +374,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)
@@ -578,7 +605,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.3.0

[PATCH 05/12] 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 672c610..395b96a 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 */
@@ -160,39 +159,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))
@@ -209,15 +184,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;
@@ -234,97 +200,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)
 {
@@ -352,34 +227,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) {
@@ -391,17 +263,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;
 
@@ -409,10 +297,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();
@@ -420,7 +311,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)
@@ -684,20 +577,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.3.0

[PATCH 06/12] 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
Addresses: https://bugs.launchpad.net/bugs/264967
Addresses: https://bugs.debian.org/305808
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 395b96a..cd53b06 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 */
@@ -188,16 +189,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,
@@ -253,13 +249,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;
@@ -298,6 +296,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) {
@@ -474,6 +479,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.3.0

[PATCH 07/12] 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 cd53b06..a7242e7 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -230,6 +230,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");
@@ -241,11 +242,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.3.0

[PATCH 08/12] 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 a7242e7..08bd922 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -223,14 +223,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);
+		}
+		/* 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");
@@ -260,57 +309,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.3.0

[PATCH 09/12] 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 08bd922..4709e10 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -149,15 +149,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)
@@ -291,7 +282,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.3.0

[PATCH 10/12] 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 4709e10..8960ac4 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -182,7 +182,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;
 }
@@ -461,14 +461,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 = {
@@ -497,7 +489,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);
@@ -534,7 +533,6 @@ int main(int argc, char **argv)
 		case 'h':
 			usage(stdout);
 			break;
-		case '?':
 		default:
 			usage(stderr);
 		}
@@ -583,6 +581,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.3.0

[PATCH 11/12] script: use gettime_monotonic() to get timing file timestamps

[Sami Kerola]

This moves the previous time to script control structure, and does
timeval calculation with timersub() that is more appropriate than
making a timeval to a double.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/Makemodule.am |  4 ++--
 term-utils/script.c      | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/term-utils/Makemodule.am b/term-utils/Makemodule.am
index 2bf916c..a360f22 100644
--- a/term-utils/Makemodule.am
+++ b/term-utils/Makemodule.am
@@ -1,9 +1,9 @@
 if BUILD_SCRIPT
 usrbin_exec_PROGRAMS += script
 dist_man_MANS += term-utils/script.1
-script_SOURCES = term-utils/script.c
+script_SOURCES = term-utils/script.c lib/monotonic.c
 script_CFLAGS = $(AM_CFLAGS) -Wno-format-y2k
-script_LDADD = $(LDADD)
+script_LDADD = $(LDADD) libcommon.la $(CLOCKGETTIME_LIBS)
 if HAVE_UTIL
 script_LDADD += -lutil
 endif
diff --git a/term-utils/script.c b/term-utils/script.c
index 8960ac4..54e10b4 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -71,6 +71,7 @@
 #include "c.h"
 #include "ttyutils.h"
 #include "all-io.h"
+#include "monotonic.h"
 
 #if defined(HAVE_LIBUTIL) && defined(HAVE_PTY_H)
 # include <pty.h>
@@ -90,6 +91,7 @@ struct script_control {
 	char *fname;		/* output file path */
 	FILE *typescriptfp;	/* output file pointer */
 	FILE *timingfp;		/* timing file pointer */
+	struct timeval oldtime;	/* previous write or command start time */
 	int master;		/* pseudoterminal master file descriptor */
 	int slave;		/* pseudoterminal slave file descriptor */
 	int poll_timeout;	/* poll() timeout, used in end of execution */
@@ -188,19 +190,17 @@ static void finish(struct script_control *ctl, int wait)
 }
 
 static void write_output(struct script_control *ctl, char *obuf,
-			    ssize_t bytes, double *oldtime)
+			    ssize_t bytes)
 {
-
 	if (ctl->tflg && ctl->timingfp) {
-		struct timeval tv;
-		double newtime;
+		struct timeval now, delta;
 
-		gettimeofday(&tv, NULL);
-		newtime = tv.tv_sec + (double)tv.tv_usec / 1000000;
-		fprintf(ctl->timingfp, "%f %zd\n", newtime - *oldtime, bytes);
+		gettime_monotonic(&now);
+		timersub(&now, &ctl->oldtime, &delta);
+		fprintf(ctl->timingfp, "%ld.%06ld %zd\n", delta.tv_sec, delta.tv_usec, bytes);
 		if (ctl->fflg)
 			fflush(ctl->timingfp);
-		*oldtime = newtime;
+		ctl->oldtime = now;
 	}
 	if (fwrite_all(obuf, 1, bytes, ctl->typescriptfp)) {
 		warn(_("cannot write script file"));
@@ -214,7 +214,7 @@ static void write_output(struct script_control *ctl, char *obuf,
 	}
 }
 
-static void handle_io(struct script_control *ctl, int fd, double *oldtime, int i)
+static void handle_io(struct script_control *ctl, int fd, int i)
 {
 	char buf[BUFSIZ];
 	ssize_t bytes;
@@ -238,7 +238,7 @@ static void handle_io(struct script_control *ctl, int fd, double *oldtime, int i
 			write_all(ctl->master, &c, 1);
 		}
 	} else
-		write_output(ctl, buf, bytes, oldtime);
+		write_output(ctl, buf, bytes);
 }
 
 static void handle_signal(struct script_control *ctl, int fd)
@@ -268,7 +268,6 @@ static void do_io(struct script_control *ctl)
 {
 	struct pollfd pfd[POLLFDS];
 	int ret, i;
-	double oldtime = time(NULL);
 	time_t tvec = time((time_t *)NULL);
 	char buf[128];
 
@@ -284,6 +283,7 @@ static void do_io(struct script_control *ctl)
 
 	strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
 	fprintf(ctl->typescriptfp, _("Script started on %s"), buf);
+	gettime_monotonic(&ctl->oldtime);
 
 	while (!ctl->die) {
 		/* wait for input or signal */
@@ -300,7 +300,7 @@ static void do_io(struct script_control *ctl)
 			if (pfd[i].revents == 0)
 				continue;
 			if (i < 2) {
-				handle_io(ctl, pfd[i].fd, &oldtime, i);
+				handle_io(ctl, pfd[i].fd, i);
 				continue;
 			}
 			if (i == 2) {
-- 
2.3.0

[PATCH 12/12] script: add noreturn function attributes

[Sami Kerola]

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

diff --git a/term-utils/script.c b/term-utils/script.c
index 54e10b4..e15ea38 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -172,7 +172,7 @@ static void __attribute__((__noreturn__)) done(struct script_control *ctl)
 	exit(EXIT_SUCCESS);
 }
 
-static void fail(struct script_control *ctl)
+static void __attribute__((__noreturn__)) fail(struct script_control *ctl)
 {
 	kill(0, SIGTERM);
 	done(ctl);
@@ -336,7 +336,7 @@ static void getslave(struct script_control *ctl)
 	ioctl(ctl->slave, TIOCSCTTY, 0);
 }
 
-static void doshell(struct script_control *ctl)
+static void __attribute__((__noreturn__)) doshell(struct script_control *ctl)
 {
 	char *shname;
 
-- 
2.3.0

Re: [PATCH 00/12] pull: script(1) changes

[Karel Zak]

On Sun, Feb 22, 2015 at 02:42:37PM +0000, Sami Kerola wrote:
> Hello,
> 
> The following changes are clean ups and improvements to script(1)
> command.  These changes are also available from my github remote
> repository.

 Seems no tested with HAVE_LIBUTEMPTER...

 term-utils/script.c: In function ‘main’:
 term-utils/script.c:564:22: error: ‘master’ undeclared (first use in  this function)
 term-utils/script.c:564:22: note: each undeclared identifier is  reported only once for each function it appears in
 Makefile:9216: recipe for target 'term-utils/script-script.o' failed
 make: *** [term-utils/script-script.o] Error 1


    Karel

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

Re: [PATCH 00/12] pull: script(1) changes

[Karel Zak]

On Sun, Feb 22, 2015 at 02:42:37PM +0000, Sami Kerola wrote:
>  term-utils/script.c      | 872 ++++++++++++++++++++---------------------------
>  tests/ts/script/race     |   4 -

Note that I'd like to see more and more tests to avoid regressions
(like we have now with logger in v2.26).

    Karel

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

Re: [PATCH 00/12] pull: script(1) changes

[Karel Zak]

On Sun, Feb 22, 2015 at 02:42:37PM +0000, Sami Kerola wrote:
> The following changes are clean ups and improvements to script(1)
> command.  These changes are also available from my github remote
> repository.
> 
>  git://github.com/kerolasa/lelux-utiliteetit.git script

I'd like to merge this patch set, any idea how to really test it? ;-)

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

Re: [PATCH 00/12] pull: script(1) changes

[Sami Kerola]

On 21 May 2015 at 12:13, Karel Zak <kzak@redhat.com> wrote:
> On Sun, Feb 22, 2015 at 02:42:37PM +0000, Sami Kerola wrote:
>> The following changes are clean ups and improvements to script(1)
>> command.  These changes are also available from my github remote
>> repository.
>>
>>  git://github.com/kerolasa/lelux-utiliteetit.git script
>
> I'd like to merge this patch set, any idea how to really test it? ;-)

Hi Karel,

I added some tests to my 'script2' branch.

https://github.com/kerolasa/lelux-utiliteetit/commit/c7adcdde3ada54c1ede04c37ac44173e9b2e7ba4
https://github.com/kerolasa/lelux-utiliteetit/commit/4f692ce1c11a02fb7f636a79dd1758ffc08f0b30

The above tests are based on git log entries about old bugs and
regressions. I am sure tests could have greater coverage, but even
with these few I found a stupid error in earlier pull request -
'Script done on <timestamp>' line was not printed to typescript file.
That is now corrected.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 00/12] pull: script(1) changes

[Karel Zak]

On Sun, Feb 22, 2015 at 02:42:37PM +0000, Sami Kerola wrote:
> Hello,
> 
> The following changes are clean ups and improvements to script(1)
> command.  These changes are also available from my github remote
> repository.
> 
>  git://github.com/kerolasa/lelux-utiliteetit.git script

 script2

  - the branch contains also rtcwake changes!
  - the "script: replay" test hangs :-(
  - no tested with HAVE_LIBUTEMPTER, you need:

          -       utempter_add_record(master, NULL);
          +       utempter_add_record(ctl.master, NULL);

 
    Karel

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

Re: [PATCH 00/12] pull: script(1) changes

[Sami Kerola]

On 2 June 2015 at 13:05, Karel Zak <kzak@redhat.com> wrote:
>  script2

Hi Karel,

Corrections are available in the git repository at:

git://github.com/kerolasa/lelux-utiliteetit.git script3

The script2 is left untouched so that you, or anyone else can diff the
script.c two branches in order to see what was done.

>   - the branch contains also rtcwake changes!

Branch rtcwake3, script3, and more3 are now on top of the commit

c24d5e3903b0872d6ee956c45c0be760fa308a18

>   - the "script: replay" test hangs :-(

I cannot reproduce the problem, and hence I don't know what might be
going on. Perhaps the test should not be merged, or maybe it should
and some debugging ought to be added to see what exactly hangs and
why.

>   - no tested with HAVE_LIBUTEMPTER, you need:
>
>           -       utempter_add_record(master, NULL);
>           +       utempter_add_record(ctl.master, NULL);

Ooops. Fixed, and compiled successfully with libutempter.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 00/12] pull: script(1) changes

[Sami Kerola]

On 7 June 2015 at 21:25, Sami Kerola <kerolasa@iki.fi> wrote:
> On 2 June 2015 at 13:05, Karel Zak <kzak@redhat.com> wrote:
>>  script2
>
> Hi Karel,
>
> Corrections are available in the git repository at:
>
> git://github.com/kerolasa/lelux-utiliteetit.git script3

[snip]

>>   - the "script: replay" test hangs :-(
>
> I cannot reproduce the problem, and hence I don't know what might be
> going on. Perhaps the test should not be merged, or maybe it should
> and some debugging ought to be added to see what exactly hangs and
> why.

My work desktop hung, and it is somehow caused by the 'sleep 0.1' in:

https://github.com/kerolasa/lelux-utiliteetit/commit/c24d5e3903b0872d6ee956c45c0be760fa308a18#diff-2b763ca5b844d35fb0ade37e5dc2f4a1R28

If I remove the sleep all works. When I put some other 'not very quick
to run' command in place of the sleep hung again. With 'sleep 0.01'
the execution is successful. I am starting to feel this might be a bug
in script(1) --command option rather than in the test. I will continue
investigation later this evening. Meanwhile all proposals what could
be going on are welcome.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 00/12] pull: script(1) changes

[Sami Kerola]

On 8 June 2015 at 10:15, Sami Kerola <kerolasa@iki.fi> wrote:
> On 7 June 2015 at 21:25, Sami Kerola <kerolasa@iki.fi> wrote:
>>>   - the "script: replay" test hangs :-(
>>
>> I cannot reproduce the problem, and hence I don't know what might be
>> going on. Perhaps the test should not be merged, or maybe it should
>> and some debugging ought to be added to see what exactly hangs and
>> why.
>
> My work desktop hung, and it is somehow caused by the 'sleep 0.1' in:

And indeed my home gear hung when sleeping a bit longer.

> https://github.com/kerolasa/lelux-utiliteetit/commit/c24d5e3903b0872d6ee956c45c0be760fa308a18#diff-2b763ca5b844d35fb0ade37e5dc2f4a1R28
>
> If I remove the sleep all works. When I put some other 'not very quick
> to run' command in place of the sleep hung again. With 'sleep 0.01'
> the execution is successful. I am starting to feel this might be a bug
> in script(1) --command option rather than in the test. I will continue
> investigation later this evening. Meanwhile all proposals what could
> be going on are welcome.

Correct guess earlier the day. The --command option had logic issue,
that is now fixed and available in my script3 branch. Fix was nothing
more than a removal of negation operator.

@@ -254,7 +254,7 @@ static void handle_io(struct script_control *ctl,
int fd, int i)
                /* without sync write_output() will write both input &
                 * shell output that looks like double echoing */
                fdatasync(ctl->master);
-               if (!ctl->isterm && !feof(stdin)) {
+               if (!ctl->isterm && feof(stdin)) {
                        char c = DEF_EOF;
                        write_all(ctl->master, &c, sizeof(char));
                }

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 00/12] pull: script(1) changes

[Karel Zak]

On Sun, Feb 22, 2015 at 02:42:37PM +0000, Sami Kerola wrote:
>  git://github.com/kerolasa/lelux-utiliteetit.git script

 Merged with some another (my) changes.

    Karel

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