Vi(1) recovery is broken. At best, you lose your new work. At the worst you can even lose saved work on disk. Pushing again to get something in place to help the situation.
I've been running this for a long time on amd64 and arm64. Besides my testing, I've also experienced real power failures without data lose or a crash during recovery. Tim. trondd <tro...@kagu-tsuchi.com> wrote: > Any other dev interested in fixing this? Feedback, suggestions, review? > > Tim. > > Andrew Hewus Fresh <and...@afresh1.com> 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;
