[PATCH] script: Also flush writes to timing file.

[PATCH] script: Also flush writes to timing file.

[Jesper Dahl Nyerup]

If both -f and -t are given, flush the timing fd on each write, similar
to the behavior on the script fd. This allows playback of still-running
sessions, and reduces the risk of ending up with empty timing files when
script(1) exits abnormally.

Signed-off-by: Jesper Dahl Nyerup <nyerup@one.com>
---
 term-utils/script.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/term-utils/script.c b/term-utils/script.c
index a4c2e0c..49f21b6 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -442,6 +442,8 @@ dooutput(FILE *timingfd) {
 		}
 		if (fflg)
 			fflush(fscript);
+			if (tflg)
+				fflush(timingfd);
 		if (write_all(STDOUT_FILENO, obuf, cc)) {
 			warn (_("write failed"));
 			fail();
-- 
1.8.4

Re: [PATCH] script: Also flush writes to timing file.

[Sami Kerola]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On 17 February 2014 21:43, Jesper Dahl Nyerup <nyerup@one.com> wrote:
> If both -f and -t are given, flush the timing fd on each write, similar
> to the behavior on the script fd. This allows playback of still-running
> sessions, and reduces the risk of ending up with empty timing files when
> script(1) exits abnormally.

Hi Jesper,

Instead of flushing all the time how about checking write status at
the end? See attached patch how that could work.

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

[-- Attachment #2: 0001-script-Also-flush-writes-to-timing-file.txt --]
[-- Type: text/plain, Size: 3783 bytes --]

From 6b1bb220173b69d1b3ea80153be08f7a8661775b Mon Sep 17 00:00:00 2001
From: Jesper Dahl Nyerup <nyerup@one.com>
Date: Mon, 17 Feb 2014 22:43:06 +0100
Subject: [PATCH] script: Also flush writes to timing file.
Organization: lastminute.com

If both -f and -t are given, flush the timing fd on each write, similar
to the behavior on the script fd. This allows playback of still-running
sessions, and reduces the risk of ending up with empty timing files when
script(1) exits abnormally.

[kerolasa@iki.fi: - check timing file write status in done()]

Signed-off-by: Jesper Dahl Nyerup <nyerup@one.com>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/script.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index a4c2e0c..4b816ec 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -78,8 +78,8 @@
 #define DEFAULT_OUTPUT "typescript"
 
 void finish(int);
-void done(void);
-void fail(void);
+void done(FILE *timingfd);
+void fail(FILE *timingfd);
 void resize(int);
 void fixtty(void);
 void getmaster(void);
@@ -237,7 +237,7 @@ main(int argc, char **argv) {
 
 	if ((fscript = fopen(fname, aflg ? "a" : "w")) == NULL) {
 		warn(_("cannot open %s"), fname);
-		fail();
+		fail(timingfd);
 	}
 
 	shell = getenv("SHELL");
@@ -268,7 +268,7 @@ main(int argc, char **argv) {
 
 	if (child < 0) {
 		warn(_("fork failed"));
-		fail();
+		fail(timingfd);
 	}
 	if (child == 0) {
 
@@ -278,7 +278,7 @@ main(int argc, char **argv) {
 
 		if (child < 0) {
 			warn(_("fork failed"));
-			fail();
+			fail(timingfd);
 		}
 		if (child) {
 			if (!timingfd)
@@ -319,7 +319,7 @@ doinput(void) {
 		if ((cc = read(STDIN_FILENO, ibuf, BUFSIZ)) > 0) {
 			if (write_all(master, ibuf, cc)) {
 				warn (_("write failed"));
-				fail();
+				fail(NULL);
 			}
 		}
 		else if (cc < 0 && errno == EINTR && resized)
@@ -355,7 +355,7 @@ doinput(void) {
 
 		if (write_all(master, &c, 1)) {
 			warn (_("write failed"));
-			fail();
+			fail(NULL);
 		}
 
 		/* wait for "exit" message from shell before we print "Script
@@ -365,7 +365,7 @@ doinput(void) {
 
 	if (!die)
 		finish(0);	/* wait for childern */
-	done();
+	done(NULL);
 }
 
 void
@@ -438,19 +438,15 @@ dooutput(FILE *timingfd) {
 		}
 		if (fwrite_all(obuf, 1, cc, fscript)) {
 			warn (_("cannot write script file"));
-			fail();
+			fail(timingfd);
 		}
-		if (fflg)
-			fflush(fscript);
 		if (write_all(STDOUT_FILENO, obuf, cc)) {
 			warn (_("write failed"));
-			fail();
+			fail(timingfd);
 		}
 	} while(1);
 
-	if (close_stream(timingfd) != 0)
-		errx(EXIT_FAILURE, _("write error"));
-	done();
+	done(timingfd);
 }
 
 void
@@ -496,7 +492,7 @@ doshell(void) {
 			execlp(shname, "-i", NULL);
 	}
 	warn(_("failed to execute %s"), shell);
-	fail();
+	fail(NULL);
 }
 
 void
@@ -513,14 +509,14 @@ fixtty(void) {
 }
 
 void
-fail(void) {
+fail(FILE *timingfd) {
 
 	kill(0, SIGTERM);
-	done();
+	done(timingfd);
 }
 
 void __attribute__((__noreturn__))
-done(void) {
+done(FILE *timingfd) {
 	time_t tvec;
 
 	if (subchild) {
@@ -530,8 +526,10 @@ done(void) {
 		my_strftime(buf, sizeof buf, "%c\n", localtime(&tvec));
 		fprintf(fscript, _("\nScript done on %s"), buf);
 
+		if (timingfd && close_stream(timingfd) != 0)
+			warn(_("write error"));
 		if (close_stream(fscript) != 0)
-			errx(EXIT_FAILURE, _("write error"));
+			err(EXIT_FAILURE, _("write error"));
 		close(master);
 
 		master = -1;
@@ -574,7 +572,7 @@ getmaster(void) {
 
 	if (rc < 0) {
 		warn(_("openpty failed"));
-		fail();
+		fail(NULL);
 	}
 #else
 	char *pty, *bank, *cp;
@@ -614,7 +612,7 @@ getmaster(void) {
 	}
 	master = -1;
 	warn(_("out of pty's"));
-	fail();
+	fail(NULL);
 #endif /* not HAVE_LIBUTIL */
 }
 
-- 
1.9.0

Re: [PATCH] script: Also flush writes to timing file.

[Jesper Dahl Nyerup]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

On Feb 18  10:08, Sami Kerola wrote:
> On 17 February 2014 21:43, Jesper Dahl Nyerup <nyerup@one.com> wrote:
> > If both -f and -t are given, flush the timing fd on each write, similar
> > to the behavior on the script fd. This allows playback of still-running
> > sessions, and reduces the risk of ending up with empty timing files when
> > script(1) exits abnormally.
> 
> Instead of flushing all the time how about checking write status at
> the end? See attached patch how that could work.

That's a good idea, and should take care of the scenario where an
abnormal exit leads to empty timings The other use case however,
scriptreplay(1)'ing transcripts while script(1) is still running, would
not be improved upon.

Besides, the man page describes the -f option behavior as `Flush output
after each write`. I would expect this to apply to the timing fd as
well, and honoring this description was the main purpose of my patch.

I think it'd be great if both patches were applied.

J.
-- 
Jesper Dahl Nyerup
Systems Engineer
One.com, nyerup@one.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH] script: Also flush writes to timing file.

[Jesper Dahl Nyerup]

If both -f and -t are given, flush the timing fd on each write, similar
to the behavior on the script fd. This allows playback of still-running
sessions, and reduces the risk of ending up with empty timing files when
script(1) exits abnormally.

Signed-off-by: Jesper Dahl Nyerup <nyerup@one.com>
---
 term-utils/script.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index a4c2e0c..d6e04d6 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -440,8 +440,11 @@ dooutput(FILE *timingfd) {
 			warn (_("cannot write script file"));
 			fail();
 		}
-		if (fflg)
+		if (fflg) {
 			fflush(fscript);
+			if (tflg)
+				fflush(timingfd);
+		}
 		if (write_all(STDOUT_FILENO, obuf, cc)) {
 			warn (_("write failed"));
 			fail();
-- 
1.8.4

Re: [PATCH] script: Also flush writes to timing file.

[Jesper Dahl Nyerup]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

On Feb 18  20:48, Jesper Dahl Nyerup wrote:
> ---
>  term-utils/script.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

I apologise.

This second patch is obviously the correct one. Please disregard my
first submission in this thread.

J.
-- 
Jesper Dahl Nyerup
Systems Engineer
One.com, nyerup@one.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] script: Also flush writes to timing file.

[Karel Zak]

On Tue, Feb 18, 2014 at 08:48:45PM +0100, Jesper Dahl Nyerup wrote:
>  term-utils/script.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

 Applied, thanks.

 I have also made the files usage more robust and close all in done()
 (based on Sami's suggestion).

    Karel

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

Re: [PATCH] script: Also flush writes to timing file.

[Jesper Dahl Nyerup]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

On Feb 21  13:44, Karel Zak wrote:
> On Tue, Feb 18, 2014 at 08:48:45PM +0100, Jesper Dahl Nyerup wrote:
> >  term-utils/script.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> 
>  Applied, thanks.
> 
>  I have also made the files usage more robust and close all in done()
>  (based on Sami's suggestion).

Thanks!

J.
-- 
Jesper Dahl Nyerup
Systems Engineer
One.com, nyerup@one.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]