git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	David Turner <novalis@novalis.org>,
	Jacob Keller <jacob.keller@gmail.com>,
	Philip Oakley <philipoakley@iee.org>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: [PATCH v4 13/23] log_ref_setup(): improve robustness against races
Date: Fri,  6 Jan 2017 17:22:33 +0100	[thread overview]
Message-ID: <ad86aeda2873070eae142a0de32bd34ec9bcfe99.1483719289.git.mhagger@alum.mit.edu> (raw)
In-Reply-To: <cover.1483719289.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 | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c8f6d82..27d4fd3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2710,6 +2710,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
@@ -2723,31 +2731,18 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 	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)) {
+			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 {
 		logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
-- 
2.9.3


  parent reply	other threads:[~2017-01-06 16:23 UTC|newest]

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

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=ad86aeda2873070eae142a0de32bd34ec9bcfe99.1483719289.git.mhagger@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jacob.keller@gmail.com \
    --cc=novalis@novalis.org \
    --cc=peff@peff.net \
    --cc=philipoakley@iee.org \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).