Re: [PATCH] clean: use warning_errno() when appropriate
Jeff King writes: > So in that sense doing the errno dance as close to the caller who cares > is the most _obvious_ thing, even if it isn't the shortest. Yup. > It would be nice if there was a way to annotate a function as > errno-safe, and then transitively compute which other functions were > errno-safe when they do not call any errno-unsafe function. I don't know > if any static analyzers allow that kind of custom annotation, though > (and also I wonder whether the cost/benefit of maintaining those > annotations would be worth it). Double yup.
Re: [PATCH] clean: use warning_errno() when appropriate
On Mon, Feb 13, 2017 at 01:53:33PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > IOW, I think this may be a case where we should be optimizing for > > programmer time (fewer lines of code, and one less thing to worry about > > in the callers) versus squeezing out every instruction. > > Fair enough. > > Unless we do the save_errno dance in all the helper functions we > commonly use to safely stash away errno as necessary and tell > developers that they can depend on it, the code in the patch that > began this discussion still needs its own saved_errno dance to be > safe, though. I do not have a feeling that we are not there yet, > even after we teach xmalloc() and its family to do so. Yeah, I certainly agree that is a potential blocker. Even if it is true today, there's nothing guaranteeing that the quote functions don't grow a new internal detail that violates. So in that sense doing the errno dance as close to the caller who cares is the most _obvious_ thing, even if it isn't the shortest. It would be nice if there was a way to annotate a function as errno-safe, and then transitively compute which other functions were errno-safe when they do not call any errno-unsafe function. I don't know if any static analyzers allow that kind of custom annotation, though (and also I wonder whether the cost/benefit of maintaining those annotations would be worth it). -Peff
Re: [PATCH] clean: use warning_errno() when appropriate
Jeff King writes: > IOW, I think this may be a case where we should be optimizing for > programmer time (fewer lines of code, and one less thing to worry about > in the callers) versus squeezing out every instruction. Fair enough. Unless we do the save_errno dance in all the helper functions we commonly use to safely stash away errno as necessary and tell developers that they can depend on it, the code in the patch that began this discussion still needs its own saved_errno dance to be safe, though. I do not have a feeling that we are not there yet, even after we teach xmalloc() and its family to do so.
Re: [PATCH] clean: use warning_errno() when appropriate
On Mon, Feb 13, 2017 at 12:38:47PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > I wonder if xmalloc() should be the one doing the saved_errno trick. > > After all, it has only two outcomes: we successfully allocated the > > memory, or we called die(). > > I would be lying if I said I did not considered it when I wrote the > message you are responding to, but I rejected it because that would > be optimizing for a wrong case, in that most callers of xmalloc() > and friends do not do so in the error codepath, and we would be > penalizing them by doing the saved_errno dance unconditionally. Yes, that also occurred to me. I'm not sure if two integer swaps is enough to care about when compared to the cost of a malloc(), though. IOW, I think this may be a case where we should be optimizing for programmer time (fewer lines of code, and one less thing to worry about in the callers) versus squeezing out every instruction. -Peff
Re: [PATCH] clean: use warning_errno() when appropriate
Jeff King writes: > I wonder if xmalloc() should be the one doing the saved_errno trick. > After all, it has only two outcomes: we successfully allocated the > memory, or we called die(). I would be lying if I said I did not considered it when I wrote the message you are responding to, but I rejected it because that would be optimizing for a wrong case, in that most callers of xmalloc() and friends do not do so in the error codepath, and we would be penalizing them by doing the saved_errno dance unconditionally.
Re: [PATCH] clean: use warning_errno() when appropriate
On Mon, Feb 13, 2017 at 10:34:06AM -0800, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > > > All these warning() calls are preceded by a system call. Report the > > actual error to help the user understand why we fail to remove > > something. > > I think this patch is probably correct in the current code, but I > say this only after following what quote_path_relative() and > relative_path() that is called from it. These warnings are preceded > by a call to a system library function, but it is not apparent that > they are immediately preceded without anything else that could have > failed in between. > > Side note. There are many calls into strbuf API in these two > functions. Any calls to xmalloc() and friends made by strbuf > functions may see ENOMEM from underlying malloc() and recover by > releasing cached resources, by which time the original errno is > unrecoverable. So the above "probably correct" is not strictly > true. > > If we care deeply enough that we want to reliably show the errno we > got from the preceding call to a system library function even after > whatever comes in between, I think you'd need the usual saved_errno > trick. Is that worth it?---I do not offhand have an opinion. I wonder if xmalloc() should be the one doing the saved_errno trick. After all, it has only two outcomes: we successfully allocated the memory, or we called die(). And that would transitively make most of the strbuf calls errno-safe (except for obvious syscall-related ones like strbuf_read_file). And in turn that makes quote_path_relative() pretty safe (at least when writing to a strbuf). -Peff
Re: [PATCH] clean: use warning_errno() when appropriate
Nguyễn Thái Ngọc Duy writes:
> All these warning() calls are preceded by a system call. Report the
> actual error to help the user understand why we fail to remove
> something.
I think this patch is probably correct in the current code, but I
say this only after following what quote_path_relative() and
relative_path() that is called from it. These warnings are preceded
by a call to a system library function, but it is not apparent that
they are immediately preceded without anything else that could have
failed in between.
Side note. There are many calls into strbuf API in these two
functions. Any calls to xmalloc() and friends made by strbuf
functions may see ENOMEM from underlying malloc() and recover by
releasing cached resources, by which time the original errno is
unrecoverable. So the above "probably correct" is not strictly
true.
If we care deeply enough that we want to reliably show the errno we
got from the preceding call to a system library function even after
whatever comes in between, I think you'd need the usual saved_errno
trick. Is that worth it?---I do not offhand have an opinion.
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/clean.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d6bc3aaaea..dc1168747e 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -175,7 +175,7 @@ static int remove_dirs(struct strbuf *path, const char
> *prefix, int force_flag,
> res = dry_run ? 0 : rmdir(path->buf);
> if (res) {
> quote_path_relative(path->buf, prefix, "ed);
> - warning(_(msg_warn_remove_failed), quoted.buf);
> + warning_errno(_(msg_warn_remove_failed), quoted.buf);
> *dir_gone = 0;
> }
> return res;
> @@ -209,7 +209,7 @@ static int remove_dirs(struct strbuf *path, const char
> *prefix, int force_flag,
> string_list_append(&dels, quoted.buf);
> } else {
> quote_path_relative(path->buf, prefix, "ed);
> - warning(_(msg_warn_remove_failed), quoted.buf);
> + warning_errno(_(msg_warn_remove_failed),
> quoted.buf);
> *dir_gone = 0;
> ret = 1;
> }
> @@ -231,7 +231,7 @@ static int remove_dirs(struct strbuf *path, const char
> *prefix, int force_flag,
> *dir_gone = 1;
> else {
> quote_path_relative(path->buf, prefix, "ed);
> - warning(_(msg_warn_remove_failed), quoted.buf);
> + warning_errno(_(msg_warn_remove_failed), quoted.buf);
> *dir_gone = 0;
> ret = 1;
> }
> @@ -982,7 +982,7 @@ int cmd_clean(int argc, const char **argv, const char
> *prefix)
> res = dry_run ? 0 : unlink(abs_path.buf);
> if (res) {
> qname = quote_path_relative(item->string, NULL,
> &buf);
> - warning(_(msg_warn_remove_failed), qname);
> + warning_errno(_(msg_warn_remove_failed), qname);
> errors++;
> } else if (!quiet) {
> qname = quote_path_relative(item->string, NULL,
> &buf);
