Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API
Brandon Casey <casey@nrlssc.navy.mil> writes:
> 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.
Improvement since my v1 are:
- close_lock_file() returns int, which is the return value from
close(2);
- remove_lock_file() avoids calling close(2) if
close_lock_file() was called earlier on the lockfile;
- commit_lock_file() does the same, and notices failure
returned from close_lock_file(). In addition, it unlinks the
lockfile and clears lk->filename upon failure;
- commit_locked_index() does the same, and notices failure
returned from close_lock_file() in alternate_index_output
codepath. It unlinks the lockfile and clears lk->filename
upon failure;
- The codepath in commit_locked_index() for writing to
alternate_index_output clears the alternate_index_output
variable when it is done.
Most of the above are thanks to the changes to lockfile.c in your version and Linus's suggestion.
I am planning to take your patch (only the parts that fix the callers, because the changes to lockfile.c are all included here) on top of this one.
-- >8 -- close_lock_file(): new function in the lockfile API
The lockfile API is a handy way to obtain a file that is cleaned up if you die(). But sometimes you would need this sequence to work:
1. hold_lock_file_for_update() to get a file descriptor for
writing; 2. write the contents out, without being able to decide if the
results should be committed or rolled back; 3. do something else that makes the decision --- and this
"something else" needs the lockfile not to have an open file
descriptor for writing (e.g. Windows do not want a open file
to be renamed); 4. call commit_lock_file() or rollback_lock_file() as
appropriately.This adds close_lock_file() you can call between step 2 and 3 in the above sequence.
[jc: updated with Brandon's stricter error checking on return values
from close() and a suggestion by Linus.]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/technical/api-lockfile.txt | 15 +++++++++++--
cache.h | 2 +-
lockfile.c | 32 ++++++++++++++++++++++++-----
3 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 5b1553e..dd89404 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -37,7 +37,8 @@ commit_lock_file::
Take a pointer to the `struct lock_file` initialized
with an earlier call to `hold_lock_file_for_update()`,
close the file descriptor and rename the lockfile to its
- final destination.
+ final destination. Returns 0 upon success, a negative
+ value on failure to close(2) or rename(2).
rollback_lock_file::
@@ -45,6 +46,12 @@ rollback_lock_file::
with an earlier call to `hold_lock_file_for_update()`,
close the file descriptor and remove the lockfile.
+close_lock_file::
+ Take a pointer to the `struct lock_file` initialized
+ with an earlier call to `hold_lock_file_for_update()`,
+ and close the file descriptor. Returns 0 upon success,
+ a negative value on failure to close(2).
+
Because the structure is used in an `atexit(3)` handler, its
storage has to stay throughout the life of the program. It
cannot be an auto variable allocated on the stack.
@@ -54,8 +61,10 @@ done writing to the file descriptor. If you do not call either
and simply `exit(3)` from the program, an `atexit(3)` handler
will close and remove the lockfile.
-You should not close the file descriptor you obtained from
-`hold_lock_file_for_update` function yourself. The `struct
+If you need to close the file descriptor you obtained from
+`hold_lock_file_for_update` function yourself, do so by calling
+`close_lock_file()`. You should never call `close(2)` yourself!
+Otherwise the `struct
lock_file` structure still remembers that the file descriptor
needs to be closed, and a later call to `commit_lock_file()` or
`rollback_lock_file()` will result in duplicate calls to
diff --git a/cache.h b/cache.h
index 39331c2..24735bd 100644
--- a/cache.h
+++ b/cache.h
@@ -308,7 +308,7 @@ extern int commit_lock_file(struct lock_file *);
extern int hold_locked_index(struct lock_file *, int);
extern int commit_locked_index(struct lock_file *);
extern void set_alternate_index_output(const char *);
-
+extern int close_lock_file(struct lock_file *);
extern void rollback_lock_file(struct lock_file *);
extern int delete_ref(const char *, const unsigned char *sha1);
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);
+}
+
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;
+ }
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;
+ }
+ return rename(lk->filename, newname);
}
else
return commit_lock_file(lk);
@@ -196,7 +215,8 @@ int commit_locked_index(struct lock_file *lk)
void rollback_lock_file(struct lock_file *lk)
{
if (lk->filename[0]) {
- close(lk->fd);
+ if (lk->fd >= 0)
+ close(lk->fd);
unlink(lk->filename);
}
lk->filename[0] = 0;
--
1.5.4.rc3.14.g44397
Brandon Casey
