git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 19/20] ref_lock: stop leaking lock_files

From
Jeff King <peff@peff.net>
Date
Sep 5, 2017, 12:15 UTC
Message-ID
<20170905121515.xejuaiadexhmya2f@sigill.intra.peff.net>
In-Reply-To
<20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net>

Since the tempfile code recently relaxed the rule that tempfile structs (and thus locks) need to hang around forever, we no longer have to leak our lock_file structs.

In fact, we don't even need to heap-allocate them anymore, since their lifetime can just match that of the surrounding ref_lock (and if we forget to delete a lock, the effect is the same as before: it will eventually go away at program exit).

Note that there is a check in unlock_ref() to only rollback a lock file if it has been allocated. We don't need that check anymore; we zero the ref_lock (and thus the lock_file), so at worst we pass a NULL pointer to delete_tempfile(), which considers that a noop.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 36fe8a986b..f3455609d6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -12,7 +12,7 @@
 
 struct ref_lock {
 	char *ref_name;
-	struct lock_file *lk;
+	struct lock_file lk;
 	struct object_id old_oid;
 };
 
@@ -418,9 +418,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 
 static void unlock_ref(struct ref_lock *lock)
 {
-	/* Do not free lock->lk -- atexit() still looks at them */
-	if (lock->lk)
-		rollback_lock_file(lock->lk);
+	rollback_lock_file(&lock->lk);
 	free(lock->ref_name);
 	free(lock);
 }
@@ -534,11 +532,8 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		goto error_return;
 	}
 
-	if (!lock->lk)
-		lock->lk = xcalloc(1, sizeof(struct lock_file));
-
 	if (hold_lock_file_for_update_timeout(
-			    lock->lk, ref_file.buf, LOCK_NO_DEREF,
+			    &lock->lk, ref_file.buf, LOCK_NO_DEREF,
 			    get_files_ref_lock_timeout_ms()) < 0) {
 		if (errno == ENOENT && --attempts_remaining > 0) {
 			/*
@@ -949,11 +944,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
 		goto error_return;
 	}
 
-	lock->lk = xcalloc(1, sizeof(struct lock_file));
-
 	lock->ref_name = xstrdup(refname);
 
-	if (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) {
+	if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {
 		last_errno = errno;
 		unable_to_lock_message(ref_file.buf, errno, err);
 		goto error_return;
@@ -1404,14 +1397,14 @@ static int files_rename_ref(struct ref_store *ref_store,
 
 static int close_ref_gently(struct ref_lock *lock)
 {
-	if (close_lock_file_gently(lock->lk))
+	if (close_lock_file_gently(&lock->lk))
 		return -1;
 	return 0;
 }
 
 static int commit_ref(struct ref_lock *lock)
 {
-	char *path = get_locked_file_path(lock->lk);
+	char *path = get_locked_file_path(&lock->lk);
 	struct stat st;
 
 	if (!lstat(path, &st) && S_ISDIR(st.st_mode)) {
@@ -1435,7 +1428,7 @@ static int commit_ref(struct ref_lock *lock)
 		free(path);
 	}
 
-	if (commit_lock_file(lock->lk))
+	if (commit_lock_file(&lock->lk))
 		return -1;
 	return 0;
 }
@@ -1627,12 +1620,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	fd = get_lock_file_fd(lock->lk);
+	fd = get_lock_file_fd(&lock->lk);
 	if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
 	    write_in_full(fd, &term, 1) != 1 ||
 	    close_ref_gently(lock) < 0) {
 		strbuf_addf(err,
-			    "couldn't write '%s'", get_lock_file_path(lock->lk));
+			    "couldn't write '%s'", get_lock_file_path(&lock->lk));
 		unlock_ref(lock);
 		return -1;
 	}
@@ -1709,7 +1702,7 @@ static int create_ref_symlink(struct ref_lock *lock, const char *target)
 {
 	int ret = -1;
 #ifndef NO_SYMLINK_HEAD
-	char *ref_path = get_locked_file_path(lock->lk);
+	char *ref_path = get_locked_file_path(&lock->lk);
 	unlink(ref_path);
 	ret = symlink(target, ref_path);
 	free(ref_path);
@@ -1745,14 +1738,14 @@ static int create_symref_locked(struct files_ref_store *refs,
 		return 0;
 	}
 
-	if (!fdopen_lock_file(lock->lk, "w"))
+	if (!fdopen_lock_file(&lock->lk, "w"))
 		return error("unable to fdopen %s: %s",
-			     lock->lk->tempfile->filename.buf, strerror(errno));
+			     lock->lk.tempfile->filename.buf, strerror(errno));
 
 	update_symref_reflog(refs, lock, refname, target, logmsg);
 
 	/* no error check; commit_ref will check ferror */
-	fprintf(lock->lk->tempfile->fp, "ref: %s\n", target);
+	fprintf(lock->lk.tempfile->fp, "ref: %s\n", target);
 	if (commit_ref(lock) < 0)
 		return error("unable to write symref for %s: %s", refname,
 			     strerror(errno));
@@ -2853,12 +2846,12 @@ static int files_reflog_expire(struct ref_store *ref_store,
 					strerror(errno));
 			rollback_lock_file(&reflog_lock);
 		} else if (update &&
-			   (write_in_full(get_lock_file_fd(lock->lk),
+			   (write_in_full(get_lock_file_fd(&lock->lk),
 				oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
-			    write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||
+			    write_str_in_full(get_lock_file_fd(&lock->lk), "\n") != 1 ||
 			    close_ref_gently(lock) < 0)) {
 			status |= error("couldn't write %s",
-					get_lock_file_path(lock->lk));
+					get_lock_file_path(&lock->lk));
 			rollback_lock_file(&reflog_lock);
 		} else if (commit_lock_file(&reflog_lock)) {
 			status |= error("unable to write reflog '%s' (%s)",

-- 
2.14.1.721.gc5bc1565f1
Previous: Jeff KingNext: Jeff King
Message 19 of 21 in “allowing struct lock_file to be deleted”
  1. 0/20 allowing struct lock_file to be deletedJeff King, Sep 5, 2017
  2. 01/20 write_index_as_tree: cleanup tempfile on errorJeff King, Sep 5, 2017
  3. 02/20 setup_temporary_shallow: avoid using inactive tempfileJeff King, Sep 5, 2017
  4. 03/20 setup_temporary_shallow: move tempfile struct into functionJeff King, Sep 5, 2017
  5. 04/20 verify_signed_buffer: prefer close_tempfile() to close()Jeff King, Sep 5, 2017
  6. 05/20 always check return value of close_tempfileJeff King, Sep 5, 2017
  7. 06/20 tempfile: do not delete tempfile on failed closeJeff King, Sep 5, 2017
  8. 07/20 lockfile: do not rollback lock on failed closeJeff King, Sep 5, 2017
  9. 08/20 tempfile: prefer is_tempfile_active to bare accessJeff King, Sep 5, 2017
  10. 09/20 tempfile: handle NULL tempfile pointers gracefullyJeff King, Sep 5, 2017
  11. 10/20 tempfile: replace die("BUG") with BUG()Jeff King, Sep 5, 2017
  12. 11/20 tempfile: factor out activationJeff King, Sep 5, 2017
  13. 12/20 tempfile: factor out deactivationJeff King, Sep 5, 2017
  14. 13/20 tempfile: robustify cleanup handlerJeff King, Sep 5, 2017
  15. 14/20 tempfile: release deactivated strbufs instead of resettingJeff King, Sep 5, 2017
  16. 16/20 tempfile: remove deactivated list entriesJeff King, Sep 5, 2017
  17. 17/20 tempfile: auto-allocate tempfiles on heapJeff King, Sep 5, 2017
  18. 18/20 lockfile: update lifetime requirements in documentationJeff King, Sep 5, 2017
  19. 19/20 ref_lock: stop leaking lock_filesJeff King, Sep 5, 2017
  20. 20/20 stop leaking lock structs in some simple casesJeff King, Sep 5, 2017
  21. 15/20 tempfile: use list.h for linked listJeff King, Sep 5, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.