Any feedback, direction, or suggestions? I'd like to see something get in as the current situation not only doesn't recover unsaved work but it sets a user up to potentially lose saved work, too.
Tim. trondd <tro...@kagu-tsuchi.com> wrote: > This is a new attempt at fixing vi(1) recovery by actually writing > to the recovery file. Previously I restored the SIGALRM method that > was deleted in the 90's but wondered if that was still the best way > to handle this. Checking and syncing to the recovery every 2 minutes > seems arbitrary and overly cautious. > > This attempt takes it to the other direction. I'm writing each > change to the recovery file immediately after the in-memory database > is modified. Though, I can see that this might have a noticeable > impact on slower file systems. > > VIM takes a sort of hybrid approach and writes to the backup every > 200 characters or after 4 seconds of idle time. Perhaps this is the > best method to not get too far behind while also not hammering the > filesystem with quick edits. > > For now, I'm sticking to the naive approach for review. The diff is > smaller than using SIGALRM and more straight forward and I'd like to > hear what method might make sense to improve the process. This code > would probably be the basis for other improvements. > > Below is my original explanation of the problem with vi(1)'s > recovery. > > This is a reference to the older SIGALRM diff (I have a version > updated to use the atomic signal flags if we want to look at the > SIGALRM process instead). > https://marc.info/?l=openbsd-tech&m=162940011614049 > > Tim. > > ----- > > While investigating an occasional crash when recovering a file with 'vi -r' > after a power failure, I noticed that the recovery files are actually never > updated during an editing session. The recovery files are created upon > initial modification of the file which saves the state of the file at the > time of the edit. You can work on a file for as long as you want and even > write it to disk but the recovery file is never updated. If the session is > then lost due to power failure or a SIGKILL and you attempt to recover with > -r, you'll be presented with the contents of the file from that first edit. > It won't contain unsaved changes nor even any changes manually written to > disk to the original file. Accepting the recovered version would lose all > of your work. > > Reading the vi docs, man page, and source comments in the OpenBSD tree, they > all mention the use of SIGALRM to periodically save changes to the recovery > file. However, the code never sets up a handler or captures SIGALRM. It > only ever updates the recovery file on a SIGTERM but then also exits, I > guess to cover the case of an inadvertent clean system shutdown. > > I dug through an nvi source repository[0] that seemed to cover it's entire > history and it seems this functionality was lost somewhere around 1994 and I > don't see it having been replaced by anything else. Our version seems to be > from 1996 and editors/nvi in ports still lacks code to update the recovery > file, as well. > > What I've done is re-implement periodic updates to the recovery file using > SIGALRM and a timer like the original implementation but rewritten a bit to > fit the newer source file layout and event handling. That keeps the recovery > updated every 2 minutes. Then it seemed silly to be able to write changes to > the original file and if a crash happens before the next SIGALRM, recovery > would try to roll you back to before those saved changes. So I've also added > a call to sync the recovery file if you explicitly write changes to disk. I > don't think the recovery system should try to punish you for actively saving > your work even if it is only at most 2 minutes worth. > > Comments or feedback? I'm unsure I've covered all caveats with this code or > if there are vi/ex usecases where it won't work correctly. For testing, I've > covered my usage and several scenarios I could contrive but I don't regularly > use ex, for example, or change many options from the default. I've been > running with this code for a week. And I suppose there must be a reason no > one has noticed or cared about this for over 20 years. Everyone else uses > vim, I guess? > > Tim. > > [0] https://repo.or.cz/nvi.git > > ----- > Index: common/exf.h =================================================================== RCS file: /cvs/src/usr.bin/vi/common/exf.h,v retrieving revision 1.5 diff -u -p -r1.5 exf.h --- common/exf.h 24 Apr 2015 21:48:31 -0000 1.5 +++ common/exf.h 9 Oct 2021 22:40:17 -0000 @@ -58,7 +58,8 @@ struct _exf { #define F_RCV_NORM 0x020 /* Don't delete recovery files. */ #define F_RCV_ON 0x040 /* Recovery is possible. */ #define F_UNDO 0x080 /* No change since last undo. */ - u_int8_t flags; +#define F_RCV_SYNC 0x100 /* Recovery file sync needed. */ + u_int16_t flags; }; /* Flags to db_get(). */ Index: common/line.c =================================================================== RCS file: /cvs/src/usr.bin/vi/common/line.c,v retrieving revision 1.15 diff -u -p -r1.15 line.c --- common/line.c 6 Jan 2016 22:28:52 -0000 1.15 +++ common/line.c 9 Oct 2021 22:40:17 -0000 @@ -218,7 +218,7 @@ db_delete(SCR *sp, recno_t lno) /* File now modified. */ if (F_ISSET(ep, F_FIRSTMODIFY)) (void)rcv_init(sp); - F_SET(ep, F_MODIFIED); + F_SET(ep, F_MODIFIED | F_RCV_SYNC); /* Update screen. */ return (scr_update(sp, lno, LINE_DELETE, 1)); @@ -266,7 +266,7 @@ db_append(SCR *sp, int update, recno_t l /* File now dirty. */ if (F_ISSET(ep, F_FIRSTMODIFY)) (void)rcv_init(sp); - F_SET(ep, F_MODIFIED); + F_SET(ep, F_MODIFIED | F_RCV_SYNC); /* Log change. */ log_line(sp, lno + 1, LOG_LINE_APPEND); @@ -334,7 +334,7 @@ db_insert(SCR *sp, recno_t lno, char *p, /* File now dirty. */ if (F_ISSET(ep, F_FIRSTMODIFY)) (void)rcv_init(sp); - F_SET(ep, F_MODIFIED); + F_SET(ep, F_MODIFIED | F_RCV_SYNC); /* Log change. */ log_line(sp, lno, LOG_LINE_INSERT); @@ -394,7 +394,7 @@ db_set(SCR *sp, recno_t lno, char *p, si /* File now dirty. */ if (F_ISSET(ep, F_FIRSTMODIFY)) (void)rcv_init(sp); - F_SET(ep, F_MODIFIED); + F_SET(ep, F_MODIFIED | F_RCV_SYNC); /* Log after change. */ log_line(sp, lno, LOG_LINE_RESET_F); Index: common/recover.c =================================================================== RCS file: /cvs/src/usr.bin/vi/common/recover.c,v retrieving revision 1.30 diff -u -p -r1.30 recover.c --- common/recover.c 22 Jul 2019 12:39:02 -0000 1.30 +++ common/recover.c 9 Oct 2021 22:40:17 -0000 @@ -252,6 +252,8 @@ rcv_sync(SCR *sp, u_int flags) /* Sync the file if it's been modified. */ if (F_ISSET(ep, F_MODIFIED)) { + /* Clear recovery sync flag. */ + F_CLR(ep, F_RCV_SYNC); if (ep->db->sync(ep->db, R_RECNOSYNC)) { F_CLR(ep, F_RCV_ON | F_RCV_NORM); msgq_str(sp, M_SYSERR, Index: ex/ex.c =================================================================== RCS file: /cvs/src/usr.bin/vi/ex/ex.c,v retrieving revision 1.21 diff -u -p -r1.21 ex.c --- ex/ex.c 19 Mar 2016 00:21:28 -0000 1.21 +++ ex/ex.c 9 Oct 2021 22:40:19 -0000 @@ -133,6 +133,10 @@ ex(SCR **spp) msgq(sp, M_ERR, "Interrupted"); } + /* Sync recovery if changes were made. */ + if (F_ISSET(sp->ep, F_RCV_SYNC)) + rcv_sync(sp, 0); + /* * If the last command caused a restart, or switched screens * or into vi, return. Index: vi/vi.c =================================================================== RCS file: /cvs/src/usr.bin/vi/vi/vi.c,v retrieving revision 1.22 diff -u -p -r1.22 vi.c --- vi/vi.c 24 Jan 2019 15:09:41 -0000 1.22 +++ vi/vi.c 9 Oct 2021 22:40:19 -0000 @@ -400,6 +400,10 @@ intr: CLR_INTERRUPT(sp); (void)sp->gp->scr_rename(sp, sp->frp->name, 1); } + /* Sync recovery if changes were made. */ + if (F_ISSET(sp->ep, F_RCV_SYNC)) + rcv_sync(sp, 0); + /* If leaving vi, return to the main editor loop. */ if (F_ISSET(gp, G_SRESTART) || F_ISSET(sp, SC_EX)) { *spp = sp;
