Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refs: cleanup errno sideband ref related functions. #1011

Closed
wants to merge 3 commits into from
Closed

refs: cleanup errno sideband ref related functions. #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e8181e
refs: remove EINVAL specification from the errno sideband in read_raw…
hanwen Apr 23, 2021
db5da7d
refs/files-backend: stop setting errno from lock_ref_oid_basic
hanwen Apr 23, 2021
7fbc1c7
refs: make errno output explicit for read_raw_ref_fn
hanwen Apr 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions refs.c
View file
Open in desktop
Expand Up @@ -1675,13 +1675,16 @@ int refs_read_raw_ref(struct ref_store *ref_store,
const char *refname, struct object_id *oid,
struct strbuf *referent, unsigned int *type)
{
int result, failure;
if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
return refs_read_special_head(ref_store, refname, oid, referent,
type);
}

return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
type);
result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
type, &failure);
errno = failure;
return result;
}

/* This function needs to return a meaningful errno on failure */
Expand Down
4 changes: 2 additions & 2 deletions refs/debug.c
View file
Open in desktop
Expand Up @@ -238,14 +238,14 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,

static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
struct object_id *oid, struct strbuf *referent,
unsigned int *type)
unsigned int *type, int *failure_errno)
{
struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
int res = 0;

oidcpy(oid, &null_oid);
res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
type);
type, failure_errno);

if (res == 0) {
trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
Expand Down
26 changes: 9 additions & 17 deletions refs/files-backend.c
View file
Open in desktop
Expand Up @@ -343,7 +343,7 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)

static int files_read_raw_ref(struct ref_store *ref_store,
const char *refname, struct object_id *oid,
struct strbuf *referent, unsigned int *type)
struct strbuf *referent, unsigned int *type, int *failure_errno)
{
struct files_ref_store *refs =
files_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
Expand All @@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,
struct stat st;
int fd;
int ret = -1;
int save_errno;
int remaining_retries = 3;

*type = 0;
Expand Down Expand Up @@ -459,10 +458,9 @@ static int files_read_raw_ref(struct ref_store *ref_store,
ret = parse_loose_ref_contents(buf, oid, referent, type);

out:
save_errno = errno;
*failure_errno = errno;
strbuf_release(&sb_path);
strbuf_release(&sb_contents);
errno = save_errno;
return ret;
}

Expand Down Expand Up @@ -541,6 +539,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
struct strbuf ref_file = STRBUF_INIT;
int attempts_remaining = 3;
int ret = TRANSACTION_GENERIC_ERROR;
int failure_errno = 0;

assert(err);
files_assert_main_repository(refs, "lock_raw_ref");
Expand Down Expand Up @@ -630,8 +629,8 @@ static int lock_raw_ref(struct files_ref_store *refs,
*/

if (files_read_raw_ref(&refs->base, refname,
&lock->old_oid, referent, type)) {
if (errno == ENOENT) {
&lock->old_oid, referent, type, &failure_errno)) {
if (failure_errno == ENOENT) {
if (mustexist) {
/* Garden variety missing reference. */
strbuf_addf(err, "unable to resolve reference '%s'",
Expand All @@ -655,7 +654,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
* reference named "refs/foo/bar/baz".
*/
}
} else if (errno == EISDIR) {
} else if (failure_errno == EISDIR) {
/*
* There is a directory in the way. It might have
* contained references that have been deleted. If
Expand Down Expand Up @@ -693,13 +692,13 @@ static int lock_raw_ref(struct files_ref_store *refs,
goto error_return;
}
}
} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
} else if (failure_errno == EINVAL && (*type & REF_ISBROKEN)) {
strbuf_addf(err, "unable to resolve reference '%s': "
"reference broken", refname);
goto error_return;
} else {
strbuf_addf(err, "unable to resolve reference '%s': %s",
refname, strerror(errno));
refname, strerror(failure_errno));
goto error_return;
}

Expand Down Expand Up @@ -910,7 +909,6 @@ static int create_reflock(const char *path, void *cb)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

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 - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

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?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

/*
* 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,
Expand All @@ -922,7 +920,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;
Expand All @@ -949,7 +946,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))
Expand All @@ -962,7 +958,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))
Expand All @@ -981,20 +977,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;
Expand All @@ -1005,7 +998,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,

out:
strbuf_release(&ref_file);
errno = last_errno;
return lock;
}

Expand Down
5 changes: 3 additions & 2 deletions refs/packed-backend.c
View file
Open in desktop
Expand Up @@ -726,7 +726,8 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)

static int packed_read_raw_ref(struct ref_store *ref_store,
const char *refname, struct object_id *oid,
struct strbuf *referent, unsigned int *type)
struct strbuf *referent, unsigned int *type,
int *failure_errno)
{
struct packed_ref_store *refs =
packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
Expand All @@ -739,7 +740,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,

if (!rec) {
/* refname is not a packed reference. */
errno = ENOENT;
*failure_errno = ENOENT;
return -1;
}

Expand Down
14 changes: 8 additions & 6 deletions refs/refs-internal.h
View file
Open in desktop
Expand Up @@ -617,11 +617,12 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
* properly-formatted or even safe reference name. NEITHER INPUT NOR
* OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.
*
* Return 0 on success. If the ref doesn't exist, set errno to ENOENT
* and return -1. If the ref exists but is neither a symbolic ref nor
* an object ID, it is broken; set REF_ISBROKEN in type, set errno to
* EINVAL, and return -1. If there is another error reading the ref,
* set errno appropriately and return -1.
* Return 0 on success. If the ref doesn't exist, set failure_errno to ENOENT and return
* -1. If the ref exists but is neither a symbolic ref nor an object ID, it is
* broken; set REF_ISBROKEN in type, and return -1. For the files backend, EISDIR and ENOTDIR
* may be set if the ref name is a directory
* If there is another error
* reading the ref, set errno appropriately and return -1.
*
* Backend-specific flags might be set in type as well, regardless of
* outcome.
Expand All @@ -637,7 +638,8 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
*/
typedef int read_raw_ref_fn(struct ref_store *ref_store,
const char *refname, struct object_id *oid,
struct strbuf *referent, unsigned int *type);
struct strbuf *referent, unsigned int *type,
int *failure_errno);

struct ref_storage_be {
struct ref_storage_be *next;
Expand Down