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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Junio C Hamano <gitster@pobox.com> writes:

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

I think we agree about the

-	errno = EINVAL;

part. Setting errno to EINVAL unconditionally means discarding the
errno left from open(), so we can't know the reason for failure
anymore with this line.

> 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?

Will add a sentence, yes.

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

Just removing the line would work with the current code, but this "if
(errno <= 0) errno = EINVAL;" allows enforcing the invariant that
errno > 0 when reaching "return -1" in a simple and reliable way (i.e.
changing the for loop later cannot break this invariant by mistake).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/