Re: Improve vi(1) recovery
+h_alrm(int signo)
+{
+ GLOBAL_CLP;
+
+ F_SET(clp, CL_SIGALRM);
F_SET is |=, which is not atomic.
This is unsafe. Safe signal handlers need to make single stores to
atomic-sized variables, which tend to be int-sized, easier to declare
as sig_atomic_t. Most often these are global scope with the following
type:
volatile sig_atomic_t variable
I can see you copied an existing practice. Sadly all the other
signal handlers are also broken in the same way.
The flag bits should be replaced with seperate sig_atomic_t variables.
Re: Improve vi(1) recovery
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 - 1.10
+++ cl/cl.h 19 Aug 2021 19:04:01 -
@@ -30,8 +30,9 @@ typedef struct _cl_private {
#defineINDX_HUP0
#defineINDX_INT1
#defineINDX_TERM 2
-#defineINDX_WINCH 3
-#defineINDX_MAX4 /* Original signal information. */
+#defineINDX_ALRM 3
+#defineINDX_WINCH 4
+#defineINDX_MAX5 /* Original signal information. */
struct sigaction oact[INDX_MAX];
enum { /* Tty group write mode. */
@@ -48,8 +49,9 @@ typedef struct _cl_private {
#defineCL_SIGHUP 0x0020 /* SIGHUP arrived. */
#defineCL_SIGINT 0x0040 /* SIGINT arrived. */
#defineCL_SIGTERM 0x0080 /* SIGTERM arrived. */
-#defineCL_SIGWINCH 0x0100 /* SIGWINCH arrived. */
-#defineCL_STDIN_TTY0x0200 /* Talking to a terminal. */
+#defineCL_SIGALRM 0x0100 /* SIGALRM arrived. */
+#defineCL_SIGWINCH 0x0200 /* SIGWINCH arrived. */
+#defineCL_STDIN_TTY0x0400 /* 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.c5 May 2016 20:36:41 - 1.33
+++ cl/cl_main.c19 Aug 2021 19:04:01 -
@@ -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, >oact[INDX_HUP], h_hup) ||
setsig(SIGINT, >oact[INDX_INT],
Re: Improve vi(1) recovery
trondd wrote:
> trondd wrote:
>
> > trondd wrote:
> >
> > >
> > > 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 exists, 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
> > >
> >
> > Got positive testing from one user. Anyone else interested? Or any other
> > feedback?
> >
> > Tim.
> >
>
> Throwing this hat back into the ring post-release. Still only gotten user
> feedback so far.
>
> Tim.
>
Gotten more positive user feedback and tests. Any developers interested in
this is some form or another?
Diff regenerated after expandtab change, which didn't really move anything.
Tim.
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 - 1.10
+++ cl/cl.h 9 May 2021 20:03:21 -
@@ -30,8 +30,9 @@ typedef struct _cl_private {
#defineINDX_HUP0
#defineINDX_INT1
#defineINDX_TERM 2
-#defineINDX_WINCH 3
-#defineINDX_MAX4 /* Original signal information. */
+#defineINDX_ALRM 3
+#defineINDX_WINCH 4
+#defineINDX_MAX5 /* Original signal information. */
struct sigaction oact[INDX_MAX];
enum { /* Tty group write mode. */
@@ -48,8 +49,9 @@ typedef struct _cl_private {
#defineCL_SIGHUP 0x0020 /* SIGHUP arrived. */
#defineCL_SIGINT 0x0040 /* SIGINT arrived. */
#defineCL_SIGTERM 0x0080 /* SIGTERM arrived. */
-#defineCL_SIGWINCH 0x0100 /* SIGWINCH arrived. */
-#defineCL_STDIN_TTY0x0200 /* Talking to a terminal. */
+#defineCL_SIGALRM 0x0100 /* SIGALRM arrived. */
+#defineCL_SIGWINCH 0x0200 /* SIGWINCH arrived. */
+#defineCL_STDIN_TTY0x0400 /* Talking to a terminal. */
u_int32_t flags;
} CL_PRIVATE;
Index: cl/cl_main.c
===
RCS
Re: Improve vi(1) recovery
trondd wrote:
> trondd wrote:
>
> >
> > 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 exists, 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
> >
>
> Got positive testing from one user. Anyone else interested? Or any other
> feedback?
>
> Tim.
>
Throwing this hat back into the ring post-release. Still only gotten user
feedback so far.
Tim.
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 - 1.10
+++ cl/cl.h 26 Dec 2020 17:47:32 -
@@ -30,8 +30,9 @@ typedef struct _cl_private {
#defineINDX_HUP0
#defineINDX_INT1
#defineINDX_TERM 2
-#defineINDX_WINCH 3
-#defineINDX_MAX4 /* Original signal information. */
+#defineINDX_ALRM 3
+#defineINDX_WINCH 4
+#defineINDX_MAX5 /* Original signal information. */
struct sigaction oact[INDX_MAX];
enum { /* Tty group write mode. */
@@ -48,8 +49,9 @@ typedef struct _cl_private {
#defineCL_SIGHUP 0x0020 /* SIGHUP arrived. */
#defineCL_SIGINT 0x0040 /* SIGINT arrived. */
#defineCL_SIGTERM 0x0080 /* SIGTERM arrived. */
-#defineCL_SIGWINCH 0x0100 /* SIGWINCH arrived. */
-#defineCL_STDIN_TTY0x0200 /* Talking to a terminal. */
+#defineCL_SIGALRM 0x0100 /* SIGALRM arrived. */
+#defineCL_SIGWINCH 0x0200 /* SIGWINCH arrived. */
+#defineCL_STDIN_TTY0x0400 /* 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.c5 May 2016 20:36:41 - 1.33
+++ cl/cl_main.c26 Dec 2020 17:47:32 -
@@ -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
Re: Improve vi(1) recovery
trondd wrote:
>
> 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 exists, 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
>
Got positive testing from one user. Anyone else interested? Or any other
feedback?
Tim.
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 - 1.10
+++ cl/cl.h 26 Dec 2020 17:47:32 -
@@ -30,8 +30,9 @@ typedef struct _cl_private {
#defineINDX_HUP0
#defineINDX_INT1
#defineINDX_TERM 2
-#defineINDX_WINCH 3
-#defineINDX_MAX4 /* Original signal information. */
+#defineINDX_ALRM 3
+#defineINDX_WINCH 4
+#defineINDX_MAX5 /* Original signal information. */
struct sigaction oact[INDX_MAX];
enum { /* Tty group write mode. */
@@ -48,8 +49,9 @@ typedef struct _cl_private {
#defineCL_SIGHUP 0x0020 /* SIGHUP arrived. */
#defineCL_SIGINT 0x0040 /* SIGINT arrived. */
#defineCL_SIGTERM 0x0080 /* SIGTERM arrived. */
-#defineCL_SIGWINCH 0x0100 /* SIGWINCH arrived. */
-#defineCL_STDIN_TTY0x0200 /* Talking to a terminal. */
+#defineCL_SIGALRM 0x0100 /* SIGALRM arrived. */
+#defineCL_SIGWINCH 0x0200 /* SIGWINCH arrived. */
+#defineCL_STDIN_TTY0x0400 /* 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.c5 May 2016 20:36:41 - 1.33
+++ cl/cl_main.c26 Dec 2020 17:47:32 -
@@ -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, >oact[INDX_HUP], h_hup) ||
setsig(SIGINT, >oact[INDX_INT], h_int) ||
setsig(SIGTERM, >oact[INDX_TERM], h_term) ||
+ setsig(SIGALRM, >oact[INDX_ALRM],
