lockfile: do not rollback lock on failed close
Since the lockfile code is based on the tempfile code, it has some of the same problems, including that close_lock_file() erases the tempfile's filename buf, making it hard for the caller to write a good error message. In practice this comes up less for lockfiles than for straight tempfiles, since we usually just report the refname. But there is at least one buggy case in write_ref_to_lockfile(). Besides, given the coupling between the lockfile and tempfile modules, it's less confusing if their close() functions have the same semantics. Just as the previous commit did for close_tempfile(), let's teach close_lock_file() and its wrapper close_ref() not to rollback on error. And just as before, we'll give them new "gently" names to catch any new callers that are added. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King committed
Sep 5, 2017 at 08:14 UTC
83a3069a3895de81fea720ffa6a3e47f9400fe04
4 files changed
+22
-26
lockfile.h
+12
-18
@@ -69,7 +69,7 @@
69
* `rollback_lock_file()`.
70
*
71
* * Close the file descriptor without removing or renaming the
72
- * lockfile by calling `close_lock_file()`, and later call
72
+ * lockfile by calling `close_lock_file_gently()`, and later call
73
* `commit_lock_file()`, `commit_lock_file_to()`,
74
* `rollback_lock_file()`, or `reopen_lock_file()`.
75
*
@@ -85,7 +85,7 @@
85
*
86
* If you need to close the file descriptor you obtained from a
87
* `hold_lock_file_for_*()` function yourself, do so by calling
88
- * `close_lock_file()`. See "tempfile.h" for more information.
88
+ * `close_lock_file_gently()`. See "tempfile.h" for more information.
89
*
90
*
91
* Under the covers, a lockfile is just a tempfile with a few helper
@@ -104,8 +104,8 @@
104
*
105
* Similarly, `commit_lock_file`, `commit_lock_file_to`, and
106
* `close_lock_file` return 0 on success. On failure they set `errno`
107
- * appropriately, do their best to roll back the lockfile, and return
108
- * -1.
107
+ * appropriately and return -1. The `commit` variants (but not `close`)
108
+ * do their best to delete the temporary file before returning.
109
*/
110
111
#include "tempfile.h"
@@ -202,8 +202,9 @@ extern NORETURN void unable_to_lock_die(const char *path, int err);
202
/*
203
* Associate a stdio stream with the lockfile (which must still be
204
* open). Return `NULL` (*without* rolling back the lockfile) on
205
- * error. The stream is closed automatically when `close_lock_file()`
206
- * is called or when the file is committed or rolled back.
205
+ * error. The stream is closed automatically when
206
+ * `close_lock_file_gently()` is called or when the file is committed or
207
+ * rolled back.
208
*/
209
static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
210
{
@@ -241,28 +242,21 @@ extern char *get_locked_file_path(struct lock_file *lk);
242
* lockfile over the file being locked. Return 0 upon success. On
243
* failure to `close(2)`, return a negative value and roll back the
244
* lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`,
244
- * or `rollback_lock_file()` should eventually be called if
245
- * `close_lock_file()` succeeds.
245
+ * or `rollback_lock_file()` should eventually be called.
246
*/
247
-static inline int close_lock_file(struct lock_file *lk)
247
+static inline int close_lock_file_gently(struct lock_file *lk)
248
{
249
- int ret = close_tempfile_gently(&lk->tempfile);
250
- if (ret) {
251
- int saved_errno = errno;
252
- delete_tempfile(&lk->tempfile);
253
- errno = saved_errno;
254
- }
255
- return ret;
249
+ return close_tempfile_gently(&lk->tempfile);
250
}
251
252
/*
259
- * Re-open a lockfile that has been closed using `close_lock_file()`
253
+ * Re-open a lockfile that has been closed using `close_lock_file_gently()`
254
* but not yet committed or rolled back. This can be used to implement
255
* a sequence of operations like the following:
256
*
257
* * Lock file.
258
*
265
- * * Write new contents to lockfile, then `close_lock_file()` to
259
+ * * Write new contents to lockfile, then `close_lock_file_gently()` to
260
* cause the contents to be written to disk.
261
*
262
* * Pass the name of the lockfile to another program to allow it (and
read-cache.c
+1
-1
@@ -2345,7 +2345,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
2345
if (flags & COMMIT_LOCK)
2346
return commit_locked_index(lock);
2347
else if (flags & CLOSE_LOCK)
2348
- return close_lock_file(lock);
2348
+ return close_lock_file_gently(lock);
2349
else
2350
return ret;
2351
}
refs/files-backend.c
+7
-6
@@ -1402,9 +1402,9 @@ static int files_rename_ref(struct ref_store *ref_store,
1402
return ret;
1403
}
1404
1405
-static int close_ref(struct ref_lock *lock)
1405
+static int close_ref_gently(struct ref_lock *lock)
1406
{
1407
- if (close_lock_file(lock->lk))
1407
+ if (close_lock_file_gently(lock->lk))
1408
return -1;
1409
return 0;
1410
}
@@ -1630,7 +1630,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
1630
fd = get_lock_file_fd(lock->lk);
1631
if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
1632
write_in_full(fd, &term, 1) != 1 ||
1633
- close_ref(lock) < 0) {
1633
+ close_ref_gently(lock) < 0) {
1634
strbuf_addf(err,
1635
"couldn't write '%s'", get_lock_file_path(lock->lk));
1636
unlock_ref(lock);
@@ -2372,7 +2372,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
2372
* the lockfile is still open. Close it to
2373
* free up the file descriptor:
2374
*/
2375
- if (close_ref(lock)) {
2375
+ if (close_ref_gently(lock)) {
2376
strbuf_addf(err, "couldn't close '%s.lock'",
2377
update->refname);
2378
return TRANSACTION_GENERIC_ERROR;
@@ -2848,14 +2848,15 @@ static int files_reflog_expire(struct ref_store *ref_store,
2848
!(type & REF_ISSYMREF) &&
2849
!is_null_oid(&cb.last_kept_oid);
2850
2851
- if (close_lock_file(&reflog_lock)) {
2851
+ if (close_lock_file_gently(&reflog_lock)) {
2852
status |= error("couldn't write %s: %s", log_file,
2853
strerror(errno));
2854
+ rollback_lock_file(&reflog_lock);
2855
} else if (update &&
2856
(write_in_full(get_lock_file_fd(lock->lk),
2857
oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
2858
write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||
2858
- close_ref(lock) < 0)) {
2859
+ close_ref_gently(lock) < 0)) {
2860
status |= error("couldn't write %s",
2861
get_lock_file_path(lock->lk));
2862
rollback_lock_file(&reflog_lock);
refs/packed-backend.c
+2
-1
@@ -545,8 +545,9 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
545
return -1;
546
}
547
548
- if (close_lock_file(&refs->lock)) {
548
+ if (close_lock_file_gently(&refs->lock)) {
549
strbuf_addf(err, "unable to close %s: %s", refs->path, strerror(errno));
550
+ rollback_lock_file(&refs->lock);
551
return -1;
552
}
553