Any other dev interested in fixing this? Feedback, suggestions, review? Tim.
Andrew Hewus Fresh <[email protected]> wrote: > In my quick test, this works a lot better than what we have now. At > least I get back more of the file I was working on. I also haven't been > able to reproduce the annoying segfault and half the original file > disappearing, although that was infrequent before so could just be luck. > > User experience with this patch is improved in my opinion, I'd like to > see it go in, so OK afresh1@ > > On Sat, Oct 09, 2021 at 08:26:13PM -0400, trondd 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;
