Re: [PATCH 2/2] New internal function __access_noerrno

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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.

--

diff --git a/hurd/hurd.h b/hurd/hurd.h
index c089d4c..ec07827 100644
--- a/hurd/hurd.h
+++ b/hurd/hurd.h
@@ -75,35 +75,6 @@ __hurd_fail (error_t err)
   errno = err;
   return -1;
 }
-
-_HURD_H_EXTERN_INLINE int
-__hurd_fail_noerrno (error_t err)
-{
-  switch (err)
-    {
-    case EMACH_SEND_INVALID_DEST:
-    case EMIG_SERVER_DIED:
-      /* The server has disappeared!  */
-      err = EIEIO;
-      break;
-
-    case KERN_NO_SPACE:
-      err = ENOMEM;
-      break;
-
-    case KERN_INVALID_ARGUMENT:
-      err = EINVAL;
-      break;
-
-    case 0:
-      return 0;
-
-    default:
-      break;
-    }
-
-  return -1;
-}
 
 /* Basic ports and info, initialized by startup.  */
 
diff --git a/include/unistd.h b/include/unistd.h
index 6144f41..16d68a1 100644
--- a/include/unistd.h
+++ b/include/unistd.h
@@ -183,7 +183,8 @@ extern int __getlogin_r_loginuid (char *name, size_t namesize)
 
 #  if IS_IN (rtld) || !defined SHARED
 /* __access variant that does not set errno.  Used in very early initialization
-   code in libc.a and ld.so.  */
+   code in libc.a and ld.so.  It follows access return semantics (zero for
+   sucess otherwise a value different than 0).  */
 extern __typeof (__access) __access_noerrno attribute_hidden;
 #  endif
 
diff --git a/sysdeps/mach/hurd/access.c b/sysdeps/mach/hurd/access.c
index 93e2166..66bf7d5 100644
--- a/sysdeps/mach/hurd/access.c
+++ b/sysdeps/mach/hurd/access.c
@@ -31,7 +31,7 @@ hurd_fail_seterrno (error_t err)
 static int
 hurd_fail_noerrno (error_t err)
 {
-  return __hurd_fail_noerrno (err);
+  return -1;
 }
 
 static int
@@ -149,7 +149,7 @@ access_common (const char *file, int type, int (*errfunc) (error_t))
 
   if (flags & ~allowed)
     /* We are not allowed all the requested types of access.  */
-    return errfunc (EACESS);
+    return errfunc (EACCES);
 
   return 0;
 }