Re: [PATCH] history locking with fcntl

[Wayne Davison <wayned@xxxxxxxxxxxxxxxxxxxxx>]

On Tue, Apr 15, 2008 at 05:31:20PM +0200, Vincent Lefevre wrote:
> I no longer get the error
> 
> zsh: failed to write history file /home/vlefevre/.zhistory: bad file descriptor

I fixed the bogus "bad file descriptor" part of that error back on March
6th, as well as making some minor improvements to the errors.  I'd like
to see what the real error is when your flock patch is not present.

I personally think this patch goes too far.  There is a lockhistfile()
call that is done to give a particular shell exclusive control over the
history files.  I'd prefer to see nay extra locking done there.

Attached is an alternative patch for the Src/utils.c file that
implements this (it is easier to see what I've done than diffing it
against the file with the current fcntl changes).  Let me know what
you think.

Additional discussion:  I think the option name should be made more
generic so that it can be made to support more types of file locking,
such as the flock() call.  Perhaps call it HIST_EXTRA_LOCKING?

..wayne..

--- Src/hist.c	14 Apr 2008 13:42:53 -0000	1.74
+++ Src/hist.c	17 Apr 2008 16:12:04 -0000
@@ -2330,6 +2330,36 @@ savehistfile(char *fn, int err, int writ
     unlockhistfile(fn);
 }
 
+#ifdef HAVE_FCNTL_H
+static int flock_fd = -1;
+
+static int
+flockhistfile(char *fn, int keep_trying)
+{
+    struct flock lck;
+    int ctr = keep_trying ? 9 : 0;
+
+    if ((flock_fd = open(unmeta(fn), O_RDWR | O_NOCTTY)) < 0)
+	return errno == ENOENT; /* "successfully" locked missing file */
+
+    lck.l_type = F_WRLCK;
+    lck.l_whence = SEEK_SET;
+    lck.l_start = 0;
+    lck.l_len = 0;  /* lock the whole file */
+
+    while (fcntl(flock_fd, F_SETLKW, &lck) == -1) {
+	if (--ctr < 0) {
+	    close(flock_fd);
+	    flock_fd = -1;
+	    return 0;
+	}
+	sleep(1);
+    }
+
+    return 1;
+}
+#endif
+
 static int lockhistct;
 
 /**/
@@ -2340,6 +2370,12 @@ lockhistfile(char *fn, int keep_trying)
 
     if (!fn && !(fn = getsparam("HISTFILE")))
 	return 0;
+
+#ifdef HAVE_FCNTL_H
+    if (isset(HISTFCNTLLOCK) && !lockhistct && !flockhistfile(fn, keep_trying))
+	return 0;
+#endif
+
     if (!lockhistct++) {
 	struct stat sb;
 	int fd;
@@ -2404,7 +2440,15 @@ lockhistfile(char *fn, int keep_trying)
 #endif /* not HAVE_LINK */
 	free(lockfile);
     }
-    return ct != lockhistct;
+
+    if (ct == lockhistct) {
+#ifdef HAVE_FCNTL_H
+	close(flock_fd);
+	flock_fd = -1;
+#endif
+	return 0;
+    }
+    return 1;
 }
 
 /* Unlock the history file if this corresponds to the last nested lock
@@ -2428,6 +2472,12 @@ unlockhistfile(char *fn)
 	sprintf(lockfile, "%s.LOCK", fn);
 	unlink(lockfile);
 	free(lockfile);
+#ifdef HAVE_FCNTL_H
+	if (flock_fd >= 0) {
+	    close(flock_fd);
+	    flock_fd = -1;
+	}
+#endif
     }
 }