Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API
From: Brandon Casey <hidden>
Date: 2016-06-15 22:44:05
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API · Brandon Casey <hidden>
- 2016-06-15 · [PATCH 2/2] close_lock_file(): new function in the lockfile API · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API · Linus Torvalds <torvalds@linux-foundation.org>
- 2016-06-15 · Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API · Brandon Casey <hidden>
Junio C Hamano wrote:
Brandon Casey [off-list ref] writes:quoted
My patch does this, though I understand it may take some time to review.This is what I have right now, squashed your change into [2/2] I sent earlier, along with a couple of further fixups.
Mainly, I prefer to not modify the data structures when a failure occurs.
Generally when commit_lock_file fails, the caller die()'s and then
remove_lock_file will delete the temporary file. If the caller were to
not call die, I think it should call rollback_lock_file() similarly to
how unlock_ref() is called everywhere in refs.c.
So I think the semantics should be:
lock_fd = hold_lock_file(&lock);
<do_something>
if (close_lock_file(&lock)) {
rollback_lock_file(&lock);
return error;
}
if (commit_lock_file(&lock)) {
rollback_lock_file(&lock);
return error;
}
quoted hunk ↗ jump to hunk
diff --git a/lockfile.c b/lockfile.c index f45d3ed..fcf9285 100644 --- a/lockfile.c +++ b/lockfile.c@@ -13,7 +13,8 @@ static void remove_lock_file(void) while (lock_file_list) { if (lock_file_list->owner == me && lock_file_list->filename[0]) { - close(lock_file_list->fd); + if (lock_file_list->fd >= 0) + close(lock_file_list->fd); unlink(lock_file_list->filename); } lock_file_list = lock_file_list->next;@@ -159,11 +160,23 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on return fd; } +int close_lock_file(struct lock_file *lk) +{ + int fd = lk->fd; + lk->fd = -1; + return close(fd); +}
minor nit on this that I mentioned in another email.
+
int commit_lock_file(struct lock_file *lk)
{
char result_file[PATH_MAX];
int i;
- close(lk->fd);
+
+ if (lk->fd >= 0 && close_lock_file(lk)) {
+ unlink(lk->filename);
+ lk->filename[0] = 0;
+ return -1;
+ }I would rather have the caller call rollback_lock_file, or fall back to remove_lock_file rather than do this unlinking and modifying lk->filename here in commit_lock_file.
quoted hunk ↗ jump to hunk
strcpy(result_file, lk->filename); i = strlen(result_file) - 5; /* .lock */ result_file[i] = 0;@@ -185,9 +198,15 @@ void set_alternate_index_output(const char *name) int commit_locked_index(struct lock_file *lk) { if (alternate_index_output) { - int result = rename(lk->filename, alternate_index_output); - lk->filename[0] = 0; - return result; + const char *newname = alternate_index_output; + alternate_index_output = NULL; + + if (lk->fd >= 0 && close_lock_file(lk)) { + unlink(lk->filename); + lk->filename[0] = 0; + return -1; + }
ditto here.
+ return rename(lk->filename, newname);
If rename succeeds, we'll try to do an unnecessary unlink atexit. Having said those things, I will defer to your more experienced judgment. -brandon