refs.c: handle locking failure during transaction better

Change lock_ref_sha1_basic to return an error instead of dying, when we fail to lock a file during a transaction. This function is only called from transaction_commit() and it knows how to handle these failures.

Cherry-picked-to-master: Stefan Beller <[email protected]>

---
 refs.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

This was part of the ref-transactions-rename series[1], but it makes sense to send this one as a separate patch.

Thanks,
Stefan

[1] https://www.mail-archive.com/[email protected]/msg60604.html

index 5ff457e..0347328 100644
@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
 	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
 	if (lock->lock_fd < 0) {
+		last_errno = errno;
 		if (errno == ENOENT && --attempts_remaining > 0)
 			/*
 			 * Maybe somebody just deleted one of the

@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 * again:
 			 */
 			goto retry;
-		else
-			unable_to_lock_die(ref_file, errno);
+		else {
+			struct strbuf err = STRBUF_INIT;
+			unable_to_lock_message(ref_file, errno, &err);
+			error("%s", err.buf);
+			strbuf_reset(&err);
+			goto error_return;
+		}
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 

2.2.0.rc2.5.gf7b9fb2

I messed up authorship on this one.
This was of course found and authored by Ronnie.

From: Ronnie Sahlberg <[email protected]>

Change lock_ref_sha1_basic to return an error instead of dying, when we fail to lock a file during a transaction. This function is only called from transaction_commit() and it knows how to handle these failures.

[sb: This was part of a larger patch series, cherry-picked to master]

---
 refs.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 5ff457e..0347328 100644
@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
 	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
 	if (lock->lock_fd < 0) {
+		last_errno = errno;
 		if (errno == ENOENT && --attempts_remaining > 0)
 			/*
 			 * Maybe somebody just deleted one of the

@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 * again:
 			 */
 			goto retry;
-		else
-			unable_to_lock_die(ref_file, errno);
+		else {
+			struct strbuf err = STRBUF_INIT;
+			unable_to_lock_message(ref_file, errno, &err);
+			error("%s", err.buf);
+			strbuf_reset(&err);
+			goto error_return;
+		}
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 

2.2.0.rc2.5.gf7b9fb2

I think this is a good thing to do. I independently wrote the same patch recently, along with some other cleanups. Here's the series I ended up with (I added Ronnie as the author of the final one, which replaces this; even though my discovery was independent, he wrote it first :) ).

[1/4]: error: save and restore errno
[2/4]: lock_ref_sha1_basic: simplify errno handling [3/4]: lock_ref_sha1_basic: simplify error code path [4/4]: lock_ref_sha1_basic: do not die on locking errors

-Peff

It's common to use error() to return from a function, like:

	if (open(...) < 0)
		return error("open failed");

Unfortunately this may clobber the errno from the open() call. So we often end up with code like this:

        if (open(...) < 0) {
		int saved_errno = errno;
		error("open failed");
		errno = saved_errno;
		return -1;
	}

which is less nice. Let's teach error() to save and restore errno in each call, so that the original errno is preserved. This is slightly less efficient for callers which do not care, but error code paths are generally not performance critical anyway.

---
It's pretty minor to just handle errno in the callers, but I feel like I've wanted this at least a dozen times, and it seems like it cannot possibly hurt (i.e., I imagine there are callers where we _should_ be doing the errno dance but have not realized it).

 usage.c | 3 +++
 1 file changed, 3 insertions(+)

index ed14645..ee44d57 100644
@@ -142,10 +142,13 @@ void NORETURN die_errno(const char *fmt, ...)
 int error(const char *err, ...)
 {
 	va_list params;
+	int saved_errno = errno;
 
 	va_start(params, err);
 	error_routine(err, params);
 	va_end(params);
+
+	errno = saved_errno;
 	return -1;
 }
 

2.1.2.596.g7379948

What the above doesn't explain is why the caller cares about errno. Are they going to print another message with strerror(errno)? Or are they going to consider some errors non-errors (like ENOENT when trying to unlink a file), in which case why is printing a message to stderr okay?

All that said, given that there are real examples of code already doing this, the patch seems sane.

[...]

I guess the unsaid bit is:

Unfortunately this may clobber the errno from the open() call. Even though error() sees the correct errno, the caller to which we are returning may see a bogus errno value.

-Peff

I am not sure if that answers the question asked.

If you have

	int frotz(...) {
		int fd = open(...);
        	if (fd < 0)
                	return error("open failed (%s)", strerror(errno));
		return fd;
	}

and the caller calls it and cares about the errno from this open, what does the caller do? Jonathan's worried about a codepath that may be familiar to us as we recently saw a patch similar to it:

	int fd = frotz(...);
        if (fd < 0) {
        	if (errno == ENOENT || errno == EISDIR)
                	; /* not quite an error */
		else
			exit(1);
	}

If ENOENT/EISDIR is expected and a non-error, it is not useful for frotz() to give an error message on its own.

I think a more appropriate answer to Jonathan's question is why is the callee (i.e. frotz()) calling error() in the first place if an unconditional error message is an issue.

Sure, but isn't that out-of-scope for this patch? We know that some functions _are_ calling error() and taking care to keep errno valid, and we would prefer to do that with less work.

If you are arguing "there are literally zero cases where that makes sense; functions should either complain themselves, _or_ they should pass on a valid errno so the caller can decide whether to complain, but doing both is a recipe for pointless and unwanted error messages", then this patch is counter-productive (and we should be fixing those call sites of error() instead).

But I am not sure that there are zero legitimate cases. Just as a hypothetical, imagine you wanted to complain to stderr _and_ propagate the error value into a specific exit code. You'd want something like:

  fd = frotz(...);
  error("frotz failed (%s)", strerror(errno));
  exit(errno == ENOENT ? 1 : 2);

Granted, I do not think we do that particular pattern in git, but we do take pains to save errno across some error() calls. I do not know which of those are legitimate and which should be refactored. In the meantime, I do not think this patch makes anything worse.

Exactly. It is not error()'s business to know what the caller wants to do, or whether or why it cares about retaining errno. But error(), since it is designed to be called in error codepaths, should aim to be minimally intrusive.

-Peff

This one makes my day.
A really good fix as it minimizes maintenance burden for checking incoming patches for that pattern.

Now that error() does not clobber errno, we do not have to take pains to save it ourselves.

---
 refs.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

index 5ff457e..169a46d 100644
@@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	char *ref_file;
 	const char *orig_refname = refname;
 	struct ref_lock *lock;
-	int last_errno = 0;
 	int type, lflags;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
 	int resolve_flags = 0;

@@ -2260,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		 */
 		ref_file = git_path("%s", orig_refname);
 		if (remove_empty_directories(ref_file)) {
-			last_errno = errno;
 			error("there are still refs under '%s'", orig_refname);
 			goto error_return;
 		}

@@ -2270,7 +2268,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	if (type_p)
 	    *type_p = type;
 	if (!refname) {
-		last_errno = errno;
 		error("unable to resolve reference %s: %s",
 			orig_refname, strerror(errno));
 		goto error_return;

@@ -2283,7 +2280,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 */
 	if (missing &&
 	     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
-		last_errno = ENOTDIR;
+		errno = ENOTDIR;
 		goto error_return;
 	}
 

@@ -2311,7 +2308,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			goto retry;
 		/* fall through */
 	default:
-		last_errno = errno;
 		error("unable to create directory for %s", ref_file);
 		goto error_return;
 	}

@@ -2332,7 +2328,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
  error_return:
 	unlock_ref(lock);
-	errno = last_errno;
 	return NULL;
 }
 

2.1.2.596.g7379948

For most errors, we jump to a goto label that unlocks the ref and returns NULL. However, in none of these error paths would we ever have actually locked the ref. By the time we actually take the lock, we follow a different path that does not ever hit this goto (we rely on verify_lock to unlock if it finds an error).

We can just drop the goto completely and return NULL as appropriate.

---
 refs.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

index 169a46d..fafae7e 100644
@@ -2260,7 +2260,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		ref_file = git_path("%s", orig_refname);
 		if (remove_empty_directories(ref_file)) {
 			error("there are still refs under '%s'", orig_refname);
-			goto error_return;
+			return NULL;
 		}
 		refname = resolve_ref_unsafe(orig_refname, resolve_flags,
 					     lock->old_sha1, &type);

@@ -2270,7 +2270,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	if (!refname) {
 		error("unable to resolve reference %s: %s",
 			orig_refname, strerror(errno));
-		goto error_return;
+		return NULL;
 	}
 	missing = is_null_sha1(lock->old_sha1);
 	/* When the ref did not exist and we are creating it,

@@ -2281,7 +2281,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	if (missing &&
 	     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
 		errno = ENOTDIR;
-		goto error_return;
+		return NULL;
 	}
 
 	lock->lk = xcalloc(1, sizeof(struct lock_file));

@@ -2309,7 +2309,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		/* fall through */
 	default:
 		error("unable to create directory for %s", ref_file);
-		goto error_return;
+		return NULL;
 	}
 
 	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);

@@ -2325,10 +2325,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			unable_to_lock_die(ref_file, errno);
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
-
- error_return:
-	unlock_ref(lock);
-	return NULL;
 }
 
 struct ref_lock *lock_any_ref_for_update(const char *refname,

2.1.2.596.g7379948

[...]

Wouldn't this leak lock (in all cases) and lock->ref_name and lock->orig_ref_name (on safe_create_leading_directories failure)?

Ah, you're right. I totally missed that unlock_ref is not just about unlocking, but about free()ing. We do need to keep the goto/unlock.

Hmph. Should we just abandon my series in favor of taking Ronnie's original patch, then? We can apply the "save/restore errno in error()" patch independently if we like.

-Peff

I liked patches 1 and 2 and the explanation from patch 4. Perhaps patch 3 should be replaced with a patch renaming unlock_ref to free_ref_lock or something.

Thanks,
Jonathan

I took a look at this, and it ends up not being very useful.

The "return NULL" from the final patch has to become a "goto error_return", so that it can call unlock_ref(). But that means it cannot save and restore errno itself, because unlock_ref may clobber errno[1].

So we still have to keep the last_errno handling in error_return. Meaning that we need to drop patch 2 (even though the other cases don't need errno saved/restore, since the goto does it unconditionally, we still need to set last_errno). And therefore patch 1 is not helping anyone (we could still apply it, but there's no immediate benefit).

I also looked at renaming unlock_ref, but it is called from other places where they _do_ care about unlocking. So renaming is out. We could build a free_ref_lock that unlock_ref builds on, but that brings up another
confusion: can we or can we not free the "struct lockfile" pointer it contains (and which should free_ref_lock do)? Whether it is safe to do so depends on whether we have actually fed it to hold_lock_file_for_update or not (even if it fails, it takes ownership of the lock). So we actually leak it in some cases, but the only case we could fix is when safe_create_leading_directories the _first_ time (before we have ever tried to lock and failed, at which point we loop). Yeesh.

So I really think we are better off leaving it as-is, and just applying some form of Ronnie's patch (which does the right thing with errno). The other cleanups end up making things worse, and the unlock_ref thing was just my misunderstanding.

So here is that patch, with my explanation. Thanks for your patience in my running around in circles. :)

-- >8 --
Subject: lock_ref_sha1_basic: do not die on locking errors

lock_ref_sha1_basic is inconsistent about when it calls die() and when it returns NULL to signal an error. This is annoying to any callers that want to recover from a locking error.

This seems to be mostly historical accident. It was added in 4bd18c4 (Improve abstraction of ref lock/write., 2006-05-17), which returned an error in all cases except calling safe_create_leading_directories, in which case it died. Later, 40aaae8 (Better error message when we are unable to lock the index file, 2006-08-12) asked hold_lock_file_for_update to die for us, leaving the resolve_ref code-path the only one which returned NULL.

We tried to correct that in 5cc3cef (lock_ref_sha1(): do not sometimes error() and sometimes die()., 2006-09-30), by converting all of the die() calls into returns. But we missed the "die" flag passed to the lock code, leaving us inconsistent. This state persisted until e5c223e (lock_ref_sha1_basic(): if locking fails with ENOENT, retry, 2014-01-18). Because of its retry scheme, it does not ask the lock code to die, but instead manually dies with unable_to_lock_die().

We can make this consistent with the other return paths by converting this to use unable_to_lock_message(), and returning NULL. This is safe to do because all callers already needed to check the return value of the function, since it could fail (and return NULL) for other reasons.

[jk: Added excessive history explanation]

---
 refs.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 5ff457e..0347328 100644
@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
 	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
 	if (lock->lock_fd < 0) {
+		last_errno = errno;
 		if (errno == ENOENT && --attempts_remaining > 0)
 			/*
 			 * Maybe somebody just deleted one of the

@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 * again:
 			 */
 			goto retry;
-		else
-			unable_to_lock_die(ref_file, errno);
+		else {
+			struct strbuf err = STRBUF_INIT;
+			unable_to_lock_message(ref_file, errno, &err);
+			error("%s", err.buf);
+			strbuf_reset(&err);
+			goto error_return;
+		}
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 

2.1.2.596.g7379948

(modulo the author attribution nit you noticed)

My head has been spinning, but I think I can relax now ~@~ ;-)

Will queue; thanks.

I just noticed that when I pasted it into the mail, I dropped the

From: Ronnie Sahlberg <[email protected]>

header. Can you please make sure to credit Ronnie as you apply?

Thanks.

-Peff

Concurred.

From: Ronnie Sahlberg <[email protected]>

lock_ref_sha1_basic is inconsistent about when it calls die() and when it returns NULL to signal an error. This is annoying to any callers that want to recover from a locking error.

This seems to be mostly historical accident. It was added in 4bd18c4 (Improve abstraction of ref lock/write., 2006-05-17), which returned an error in all cases except calling safe_create_leading_directories, in which case it died. Later, 40aaae8 (Better error message when we are unable to lock the index file, 2006-08-12) asked hold_lock_file_for_update to die for us, leaving the resolve_ref code-path the only one which returned NULL.

We tried to correct that in 5cc3cef (lock_ref_sha1(): do not sometimes error() and sometimes die()., 2006-09-30), by converting all of the die() calls into returns. But we missed the "die" flag passed to the lock code, leaving us inconsistent. This state persisted until e5c223e (lock_ref_sha1_basic(): if locking fails with ENOENT, retry, 2014-01-18). Because of its retry scheme, it does not ask the lock code to die, but instead manually dies with unable_to_lock_die().

We can make this consistent with the other return paths by converting this to use unable_to_lock_message(), and returning NULL. This is safe to do because all callers already needed to check the return value of the function, since it could fail (and return NULL) for other reasons.

[jk: Added excessive history explanation and rebased on
nearby cleanups.]

---
It's a little sad to have to do the errno dance here after our earlier cleanups. But while error() is safe, unable_to_lock_message is anything but (the original patch from Ronnie did not need this, because it leaves the goto and last_errno in place). So I dunno. Maybe my cleanups were all for naught, and this end result is worse.

 refs.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index fafae7e..1f7e136 100644
@@ -2321,8 +2321,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 * again:
 			 */
 			goto retry;
-		else
-			unable_to_lock_die(ref_file, errno);
+		else {
+			int saved_errno = errno;
+			struct strbuf buf = STRBUF_INIT;
+			unable_to_lock_message(ref_file, errno, &buf);
+			error("%s", buf.buf);
+			strbuf_release(&buf);
+			errno = saved_errno;
+			return NULL;
+		}
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 }

2.1.2.596.g7379948

And in addition to the modern transaction stuff, rename_ref() has been such a caller for a long time.

Thanks for a pleasant explanation.

[...]

No need for my sign-off here --- it was just an artifact of my forwarding the patch to the list before.