Message ID | db5da7d7fb5178c14c1f5733d35bb69813c9c644.1619191907.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | refs: cleanup errno sideband ref related functions. | expand |
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Han-Wen Nienhuys <hanwen@google.com> > > Errno is a global variable written by almost all system calls, and therefore it > is hard to reason about its state. > > This is a functional noop, because calls to lock_ref_oid_basic() in this file > are followed by: > > * lock_ref_oid_basic (copy/rename rollback error path) > > * write_ref_to_lockfile (both in the rollback path and the success path of > copy/rename) > > * create_symref_locked (files_create_symref) > > * refs_reflog_exists (reflog expiry) > > These calls do I/O and therefore clobber errno. They are not inspecting the > incoming errno. Hmph, are you saying that these calls do I/O and always the I/O would fail? A system call that is successfull don't touch errno; only the calls that resulted in failure do.
On Wed, Apr 28, 2021 at 6:20 AM Junio C Hamano <gitster@pobox.com> wrote: > > These calls do I/O and therefore clobber errno. They are not inspecting the > > incoming errno. > > Hmph, are you saying that these calls do I/O and always the I/O > would fail? A system call that is successfull don't touch errno; > only the calls that resulted in failure do. I'm saying that callers cannot reliably observe the errno result of lock_ref_oid_basic, because it might be clobbered by a failing follow-up call.
Han-Wen Nienhuys <hanwen@google.com> writes: > On Wed, Apr 28, 2021 at 6:20 AM Junio C Hamano <gitster@pobox.com> wrote: >> > These calls do I/O and therefore clobber errno. They are not inspecting the >> > incoming errno. >> >> Hmph, are you saying that these calls do I/O and always the I/O >> would fail? A system call that is successfull don't touch errno; >> only the calls that resulted in failure do. > > I'm saying that callers cannot reliably observe the errno result of > lock_ref_oid_basic, because it might be clobbered by a failing > follow-up call. Sorry, I still do not quite get it. For example, you cite that a call to lock_ref_oid_basic() in files_create_symref() is followed by create_symref_locked() that may clobber errno when the latter fails. But a failing lock_ref_oid_basic() would yield NULL and causes the caller to leave, before calling create_symref_locked() and letting it clobber errno, and the caller of files_create_symref() can observe, when it returns -1 to signal an error, the errno left by lock_ref_oid_basic(), no? I would understand it if no caller of files_create_symref() cares what is in errno when it receives negative return to signal a failure, though. And when lock_ref_oid_basic() did not fail, create_symref_locked() calls helpers that can fail (e.g. fdopen_lock_file()) and result in errno getting updated to record how it failed (this is also reported to the user via "error(... strerror(errno))"). So a caller of files_create_symref() may not be able to tell between lock_ref_oid_basic() and create_symref_locked() which one caused the files_create_symref() call to fail, but in either case it should be able to inspect errno to learn what kind of error we got from the underlying system, no?
On Thu, Apr 29, 2021 at 3:55 AM Junio C Hamano <gitster@pobox.com> wrote: > > Han-Wen Nienhuys <hanwen@google.com> writes: > > > On Wed, Apr 28, 2021 at 6:20 AM Junio C Hamano <gitster@pobox.com> wrote: > >> > These calls do I/O and therefore clobber errno. They are not inspecting the > >> > incoming errno. > >> > >> Hmph, are you saying that these calls do I/O and always the I/O > >> would fail? A system call that is successfull don't touch errno; > >> only the calls that resulted in failure do. > > > > I'm saying that callers cannot reliably observe the errno result of > > lock_ref_oid_basic, because it might be clobbered by a failing > > follow-up call. > > Sorry, I still do not quite get it. For example, you cite that a > call to lock_ref_oid_basic() in files_create_symref() is followed by > create_symref_locked() that may clobber errno when the latter fails. > > But a failing lock_ref_oid_basic() would yield NULL and causes the > caller to leave, before calling create_symref_locked() and letting > it clobber errno, and the caller of files_create_symref() can > observe, when it returns -1 to signal an error, the errno left by > lock_ref_oid_basic(), no? I would understand it if no caller of > files_create_symref() cares what is in errno when it receives > negative return to signal a failure, though. You're right; I didn't look carefully enough. I did a grep over the source code for create_symref() now, and couldn't find callers that inspect errno; the same for reflog_expire(). I'll update the commit message to reflect this. > And when lock_ref_oid_basic() did not fail, create_symref_locked() > calls helpers that can fail (e.g. fdopen_lock_file()) and result in > errno getting updated to record how it failed (this is also reported > to the user via "error(... strerror(errno))"). > > So a caller of files_create_symref() may not be able to tell between > lock_ref_oid_basic() and create_symref_locked() which one caused the > files_create_symref() call to fail, but in either case it should be > able to inspect errno to learn what kind of error we got from the > underlying system, no? I disagree. create_symref in the refs API gets an error strbuf_t. If the function wants to say something to the user, it should use that mechanism. If other operations are meant to provide reasonable error messages, they should also get an error strbuf. The files backend touches many files as part of its operation. If the error is something like EPERM, errno reporting leaves no channel to describe which file and which syscall is the offending one (is it packed-refs.lock, refs/heads/branch.lock, refs/heads/ ; is it the creat/rename/unlink syscall?). It's not a realistic mechanism to use for errors that are meant to be understandable for users. The errno mechanism is also poorly adjusted for alternate backends. If there is corrupted data in a reftable file, the library returns REFTABLE_FORMAT_ERROR, but what errno would correspond to that?
diff --git a/refs/files-backend.c b/refs/files-backend.c index 119972ee16f8..c9511da1d387 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -910,7 +910,6 @@ static int create_reflock(const char *path, void *cb) /* * Locks a ref returning the lock on success and NULL on failure. - * On failure errno is set to something meaningful. */ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, const char *refname, @@ -922,7 +921,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, { struct strbuf ref_file = STRBUF_INIT; struct ref_lock *lock; - int last_errno = 0; int mustexist = (old_oid && !is_null_oid(old_oid)); int resolve_flags = RESOLVE_REF_NO_RECURSE; int resolved; @@ -949,7 +947,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, * to remain. */ if (remove_empty_directories(&ref_file)) { - last_errno = errno; if (!refs_verify_refname_available( &refs->base, refname, extras, skip, err)) @@ -962,7 +959,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, &lock->old_oid, type); } if (!resolved) { - last_errno = errno; + int last_errno = errno; if (last_errno != ENOTDIR || !refs_verify_refname_available(&refs->base, refname, extras, skip, err)) @@ -981,20 +978,17 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, if (is_null_oid(&lock->old_oid) && refs_verify_refname_available(refs->packed_ref_store, refname, extras, skip, err)) { - last_errno = ENOTDIR; goto error_return; } lock->ref_name = xstrdup(refname); 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; } if (verify_lock(&refs->base, lock, old_oid, mustexist, err)) { - last_errno = errno; goto error_return; } goto out; @@ -1005,7 +999,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, out: strbuf_release(&ref_file); - errno = last_errno; return lock; }