# [PATCH] lockfile.c: schedule remove_lock_file only once. 3 messages from 2007-07-13 to 2007-07-15. Participants: Sven Verdoolaege, Junio C Hamano. Thread: https://gitlist.dev/t/9021 ## Sven Verdoolaege, 2007-07-13 14:14 Subject: [PATCH] lockfile.c: schedule remove_lock_file only once. Message-ID: <20070713141450.GA8392MdfPADPa@greensroom.kotnet.org> URL: https://gitlist.dev/e/20070713141450.GA8392MdfPADPa%40greensroom.kotnet.org ``` Removing a lockfile once should be enough. Signed-off-by: Sven Verdoolaege --- ...unless we're running on VMS. Anyway, it's not clear to me why we can't remove lk from lock_file_list (and then free it) after we unlink it in unlock_ref. skimo lockfile.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lockfile.c b/lockfile.c index 5ad2858..fb8f13b 100644 --- a/lockfile.c +++ b/lockfile.c @@ -31,16 +31,16 @@ static int lock_file(struct lock_file *lk, const char *path) sprintf(lk->filename, "%s.lock", path); fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666); if (0 <= fd) { + if (!lock_file_list) { + signal(SIGINT, remove_lock_file_on_signal); + atexit(remove_lock_file); + } lk->owner = getpid(); if (!lk->on_list) { lk->next = lock_file_list; lock_file_list = lk; lk->on_list = 1; } - if (lock_file_list) { - signal(SIGINT, remove_lock_file_on_signal); - atexit(remove_lock_file); - } if (adjust_shared_perm(lk->filename)) return error("cannot fix permission bits on %s", lk->filename); -- 1.5.3.rc1.10.gae1ae ``` ## Junio C Hamano, 2007-07-13 18:23 Subject: Re: [PATCH] lockfile.c: schedule remove_lock_file only once. Message-ID: <7vabu0noxg.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vabu0noxg.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20070713141450.GA8392MdfPADPa@greensroom.kotnet.org> ``` Sven Verdoolaege writes: > Removing a lockfile once should be enough. Yeah. I wonder what we were smoking. 415e96c8 which introduces the atexit to index.c does: int hold_index_file_for_update(struct cache_file *cf, const char *path) { sprintf(cf->lockfile, "%s.lock", path); cf->next = cache_file_list; cache_file_list = cf; if (!cf->next) { signal(SIGINT, remove_lock_file_on_signal); atexit(remove_lock_file); } return open(cf->lockfile, O_RDWR | O_CREAT | O_EXCL, 0600); } whose intent is exactly "do this once, only for the first one". The reason we do not use lk->next but instead check lk->on_list, and the reason why we do not remove the lock from the list, are described in 1084b845. But your "fire atexit() once" fix is needed. Thanks. ``` ## Sven Verdoolaege, 2007-07-15 08:35 Subject: Re: [PATCH] lockfile.c: schedule remove_lock_file only once. Message-ID: <20070715083521.GB999MdfPADPa@greensroom.kotnet.org> URL: https://gitlist.dev/e/20070715083521.GB999MdfPADPa%40greensroom.kotnet.org In-Reply-To: <7vabu0noxg.fsf@assigned-by-dhcp.cox.net> ``` On Fri, Jul 13, 2007 at 11:23:07AM -0700, Junio C Hamano wrote: > The reason we do not use lk->next but instead check lk->on_list, > and the reason why we do not remove the lock from the list, are > described in 1084b845. I'm afraid I'm still missing something: 1084b845 commit message: We cannot remove the list element in commit_lock_file(); if we are interrupted in the middle of list manipulation, the call to remove_lock_file_on_signal() will happen with a broken list structure pointed by lock_file_list, which would cause the cruft to remain, so not removing the list element is the right thing to do. Instead we should be reusing the element already on the list. We have a list list--->A--->B--->C and we overwrite one next pointer to remove an element, say B list--->A-------->C At what point is the list structure broken? If you are worried that the interrupt could happen in the middle of writing the pointer (could it?), then shouldn't you worry about adding elements too? skimo ```