Bug 248377 - [PATCH] script(1): Enable proper playback [-p] of curses sessions
Summary: [PATCH] script(1): Enable proper playback [-p] of curses sessions
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Mark Johnston
URL:
Keywords:
: 248372 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-07-31 05:48 UTC by SOUMENDRA GANGULY
Modified: 2020-08-11 16:26 UTC (History)
3 users (show)

See Also:


Attachments
old.diff (2.61 KB, patch)
2020-07-31 05:48 UTC, SOUMENDRA GANGULY
no flags Details | Diff
script.diff (1.05 KB, patch)
2020-08-02 10:06 UTC, SOUMENDRA GANGULY
no flags Details | Diff
script.diff (941 bytes, patch)
2020-08-03 04:18 UTC, SOUMENDRA GANGULY
no flags Details | Diff
1.diff (957 bytes, patch)
2020-08-07 07:34 UTC, SOUMENDRA GANGULY
no flags Details | Diff
2.diff (1.11 KB, patch)
2020-08-08 10:49 UTC, SOUMENDRA GANGULY
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description SOUMENDRA GANGULY 2020-07-31 05:48:15 UTC
Created attachment 216900 [details]
old.diff

The current manpage of script(1) says the following.

"Certain interactive commands, such as vi(1), create garbage in the typescript file. The script utility works best with commands that do not manipulate the screen. The results are meant to emulate a hardcopy terminal, not an addressable one."

script(1) is currently able to record [-r] curses sessions perfectly; only the playback functionality [-p] has the above restriction; this patch removes this restriction by appropriately setting terminal attributes.
Comment 1 SOUMENDRA GANGULY 2020-07-31 05:50:59 UTC
Additional notes:

. I used https://github.com/iiSeymour/game-of-life to reproduce the problem; authored by GitHub user iiSeymour, this, in the words of the author, is "a python implementation of Conway's Game of Life using the curses module with a small evolutionary twist"; it was written for Linux; therefore, one must call "python gol.py" on FreeBSD to bypass the "#!".

. This is a duplicate of https://github.com/freebsd/freebsd/pull/433, which was also created by me. This was done to maximize the chances of getting noticed. If this patch on Bugzilla is accepted, I will close the GitHub pull request, and vice versa.

. I submitted a similar patch for "scriptreplay(1)" from util-linux; it has now been merged by the maintainer of the project.
Comment 2 SOUMENDRA GANGULY 2020-07-31 05:56:24 UTC
*** Bug 248372 has been marked as a duplicate of this bug. ***
Comment 3 Conrad Meyer freebsd_committer 2020-07-31 15:44:44 UTC
Next time it is fine to just update the patch on the existing bug, FYI.
Comment 4 SOUMENDRA GANGULY 2020-07-31 19:44:32 UTC
(In reply to Conrad Meyer from comment #3)
Got it! Sorry about the mess. Bugzilla n00b here. I wanted to add author info to the patch and restructure the comments, but could not find a way to edit the comments. Thank you for the suggestion.
Comment 5 SOUMENDRA GANGULY 2020-08-01 07:55:25 UTC
Note: To reproduce the problem, please run the script(1) session in a terminal that is not in raw mode. Thank you.
Comment 6 SOUMENDRA GANGULY 2020-08-02 03:54:08 UTC
Conceptually more accurate version of previous note: To reproduce the problem, please run the script(1) session in a terminal that does not have post processing turned off.

NetBSD has accepted the corresponding patch. They modified it slightly: they call atexit(termreset) instead of the several termreset() calls that are in this patch. While not necessary, upon request, I can edit this patch to reflect that change.
Comment 7 SOUMENDRA GANGULY 2020-08-02 10:06:35 UTC
Created attachment 216951 [details]
script.diff

This patch removes a lot of redundant code that was there in the old one.
Comment 8 SOUMENDRA GANGULY 2020-08-03 04:18:03 UTC
Created attachment 216973 [details]
script.diff

Removes unnecessary "ttyflg = 0" at the beginning of playback() in previous patch because static global variable ttyflg will automatically be initialized to 0 by compiler.
Comment 9 SOUMENDRA GANGULY 2020-08-07 07:34:23 UTC
Created attachment 217063 [details]
1.diff

Adds elegant error handling. For example, instead of using isatty followed by tcgetattr, tcgetattr is directly used. This is logical because freebsd/lib/libc/gen/isatty.c suggests that isatty is actually implemented using tcgetattr. Upon failure of tcgetattr, errno is checked to decide if stdout is a tty. 

Note: I am comfortable with Bugzilla now. Therefore, I am closing the GitHub pull request.
Comment 10 Mark Johnston freebsd_committer 2020-08-07 18:46:48 UTC
I think this is fine.  I tested it with a few curses-based games and it fixed some issues with characters appearing in the wrong place during playback.  Some curses programs appear to play back properly with or without the change.
Comment 11 commit-hook freebsd_committer 2020-08-07 18:49:24 UTC
A commit references this bug:

Author: markj
Date: Fri Aug  7 18:48:57 UTC 2020
New revision: 364039
URL: https://svnweb.freebsd.org/changeset/base/364039

Log:
  script: Put the terminal in raw mode when playing back a session.

  Otherwise recorded sessions of some interactive programs do not play
  back properly.

  PR:		248377
  Submitted by:	Soumendra Ganguly <0.gangzta@gmail.com>
  MFC after:	1 week

Changes:
  head/usr.bin/script/script.c
Comment 12 SOUMENDRA GANGULY 2020-08-07 20:39:42 UTC
(In reply to Mark Johnston from comment #10)
Thank you! I am glad I could contribute to FreeBSD. I love it.

You are correct; some programs do fine without it.
Comment 13 Mark Johnston freebsd_committer 2020-08-07 22:29:24 UTC
(In reply to SOUMENDRA GANGULY from comment #12)
No problem, thanks for taking the time to submit a patch.
Comment 14 SOUMENDRA GANGULY 2020-08-08 10:42:36 UTC
(In reply to Mark Johnston from comment #13)
Thank you. Can we please keep this in "in progress" status for a little while more? I am still working on the program.
Comment 15 SOUMENDRA GANGULY 2020-08-08 10:49:36 UTC
Created attachment 217091 [details]
2.diff

As pointed out earlier, isatty is unnecessary. It simply checks for tcgetattr failure. This adds more elegant error handling to certain parts based on errno set by tcgetattr.

Also, upon more testing, I found out that errno == EBADF is very unusual for tcgetattr in this case: script(1) of course does not close its own stdout voluntarily mid execution.

gdb was able to force an EBADF [ p close(1) ]. Therefore, the EBADF error handling is not completely removed by this patch; it is only changed to errno != ENOTTY.

Thank you.
Comment 16 SOUMENDRA GANGULY 2020-08-09 06:26:59 UTC
(In reply to commit-hook from comment #11)
Also, I am not not sure if this is allowed/possible, but if it is, then could you please change/set my email to <soumendraganguly@gmail.com> in the "Submitted by:" field? At least for 2.diff? Name of course stays the same.
Comment 17 Mark Johnston freebsd_committer 2020-08-10 14:57:46 UTC
(In reply to SOUMENDRA GANGULY from comment #16)
Sorry, it's not possible to edit svn commit logs after the fact.  I just used the email associated with your bugzilla account.  I'll try to use the other email for future commits, but I'd suggest making another account to ensure that others don't do what I did.
Comment 18 Mark Johnston freebsd_committer 2020-08-10 15:38:10 UTC
(In reply to SOUMENDRA GANGULY from comment #15)
Are you happy with this version of the patch?
Comment 19 SOUMENDRA GANGULY 2020-08-10 20:03:53 UTC
(In reply to Mark Johnston from comment #18)
Yes, I am happy with this. Thank you.

Note: 1.diff is the patch that you applied before. 2.diff changes other things.

Thank you.
Comment 20 commit-hook freebsd_committer 2020-08-11 14:19:26 UTC
A commit references this bug:

Author: markj
Date: Tue Aug 11 14:19:06 UTC 2020
New revision: 364112
URL: https://svnweb.freebsd.org/changeset/base/364112

Log:
  script: Minor cleanups.

  - Instead of using isatty() to decide whether to call tcgetattr(), just
    call tcgetattr() directly, since that's all that isatty() does anyway.
  - Simplify error handling in termset().  Check for errno != ENOTTY from
    tcgetattr() to handle errors that may be raised while running
    script(1) under a debugger.

  PR:		248377
  Submitted by:	Soumendra Ganguly <soumendraganguly@gmail.com>
  MFC after:	1 week

Changes:
  head/usr.bin/script/script.c
Comment 21 SOUMENDRA GANGULY 2020-08-11 16:26:45 UTC
(In reply to commit-hook from comment #20)
Thank you, sir.