All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Karl Moskowski <kmoskowski@me.com>,
	Jeff King <peff@peff.net>, Mike Hommey <mh@glandium.org>,
	David Turner <dturner@twopensource.com>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: [PATCH 08/20] log_ref_setup(): improve robustness against races
Date: Tue, 16 Feb 2016 14:22:21 +0100	[thread overview]
Message-ID: <9cee9cea6d284315f9a30e925dd0057bddc77c82.1455626201.git.mhagger@alum.mit.edu> (raw)
In-Reply-To: <cover.1455626201.git.mhagger@alum.mit.edu>

Change log_ref_setup() to use raceproof_create_file() to create the new
logfile. This makes it more robust against a race against another
process that might be trying to clean up empty directories while we are
trying to create a new logfile.

This also means that it will only call create_leading_directories() if
open() fails, which should be a net win. Even in the cases where we are
willing to create a new logfile, it will usually be the case that the
logfile already exists, or if not then that the directory containing the
logfile already exists. In such cases, we will save some work that was
previously done unconditionally.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 46 +++++++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f54d95b..0cfe1ce 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2582,6 +2582,14 @@ static int commit_ref(struct ref_lock *lock)
 	return 0;
 }
 
+static int open_or_create_logfile(const char *path, void *cb)
+{
+	int *fd = cb;
+
+	*fd = open(path, O_APPEND | O_WRONLY | O_CREAT, 0666);
+	return (*fd < 0) ? -1 : 0;
+}
+
 /*
  * Create a reflog for a ref.  If force_create = 0, the reflog will
  * only be created for certain refs (those for which
@@ -2593,36 +2601,24 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 	int logfd;
 
 	strbuf_git_path(logfile, "logs/%s", refname);
+
 	if (force_create || should_autocreate_reflog(refname)) {
-		if (safe_create_leading_directories(logfile->buf) < 0) {
-			strbuf_addf(err, "unable to create directory for %s: "
-				    "%s", logfile->buf, strerror(errno));
-			return -1;
-		}
-		logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
-		if (logfd < 0) {
-			if (errno == EISDIR) {
-				/*
-				 * The directory that is in the way might be
-				 * empty. Try to remove it.
-				 */
-				if (remove_empty_directories(logfile)) {
-					strbuf_addf(err, "There are still logs under "
-						    "'%s'", logfile->buf);
-					return -1;
-				}
-				logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
-			}
-
-			if (logfd < 0) {
+		if (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd) < 0) {
+			if (errno == ENOENT) {
+				strbuf_addf(err, "unable to create directory for %s: "
+					    "%s", logfile->buf, strerror(errno));
+			} else if (errno == EISDIR) {
+				strbuf_addf(err, "there are still logs under %s",
+					    logfile->buf);
+			} else {
 				strbuf_addf(err, "unable to append to %s: %s",
 					    logfile->buf, strerror(errno));
-				return -1;
 			}
+			return -1;
+		} else {
+			adjust_shared_perm(logfile->buf);
+			close(logfd);
 		}
-
-		adjust_shared_perm(logfile->buf);
-		close(logfd);
 	} else {
 		logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
 		if (logfd < 0) {
-- 
2.7.0

  parent reply	other threads:[~2016-02-16 13:32 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-16 13:22 [PATCH 00/20] Delete directories left empty after ref deletion Michael Haggerty
2016-02-16 13:22 ` [PATCH 01/20] safe_create_leading_directories_const(): preserve errno Michael Haggerty
2016-02-16 23:45   ` Jeff King
2016-02-16 13:22 ` [PATCH 02/20] safe_create_leading_directories(): set errno on SCLD_EXISTS Michael Haggerty
2016-02-17 19:23   ` Junio C Hamano
2016-02-18 15:33     ` Michael Haggerty
2016-02-16 13:22 ` [PATCH 03/20] raceproof_create_file(): new function Michael Haggerty
2016-02-17 19:38   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 04/20] lock_ref_sha1_basic(): use raceproof_create_file() Michael Haggerty
2016-02-17 20:44   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 05/20] rename_tmp_log(): " Michael Haggerty
2016-02-17 20:53   ` Junio C Hamano
2016-02-19 16:07     ` Michael Haggerty
2016-02-19 17:15       ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 06/20] rename_tmp_log(): improve error reporting Michael Haggerty
2016-02-18 22:14   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 07/20] log_ref_setup(): separate code for create vs non-create Michael Haggerty
2016-02-16 13:22 ` Michael Haggerty [this message]
2016-02-18 22:17   ` [PATCH 08/20] log_ref_setup(): improve robustness against races Junio C Hamano
2016-02-16 13:22 ` [PATCH 09/20] log_ref_setup(): pass the open file descriptor back to the caller Michael Haggerty
2016-02-18 22:21   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 10/20] log_ref_write_1(): don't depend on logfile Michael Haggerty
2016-02-16 13:22 ` [PATCH 11/20] log_ref_setup(): manage the name of the reflog file internally Michael Haggerty
2016-02-16 13:22 ` [PATCH 12/20] log_ref_write_1(): inline function Michael Haggerty
2016-02-18 22:23   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 13/20] try_remove_empty_parents(): rename parameter "name" -> "refname" Michael Haggerty
2016-02-16 13:22 ` [PATCH 14/20] try_remove_empty_parents(): don't trash argument contents Michael Haggerty
2016-02-16 13:22 ` [PATCH 15/20] try_remove_empty_parents(): don't accommodate consecutive slashes Michael Haggerty
2016-02-16 13:22 ` [PATCH 16/20] t5505: use "for-each-ref" to test for the non-existence of references Michael Haggerty
2016-02-16 13:22 ` [PATCH 17/20] delete_ref_loose(): derive loose reference path from lock Michael Haggerty
2016-02-16 13:22 ` [PATCH 18/20] delete_ref_loose(): inline function Michael Haggerty
2016-02-16 13:22 ` [PATCH 19/20] try_remove_empty_parents(): teach to remove parents of reflogs, too Michael Haggerty
2016-02-16 13:22 ` [PATCH 20/20] ref_transaction_commit(): clean up empty directories Michael Haggerty
2016-02-17  0:08 ` [PATCH 00/20] Delete directories left empty after ref deletion Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9cee9cea6d284315f9a30e925dd0057bddc77c82.1455626201.git.mhagger@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=dturner@twopensource.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kmoskowski@me.com \
    --cc=mh@glandium.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.