* [PATCH 15/22] script: use signalfd() to catch signals
2014-12-27 17:17 [PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes Sami Kerola
@ 2014-12-27 17:17 ` Sami Kerola
2014-12-27 17:17 ` [PATCH 16/22] script: use poll() rather than select() Sami Kerola
` (6 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Sami Kerola @ 2014-12-27 17:17 UTC (permalink / raw)
To: util-linux; +Cc: Sami Kerola, Wolfgang Richter, Ruediger Meier
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 16/22] script: use poll() rather than select()
2014-12-27 17:17 [PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes Sami Kerola
2014-12-27 17:17 ` [PATCH 15/22] script: use signalfd() to catch signals Sami Kerola
@ 2014-12-27 17:17 ` Sami Kerola
2014-12-27 17:17 ` [PATCH 17/22] script: merge doinput() and output() functions to do_io() Sami Kerola
` (5 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Sami Kerola @ 2014-12-27 17:17 UTC (permalink / raw)
To: util-linux; +Cc: Sami Kerola, Wolfgang Richter, Ruediger Meier
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 17/22] script: merge doinput() and output() functions to do_io()
2014-12-27 17:17 [PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes Sami Kerola
2014-12-27 17:17 ` [PATCH 15/22] script: use signalfd() to catch signals Sami Kerola
2014-12-27 17:17 ` [PATCH 16/22] script: use poll() rather than select() Sami Kerola
@ 2014-12-27 17:17 ` Sami Kerola
2014-12-27 17:17 ` [PATCH 18/22] script: remove io vs signal race Sami Kerola
` (4 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Sami Kerola @ 2014-12-27 17:17 UTC (permalink / raw)
To: util-linux; +Cc: Sami Kerola, Wolfgang Richter, Ruediger Meier
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 18/22] script: remove io vs signal race
2014-12-27 17:17 [PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes Sami Kerola
` (2 preceding siblings ...)
2014-12-27 17:17 ` [PATCH 17/22] script: merge doinput() and output() functions to do_io() Sami Kerola
@ 2014-12-27 17:17 ` Sami Kerola
2014-12-27 17:17 ` [PATCH 19/22] script: add 'Script started' line always to capture file Sami Kerola
` (3 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Sami Kerola @ 2014-12-27 17:17 UTC (permalink / raw)
To: util-linux; +Cc: Sami Kerola, Wolfgang Richter, Ruediger Meier
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 19/22] script: add 'Script started' line always to capture file
2014-12-27 17:17 [PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes Sami Kerola
` (3 preceding siblings ...)
2014-12-27 17:17 ` [PATCH 18/22] script: remove io vs signal race Sami Kerola
@ 2014-12-27 17:17 ` Sami Kerola
2014-12-27 17:17 ` [PATCH 20/22] script: move do_io() content to small functions Sami Kerola
` (2 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Sami Kerola @ 2014-12-27 17:17 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 20/22] script: move do_io() content to small functions
2014-12-27 17:17 [PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes Sami Kerola
` (4 preceding siblings ...)
2014-12-27 17:17 ` [PATCH 19/22] script: add 'Script started' line always to capture file Sami Kerola
@ 2014-12-27 17:17 ` Sami Kerola
2014-12-27 17:17 ` [PATCH 21/22] script: replace strftime() workaround with CFLAGS = -Wno-format-y2k Sami Kerola
2014-12-27 17:17 ` [PATCH 22/22] script: use correct input type, move comment, and so on Sami Kerola
7 siblings, 0 replies; 10+ messages in thread
From: Sami Kerola @ 2014-12-27 17:17 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 21/22] script: replace strftime() workaround with CFLAGS = -Wno-format-y2k
2014-12-27 17:17 [PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes Sami Kerola
` (5 preceding siblings ...)
2014-12-27 17:17 ` [PATCH 20/22] script: move do_io() content to small functions Sami Kerola
@ 2014-12-27 17:17 ` Sami Kerola
2014-12-27 17:17 ` [PATCH 22/22] script: use correct input type, move comment, and so on Sami Kerola
7 siblings, 0 replies; 10+ messages in thread
From: Sami Kerola @ 2014-12-27 17:17 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 22/22] script: use correct input type, move comment, and so on
2014-12-27 17:17 [PATCH 00/22] pull: follow-up to chsh, newgrp, and script changes Sami Kerola
` (6 preceding siblings ...)
2014-12-27 17:17 ` [PATCH 21/22] script: replace strftime() workaround with CFLAGS = -Wno-format-y2k Sami Kerola
@ 2014-12-27 17:17 ` Sami Kerola
2015-01-06 11:11 ` Karel Zak
7 siblings, 1 reply; 10+ messages in thread
From: Sami Kerola @ 2014-12-27 17:17 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 22/22] script: use correct input type, move comment, and so on
2014-12-27 17:17 ` [PATCH 22/22] script: use correct input type, move comment, and so on Sami Kerola
@ 2015-01-06 11:11 ` Karel Zak
0 siblings, 0 replies; 10+ messages in thread
From: Karel Zak @ 2015-01-06 11:11 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux
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
^ permalink raw reply [flat|nested] 10+ messages in thread