[2/2] New internal function __access_noerrno

Thanks for the review.

On 17/11/2016 18:54, Kalle Olavi Niemitalo wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> +/* __access variant that does not set errno.  Used in very early initialization
>> +   code in libc.a and ld.so.  */
>> +extern __typeof (__access) __access_noerrno attribute_hidden;
> 
> Please document what __access_noerrno is supposed to return on error.
> The NaCl and Linux implementations appear to return the error code,
> but the stub and Hurd implementations return -1 like access does,
> so I'm not sure what you intended.
> In "[PATCH 1/4] Add framework for tunables", the caller
> only checks whether the result is zero or nonzero.

My understanding of the tunables usage is it should returns what is
current described by access minus without setting errno, i.e, on
success zero is returned otherwise a value different than 0.

> 
>> +__hurd_fail_noerrno (error_t err)
> 
> Your __hurd_fail_noerrno just returns (err == 0 ? 0 : -1).
> All the assignments to err in the switch statement are dead.
> 
> If __access_noerrno is not required to return the error code,
> then I don't think __hurd_fail_noerrno is necessary at all...
> 
>> +static int
>> +hurd_fail_noerrno (error_t err)
>> +{
>> +  return __hurd_fail_noerrno (err);
>> +}
> 
> ... instead, hurd_fail_noerrno could just return -1
> unconditionally, because it is not even called if err == 0.
> (Perhaps then rename it, to avoid suggesting it's similar to __hurd_fail.)

Alright, I removed __hurd_fail_noerrno and made hurd_fail_noerrno
just return -1.

> 
>> -    return __hurd_fail (EACCES);
>> +    return errfunc (EACESS);
> 
> Typo here.
> 
> The t/faccessat branch at git://git.sv.gnu.org/hurd/glibc.git
> changes this code path quite a lot.  Until that branch is merged
> to upstream glibc, I think your function-pointer scheme is OK,
> apart from the comments above.
> I didn't yet try building it though.
> 

Below is all the fixes you proposed, if you think it is ok I will
prepare a patch and commit.

--

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> Below is all the fixes you proposed, if you think it is ok I will
> prepare a patch and commit.

Thanks.  It looks like an improvement.
I'd like to do some testing during the weekend.

However, I suspect that this __access_noerrno may be unsafe to run
during initialization of tunables on the Hurd.  access_common calls
__hurd_file_name_lookup, which calls __hurd_file_name_lookup_retry,
which can use errno, __strtoul_internal, _itoa, strcpy, and memcmp.
I'm not yet sure whether this is a problem, and I'm not asking you
to spend time on it.

On 17/11/2016 22:21, Kalle Olavi Niemitalo wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> Below is all the fixes you proposed, if you think it is ok I will
>> prepare a patch and commit.
> 
> Thanks.  It looks like an improvement.
> I'd like to do some testing during the weekend.

I pushed a patch based on the RFC I sent earlier.

> 
> However, I suspect that this __access_noerrno may be unsafe to run
> during initialization of tunables on the Hurd.  access_common calls
> __hurd_file_name_lookup, which calls __hurd_file_name_lookup_retry,
> which can use errno, __strtoul_internal, _itoa, strcpy, and memcmp.
> I'm not yet sure whether this is a problem, and I'm not asking you
> to spend time on it.
> 

I think it might be an issue depending of whether errno is accessible
on tunables initialization (it might be the case where is not yet
relocated/allocated).  Anyway I do not have a working system to 
actually even test a build on Hurd (the instruction at [1] points
to a non bootable VM).

If you could provide us with a working toolchain or a working VM
to actually check hurd builds it would be valuable.  Also it could
be case to add a hurd configuration to build-many-glibcs.py.

[1] https://www.gnu.org/software/hurd/hurd/running/qemu.html

On Saturday 19 November 2016 12:23 AM, Adhemerval Zanella wrote:
> I think it might be an issue depending of whether errno is accessible
> on tunables initialization (it might be the case where is not yet

That is correct.  Tunables initialization happens very early, earlier
than TLS setup and errno is a TLS variable.

Siddhesh