GNU bug report logs - #8496
some file locking code in insert-file-contents cannot be right

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 8496 in the body.
You can then email your comments to 8496 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Subject: some file locking code in insert-file-contents cannot be right
Date: Wed, 13 Apr 2011 17:40:49 -0700

As a result of the "mark unexported symbols 'static'" patch that I'm
working on, in the Emacs trunk I found some file-locking code inside
insert-file-contents that cannot be right:

     if (NILP (visit) && inserted > 0)
       {
   #ifdef CLASH_DETECTION
	 if (!NILP (BVAR (current_buffer, file_truename))
	     /* Make binding buffer-file-name to nil effective.  */
	     && !NILP (BVAR (current_buffer, filename))
	     && SAVE_MODIFF >= MODIFF)
	   we_locked_file = 1;
   #endif /* CLASH_DETECTION */
	 prepare_to_modify_buffer (GPT, GPT, NULL);
       }

The above code is equivalent to a no-op, since 'inserted' must
be zero here.  There's later code:

   #ifdef CLASH_DETECTION
	 if (we_locked_file)
	   unlock_file (BVAR (current_buffer, file_truename));
   #endif

that is also a no-op, because 'we_locked_file' must be zero.

GCC merrily optimizes all this code away, but I don't think
that was intended.  What *is* intended here?

Message #8 received at 8496 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 8496 <at> debbugs.gnu.org
Subject: Re: bug#8496: some file locking code in insert-file-contents cannot be
	right
Date: Thu, 14 Apr 2011 02:48:28 -0400

> Date: Wed, 13 Apr 2011 17:40:49 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> 
> As a result of the "mark unexported symbols 'static'" patch that I'm
> working on, in the Emacs trunk I found some file-locking code inside
> insert-file-contents that cannot be right:
> 
>      if (NILP (visit) && inserted > 0)
>        {
>    #ifdef CLASH_DETECTION
> 	 if (!NILP (BVAR (current_buffer, file_truename))
> 	     /* Make binding buffer-file-name to nil effective.  */
> 	     && !NILP (BVAR (current_buffer, filename))
> 	     && SAVE_MODIFF >= MODIFF)
> 	   we_locked_file = 1;
>    #endif /* CLASH_DETECTION */
> 	 prepare_to_modify_buffer (GPT, GPT, NULL);
>        }
> 
> The above code is equivalent to a no-op, since 'inserted' must
> be zero here.

In Emacs 21.4a, the corresponding code was this:

    if (!NILP (visit))
      {
	...
  #ifdef CLASH_DETECTION
	if (NILP (handler))
	  {
	    if (!NILP (current_buffer->file_truename))
	      unlock_file (current_buffer->file_truename);
	    unlock_file (filename);
	  }
  #endif /* CLASH_DETECTION */

The change to the present form was between 21.4a and 22.1.  It appears
in the trunk history as part of a huge merge from a branch, probably
the Emacs 22.1 release branch.  I need more forensic work to find out
why the change was made, but at this point it certainly looks like
someone goofed with the condition.

What platforms use CLASH_DETECTION?

Message #11 received at 8496 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 8496 <at> debbugs.gnu.org
Subject: Re: bug#8496: some file locking code in insert-file-contents cannot
	be	right
Date: Thu, 14 Apr 2011 00:53:17 -0700

On 04/13/2011 11:48 PM, Eli Zaretskii wrote:
> What platforms use CLASH_DETECTION?

According to the .h files,
pretty much all the POSIXish platforms do it
(GNU/Linux, BSD, MacOS, AIX, etc.)
and it's also done on Cygwin.

Message #16 received at 8496-done <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 8496-done <at> debbugs.gnu.org
Subject: Re: bug#8496: some file locking code in insert-file-contents cannot
	be right
Date: Thu, 14 Apr 2011 10:00:14 +0200

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> What *is* intended here?

If you look at 9a95c4d it is obvious.

Andreas.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Message #19 received at 8496 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 8496 <at> debbugs.gnu.org
Subject: Re: bug#8496: some file locking code in insert-file-contents cannot
	be	right
Date: Thu, 14 Apr 2011 04:07:38 -0400

> Date: Thu, 14 Apr 2011 00:53:17 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: 8496 <at> debbugs.gnu.org
> 
> On 04/13/2011 11:48 PM, Eli Zaretskii wrote:
> > What platforms use CLASH_DETECTION?
> 
> According to the .h files,
> pretty much all the POSIXish platforms do it
> (GNU/Linux, BSD, MacOS, AIX, etc.)
> and it's also done on Cygwin.

That's what I thought.  I guess that pretty much excludes the
possibility that this went undetected because no one uses it.

Message #22 received at 8496 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: eggert <at> cs.ucla.edu, 8496 <at> debbugs.gnu.org
Subject: Re: bug#8496: some file locking code in insert-file-contents cannot be
	right
Date: Thu, 14 Apr 2011 04:55:14 -0400

> Date: Thu, 14 Apr 2011 02:48:28 -0400
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 8496 <at> debbugs.gnu.org
> Reply-To: Eli Zaretskii <eliz <at> gnu.org>
> 
> >      if (NILP (visit) && inserted > 0)
> >        {
> >    #ifdef CLASH_DETECTION
> > 	 if (!NILP (BVAR (current_buffer, file_truename))
> > 	     /* Make binding buffer-file-name to nil effective.  */
> > 	     && !NILP (BVAR (current_buffer, filename))
> > 	     && SAVE_MODIFF >= MODIFF)
> > 	   we_locked_file = 1;
> >    #endif /* CLASH_DETECTION */
> > 	 prepare_to_modify_buffer (GPT, GPT, NULL);
> >        }
> > 
> > The above code is equivalent to a no-op, since 'inserted' must
> > be zero here.
> 
> In Emacs 21.4a, the corresponding code was this:
> 
>     if (!NILP (visit))
>       {
> 	...
>   #ifdef CLASH_DETECTION
> 	if (NILP (handler))
> 	  {
> 	    if (!NILP (current_buffer->file_truename))
> 	      unlock_file (current_buffer->file_truename);
> 	    unlock_file (filename);
> 	  }
>   #endif /* CLASH_DETECTION */

Sorry, that was wrong.  The previous version was this:

   if (NILP (visit) && total > 0)
     prepare_to_modify_buffer (PT, PT, NULL);

The ChangeLog entry for the change (in revision 62259) is this:

     (Finsert_file_contents): If we read 0 bytes from a special file,
     unlock the visited file if we locked it.

Message #23 received at 8496-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 8496-done <at> debbugs.gnu.org
Subject: Re: bug#8496: some file locking code in insert-file-contents cannot
	be right
Date: Thu, 14 Apr 2011 11:56:06 -0700

My goodness, that was fast!  Thanks for the fix.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.