Create backup file when writing new xml file?

[Linus Torvalds]

On Sun, Feb 16, 2014 at 11:52 AM, Linus Torvalds
<torvalds at linux-foundation.org> wrote:
>
> I'm still thinking about better file formats and backup, but haven't come
> up with anything that I'm actually happy about.

So I really haven't gotten anywhere about a new format, because
anything that just gets rid of the nasty xml is too much pain for the
gain, and the more ambitious ideas I have have lots of other
painpoints.

Anyway, one thing I do think we should do is be more careful about not
overwriting our old xml file. In particular, right now if we crash
while writing the new xml file, everything is gone, both new and old.

Now, we could fix that crash case by just being much more careful when
writing: write to a different filename, fsync() the new file, and then
rename() the new file over the old one. Except as usual, windows is a
pain for any filesystem activity, and I have no idea how well fsync
works, and I *know* the rename needs some magic too.

And even if we do that careful write, that will fix the "oops, we
crashed while writing" problem, but not the "uhhuh, I made a mistake
and overwrote the old xml file".

So here's a rather less ambitious patch that still does the "windows
needs magic rename support because their VFS layer is using that
insane crap UCS-2 wchar idiocy", but just says "let's rename the old
file from "xml" to "bak" before writing the new file.

I don't personally really need it, since I use git to track my dives
anyway, so I have backups of my own, but I know others don't. So..
Comments? Is this just a bad idea?

Also, I know Windows also has issues with strncasecmp (that I use to
test that the backup really only gets written if the filename is
called ".xml") under msvc, but I really don't know if anybody really
builds it that way. So I just added a comment. A
"-Dstrncasecmp=_strnicmp" in the MSVC-specific build file might be
sufficient. Or maybe there is some Qt helper. Whatever.

As mentioned, I don't even know if this is really the direction we
want to go in.

                 Linus
-------------- next part --------------
 dive.h     |  1 +
 linux.c    |  5 +++++
 macos.c    |  5 +++++
 save-xml.c | 34 ++++++++++++++++++++++++++++++++++
 windows.c  | 16 ++++++++++++++++
 5 files changed, 61 insertions(+)

diff --git a/dive.h b/dive.h
index 2e0039f80f42..ae2645ac0d48 100644
--- a/dive.h
+++ b/dive.h
@@ -656,6 +656,7 @@ extern void save_dives_logic(const char *filename, bool select_only);
 extern void save_dive(FILE *f, struct dive *dive);
 extern void export_dives_uddf(const char *filename, const bool selected);
 
+extern int subsurface_rename(const char *path, const char *newpath);
 extern int subsurface_open(const char *path, int oflags, mode_t mode);
 extern FILE *subsurface_fopen(const char *path, const char *mode);
 extern void *subsurface_opendir(const char *path);
diff --git a/linux.c b/linux.c
index 1feefd6f1dca..ac1174a5cb17 100644
--- a/linux.c
+++ b/linux.c
@@ -101,6 +101,11 @@ int enumerate_devices (device_callback_t callback, void *userdata)
 }
 
 /* NOP wrappers to comform with windows.c */
+int subsurface_rename(const char *path, const char *newpath)
+{
+	return rename(path, newpath);
+}
+
 int subsurface_open(const char *path, int oflags, mode_t mode)
 {
 	return open(path, oflags, mode);
diff --git a/macos.c b/macos.c
index ba461ee4fd3e..aa5036ea2176 100644
--- a/macos.c
+++ b/macos.c
@@ -82,6 +82,11 @@ int enumerate_devices (device_callback_t callback, void *userdata)
 }
 
 /* NOP wrappers to comform with windows.c */
+int subsurface_rename(const char *path, const char *newpath)
+{
+	return rename(path, newpath);
+}
+
 int subsurface_open(const char *path, int oflags, mode_t mode)
 {
 	return open(path, oflags, mode);
diff --git a/save-xml.c b/save-xml.c
index 7bb2642bd74b..a954873135b5 100644
--- a/save-xml.c
+++ b/save-xml.c
@@ -577,12 +577,46 @@ void save_dives_buffer(struct membuffer *b, const bool select_only)
 	put_format(b, "</dives>\n</divelog>\n");
 }
 
+static void save_backup(const char *name, const char *ext, const char *new_ext)
+{
+	int len = strlen(name);
+	int a = strlen(ext), b = strlen(new_ext);
+	char *newname;
+
+	/* len up to and including the final '.' */
+	len -= a;
+	if (len <= 1)
+		return;
+	if (name[len-1] != '.')
+		return;
+	/* msvc doesn't have strncasecmp, has _strnicmp instead - crazy */
+	if (strncasecmp(name+len, ext, a))
+		return;
+
+	newname = malloc(len + b + 1);
+	if (!newname)
+		return;
+
+	memcpy(newname, name, len);
+	memcpy(newname+len, new_ext, b+1);
+
+	/*
+	 * Ignore errors. Maybe we can't create the backup file,
+	 * maybe no old file existed.  Regardless, we'll write the
+	 * new file.
+	 */
+	subsurface_rename(name, newname);
+	free(newname);
+}
+
 void save_dives_logic(const char *filename, const bool select_only)
 {
 	struct membuffer buf = {0};
 	FILE *f;
 
 	save_dives_buffer(&buf, select_only);
+	/* Maybe we might want to make this configurable? */
+	save_backup(filename, "xml", "bak");
 	f = subsurface_fopen(filename, "w");
 	if (f) {
 		flush_buffer(&buf, f);
diff --git a/windows.c b/windows.c
index 7c537f6e5fb3..1ecab41ea8a2 100644
--- a/windows.c
+++ b/windows.c
@@ -111,6 +111,22 @@ static wchar_t *utf8_to_utf16_fl(const char *utf8, char *file, int line)
 /* bellow we provide a set of wrappers for some I/O functions to use wchar_t.
  * on win32 this solves the issue that we need paths to be utf-16 encoded.
  */
+int subsurface_rename(const char *path, const char *newpath)
+{
+	int ret = -1;
+	if (!path || !newpath)
+		return -1;
+
+	wchar_t *wpath = utf8_to_utf16(path);
+	wchar_t *wnewpath = utf8_to_utf16(newpath);
+
+	if (wpath && wnewpath) {
+		ret = _wrename(wpath, wnewpath);
+	free((void *)wpath);
+	free((void *)wnewpath);
+	return ret;
+}
+
 int subsurface_open(const char *path, int oflags, mode_t mode)
 {
 	int ret = -1;