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;

Reply via email to