[PATCH] clean: use warning_errno() when appropriate

[PATCH] clean: use warning_errno() when appropriate

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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, &quoted);
-			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, &quoted);
-				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, &quoted);
-			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);
-- 
2.11.0.157.gd943d85

Re: [PATCH] clean: use warning_errno() when appropriate

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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 <pclouds@gmail.com>
> ---
>  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, &quoted);
> -			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, &quoted);
> -				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, &quoted);
> -			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);

Re: [PATCH] clean: use warning_errno() when appropriate

[Jeff King]

On Mon, Feb 13, 2017 at 10:34:06AM -0800, Junio C Hamano wrote:

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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

[Junio C Hamano]

Jeff King <peff@peff.net> 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

[Jeff King]

On Mon, Feb 13, 2017 at 12:38:47PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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

[Junio C Hamano]

Jeff King <peff@peff.net> 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

[Jeff King]

On Mon, Feb 13, 2017 at 01:53:33PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

[PATCH v2] clean: use warning_errno() when appropriate

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v2 dances with errno

 builtin/clean.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index d6bc3aaae..3569736f6 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	struct strbuf quoted = STRBUF_INIT;
 	struct dirent *e;
 	int res = 0, ret = 0, gone = 1, original_len = path->len, len;
+	int saved_errno;
 	struct string_list dels = STRING_LIST_INIT_DUP;
 
 	*dir_gone = 1;
@@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	if (!dir) {
 		/* an empty dir could be removed even if it is unreadble */
 		res = dry_run ? 0 : rmdir(path->buf);
+		saved_errno = errno;
 		if (res) {
 			quote_path_relative(path->buf, prefix, &quoted);
-			warning(_(msg_warn_remove_failed), quoted.buf);
+			errno = saved_errno;
+			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
 		}
 		return res;
@@ -204,12 +207,14 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			continue;
 		} else {
 			res = dry_run ? 0 : unlink(path->buf);
+			saved_errno = errno;
 			if (!res) {
 				quote_path_relative(path->buf, prefix, &quoted);
 				string_list_append(&dels, quoted.buf);
 			} else {
 				quote_path_relative(path->buf, prefix, &quoted);
-				warning(_(msg_warn_remove_failed), quoted.buf);
+				errno = saved_errno;
+				warning_errno(_(msg_warn_remove_failed), quoted.buf);
 				*dir_gone = 0;
 				ret = 1;
 			}
@@ -227,11 +232,13 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 
 	if (*dir_gone) {
 		res = dry_run ? 0 : rmdir(path->buf);
+		saved_errno = errno;
 		if (!res)
 			*dir_gone = 1;
 		else {
 			quote_path_relative(path->buf, prefix, &quoted);
-			warning(_(msg_warn_remove_failed), quoted.buf);
+			errno = saved_errno;
+			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
 			ret = 1;
 		}
@@ -853,7 +860,7 @@ static void interactive_main_loop(void)
 
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
-	int i, res;
+	int i, res, saved_errno;
 	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
 	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
 	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
@@ -980,9 +987,11 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			}
 		} else {
 			res = dry_run ? 0 : unlink(abs_path.buf);
+			saved_errno = errno;
 			if (res) {
 				qname = quote_path_relative(item->string, NULL, &buf);
-				warning(_(msg_warn_remove_failed), qname);
+				errno = saved_errno;
+				warning_errno(_(msg_warn_remove_failed), qname);
 				errors++;
 			} else if (!quiet) {
 				qname = quote_path_relative(item->string, NULL, &buf);
-- 
2.11.0.157.gd943d85

Re: [PATCH v2] clean: use warning_errno() when appropriate

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  v2 dances with errno

Thanks.

>
>  builtin/clean.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d6bc3aaae..3569736f6 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>  	struct strbuf quoted = STRBUF_INIT;
>  	struct dirent *e;
>  	int res = 0, ret = 0, gone = 1, original_len = path->len, len;
> +	int saved_errno;
>  	struct string_list dels = STRING_LIST_INIT_DUP;
>  
>  	*dir_gone = 1;
> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>  	if (!dir) {
>  		/* an empty dir could be removed even if it is unreadble */
>  		res = dry_run ? 0 : rmdir(path->buf);
> +		saved_errno = errno;
>  		if (res) {
>  			quote_path_relative(path->buf, prefix, &quoted);

I think this part should be more like

		res = ... : rmdir(...);
		if (res) {
			int saved_errno = errno;
			... do other things that can touch errno ...
			errno = saved_errno;
			... now we know what the original error was ...

The reason to store the errno in saved_errno here is not because we
want to help code after "if (res) {...}", but the patch sent as-is
gives that impression and is confusing to the readers.  

Perhaps all hunks of this patch share the same issue?  I could
locally amend, of course, but I'd like to double check before doing
so myself---perhaps you did it this way for a good reason that I am
missing?

Re: [PATCH v2] clean: use warning_errno() when appropriate

[Duy Nguyen]

On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  v2 dances with errno
>
> Thanks.
>
>>
>>  builtin/clean.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index d6bc3aaae..3569736f6 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>>       struct strbuf quoted = STRBUF_INIT;
>>       struct dirent *e;
>>       int res = 0, ret = 0, gone = 1, original_len = path->len, len;
>> +     int saved_errno;
>>       struct string_list dels = STRING_LIST_INIT_DUP;
>>
>>       *dir_gone = 1;
>> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>>       if (!dir) {
>>               /* an empty dir could be removed even if it is unreadble */
>>               res = dry_run ? 0 : rmdir(path->buf);
>> +             saved_errno = errno;
>>               if (res) {
>>                       quote_path_relative(path->buf, prefix, &quoted);
>
> I think this part should be more like
>
>                 res = ... : rmdir(...);
>                 if (res) {
>                         int saved_errno = errno;
>                         ... do other things that can touch errno ...
>                         errno = saved_errno;
>                         ... now we know what the original error was ...
>
> The reason to store the errno in saved_errno here is not because we
> want to help code after "if (res) {...}", but the patch sent as-is
> gives that impression and is confusing to the readers.
>
> Perhaps all hunks of this patch share the same issue?  I could
> locally amend, of course, but I'd like to double check before doing
> so myself---perhaps you did it this way for a good reason that I am
> missing?

One thing I like about putting saved_errno right next to the related
syscall is, the syscall is visible from the diff (previously some are
out of context). This is really minor though. I briefly thought of
introducing rmdir_errno() and friends that return -errno on error, so
we could do

res = ... : rmdir_errno(..);
if (res) {
    errno = -res;
    warning_errno(...);
}

But that's more work and the errno = -res is not particularly pleasing
to read. I'm fine with moving saved_errno in the error handling
blocks.
-- 
Duy

Re: [PATCH v2] clean: use warning_errno() when appropriate

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> The reason to store the errno in saved_errno here is not because we
>> want to help code after "if (res) {...}", but the patch sent as-is
>> gives that impression and is confusing to the readers.
>>
>> Perhaps all hunks of this patch share the same issue?  I could
>> locally amend, of course, but I'd like to double check before doing
>> so myself---perhaps you did it this way for a good reason that I am
>> missing?
>
> One thing I like about putting saved_errno right next to the related
> syscall is, the syscall is visible from the diff (previously some are
> out of context). This is really minor though.

I agree that this is minor.

I care more about looking at errno ONLY after we saw our call to a
system library function indicated an error, and I wanted to avoid
doing:

	res = dry_run ? 0 : rmdir(path);
        saved_errno = errno;
	if (res) {
		... do something else ...
		errno = saved_errno;
                call_something_that_uses_errno();

When our call to rmdir() did not fail, or when we didn't even call
rmdir() at all, what is in errno has nothing to do with what we are
doing, and making a copy of it makes the code doubly confusing.

Rather, I'd prefer to see:

	res = dry_run ? 0 : rmdir(path);
	if (res) {
                int saved_errno = errno;
		... do something else ...
		errno = saved_errno;
                call_something_that_uses_errno();

which makes it clear what is going on when reading the resulting
code.

For now, I'll queue a separate SQUASH??? and wait for others to
comment.

Thanks.

Re: [PATCH v2] clean: use warning_errno() when appropriate

[Jeff King]

On Tue, Feb 14, 2017 at 05:28:36PM -0800, Junio C Hamano wrote:

> I care more about looking at errno ONLY after we saw our call to a
> system library function indicated an error, and I wanted to avoid
> doing:
> 
> 	res = dry_run ? 0 : rmdir(path);
>         saved_errno = errno;
> 	if (res) {
> 		... do something else ...
> 		errno = saved_errno;
>                 call_something_that_uses_errno();
> 
> When our call to rmdir() did not fail, or when we didn't even call
> rmdir() at all, what is in errno has nothing to do with what we are
> doing, and making a copy of it makes the code doubly confusing.
> 
> Rather, I'd prefer to see:
> 
> 	res = dry_run ? 0 : rmdir(path);
> 	if (res) {
>                 int saved_errno = errno;
> 		... do something else ...
> 		errno = saved_errno;
>                 call_something_that_uses_errno();
> 
> which makes it clear what is going on when reading the resulting
> code.
> 
> For now, I'll queue a separate SQUASH??? and wait for others to
> comment.

I don't have a strong feeling either way, but I think your second
example there is probably preferable. The reason to save errno is
because of the "something else" that may affect it, and it puts the
saving close to that.

Duy's version above keeps the saved_errno close to the syscall that
caused it, which is nicer for making sure we're saving the right thing,
and didn't get fooled by:

  res = rmdir(path);
  ... some other stuff ...
  if (res) {
          int saved_errno = errno;
	  ... something else ...
	  errno = saved_errno;

That's wrong if "some other stuff" touches errno. But I think
"saved_errno" is not the right pattern there. It is "stuff away the
result _and_ errno for this thing so we can use it later".

IOW, I'd expect it to be more like:

  rmdir_result = rmdir(path);
  rmdir_errno = errno;
  ... some other stuff ...
  if (rmdir_result)
      show_error(rmdir_errno);

And that leads to the "gee, why don't we just encode error values as
negative integers" pattern. Which I agree is nicer, but I'm not sure I
want to get into wrapping every syscall to give it a better interface.

-Peff