Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

From: Jeff King <peff@peff.net>
Date: Jun 2, 2011, 14:29 UTC
Message-ID: <20110602142933.GA32487@sigill.intra.peff.net>
In-Reply-To: <BANLkTinc=9aW_y6s-Zb578QsMawnxFD55Q@mail.gmail.com>

On Thu, Jun 02, 2011 at 06:23:03PM +0530, Ramkumar Ramachandra wrote:

Show 10 quoted lines

> On a slightly unrelated note, I just found out that the persist_todo
> can't be called twice due to a lockfile API limitation* -- the
> atexit(3) cleanup handler is supposed to have a linked list of
> lockfiles to clean up. When one lockfile is used twice, it becomes a
> circularly linked list, and the entire program hangs at exit time
> since traversing a circularly linked list is never-ending. Couple of
> comments:
> 1. By reading the lockfile implementation, I understand why this
> happens -- can't it be implemented differently to remove this
> limitation? If not, shouldn't this limitation be documented somewhere?

Doesn't lock_file handle multiple locking already via the on_list parameter?

    $ grep -A2 on_list lockfile.c
            if (!lk->on_list) {
                    lk->next = lock_file_list;
                    lock_file_list = lk;
                    lk->on_list = 1;
            }

However, I think you have a bigger problem, which is that you are allocating the lock_file on the stack in persist_todo and persist_head. So by the time the atexit() handler is called, this storage has gone away and you are just reading random data (not to mention that it also should be zero-initialized before being passed to lock_file).

-Peff