I've switched this to using alarm(3) instead of setitimer(2) which is a
little simpler in the code but most of the changes are just vi event
handling boilderplate.  There is very little new functional code.

Original investigation write-up follows.  Have had quite a bit of user
feedback and testing in support of fixing this.

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: cl/cl.h
===================================================================
RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
retrieving revision 1.10
diff -u -p -r1.10 cl.h
--- cl/cl.h     27 May 2016 09:18:11 -0000      1.10
+++ cl/cl.h     19 Aug 2021 19:04:01 -0000
@@ -30,8 +30,9 @@ typedef struct _cl_private {
 #define        INDX_HUP        0
 #define        INDX_INT        1
 #define        INDX_TERM       2
-#define        INDX_WINCH      3
-#define        INDX_MAX        4       /* Original signal information. */
+#define        INDX_ALRM       3
+#define        INDX_WINCH      4
+#define        INDX_MAX        5       /* Original signal information. */
        struct sigaction oact[INDX_MAX];
 
        enum {                  /* Tty group write mode. */
@@ -48,8 +49,9 @@ typedef struct _cl_private {
 #define        CL_SIGHUP       0x0020  /* SIGHUP arrived. */
 #define        CL_SIGINT       0x0040  /* SIGINT arrived. */
 #define        CL_SIGTERM      0x0080  /* SIGTERM arrived. */
-#define        CL_SIGWINCH     0x0100  /* SIGWINCH arrived. */
-#define        CL_STDIN_TTY    0x0200  /* Talking to a terminal. */
+#define        CL_SIGALRM      0x0100  /* SIGALRM arrived. */
+#define        CL_SIGWINCH     0x0200  /* SIGWINCH arrived. */
+#define        CL_STDIN_TTY    0x0400  /* Talking to a terminal. */
        u_int32_t flags;
 } CL_PRIVATE;
 
Index: cl/cl_main.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
retrieving revision 1.33
diff -u -p -r1.33 cl_main.c
--- cl/cl_main.c        5 May 2016 20:36:41 -0000       1.33
+++ cl/cl_main.c        19 Aug 2021 19:04:01 -0000
@@ -239,6 +239,14 @@ h_term(int signo)
 }
 
 static void
+h_alrm(int signo)
+{
+       GLOBAL_CLP;
+
+       F_SET(clp, CL_SIGALRM);
+}
+
+static void
 h_winch(int signo)
 {
        GLOBAL_CLP;
@@ -264,6 +272,7 @@ sig_init(GS *gp, SCR *sp)
                if (setsig(SIGHUP, &clp->oact[INDX_HUP], h_hup) ||
                    setsig(SIGINT, &clp->oact[INDX_INT], h_int) ||
                    setsig(SIGTERM, &clp->oact[INDX_TERM], h_term) ||
+                   setsig(SIGALRM, &clp->oact[INDX_ALRM], h_alrm) ||
                    setsig(SIGWINCH, &clp->oact[INDX_WINCH], h_winch)
                    )
                        err(1, NULL);
@@ -271,6 +280,7 @@ sig_init(GS *gp, SCR *sp)
                if (setsig(SIGHUP, NULL, h_hup) ||
                    setsig(SIGINT, NULL, h_int) ||
                    setsig(SIGTERM, NULL, h_term) ||
+                   setsig(SIGALRM, NULL, h_alrm) ||
                    setsig(SIGWINCH, NULL, h_winch)
                    ) {
                        msgq(sp, M_SYSERR, "signal-reset");
@@ -313,6 +323,7 @@ sig_end(GS *gp)
        (void)sigaction(SIGHUP, NULL, &clp->oact[INDX_HUP]);
        (void)sigaction(SIGINT, NULL, &clp->oact[INDX_INT]);
        (void)sigaction(SIGTERM, NULL, &clp->oact[INDX_TERM]);
+       (void)sigaction(SIGALRM, NULL, &clp->oact[INDX_ALRM]);
        (void)sigaction(SIGWINCH, NULL, &clp->oact[INDX_WINCH]);
 }
 
Index: cl/cl_read.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/cl/cl_read.c,v
retrieving revision 1.21
diff -u -p -r1.21 cl_read.c
--- cl/cl_read.c        27 May 2016 09:18:11 -0000      1.21
+++ cl/cl_read.c        19 Aug 2021 19:04:01 -0000
@@ -62,7 +62,7 @@ retest:       if (LF_ISSET(EC_INTERRUPT) || F_
                        evp->e_event = E_TIMEOUT;
                return (0);
        }
-       if (F_ISSET(clp, CL_SIGHUP | CL_SIGTERM | CL_SIGWINCH)) {
+       if (F_ISSET(clp, CL_SIGHUP | CL_SIGTERM | CL_SIGWINCH | CL_SIGALRM)) {
                if (F_ISSET(clp, CL_SIGHUP)) {
                        evp->e_event = E_SIGHUP;
                        return (0);
@@ -81,6 +81,11 @@ retest:      if (LF_ISSET(EC_INTERRUPT) || F_
                                return (0);
                        }
                        /* No real change, ignore the signal. */
+               }
+               if (F_ISSET(clp, CL_SIGALRM)) {
+                       F_CLR(clp, CL_SIGALRM);
+                       evp->e_event = E_SIGALRM;
+                       return (0);
                }
        }
 
Index: common/key.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/common/key.c,v
retrieving revision 1.19
diff -u -p -r1.19 key.c
--- common/key.c        18 Apr 2017 01:45:35 -0000      1.19
+++ common/key.c        19 Aug 2021 19:04:01 -0000
@@ -542,6 +542,10 @@ loop:              if (gp->scr_event(sp, argp,
                        v_sync(sp, RCV_ENDSESSION | RCV_PRESERVE |
                            (argp->e_event == E_SIGTERM ? 0: RCV_EMAIL));
                        return (1);
+               case E_SIGALRM:
+                       /* Sync to recovery if the file has been modified. */
+                       v_sync(sp, 0);
+                       return(0);
                case E_TIMEOUT:
                        istimeout = 1;
                        break;
@@ -756,6 +760,9 @@ v_event_err(SCR *sp, EVENT *evp)
                break;
        case E_WRITE:
                msgq(sp, M_ERR, "Unexpected write event");
+               break;
+       case E_SIGALRM:
+               msgq(sp, M_ERR, "Unexpected SIGALRM event");
                break;
 
        /*
Index: common/key.h
===================================================================
RCS file: /cvs/src/usr.bin/vi/common/key.h,v
retrieving revision 1.8
diff -u -p -r1.8 key.h
--- common/key.h        27 May 2016 09:18:11 -0000      1.8
+++ common/key.h        19 Aug 2021 19:04:01 -0000
@@ -42,6 +42,7 @@ typedef enum {
        E_QUIT,                         /* Quit. */
        E_REPAINT,                      /* Repaint: e_flno, e_tlno set. */
        E_SIGHUP,                       /* SIGHUP. */
+       E_SIGALRM,                      /* SIGALRM. */
        E_SIGTERM,                      /* SIGTERM. */
        E_STRING,                       /* Input string: e_csp, e_len set. */
        E_TIMEOUT,                      /* Timeout. */
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    19 Aug 2021 19:04:01 -0000
@@ -219,6 +219,12 @@ rcv_init(SCR *sp)
        /* Turn off the owner execute bit. */
        (void)chmod(ep->rcv_path, S_IRUSR | S_IWUSR);
 
+       if (!F_ISSET(sp->gp, G_RECOVER_SET)) {
+               /* Start the recovery timer. */
+               alarm(RCV_PERIOD);
+       }
+
+
        /* We believe the file is recoverable. */
        F_SET(ep, F_RCV_ON);
        return (0);
@@ -252,12 +258,16 @@ rcv_sync(SCR *sp, u_int flags)
 
        /* Sync the file if it's been modified. */
        if (F_ISSET(ep, F_MODIFIED)) {
+               /* Prevent SIGALRM during sync then reset the timer. */
+               alarm(0);
                if (ep->db->sync(ep->db, R_RECNOSYNC)) {
                        F_CLR(ep, F_RCV_ON | F_RCV_NORM);
                        msgq_str(sp, M_SYSERR,
                            ep->rcv_path, "File backup failed: %s");
+                       alarm(RCV_PERIOD);
                        return (1);
                }
+               alarm(RCV_PERIOD);
 
                /* REQUEST: don't remove backing file on exit. */
                if (LF_ISSET(RCV_PRESERVE))
Index: ex/ex_txt.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/ex/ex_txt.c,v
retrieving revision 1.17
diff -u -p -r1.17 ex_txt.c
--- ex/ex_txt.c 30 Apr 2020 10:40:21 -0000      1.17
+++ ex/ex_txt.c 19 Aug 2021 19:04:01 -0000
@@ -117,6 +117,7 @@ newtp:              if ((tp = text_init(sp, NULL, 0,
                        break;
                case E_ERR:
                        goto err;
+               case E_SIGALRM:
                case E_REPAINT:
                case E_WRESIZE:
                        continue;
Index: ex/ex_write.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/ex/ex_write.c,v
retrieving revision 1.13
diff -u -p -r1.13 ex_write.c
--- ex/ex_write.c       6 Jan 2016 22:28:52 -0000       1.13
+++ ex/ex_write.c       19 Aug 2021 19:04:01 -0000
@@ -127,6 +127,9 @@ exwr(SCR *sp, EXCMD *cmdp, enum which cm
        int flags;
        char *name, *p = NULL;
 
+       /* Sync recovery file, too. */
+       rcv_sync(sp, 0);
+
        NEEDFILE(sp, cmdp);
 
        /* All write commands can have an associated '!'. */
Index: vi/v_replace.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/vi/v_replace.c,v
retrieving revision 1.9
diff -u -p -r1.9 v_replace.c
--- vi/v_replace.c      6 Jan 2016 22:28:52 -0000       1.9
+++ vi/v_replace.c      19 Aug 2021 19:04:01 -0000
@@ -125,6 +125,9 @@ next:               if (v_event_get(sp, &ev, 0, 0))
                case E_INTERRUPT:
                        /* <interrupt> means they changed their minds. */
                        return (0);
+               case E_SIGALRM:
+                       /* Do nothing. */
+                       goto next;
                case E_WRESIZE:
                        /* <resize> interrupts the input mode. */
                        v_emsg(sp, NULL, VIM_WRESIZE);
Index: vi/v_txt.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/vi/v_txt.c,v
retrieving revision 1.35
diff -u -p -r1.35 v_txt.c
--- vi/v_txt.c  13 Apr 2021 15:34:41 -0000      1.35
+++ vi/v_txt.c  19 Aug 2021 19:04:02 -0000
@@ -502,6 +502,9 @@ next:       if (v_event_get(sp, evp, 0, ec_fla
        case E_WRESIZE:
                /* <resize> interrupts the input mode. */
                v_emsg(sp, NULL, VIM_WRESIZE);
+       case E_SIGALRM:
+               /* Don't repeat last input. */
+               goto next;
        /* FALLTHROUGH */
        default:
                if (evp->e_event != E_INTERRUPT && evp->e_event != E_WRESIZE)
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     19 Aug 2021 19:04:02 -0000
@@ -1197,6 +1197,9 @@ v_key(SCR *sp, int command_events, EVENT
                        break;
                case E_WRESIZE:
                        return (GC_ERR);
+               case E_SIGALRM:
+                       /* Prevent unexpected sigalarm message. */
+                       return (GC_ERR);
                case E_QUIT:
                case E_WRITE:
                        if (command_events)

Reply via email to