{"thread":{"id":"9021","subject":"[PATCH] lockfile.c: schedule remove_lock_file only once.","startedAt":"2007-07-13T14:14:50Z","lastAt":"2007-07-15T08:35:21Z","messageCount":3,"participants":["Sven Verdoolaege","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"47267","messageId":"20070713141450.GA8392MdfPADPa@greensroom.kotnet.org","threadId":"9021","inReplyTo":null,"subject":"[PATCH] lockfile.c: schedule remove_lock_file only once.","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2007-07-13T14:14:50Z","receivedAt":"2007-07-13T14:14:50Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"Removing a lockfile once should be enough.\n\nSigned-off-by: Sven Verdoolaege <skimo@kotnet.org>\n---\n...unless we're running on VMS.\n\nAnyway, it's not clear to me why we can't remove lk from\nlock_file_list (and then free it) after we unlink it\nin unlock_ref.\n\nskimo\n\n lockfile.c |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 5ad2858..fb8f13b 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -31,16 +31,16 @@ static int lock_file(struct lock_file *lk, const char *path)\n \tsprintf(lk->filename, \"%s.lock\", path);\n \tfd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= fd) {\n+\t\tif (!lock_file_list) {\n+\t\t\tsignal(SIGINT, remove_lock_file_on_signal);\n+\t\t\tatexit(remove_lock_file);\n+\t\t}\n \t\tlk->owner = getpid();\n \t\tif (!lk->on_list) {\n \t\t\tlk->next = lock_file_list;\n \t\t\tlock_file_list = lk;\n \t\t\tlk->on_list = 1;\n \t\t}\n-\t\tif (lock_file_list) {\n-\t\t\tsignal(SIGINT, remove_lock_file_on_signal);\n-\t\t\tatexit(remove_lock_file);\n-\t\t}\n \t\tif (adjust_shared_perm(lk->filename))\n \t\t\treturn error(\"cannot fix permission bits on %s\",\n \t\t\t\t     lk->filename);\n-- \n1.5.3.rc1.10.gae1ae\n"},{"id":"47282","messageId":"7vabu0noxg.fsf@assigned-by-dhcp.cox.net","threadId":"9021","inReplyTo":"20070713141450.GA8392MdfPADPa@greensroom.kotnet.org","subject":"Re: [PATCH] lockfile.c: schedule remove_lock_file only once.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-13T18:23:07Z","receivedAt":"2007-07-13T18:23:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sven Verdoolaege <skimo@kotnet.org> writes:\n\n> Removing a lockfile once should be enough.\n\nYeah.  I wonder what we were smoking.  415e96c8 which introduces\nthe atexit to index.c does:\n\n    int hold_index_file_for_update(struct cache_file *cf, const char *path)\n    {\n           sprintf(cf->lockfile, \"%s.lock\", path);\n           cf->next = cache_file_list;\n           cache_file_list = cf;\n           if (!cf->next) {\n                   signal(SIGINT, remove_lock_file_on_signal);\n                   atexit(remove_lock_file);\n           }\n           return open(cf->lockfile, O_RDWR | O_CREAT | O_EXCL, 0600);\n    }\n\nwhose intent is exactly \"do this once, only for the first one\".\n\nThe reason we do not use lk->next but instead check lk->on_list,\nand the reason why we do not remove the lock from the list, are\ndescribed in 1084b845.\n\nBut your \"fire atexit() once\" fix is needed.  Thanks.\n"},{"id":"47404","messageId":"20070715083521.GB999MdfPADPa@greensroom.kotnet.org","threadId":"9021","inReplyTo":"7vabu0noxg.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] lockfile.c: schedule remove_lock_file only once.","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2007-07-15T08:35:21Z","receivedAt":"2007-07-15T08:35:21Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"On Fri, Jul 13, 2007 at 11:23:07AM -0700, Junio C Hamano wrote:\n> The reason we do not use lk->next but instead check lk->on_list,\n> and the reason why we do not remove the lock from the list, are\n> described in 1084b845.\n\nI'm afraid I'm still missing something:\n\n1084b845 commit message:\n    We cannot remove the list element in commit_lock_file(); if we\n    are interrupted in the middle of list manipulation, the call to\n    remove_lock_file_on_signal() will happen with a broken list\n    structure pointed by lock_file_list, which would cause the cruft\n    to remain, so not removing the list element is the right thing\n    to do.  Instead we should be reusing the element already on the\n    list.\n\nWe have a list\n\nlist--->A--->B--->C\n\nand we overwrite one next pointer to remove an element, say B\n\nlist--->A-------->C\n\nAt what point is the list structure broken?\n\nIf you are worried that the interrupt could happen in the middle\nof writing the pointer (could it?), then shouldn't you worry\nabout adding elements too?\n\nskimo\n"}]}