On 04/02/2014 06:58 PM, Junio C Hamano wrote:
> Jeff King <p...@peff.net> writes:
>> On Tue, Apr 01, 2014 at 05:58:12PM +0200, Michael Haggerty wrote:
>>> When rolling back the lockfile, call close_lock_file() so that the
>>> lock_file's fd field gets set back to -1.  This could help prevent
>>> confusion in the face of hypothetical future programming errors.
>> This also solves a race. We could be in the middle of rollback_lock_file
>> when we get a signal, and double-close. It's probably not a big deal,
>> though, since nobody could have opened a new descriptor in the interim
>> that got the same number (so the second close will just fail silently).
>> Still, this seems like a definite improvement.
> This is probably related to my comments on 2/22, but is "fd" the
> only thing that has a non-zero safe value?  Perhaps lock_file_init()
> that clears the structure fields to 0/NULL and fd to -1, and a
> convenience function lock_file_alloc() that does xmalloc() and then
> calls lock_file_init() may help us a bit when the lockfile structure
> is reused?

The first use of a lock_file object necessarily passes through
lock_file().  The only precondition for that function is that the
on_list field is zero, which is satisfied by a xcalloc()ed object.

Subsequent uses of a lock_file object must *not* zero the object.
lock_file objects are added to the lock_file_list and never removed.  So
zeroing a lock_file object would discard the rest of the linked list.
But subsequent uses must also pass through lock_file(), which sees that
on_list is set, and assumes that the object is in a self-consistent
state as left by commit_lock_file() or rollback_lock_file().

At least that's how it is supposed to work.  But lock_file objects are
in fact not cleaned up correctly in all circumstances.  The next version
of this patch series will work to fix that.


Michael Haggerty
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to