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

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

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