Re: 4.3.4-dev-4 and 4.2.6-dev-2 available

[Vincent Lefevre <vincent@xxxxxxxxxx>]

On 2007-12-12 09:48:50 +0000, Peter Stephenson wrote:
> > FYI, I tried to patch zsh to use fcntl locking in the past, but
> > the zsh history code is so complex and unintuitive that I couldn't.
> 
> The strategy was deliberately to avoid file locking known to be
> buggy in older NFS implementations.

I've undone Wayne's patch zsh-users/8494 that didn't seem to work
anyway and retried my patch, and while I got history corruption
with the unpatched version a few minutes before (as usual), I can't
reproduce the corruption with my patched version.

However, I get the following error when I quit the shell:

zsh: failed to write history file /home/vlefevre/.zhistory: bad file descriptor

but nothing is missing in the history!

I've attached my patch against 4.3.4-dev-4 (which can probably be
improved to handle errors in a better way -- it was just for testing
until now and I wanted to keep it simple). I've tried it only under
my configuration, which is:

vin:~> setopt | grep hist
extendedhistory
histignoredups
histignorespace
histnofunctions
histnostore
histreduceblanks
incappendhistory

-- 
Vincent Lefèvre <vincent@xxxxxxxxxx> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / Arenaire project (LIP, ENS-Lyon)

--- hist.c.old	2007-08-14 11:50:25.000000000 +0000
+++ hist.c	2007-12-14 16:27:49.000000000 +0000
@@ -2021,6 +2021,18 @@
     else if (!lockhistfile(fn, 1))
 	return;
     if ((in = fopen(unmeta(fn), "r"))) {
+#ifdef HAVE_FCNTL_H
+	struct flock lck;
+
+	lck.l_type = F_RDLCK;
+	lck.l_whence = SEEK_SET;
+	lck.l_start = 0;
+	lck.l_len = 0;  /* lock the whole file */
+	if (fcntl(fileno(in), F_SETLKW, &lck) == -1) {
+	    fclose(in);
+	    return;
+	}
+#endif
 	nwordlist = 64;
 	wordlist = (short *)zalloc(nwordlist*sizeof(short));
 	bufsiz = 1024;
@@ -2153,11 +2165,12 @@
 void
 savehistfile(char *fn, int err, int writeflags)
 {
-    char *t, *tmpfile, *start = NULL;
+    char *t, *start = NULL;
     FILE *out;
     Histent he;
     zlong xcurhist = curhist - !!(histactive & HA_ACTIVE);
     int extended_history = isset(EXTENDEDHISTORY);
+    int truncate_history = 0;
     int ret;
 
     if (!interact || savehistsiz <= 0 || !hist_ring
@@ -2192,39 +2205,44 @@
     }
     errno = 0;
     if (writeflags & HFILE_APPEND) {
-	tmpfile = NULL;
 	out = fdopen(open(unmeta(fn),
 			O_CREAT | O_WRONLY | O_APPEND | O_NOCTTY, 0600), "a");
     } else if (!isset(HISTSAVEBYCOPY)) {
-	tmpfile = NULL;
 	out = fdopen(open(unmeta(fn),
 			 O_CREAT | O_WRONLY | O_TRUNC | O_NOCTTY, 0600), "w");
+	/* The file should be truncated after its locking. */
+	truncate_history = 1;
     } else {
-	tmpfile = bicat(unmeta(fn), ".new");
-	if (unlink(tmpfile) < 0 && errno != ENOENT)
-	    out = NULL;
-	else {
-	    struct stat sb;
-	    int old_exists = stat(unmeta(fn), &sb) == 0;
-
-	    if (old_exists && sb.st_uid != geteuid()) {
-		free(tmpfile);
-		tmpfile = NULL; /* Avoid an error about HISTFILE.new */
-		out = NULL;
-	    } else
-		out = fdopen(open(tmpfile, O_CREAT | O_WRONLY | O_EXCL, 0600), "w");
+	struct stat sb;
+	int old_exists = stat(unmeta(fn), &sb) == 0;
 
+	if (old_exists && sb.st_uid != geteuid())
+	    out = NULL;
+	else
+	    out = fdopen(open(unmeta(fn),
+			      O_CREAT | O_WRONLY | O_EXCL, 0600), "w");
 #ifdef HAVE_FCHMOD
-	    if (old_exists && out) {
+	if (old_exists && out) {
 #ifdef HAVE_FCHOWN
-		fchown(fileno(out), sb.st_uid, sb.st_gid);
-#endif
-		fchmod(fileno(out), sb.st_mode);
-	    }
+	    fchown(fileno(out), sb.st_uid, sb.st_gid);
 #endif
+	    fchmod(fileno(out), sb.st_mode);
 	}
+#endif
     }
     if (out) {
+#ifdef HAVE_FCNTL_H
+	struct flock lck;
+
+	lck.l_type = F_WRLCK;
+	lck.l_whence = SEEK_SET;
+	lck.l_start = 0;
+	lck.l_len = 0;
+	while (fcntl(fileno(out), F_SETLKW, &lck) == -1)
+	    sleep (1);
+#endif
+	if (truncate_history)
+	    ftruncate(fileno(out), 0);
 	ret = 0;
 	for (; he && he->histnum <= xcurhist; he = down_histent(he)) {
 	    if ((writeflags & HFILE_SKIPDUPS && he->node.flags & HIST_DUP)
@@ -2274,12 +2292,6 @@
 	if (fclose(out) < 0 && ret >= 0)
 	    ret = -1;
 	if (ret >= 0) {
-	    if (tmpfile) {
-		if (rename(tmpfile, unmeta(fn)) < 0)
-		    zerr("can't rename %s.new to $HISTFILE", fn);
-		free(tmpfile);
-	    }
-
 	    if (writeflags & HFILE_SKIPOLD
 		&& !(writeflags & (HFILE_FAST | HFILE_NO_REWRITE))) {
 		int remember_histactive = histactive;
@@ -2302,13 +2314,8 @@
     } else
 	ret = -1;
 
-    if (ret < 0 && err) {
-	if (tmpfile) {
-	    zerr("failed to write history file %s.new: %e", fn, errno);
-	    free(tmpfile);
-	} else
-	    zerr("failed to write history file %s: %e", fn, errno);
-    }
+    if (ret < 0 && err)
+	zerr("failed to write history file %s: %e", fn, errno);
 
     unlockhistfile(fn);
 }