[PATCH] history locking with fcntl

[PATCH] history locking with fcntl

[Vincent Lefevre]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

I've rewritten my patch to lock the history with fcntl, and added
an option. It works well with my settings (INC_APPEND_HISTORY), but
I haven't tested it with other settings. I no longer get the error

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

-- 
Vincent Lefèvre <vincent@vinc17.org> - 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)

[-- Attachment #2: zsh-histlock.patch --]
[-- Type: text/plain, Size: 5992 bytes --]

Index: Doc/Zsh/options.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
retrieving revision 1.58
diff -d -u -r1.58 options.yo
--- Doc/Zsh/options.yo	13 Apr 2008 16:58:42 -0000	1.58
+++ Doc/Zsh/options.yo	15 Apr 2008 00:03:26 -0000
@@ -577,6 +577,11 @@
 events, otherwise this option will behave just like
 tt(HIST_IGNORE_ALL_DUPS) once the history fills up with unique events.
 )
+pindex(HIST_FCNTL_LOCK)
+item(tt(HIST_FCNTL_LOCK))(
+Use fcntl locking when writing out the history file, in order to avoid
+history corruption under NFS.
+)
 pindex(HIST_FIND_NO_DUPS)
 cindex(history, ignoring duplicates in search)
 item(tt(HIST_FIND_NO_DUPS))(
Index: Src/hist.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/hist.c,v
retrieving revision 1.74
diff -d -u -r1.74 hist.c
--- Src/hist.c	14 Apr 2008 13:42:53 -0000	1.74
+++ Src/hist.c	15 Apr 2008 00:03:27 -0000
@@ -2021,6 +2021,20 @@
     else if (!lockhistfile(fn, 1))
 	return;
     if ((in = fopen(unmeta(fn), "r"))) {
+#ifdef HAVE_FCNTL_H
+	if (isset(HISTFCNTLLOCK)) {
+	    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;
@@ -2149,6 +2163,68 @@
     unlockhistfile(fn);
 }
 
+#ifdef HAVE_FCNTL_H
+/**/
+static int
+wlockfile(int fd)
+{
+    struct flock lck;
+    int ctr = 8;
+
+    lck.l_type = F_WRLCK;
+    lck.l_whence = SEEK_SET;
+    lck.l_start = 0;
+    lck.l_len = 0;
+    while (fcntl(fd, F_SETLKW, &lck) == -1) {
+	if (--ctr < 0)
+	    return 1;
+	sleep (1);
+    }
+    return 0;
+}
+#endif
+
+/**/
+static int
+safe_unlink(const char *pathname)
+{
+#ifdef HAVE_FCNTL_H
+    if (isset(HISTFCNTLLOCK)) {
+	int fd = open(pathname, O_WRONLY | O_NOCTTY, 0600);
+	if (fd >= 0) {
+	    int err = wlockfile(fd) || unlink(pathname);
+	    close(fd);
+	    return err;
+	} else {
+	    return errno != ENOENT;
+	}
+    }
+#endif
+    return unlink(pathname) && errno != ENOENT;
+}
+
+/**/
+static int
+safe_rename(const char *oldpath, const char *newpath)
+{
+#ifdef HAVE_FCNTL_H
+    if (isset(HISTFCNTLLOCK)) {
+	int fd = open(newpath, O_CREAT | O_WRONLY | O_NOCTTY, 0600);
+	if (fd < 0) {
+	    return 1;
+	} else if (wlockfile(fd)) {
+	    close(fd);
+	    return 1;
+	} else {
+	    int err = rename(oldpath, newpath);
+	    close(fd);
+	    return err;
+	}
+    }
+#endif
+    return rename(oldpath, newpath);
+}
+
 /**/
 void
 savehistfile(char *fn, int err, int writeflags)
@@ -2158,6 +2234,7 @@
     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
@@ -2196,12 +2273,14 @@
 	tmpfile = NULL;
 	out = fd >= 0 ? fdopen(fd, "a") : NULL;
     } else if (!isset(HISTSAVEBYCOPY)) {
-	int fd = open(unmeta(fn), O_CREAT | O_WRONLY | O_TRUNC | O_NOCTTY, 0600);
+	int fd = open(unmeta(fn), O_CREAT | O_WRONLY | O_NOCTTY, 0600);
 	tmpfile = NULL;
 	out = fd >= 0 ? fdopen(fd, "w") : NULL;
+	/* The file should be truncated after its locking. */
+	truncate_history = 1;
     } else {
 	tmpfile = bicat(unmeta(fn), ".new");
-	if (unlink(tmpfile) < 0 && errno != ENOENT)
+	if (safe_unlink(tmpfile))
 	    out = NULL;
 	else {
 	    struct stat sb;
@@ -2239,7 +2318,16 @@
 #endif
 	}
     }
+#ifdef HAVE_FCNTL_H
+    if (out && isset(HISTFCNTLLOCK) && wlockfile(fileno(out))) {
+	zerr("can't lock file (timeout) -- history %s not updated", fn);
+	err = 0; /* Don't report a generic error below. */
+	out = NULL;
+    }
+#endif
     if (out) {
+	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)
@@ -2285,16 +2373,19 @@
 		zsfree(lasthist.text);
 		lasthist.text = ztrdup(start);
 	    }
-	}
-	if (fclose(out) < 0 && ret >= 0)
+	} else if (ret >= 0 && fflush(out) < 0) {
 	    ret = -1;
+	}
 	if (ret >= 0) {
 	    if (tmpfile) {
-		if (rename(tmpfile, unmeta(fn)) < 0)
+		/* out has been flushed and the file must be renamed while
+		   being open so that the lock is still valid */
+		if (safe_rename(tmpfile, unmeta(fn)))
 		    zerr("can't rename %s.new to $HISTFILE", fn);
 		free(tmpfile);
 		tmpfile = NULL;
 	    }
+	    fclose(out);
 
 	    if (writeflags & HFILE_SKIPOLD
 		&& !(writeflags & (HFILE_FAST | HFILE_NO_REWRITE))) {
@@ -2314,6 +2405,8 @@
 		pophiststack();
 		histactive = remember_histactive;
 	    }
+	} else {
+	    fclose(out);
 	}
     } else
 	ret = -1;
Index: Src/options.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/options.c,v
retrieving revision 1.40
diff -d -u -r1.40 options.c
--- Src/options.c	13 Apr 2008 16:58:42 -0000	1.40
+++ Src/options.c	15 Apr 2008 00:03:27 -0000
@@ -135,6 +135,7 @@
 {{NULL, "histallowclobber",   0},			 HISTALLOWCLOBBER},
 {{NULL, "histbeep",	      OPT_ALL},			 HISTBEEP},
 {{NULL, "histexpiredupsfirst",0},			 HISTEXPIREDUPSFIRST},
+{{NULL, "histfcntllock",      0},			 HISTFCNTLLOCK},
 {{NULL, "histfindnodups",     0},			 HISTFINDNODUPS},
 {{NULL, "histignorealldups",  0},			 HISTIGNOREALLDUPS},
 {{NULL, "histignoredups",     0},			 HISTIGNOREDUPS},
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.121
diff -d -u -r1.121 zsh.h
--- Src/zsh.h	13 Apr 2008 16:58:42 -0000	1.121
+++ Src/zsh.h	15 Apr 2008 00:03:27 -0000
@@ -1749,6 +1749,7 @@
     HISTALLOWCLOBBER,
     HISTBEEP,
     HISTEXPIREDUPSFIRST,
+    HISTFCNTLLOCK,
     HISTFINDNODUPS,
     HISTIGNOREALLDUPS,
     HISTIGNOREDUPS,

Re: [PATCH] history locking with fcntl

[Peter Stephenson]

On Tue, 15 Apr 2008 17:31:20 +0200
Vincent Lefevre <vincent@vinc17.org> wrote:
> I've rewritten my patch to lock the history with fcntl, and added
> an option. It works well with my settings (INC_APPEND_HISTORY), but
> I haven't tested it with other settings.

Thanks, I've committed it with modified documentation.  It will no doubt
need some field testing.

I think ftruncate() probably needs a configure test.  I haven't done
this yet.

pindex(HIST_FCNTL_LOCK)
item(tt(HIST_FCNTL_LOCK))(
When writing out the history file, by default zsh uses ad-hoc file locking
to avoid known problems with locking on some operating systems.  With this
option locking is done by means of the system's tt(fcntl) call, where
this method is available.  On recent operating systems this may
provide better performance, in particular avoiding history corruption when
files are stored on NFS.
)

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: [PATCH] history locking with fcntl

[Bart Schaefer]

On Apr 17, 10:40am, Peter Stephenson wrote:
}
} pindex(HIST_FCNTL_LOCK)
} provide better performance, in particular avoiding history corruption when
} files are stored on NFS.

Really?  Traditionally fcntl() has been a lousy way to lock over NFS.
Procmail, for example, has acres of code to handle using secondary
files as semaphores precisely because nothing else was reliable over
NFS (more specifically, only creat() was guaranteed to be atomic and
lock daemons were flaky).

I guess my point is that while fcntl() may be good on recent OSs (or
recent versions of NFS, more likely) it's dangerous in an environment
where you don't know what the NFS server is using (no matter how recent
the local NFS client is).

Re: [PATCH] history locking with fcntl

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

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..

[-- Attachment #2: fcntl.patch --]
[-- Type: text/x-diff, Size: 1719 bytes --]

--- 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
     }
 }
 

Re: [PATCH] history locking with fcntl

[Peter Stephenson]

Wayne Davison wrote:
> 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 any extra locking done there.

Sounds reasonable to me, but you have more idea of the structure of this
bit.

> 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?

I wondered about that, but something along the lines of Bart's concerns
came to me.  Consequently, I was quite happy with an option that read as
THIS_IS_A_BIT_TECHY_AND_YOU_PROBABLY_OUGHT_TO_KNOW_WHAT_YOU_ARE_DONG
rather than GREAT_NEW_OPTION_TRY_ME_TRY_ME.  However, a suitably worded
caveat in the documentation might be a better method.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: [PATCH] history locking with fcntl

[Vincent Lefevre]

On 2008-04-17 06:58:34 -0700, Bart Schaefer wrote:
> On Apr 17, 10:40am, Peter Stephenson wrote:
> } pindex(HIST_FCNTL_LOCK)
> } provide better performance, in particular avoiding history corruption when
> } files are stored on NFS.
> 
> Really?  Traditionally fcntl() has been a lousy way to lock over NFS.

AFAIK, it's the only way to prevent some corruption due to file caching.

> Procmail, for example, has acres of code to handle using secondary
> files as semaphores precisely because nothing else was reliable over
> NFS

But procmail *also* uses fcntl. My patch still uses the old lock
mechanism, it just adds fcntl locking. That said, I often use fcntl
only, without any problem.

> (more specifically, only creat() was guaranteed to be atomic and
> lock daemons were flaky).

Some software, like Firefox, uses symlink() locking. So, if symlink()
isn't atomic, there will be problems with such software.

> I guess my point is that while fcntl() may be good on recent OSs (or
> recent versions of NFS, more likely) it's dangerous in an environment
> where you don't know what the NFS server is using (no matter how recent
> the local NFS client is).

I don't see why it can be dangerous if it is used in addition to
another lock mechanism, like it currently is.

-- 
Vincent Lefèvre <vincent@vinc17.org> - 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)

Re: [PATCH] history locking with fcntl

[Vincent Lefevre]

On 2008-04-17 09:23:07 -0700, Wayne Davison wrote:
> 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.

Note: it's fcntl, not flock (flock does not lock over NFS).

If I do not set the option (which is equivalent to not using the patch),
then I get immediate history corruption when using two machines (well,
at least 64-bit ones), 100% reproducible.

> 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.

I'll try it.

> 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?

Yes, perhaps. But this would give the user less control on the lock
mechanisms.

-- 
Vincent Lefèvre <vincent@vinc17.org> - 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)

Re: [PATCH] history locking with fcntl

[Bart Schaefer]

On Apr 18,  2:49am, Vincent Lefevre wrote:
}
} > (more specifically, only creat() was guaranteed to be atomic and
} > lock daemons were flaky).
} 
} Some software, like Firefox, uses symlink() locking. So, if symlink()
} isn't atomic, there will be problems with such software.

(1) Note I said "was", not "is".

(2) Procmail's code pre-dates near-universal availability of symlink().

} I don't see why it can be dangerous if it is used in addition to
} another lock mechanism, like it currently is.

That's OK, then (as long as the equivalent of rpc.lockd is running, of
course).  My copy of your message didn't actually have the patch in it, 
and I've been running back and forth to a conference all this week so
I didn't go out of my way to check whether this was an "either" or an
"as well."

Re: [PATCH] history locking with fcntl

[Wayne Davison]

On Fri, Apr 18, 2008 at 02:59:59AM +0200, Vincent Lefevre wrote:
> If I do not set the option (which is equivalent to not using the patch),
> then I get immediate history corruption when using two machines (well,
> at least 64-bit ones), 100% reproducible.

Cool.  Then it will be easy for you to get the real error now that that
"bad file descriptor" bug is fixed.  I'm curious why the old code is
failing.

> I'll try it.

Much appreciated.

..wayne..

Re: [PATCH] history locking with fcntl

[Bart Schaefer]

On Apr 18,  7:31pm, Wayne Davison wrote:
}
} On Fri, Apr 18, 2008 at 02:59:59AM +0200, Vincent Lefevre wrote:
} > If I do not set the option (which is equivalent to not using the patch),
} > then I get immediate history corruption when using two machines (well,
} > at least 64-bit ones), 100% reproducible.
} 
} Cool.  Then it will be easy for you to get the real error now

I don't think there is a "real error" in the sense of a -1 return from
a system call and/or an errno value.  I think everything appears to
succeed as seen from the C code, but the file is corrupted anyway.

I hope (in some sense) that I turn out to be wrong.

Re: [PATCH] history locking with fcntl

[Wayne Davison]

On Fri, Apr 18, 2008 at 10:23:12PM -0700, Bart Schaefer wrote:
> I don't think there is a "real error" in the sense of a -1 return from
> a system call and/or an errno value.

I'm assuming that there is based on this in the original report:

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

This seems to indicate that an open() call failed for some reason, but
the fdopen() call masked the error.

..wayne..

Re: [PATCH] history locking with fcntl

[Vincent Lefevre]

On 2008-04-18 19:31:55 -0700, Wayne Davison wrote:
> > I'll try it.
> 
> Much appreciated.

I've done a few tests and don't get history corruption.

But my patch avoided a possible problem in rename() when there is a
tmpfile. With your patch, one still has:

        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);
                tmpfile = NULL;
            }

Note that since out has been closed, tmpfile is no longer locked.
The target file isn't locked either.

Now, I don't think that rename() can be cached. So, I'd say that there
should be no problems under "normal" conditions.

-- 
Vincent Lefèvre <vincent@vinc17.org> - 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)

Re: [PATCH] history locking with fcntl

[Wayne Davison]

On Mon, Apr 21, 2008 at 03:19:37PM +0200, Vincent Lefevre wrote:
> Note that since out has been closed, tmpfile is no longer locked.
> The target file isn't locked either.

The target file was locked prior to the rename (since that's the only
file my code is locking -- the HISTFILE itself).  So, the rename does
essentially unlock the HISTFILE as far as fcntl() is concerned.  Since
it is done with the write at that point, it should be OK (in my latest
version).

My code was deficient if it then went on to rewrite the HISTFILE before
returning, as it would not lock the file using fcntl() if it thought it
was already locked.  I've fixed that, and checked in the result.

..wayne..

Re: [PATCH] history locking with fcntl

[Vincent Lefevre]

On 2008-05-04 18:25:51 -0700, Wayne Davison wrote:
> On Mon, Apr 21, 2008 at 03:19:37PM +0200, Vincent Lefevre wrote:
> > Note that since out has been closed, tmpfile is no longer locked.
> > The target file isn't locked either.
> 
> The target file was locked prior to the rename (since that's the only
> file my code is locking -- the HISTFILE itself).  So, the rename does
> essentially unlock the HISTFILE as far as fcntl() is concerned.

I've looked again at the current zsh code (which corresponds to your
patch). This is incorrect.

As I've said, there's a fclose() that has the effect to unlock the file:

        if (fclose(out) < 0 && ret >= 0)
            ret = -1;

Indeed, out is a stream associated with fd, which corresponds to the
history file in general (there's also a case of a tmp file, let's
forget this one). And according to the Linux fclose() man page, the
fclose() function closes the underlying file descriptor:

    The fclose() function will flush the stream pointed to by fp
    (writing any buffered output data using fflush(3)) and close
    the underlying file descriptor.

As a file descriptor associated with the file has been closed, the
file is no longer locked by the process, according to the Linux
fcntl(2) man page:

    As well as being removed by an explicit F_UNLCK, record locks
    are automatically released when the process terminates or if
    it closes _any_ file descriptor referring to a file on which
    locks are held.

So, there's a possible race condition, in particular knowning the
fact that preemption at system calls are more likely to occur. If
  1. the kernel preempts the process (A) at the end of the close()
     [the system call used by fclose()] or just before the rename(),
  2. another process (B) locks the history file,
  3. B is preempted,
  4. (A) does the rename,
then bad things can happen.

In my patch, I did a fflush() instead of fclose() to avoid removing
the lock at this time, and I did the fclose() after the rename().

-- 
Vincent Lefèvre <vincent@vinc17.org> - 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)

Re: [PATCH] history locking with fcntl

[Vincent Lefevre]

On 2009-01-01 05:01:19 +0100, Vincent Lefevre wrote:
> On 2008-05-04 18:25:51 -0700, Wayne Davison wrote:
> > On Mon, Apr 21, 2008 at 03:19:37PM +0200, Vincent Lefevre wrote:
> > > Note that since out has been closed, tmpfile is no longer locked.
> > > The target file isn't locked either.
> > 
> > The target file was locked prior to the rename (since that's the only
> > file my code is locking -- the HISTFILE itself).  So, the rename does
> > essentially unlock the HISTFILE as far as fcntl() is concerned.
> 
> I've looked again at the current zsh code (which corresponds to your
> patch). This is incorrect.

Oops, forget my mail. The rename is done only when there's a tmp file,
which was the case I excluded. So, the code is OK (I've also re-read
it, and there don't seem to be other closes() calls that could remove
the lock). Perhaps it lacks a bit documentation, in case the code will
have to be modified in the future.

-- 
Vincent Lefèvre <vincent@vinc17.org> - 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)