From: Junio C Hamano <gitster@pobox.com>
To: "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Han-Wen Nienhuys <hanwenn@gmail.com>,
Han-Wen Nienhuys <hanwen@google.com>
Subject: Re: [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn
Date: Fri, 30 Apr 2021 11:38:40 +0900 [thread overview]
Message-ID: <xmqqk0okebtb.fsf@gitster.g> (raw)
In-Reply-To: <7e8181e77d409af7595e357ad233b7781e026b78.1619710329.git.gitgitgadget@gmail.com> (Han-Wen Nienhuys via GitGitGadget's message of "Thu, 29 Apr 2021 15:32:01 +0000")
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> references.
We often use a pattern (which is common) like this:
if (some_func_in_ref_API(...) < 0) {
if (errno == ENOENT || errno == EISDIR)
... it is OK for the file to be missing ...
else
... error ...
}
If a piece of code currently sets EINVAL to errno manually when
signalling a failure by returning a negative value to communicate
with such a caller, we wouldn't see EINVAL mentioned, so such a grep
alone would not help us guarantee the correctness of an update to
stop assignment of EINVAL at all. The callers must be vetted more
carefully than "we are happy that nobody explicitly mentions EINVAL".
> The files ref backend does use EINVAL so parse_loose_ref_contents() can
> communicate to lock_raw_ref() about garbage following the hex SHA1, or a short
> read in files_read_raw_ref(), but the files backend does not call into
> refs_read_raw_ref(), so its EINVAL sideband error is unused.
This paragraph is confusing. It says EINVAL is used to signal
lock_raw_ref(), and it says EINVAL is not used by the same files
backend. Which is correct? If one part of the backend uses it, and
other parts don't, wouldn't the backend as a whole still use it?
Having said that, ...
> - * 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.
... the mention (and requirement) of EINVAL seems redundant, as it
sounds sufficient for the caller to inspect 'type' to see if it is
REF_ISBOKEN. So it may be OK for the code that gives REF_ISBROKEN
to type *not* to set errno to EINVAL, as long as it won't leave it
as ENOENT (meaning, an unrelated system call failed earlier may have
set errno to ENOENT, and after having dealt with such an error, the
control may have reached to the codepath we are interested in
here---errno must be cleared to some value other than ENOENT, and
assigning EINVAL is as good as any).
That is because there is a codeflow like this:
if (files_read_raw_ref(...)) {
if (errno == ENOENT) {
... do various things ...
} else if (errno == EISDIR) {
... do different and various things ...
} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
... deal with broken ref ...
}
...
}
where errno is looked at.
> + * 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, and return -1. If there is another error
> + * reading the ref, set errno appropriately and return -1.
So, this is not sufficient to let caller correctly and safely handle
errors. "set REF_ISBROKEN in type, set errno to something other
than ENOENT or EISDIR, and then return -1" is necessary, I would
think.
next prev parent reply other threads:[~2021-04-30 2:38 UTC|newest]
Thread overview: 148+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
2021-04-29 15:32 ` [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-04-30 2:38 ` Junio C Hamano [this message]
2021-05-19 12:25 ` Han-Wen Nienhuys
2021-06-03 2:19 ` Jonathan Tan
2021-06-09 11:28 ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-04-30 3:10 ` Junio C Hamano
2021-05-19 12:29 ` Han-Wen Nienhuys
2021-06-03 2:33 ` Jonathan Tan
2021-06-10 10:02 ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-04-30 3:34 ` Junio C Hamano
2021-04-30 6:02 ` Junio C Hamano
2021-05-19 12:33 ` Han-Wen Nienhuys
2021-06-03 2:37 ` Jonathan Tan
2021-06-10 10:05 ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-06-03 2:51 ` Jonathan Tan
2021-06-10 11:27 ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 5/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-04-29 15:32 ` [PATCH 6/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
2021-06-03 2:53 ` Jonathan Tan
2021-06-10 11:45 ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 7/8] refs: stop setting EINVAL and ELOOP in symref resolution Han-Wen Nienhuys via GitGitGadget
2021-06-03 2:55 ` Jonathan Tan
2021-06-10 11:58 ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
2021-06-03 2:13 ` [PATCH 0/8] refs: cleanup errno sideband ref related functions Jonathan Tan
2021-06-09 11:29 ` Han-Wen Nienhuys
2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57 ` [PATCH v2 1/8] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57 ` [PATCH v2 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-07-01 11:13 ` Ævar Arnfjörð Bjarmason
2021-07-05 14:16 ` Han-Wen Nienhuys
2021-06-10 12:57 ` [PATCH v2 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-01 11:34 ` Ævar Arnfjörð Bjarmason
2021-07-05 14:34 ` Han-Wen Nienhuys
2021-06-10 12:57 ` [PATCH v2 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-07-01 11:56 ` Ævar Arnfjörð Bjarmason
2021-06-10 12:57 ` [PATCH v2 5/8] refs: use refs_resolve_ref_unsafe_with_errno() where needed Han-Wen Nienhuys via GitGitGadget
2021-07-01 11:58 ` Ævar Arnfjörð Bjarmason
2021-06-10 12:57 ` [PATCH v2 6/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-07-01 12:06 ` Ævar Arnfjörð Bjarmason
2021-06-10 12:57 ` [PATCH v2 7/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
2021-07-01 12:19 ` Ævar Arnfjörð Bjarmason
2021-06-10 12:57 ` [PATCH v2 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
2021-07-01 12:26 ` Ævar Arnfjörð Bjarmason
2021-07-05 16:09 ` Han-Wen Nienhuys
2021-07-05 19:08 ` Ævar Arnfjörð Bjarmason
2021-07-05 19:39 ` Han-Wen Nienhuys
2021-06-14 10:10 ` [PATCH v2 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys
2021-07-05 20:56 ` [PATCH v3 0/5] " Han-Wen Nienhuys via GitGitGadget
2021-07-05 20:56 ` [PATCH v3 1/5] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-05 20:56 ` [PATCH v3 2/5] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-07-05 20:56 ` [PATCH v3 3/5] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-05 20:56 ` [PATCH v3 4/5] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-07-06 19:28 ` Junio C Hamano
2021-07-05 20:56 ` [PATCH v3 5/5] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-07-06 0:38 ` [PATCH v3 0/5] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-06 9:53 ` Han-Wen Nienhuys
2021-07-06 14:27 ` Ævar Arnfjörð Bjarmason
2021-07-06 18:36 ` Han-Wen Nienhuys
2021-07-06 18:55 ` [PATCH v4 0/6] " Han-Wen Nienhuys via GitGitGadget
2021-07-06 18:55 ` [PATCH v4 1/6] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-06 18:55 ` [PATCH v4 2/6] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-07-06 18:55 ` [PATCH v4 3/6] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-06 18:55 ` [PATCH v4 4/6] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-07-06 19:39 ` Junio C Hamano
2021-07-06 18:55 ` [PATCH v4 5/6] refs: explicitly return failure_errno from parse_loose_ref_contents Han-Wen Nienhuys via GitGitGadget
2021-07-06 19:37 ` Junio C Hamano
2021-07-07 8:20 ` Han-Wen Nienhuys
2021-07-06 18:55 ` [PATCH v4 6/6] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-07-07 19:07 ` [PATCH v5 0/6] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
2021-07-07 19:07 ` [PATCH v5 1/6] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-07 19:07 ` [PATCH v5 2/6] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-07-11 11:38 ` Ævar Arnfjörð Bjarmason
2021-07-13 8:00 ` Han-Wen Nienhuys
2021-07-07 19:07 ` [PATCH v5 3/6] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-11 11:52 ` Ævar Arnfjörð Bjarmason
2021-07-07 19:07 ` [PATCH v5 4/6] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-07-11 11:59 ` Ævar Arnfjörð Bjarmason
2021-07-13 8:02 ` Han-Wen Nienhuys
2021-07-07 19:07 ` [PATCH v5 5/6] refs: explicitly return failure_errno from parse_loose_ref_contents Han-Wen Nienhuys via GitGitGadget
2021-07-11 12:41 ` Ævar Arnfjörð Bjarmason
2021-07-07 19:07 ` [PATCH v5 6/6] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-07-07 20:44 ` [PATCH v5 0/6] refs: cleanup errno sideband ref related functions Junio C Hamano
2021-07-11 16:30 ` [PATCH v6? 00/17] refs API: get rid of errno setting entirely Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 01/17] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 02/17] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 03/17] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 04/17] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 05/17] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 06/17] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 07/17] refs: make errno ignoring explicit in lock_ref_oid_basic() Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 08/17] refs file-backend.c: stop setting "EBUSY" in verify_lock() Ævar Arnfjörð Bjarmason
2021-07-13 8:08 ` Han-Wen Nienhuys
2021-07-11 16:30 ` [PATCH v6? 09/17] refs file-backend.c: deal with errno directly " Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 10/17] refs API: remove refs_read_ref_full() wrapper Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 11/17] refs API: make resolve_gitlink_ref() not set errno Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 12/17] refs API: make refs_resolve_ref_unsafe() static Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 13/17] refs API: make refs_resolve_refdup() not set errno Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 14/17] refs API: make refs_ref_exists() " Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 15/17] refs API: make resolve_ref_unsafe() " Ævar Arnfjörð Bjarmason
2021-07-13 8:13 ` Han-Wen Nienhuys
2021-07-14 8:32 ` Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 16/17] refs API: make expand_ref() and repo_dwim_log() " Ævar Arnfjörð Bjarmason
2021-07-11 16:30 ` [PATCH v6? 17/17] refs API: don't leak "errno" in run_transaction_hook() Ævar Arnfjörð Bjarmason
2021-07-13 8:28 ` [PATCH v6? 00/17] refs API: get rid of errno setting entirely Han-Wen Nienhuys
2021-07-13 18:26 ` Ævar Arnfjörð Bjarmason
2021-07-14 8:38 ` Ævar Arnfjörð Bjarmason
2021-07-14 8:43 ` Han-Wen Nienhuys
2021-07-14 11:43 ` [PATCH v7 0/6] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-14 11:43 ` [PATCH v7 1/6] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-14 11:43 ` [PATCH v7 2/6] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-07-14 11:43 ` [PATCH v7 3/6] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-14 11:43 ` [PATCH v7 4/6] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-07-14 11:43 ` [PATCH v7 5/6] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-07-14 11:43 ` [PATCH v7 6/6] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason
2021-07-16 14:22 ` [PATCH v8 0/7] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-16 14:22 ` [PATCH v8 1/7] refs file backend: move raceproof_create_file() here Ævar Arnfjörð Bjarmason
2021-07-16 14:22 ` [PATCH v8 2/7] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-16 14:22 ` [PATCH v8 3/7] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-07-16 14:22 ` [PATCH v8 4/7] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-16 14:22 ` [PATCH v8 5/7] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-07-16 14:22 ` [PATCH v8 6/7] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-07-16 14:22 ` [PATCH v8 7/7] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason
2021-07-20 10:33 ` [PATCH v9 0/7] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-20 10:33 ` [PATCH v9 1/7] refs file backend: move raceproof_create_file() here Ævar Arnfjörð Bjarmason
2021-07-20 10:33 ` [PATCH v9 2/7] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-20 10:33 ` [PATCH v9 3/7] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-07-20 10:33 ` [PATCH v9 4/7] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-08-16 13:00 ` Han-Wen Nienhuys
2021-07-20 10:33 ` [PATCH v9 5/7] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-07-20 10:33 ` [PATCH v9 6/7] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-08-13 20:54 ` Jonathan Tan
2021-07-20 10:33 ` [PATCH v9 7/7] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason
2021-07-26 23:49 ` [PATCH v9 0/7] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-27 0:18 ` Junio C Hamano
2021-08-23 11:52 ` [PATCH v10 0/8] " Ævar Arnfjörð Bjarmason
2021-08-23 11:52 ` [PATCH v10 1/8] refs file backend: move raceproof_create_file() here Ævar Arnfjörð Bjarmason
2021-08-23 11:52 ` [PATCH v10 2/8] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-08-23 11:52 ` [PATCH v10 3/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-08-23 11:52 ` [PATCH v10 4/8] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-08-23 11:52 ` [PATCH v10 5/8] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-08-23 11:52 ` [PATCH v10 6/8] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
2021-08-23 11:52 ` [PATCH v10 7/8] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-08-23 11:52 ` [PATCH v10 8/8] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqk0okebtb.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=hanwen@google.com \
--cc=hanwenn@gmail.com \
/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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.