| Message ID | patch-v3-1.3-a45268ac24b-20220112T123117Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cac15b3fb422efb2cd1572cc654793d5df5fa434 |
| Headers | show |
| Series | For v2.35.0: refs: ab/refs-errno-cleanup fixup + remove "failure_errno" | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > @@ -1722,8 +1722,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, > if (refs_read_raw_ref(refs, refname, oid, &sb_refname, > &read_flags, failure_errno)) { > *flags |= read_flags; > - if (errno) > - *failure_errno = errno; Looks good. The whole point of passing failure_errno down to refs_read_raw_ref() is that we capture the reason of the failure there without having to rely on errno at this point in the flow. Removal of this assignment makes perfect sense. But ... > diff --git a/refs/files-backend.c b/refs/files-backend.c > index b529fdf237e..43a3b882d7c 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -382,7 +382,6 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, > if (lstat(path, &st) < 0) { > int ignore_errno; > myerr = errno; > - errno = 0; > if (myerr != ENOENT) > goto out; > if (refs_read_raw_ref(refs->packed_ref_store, refname, oid, > @@ -399,7 +398,6 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, > strbuf_reset(&sb_contents); > if (strbuf_readlink(&sb_contents, path, st.st_size) < 0) { > myerr = errno; > - errno = 0; > if (myerr == ENOENT || myerr == EINVAL) > /* inconsistent with lstat; retry */ > goto stat_ref; > @@ -469,6 +467,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, > > strbuf_release(&sb_path); > strbuf_release(&sb_contents); > + errno = 0; > return ret; > } ... it is not clear to me if this part makes sense. If everybody above the callstack uses failure_errno as the source of truth, clearing errno here in this function should not be necessary and is misleading to readers of the code, no?
On Wed, Jan 12 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> @@ -1722,8 +1722,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, >> if (refs_read_raw_ref(refs, refname, oid, &sb_refname, >> &read_flags, failure_errno)) { >> *flags |= read_flags; >> - if (errno) >> - *failure_errno = errno; > > Looks good. > > The whole point of passing failure_errno down to refs_read_raw_ref() > is that we capture the reason of the failure there without having to > rely on errno at this point in the flow. Removal of this assignment > makes perfect sense. > > But ... > >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index b529fdf237e..43a3b882d7c 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -382,7 +382,6 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, >> if (lstat(path, &st) < 0) { >> int ignore_errno; >> myerr = errno; >> - errno = 0; >> if (myerr != ENOENT) >> goto out; >> if (refs_read_raw_ref(refs->packed_ref_store, refname, oid, >> @@ -399,7 +398,6 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, >> strbuf_reset(&sb_contents); >> if (strbuf_readlink(&sb_contents, path, st.st_size) < 0) { >> myerr = errno; >> - errno = 0; >> if (myerr == ENOENT || myerr == EINVAL) >> /* inconsistent with lstat; retry */ >> goto stat_ref; >> @@ -469,6 +467,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, >> >> strbuf_release(&sb_path); >> strbuf_release(&sb_contents); >> + errno = 0; >> return ret; >> } > > ... it is not clear to me if this part makes sense. If everybody > above the callstack uses failure_errno as the source of truth, > clearing errno here in this function should not be necessary and is > misleading to readers of the code, no? It's consistent with various other existing refs* code as we made this transition, see: git grep -W 'errno = 0' -- 'refs*' I.e. we'd like to not only transition existing users away from errno, but to ensure that the file backend errno semantics don't inadvertently leak outside the API. Doing so is a bit pedantic for sure, but ensures that we won't need to deal with any subtle bugs in that are with the reftable backend.
diff --git a/refs.c b/refs.c index bd2546ae230..addb26293b4 100644 --- a/refs.c +++ b/refs.c @@ -1722,8 +1722,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, if (refs_read_raw_ref(refs, refname, oid, &sb_refname, &read_flags, failure_errno)) { *flags |= read_flags; - if (errno) - *failure_errno = errno; /* In reading mode, refs must eventually resolve */ if (resolve_flags & RESOLVE_REF_READING) diff --git a/refs/files-backend.c b/refs/files-backend.c index b529fdf237e..43a3b882d7c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -382,7 +382,6 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, if (lstat(path, &st) < 0) { int ignore_errno; myerr = errno; - errno = 0; if (myerr != ENOENT) goto out; if (refs_read_raw_ref(refs->packed_ref_store, refname, oid, @@ -399,7 +398,6 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, strbuf_reset(&sb_contents); if (strbuf_readlink(&sb_contents, path, st.st_size) < 0) { myerr = errno; - errno = 0; if (myerr == ENOENT || myerr == EINVAL) /* inconsistent with lstat; retry */ goto stat_ref; @@ -469,6 +467,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, strbuf_release(&sb_path); strbuf_release(&sb_contents); + errno = 0; return ret; }
Fix a logic error in refs_resolve_ref_unsafe() introduced in a recent series of mine to abstract the refs API away from errno. See 96f6623ada0 (Merge branch 'ab/refs-errno-cleanup', 2021-11-29)for that series. In that series introduction of "failure_errno" to refs_resolve_ref_unsafe came in ef18119dec8 (refs API: add a version of refs_resolve_ref_unsafe() with "errno", 2021-10-16). There we'd set "errno = 0" immediately before refs_read_raw_ref(), and then set "failure_errno" to "errno" if errno was non-zero afterwards. Then in the next commit 8b72fea7e91 (refs API: make refs_read_raw_ref() not set errno, 2021-10-16) we started expecting "refs_read_raw_ref()" to set "failure_errno". It would do that if refs_read_raw_ref() failed, but it wouldn't be the same errno. So we might set the "errno" here to any arbitrary bad value, and end up e.g. returning NULL when we meant to return the refname from refs_resolve_ref_unsafe(), or the other way around. Instrumenting this code will reveal cases where refs_read_raw_ref() will fail, and "errno" and "failure_errno" will be set to different values. In practice I haven't found a case where this scary bug changed anything in practice. The reason for that is that we'll not care about the actual value of "errno" here per-se, but only whether: 1. We have an errno 2. If it's one of ENOENT, EISDIR or ENOTDIR. See the adjacent code added in a1c1d8170db (refs_resolve_ref_unsafe: handle d/f conflicts for writes, 2017-10-06) I.e. if we clobber "failure_errno" with "errno", but it happened to be one of those three, and we'll clobber it with another one of the three we were OK. Perhaps there are cases where the difference ended up mattering, but I haven't found them. Instrumenting the test suite to fail if "errno" and "failure_errno" are different shows a lot of failures, checking if they're different *and* one is but not the other is outside that list of three "errno" values yields no failures. But let's fix the obvious bug. We should just stop paying attention to "errno" in refs_resolve_ref_unsafe(). In addition let's change the partial resetting of "errno" in files_read_raw_ref() to happen just before the "return", to ensure that any such bug will be more easily spotted in the future. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- refs.c | 2 -- refs/files-backend.c | 3 +-- 2 files changed, 1 insertion(+), 4 deletions(-)