Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error.

[Junio C Hamano <gitster@pobox.com>]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>  path.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/path.c b/path.c
> index 005b836..2886eb6 100644
> --- a/path.c
> +++ b/path.c
> @@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  	}
>  	/* We return the null string if we can't find a unique file name.  */
>  	pattern[0] = '\0';
> -	errno = EINVAL;
> +	/* Make sure errno signals an error on failure */
> +	if (errno <= 0)
> +		errno = EINVAL;
>  	return -1;
>  }

Please explain this change a bit better.

A successful call into system library does not have to clear errno (POSIX
even says "no function in ... POSIX ... shall set errno to 0"), and you do
not clear errno to zero anywhere in this function either, so it looks as
if you are reading an undefined errno and basing your action on that
result.

Because TMP_MAX is non-zero, you are always reading from errno left by
open() in the loop, so the above paragraph is a misunderstanding.  But
that needs to be in the log message, no?

I think you are trying to avoid stomping on the errno when we broke out of
the loop early, due to getting an error.  But errno is always valid at
this point in this codepath, and errno.h macros shall expand to integer
constant expressions with type int, distinct positive values.  So I think
you can safely remove the assignment without "if (errno <= 0)".  Returning
EINVAL from a variant of mkstemp when the error is anything but "The last
six characters were not XXXXXX" is wrong.