[PATCH 1/3] linux: Use INTERNAL_SYSCALL on fstatat{64}

[PATCH 1/3] linux: Use INTERNAL_SYSCALL on fstatat{64}

[Adhemerval Zanella via Libc-alpha]

Although not required by the standards, some code expects that a
successful stat call should not set errno.  However since aa03f722f3b99
'linux: Add {f}stat{at} y2038 support', stat implementation will first
try a __NR_statx call and if it fails with ENOSYS then issue the
required stat syscall.

On 32-bit architecture running on kernel without __NR_statx support the
first call will set the errno to ENOSYS, even when the following stat
syscall does not fail.

This patch fixes by using INTERNAL_SYSCALL and only setting the errno
value when function returns.

Checked on i686-linux-gnu, x86_64-linux-gnu, sparc64-linux-gnu,
sparcv9-linux-gnu, powerpc64-linux-gnu, powerpc64le-linux-gnu,
arm-linux-gnueabihf, and aarch64-linux-gnu.
---
 sysdeps/unix/sysv/linux/fstatat.c             | 28 ++++++++++------
 sysdeps/unix/sysv/linux/fstatat64.c           | 33 ++++++++++---------
 .../unix/sysv/linux/mips/mips64/kstat_cp.h    |  8 ++---
 .../unix/sysv/linux/sparc/sparc64/kstat_cp.h  |  4 +--
 4 files changed, 40 insertions(+), 33 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fstatat.c b/sysdeps/unix/sysv/linux/fstatat.c
index c7fcfaf277..78fad51961 100644
--- a/sysdeps/unix/sysv/linux/fstatat.c
+++ b/sysdeps/unix/sysv/linux/fstatat.c
@@ -26,22 +26,24 @@
 int
 __fstatat (int fd, const char *file, struct stat *buf, int flag)
 {
+  int r;
+
 # if STAT_IS_KERNEL_STAT
   /* New kABIs which uses generic pre 64-bit time Linux ABI, e.g.
      csky, nios2  */
-  int r = INLINE_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
-  if (r == 0 && (buf->__st_ino_pad != 0
-		 || buf->__st_size_pad != 0
-		 || buf->__st_blocks_pad != 0))
+  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
+  if (! INTERNAL_SYSCALL_ERROR_P (r)
+      && (buf->__st_ino_pad != 0
+	  || buf->__st_size_pad != 0
+	  || buf->__st_blocks_pad != 0))
     return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOVERFLOW);
-  return r;
 # else
 #  ifdef __NR_fstatat64
   /* Old KABIs with old non-LFS support, e.g. arm, i386, hppa, m68k, mips32,
      microblaze, s390, sh, powerpc, and sparc.  */
   struct stat64 st64;
-  int r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &st64, flag);
-  if (r == 0)
+  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, &st64, flag);
+  if (! INTERNAL_SYSCALL_ERROR_P (r))
     {
       if (! in_ino_t_range (st64.st_ino)
 	  || ! in_off_t_range (st64.st_size)
@@ -67,15 +69,21 @@ __fstatat (int fd, const char *file, struct stat *buf, int flag)
       buf->st_mtim.tv_nsec = st64.st_mtim.tv_nsec;
       buf->st_ctim.tv_sec = st64.st_ctim.tv_sec;
       buf->st_ctim.tv_nsec = st64.st_ctim.tv_nsec;
+
+      return 0;
     }
-  return r;
 #  else
   /* 64-bit kabi outlier, e.g. mips64 and mips64-n32.  */
   struct kernel_stat kst;
-  int r = INLINE_SYSCALL_CALL (newfstatat, fd, file, &kst, flag);
-  return r ?: __cp_kstat_stat (&kst, buf);
+  r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, &kst, flag);
+  if (! INTERNAL_SYSCALL_ERROR_P (r))
+    r = __cp_kstat_stat (&kst, buf);
 #  endif /* __nr_fstatat64  */
 # endif /* STAT_IS_KERNEL_STAT  */
+
+  return INTERNAL_SYSCALL_ERROR_P (r)
+	 ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r)
+	 : 0;
 }
 
 weak_alias (__fstatat, fstatat)
diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
index ae8fc101c5..f9b603ce5a 100644
--- a/sysdeps/unix/sysv/linux/fstatat64.c
+++ b/sysdeps/unix/sysv/linux/fstatat64.c
@@ -39,31 +39,32 @@ __fstatat64_time64 (int fd, const char *file, struct __stat64_t64 *buf,
   /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
      64-bit time_t support is done through statx syscall.  */
   struct statx tmp;
-  r = INLINE_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
-			   STATX_BASIC_STATS, &tmp);
-  if (r == 0 || errno != ENOSYS)
+  r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
+			     STATX_BASIC_STATS, &tmp);
+  if (! INTERNAL_SYSCALL_ERROR_P (r))
     {
-      if (r == 0)
-	__cp_stat64_t64_statx (buf, &tmp);
+      __cp_stat64_t64_statx (buf, &tmp);
       return r;
     }
+  if (-r != ENOSYS)
+    return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r);
 #endif
 
 #if XSTAT_IS_XSTAT64
 # ifdef __NR_newfstatat
   /* 64-bit kABI, e.g. aarch64, ia64, powerpc64*, s390x, riscv64, and
      x86_64.  */
-  r = INLINE_SYSCALL_CALL (newfstatat, fd, file, buf, flag);
+  r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag);
 # elif defined __NR_fstatat64
 #  if STAT64_IS_KERNEL_STAT64
   /* 64-bit kABI outlier, e.g. alpha  */
-  r = INLINE_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
+  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
 #  else
   /* 64-bit kABI outlier, e.g. sparc64.  */
   struct kernel_stat64 kst64;
-  r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &kst64, flag);
-  if (r == 0)
-    r = __cp_stat64_kstat64 (buf, &kst64);
+  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, &kst64, flag);
+  if (! INTERNAL_SYSCALL_ERROR_P (r))
+    __cp_stat64_kstat64 (buf, &kst64);
 #  endif
 # endif
 #else
@@ -72,8 +73,8 @@ __fstatat64_time64 (int fd, const char *file, struct __stat64_t64 *buf,
      e.g. arm, csky, i386, hppa, m68k, microblaze, nios2, sh, powerpc32,
      and sparc32.  */
   struct stat64 st64;
-  r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &st64, flag);
-  if (r == 0)
+  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, &st64, flag);
+  if (! INTERNAL_SYSCALL_ERROR_P (r))
     {
       /* Clear both pad and reserved fields.  */
       memset (buf, 0, sizeof (*buf));
@@ -95,13 +96,15 @@ __fstatat64_time64 (int fd, const char *file, struct __stat64_t64 *buf,
 # else
   /* 64-bit kabi outlier, e.g. mips64 and mips64-n32.  */
   struct kernel_stat kst;
-  r = INLINE_SYSCALL_CALL (newfstatat, fd, file, &kst, flag);
+  r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, &kst, flag);
   if (r == 0)
-    r =  __cp_kstat_stat64_t64 (&kst, buf);
+    __cp_kstat_stat64_t64 (&kst, buf);
 # endif
 #endif
 
-  return r;
+  return INTERNAL_SYSCALL_ERROR_P (r)
+	 ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r)
+	 : 0;
 }
 #if __TIMESIZE != 64
 hidden_def (__fstatat64_time64)
diff --git a/sysdeps/unix/sysv/linux/mips/mips64/kstat_cp.h b/sysdeps/unix/sysv/linux/mips/mips64/kstat_cp.h
index 1805d4b85f..71fe39fdd0 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/kstat_cp.h
+++ b/sysdeps/unix/sysv/linux/mips/mips64/kstat_cp.h
@@ -19,13 +19,13 @@
 #include <sys/stat.h>
 #include <kernel_stat.h>
 
-static inline int
+static inline long int
 __cp_kstat_stat (const struct kernel_stat *kst, struct stat *st)
 {
   if (! in_ino_t_range (kst->st_ino)
       || ! in_off_t_range (kst->st_size)
       || ! in_blkcnt_t_range (kst->st_blocks))
-    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOVERFLOW);
+    return -EOVERFLOW;
 
   st->st_dev = kst->st_dev;
   memset (&st->st_pad1, 0, sizeof (st->st_pad1));
@@ -51,7 +51,7 @@ __cp_kstat_stat (const struct kernel_stat *kst, struct stat *st)
   return 0;
 }
 
-static inline int
+static inline void
 __cp_kstat_stat64_t64 (const struct kernel_stat *kst, struct __stat64_t64 *st)
 {
   st->st_dev = kst->st_dev;
@@ -70,6 +70,4 @@ __cp_kstat_stat64_t64 (const struct kernel_stat *kst, struct __stat64_t64 *st)
   st->st_mtim.tv_nsec = kst->st_mtime_nsec;
   st->st_ctim.tv_sec = kst->st_ctime_sec;
   st->st_ctim.tv_nsec = kst->st_ctime_nsec;
-
-  return 0;
 }
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/kstat_cp.h b/sysdeps/unix/sysv/linux/sparc/sparc64/kstat_cp.h
index 0599b6a49e..d3f2841ade 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/kstat_cp.h
+++ b/sysdeps/unix/sysv/linux/sparc/sparc64/kstat_cp.h
@@ -18,7 +18,7 @@
 
 #include <errno.h>
 
-static inline int
+static inline void
 __cp_stat64_kstat64 (struct stat64 *st64, const struct kernel_stat64 *kst64)
 {
   st64->st_dev = kst64->st_dev;
@@ -41,6 +41,4 @@ __cp_stat64_kstat64 (struct stat64 *st64, const struct kernel_stat64 *kst64)
   st64->st_ctim.tv_nsec = kst64->st_ctime_nsec;
   st64->__glibc_reserved4 = 0;
   st64->__glibc_reserved5 = 0;
-
-  return 0;
 }
-- 
2.25.1

[PATCH 2/3] rtld: Fix wrong errno check on open_path

[Adhemerval Zanella via Libc-alpha]

At open_path code:

elf/dl-load.c

1982       if (here_any && (err = errno) != ENOENT && err != EACCES)
1983         /* The file exists and is readable, but something went wrong.  */
1984         return -1;

This code checks the errno value without checking whether the previous
function call that changed 'err' actually has failed (in this specific
case the stat64 at line 1931).  This due how we currently implemented
the y2038 support with INLINE_SYSCALL_CALL (since a function that
succeeds is allowed to change errno and it simplifies the resulting
y2038 support a bit).

In fact this check does not really make much sense, since either 'fd'
will be different than '0' (meaning it has being opened) or the 'stat64'
at line 1931 failed and 'here_any' will not be set (the stat64 at line
1951 already explicit sets errno in failure case).  Also, git history
does not give much information on why it was added at fist place.

Checked on i686-linux-gnu, x86_64-linux-gnu, sparc64-linux-gnu,
sparcv9-linux-gnu, powerpc64-linux-gnu, powerpc64le-linux-gnu,
arm-linux-gnueabihf, and aarch64-linux-gnu.
---
 elf/dl-load.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/elf/dl-load.c b/elf/dl-load.c
index f3201e7c14..e6972a6fe6 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1878,7 +1878,6 @@ open_path (const char *name, size_t namelen, int mode,
       size_t cnt;
       char *edp;
       int here_any = 0;
-      int err;
 
       /* If we are debugging the search for libraries print the path
 	 now if it hasn't happened now.  */
@@ -1979,9 +1978,6 @@ open_path (const char *name, size_t namelen, int mode,
 	      return -1;
 	    }
 	}
-      if (here_any && (err = errno) != ENOENT && err != EACCES)
-	/* The file exists and is readable, but something went wrong.  */
-	return -1;
 
       /* Remember whether we found anything.  */
       any |= here_any;
@@ -2002,6 +1998,8 @@ open_path (const char *name, size_t namelen, int mode,
 	sps->dirs = (void *) -1;
     }
 
+  /* The errno is used by _dl_signal_error.  */
+  __set_errno (ENOENT);
   return -1;
 }
 
-- 
2.25.1

[PATCH 3/3] locale: Fix locale construct_output_path error handling

[Adhemerval Zanella via Libc-alpha]

On following localedef code:

237   output_path  = construct_output_path (argv[remaining]);
238   if (output_path == NULL && ! no_archive)
239     error (4, errno, _("cannot create directory for output files"));
240   cannot_write_why = errno;

The 'cannot_write_why' will be set to a non 0 value on success if
euidaccess or mkdir (called by construct_output_path) change the errno
on a success call.

Instead o relying on the errno value regardless the previous libc
call fails or not, explicit set the erro code on failure at
construct_output_path.

Checked on x86_64-linux-gnu.
---
 locale/programs/localedef.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/locale/programs/localedef.c b/locale/programs/localedef.c
index b048bd05b9..4b488d5c2e 100644
--- a/locale/programs/localedef.c
+++ b/locale/programs/localedef.c
@@ -180,7 +180,7 @@ static struct argp argp =
 
 /* Prototypes for local functions.  */
 static void error_print (void);
-static char *construct_output_path (char *path);
+static char *construct_output_path (char *path, int *cannot_write_why);
 static char *normalize_codeset (const char *codeset, size_t name_len);
 
 
@@ -234,10 +234,9 @@ main (int argc, char *argv[])
   /* The parameter describes the output path of the constructed files.
      If the described files cannot be written return a NULL pointer.
      We don't free output_path because we will exit.  */
-  output_path  = construct_output_path (argv[remaining]);
+  output_path  = construct_output_path (argv[remaining], &cannot_write_why);
   if (output_path == NULL && ! no_archive)
     error (4, errno, _("cannot create directory for output files"));
-  cannot_write_why = errno;
 
   /* Now that the parameters are processed we have to reset the local
      ctype locale.  (P1003.2 4.35.5.2)  */
@@ -478,7 +477,7 @@ error_print (void)
    '/' character it is a relative path.  Otherwise it names the locale this
    definition is for.   The returned path must be freed by the caller. */
 static char *
-construct_output_path (char *path)
+construct_output_path (char *path, int *cannot_write_why)
 {
   char *result;
 
@@ -533,6 +532,7 @@ construct_output_path (char *path)
     }
 
   errno = 0;
+  *cannot_write_why = 0;
 
   if (no_archive && euidaccess (result, W_OK) == -1)
     {
@@ -545,14 +545,18 @@ construct_output_path (char *path)
 	      record_verbose (stderr,
 			      _("cannot create output path \'%s\': %s"),
 			      result, strerror (errno));
+	      *cannot_write_why = errno;
 	      free (result);
-	      return NULL;
+	      result = NULL;
 	    }
 	}
       else
-	record_verbose (stderr,
-			_("no write permission to output path \'%s\': %s"),
-			result, strerror (errno));
+	{
+	  record_verbose (stderr,
+			  _("no write permission to output path \'%s\': %s"),
+			  result, strerror (errno));
+          *cannot_write_why = errno;
+	}
     }
 
   return result;
-- 
2.25.1

Re: [PATCH 1/3] linux: Use INTERNAL_SYSCALL on fstatat{64}

[Adhemerval Zanella via Libc-alpha]

On 14/10/2020 17:37, Adhemerval Zanella wrote:
> Although not required by the standards, some code expects that a
> successful stat call should not set errno.  However since aa03f722f3b99
> 'linux: Add {f}stat{at} y2038 support', stat implementation will first
> try a __NR_statx call and if it fails with ENOSYS then issue the
> required stat syscall.
> 
> On 32-bit architecture running on kernel without __NR_statx support the
> first call will set the errno to ENOSYS, even when the following stat
> syscall does not fail.
> 
> This patch fixes by using INTERNAL_SYSCALL and only setting the errno
> value when function returns.
> 
> Checked on i686-linux-gnu, x86_64-linux-gnu, sparc64-linux-gnu,
> sparcv9-linux-gnu, powerpc64-linux-gnu, powerpc64le-linux-gnu,
> arm-linux-gnueabihf, and aarch64-linux-gnu.

So this also fixes the regressions I was seeing on mips-linux-gnu and
mips64-n32-linux-gnu.  I will commit this shortly if no one opposes.

> ---
>  sysdeps/unix/sysv/linux/fstatat.c             | 28 ++++++++++------
>  sysdeps/unix/sysv/linux/fstatat64.c           | 33 ++++++++++---------
>  .../unix/sysv/linux/mips/mips64/kstat_cp.h    |  8 ++---
>  .../unix/sysv/linux/sparc/sparc64/kstat_cp.h  |  4 +--
>  4 files changed, 40 insertions(+), 33 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/fstatat.c b/sysdeps/unix/sysv/linux/fstatat.c
> index c7fcfaf277..78fad51961 100644
> --- a/sysdeps/unix/sysv/linux/fstatat.c
> +++ b/sysdeps/unix/sysv/linux/fstatat.c
> @@ -26,22 +26,24 @@
>  int
>  __fstatat (int fd, const char *file, struct stat *buf, int flag)
>  {
> +  int r;
> +
>  # if STAT_IS_KERNEL_STAT
>    /* New kABIs which uses generic pre 64-bit time Linux ABI, e.g.
>       csky, nios2  */
> -  int r = INLINE_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
> -  if (r == 0 && (buf->__st_ino_pad != 0
> -		 || buf->__st_size_pad != 0
> -		 || buf->__st_blocks_pad != 0))
> +  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
> +  if (! INTERNAL_SYSCALL_ERROR_P (r)
> +      && (buf->__st_ino_pad != 0
> +	  || buf->__st_size_pad != 0
> +	  || buf->__st_blocks_pad != 0))
>      return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOVERFLOW);
> -  return r;
>  # else
>  #  ifdef __NR_fstatat64
>    /* Old KABIs with old non-LFS support, e.g. arm, i386, hppa, m68k, mips32,
>       microblaze, s390, sh, powerpc, and sparc.  */
>    struct stat64 st64;
> -  int r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &st64, flag);
> -  if (r == 0)
> +  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, &st64, flag);
> +  if (! INTERNAL_SYSCALL_ERROR_P (r))
>      {
>        if (! in_ino_t_range (st64.st_ino)
>  	  || ! in_off_t_range (st64.st_size)
> @@ -67,15 +69,21 @@ __fstatat (int fd, const char *file, struct stat *buf, int flag)
>        buf->st_mtim.tv_nsec = st64.st_mtim.tv_nsec;
>        buf->st_ctim.tv_sec = st64.st_ctim.tv_sec;
>        buf->st_ctim.tv_nsec = st64.st_ctim.tv_nsec;
> +
> +      return 0;
>      }
> -  return r;
>  #  else
>    /* 64-bit kabi outlier, e.g. mips64 and mips64-n32.  */
>    struct kernel_stat kst;
> -  int r = INLINE_SYSCALL_CALL (newfstatat, fd, file, &kst, flag);
> -  return r ?: __cp_kstat_stat (&kst, buf);
> +  r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, &kst, flag);
> +  if (! INTERNAL_SYSCALL_ERROR_P (r))
> +    r = __cp_kstat_stat (&kst, buf);
>  #  endif /* __nr_fstatat64  */
>  # endif /* STAT_IS_KERNEL_STAT  */
> +
> +  return INTERNAL_SYSCALL_ERROR_P (r)
> +	 ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r)
> +	 : 0;
>  }
>  
>  weak_alias (__fstatat, fstatat)
> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
> index ae8fc101c5..f9b603ce5a 100644
> --- a/sysdeps/unix/sysv/linux/fstatat64.c
> +++ b/sysdeps/unix/sysv/linux/fstatat64.c
> @@ -39,31 +39,32 @@ __fstatat64_time64 (int fd, const char *file, struct __stat64_t64 *buf,
>    /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
>       64-bit time_t support is done through statx syscall.  */
>    struct statx tmp;
> -  r = INLINE_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
> -			   STATX_BASIC_STATS, &tmp);
> -  if (r == 0 || errno != ENOSYS)
> +  r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
> +			     STATX_BASIC_STATS, &tmp);
> +  if (! INTERNAL_SYSCALL_ERROR_P (r))
>      {
> -      if (r == 0)
> -	__cp_stat64_t64_statx (buf, &tmp);
> +      __cp_stat64_t64_statx (buf, &tmp);
>        return r;
>      }
> +  if (-r != ENOSYS)
> +    return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r);
>  #endif
>  
>  #if XSTAT_IS_XSTAT64
>  # ifdef __NR_newfstatat
>    /* 64-bit kABI, e.g. aarch64, ia64, powerpc64*, s390x, riscv64, and
>       x86_64.  */
> -  r = INLINE_SYSCALL_CALL (newfstatat, fd, file, buf, flag);
> +  r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag);
>  # elif defined __NR_fstatat64
>  #  if STAT64_IS_KERNEL_STAT64
>    /* 64-bit kABI outlier, e.g. alpha  */
> -  r = INLINE_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
> +  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
>  #  else
>    /* 64-bit kABI outlier, e.g. sparc64.  */
>    struct kernel_stat64 kst64;
> -  r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &kst64, flag);
> -  if (r == 0)
> -    r = __cp_stat64_kstat64 (buf, &kst64);
> +  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, &kst64, flag);
> +  if (! INTERNAL_SYSCALL_ERROR_P (r))
> +    __cp_stat64_kstat64 (buf, &kst64);
>  #  endif
>  # endif
>  #else
> @@ -72,8 +73,8 @@ __fstatat64_time64 (int fd, const char *file, struct __stat64_t64 *buf,
>       e.g. arm, csky, i386, hppa, m68k, microblaze, nios2, sh, powerpc32,
>       and sparc32.  */
>    struct stat64 st64;
> -  r = INLINE_SYSCALL_CALL (fstatat64, fd, file, &st64, flag);
> -  if (r == 0)
> +  r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, &st64, flag);
> +  if (! INTERNAL_SYSCALL_ERROR_P (r))
>      {
>        /* Clear both pad and reserved fields.  */
>        memset (buf, 0, sizeof (*buf));
> @@ -95,13 +96,15 @@ __fstatat64_time64 (int fd, const char *file, struct __stat64_t64 *buf,
>  # else
>    /* 64-bit kabi outlier, e.g. mips64 and mips64-n32.  */
>    struct kernel_stat kst;
> -  r = INLINE_SYSCALL_CALL (newfstatat, fd, file, &kst, flag);
> +  r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, &kst, flag);
>    if (r == 0)
> -    r =  __cp_kstat_stat64_t64 (&kst, buf);
> +    __cp_kstat_stat64_t64 (&kst, buf);
>  # endif
>  #endif
>  
> -  return r;
> +  return INTERNAL_SYSCALL_ERROR_P (r)
> +	 ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r)
> +	 : 0;
>  }
>  #if __TIMESIZE != 64
>  hidden_def (__fstatat64_time64)
> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/kstat_cp.h b/sysdeps/unix/sysv/linux/mips/mips64/kstat_cp.h
> index 1805d4b85f..71fe39fdd0 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips64/kstat_cp.h
> +++ b/sysdeps/unix/sysv/linux/mips/mips64/kstat_cp.h
> @@ -19,13 +19,13 @@
>  #include <sys/stat.h>
>  #include <kernel_stat.h>
>  
> -static inline int
> +static inline long int
>  __cp_kstat_stat (const struct kernel_stat *kst, struct stat *st)
>  {
>    if (! in_ino_t_range (kst->st_ino)
>        || ! in_off_t_range (kst->st_size)
>        || ! in_blkcnt_t_range (kst->st_blocks))
> -    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOVERFLOW);
> +    return -EOVERFLOW;
>  
>    st->st_dev = kst->st_dev;
>    memset (&st->st_pad1, 0, sizeof (st->st_pad1));
> @@ -51,7 +51,7 @@ __cp_kstat_stat (const struct kernel_stat *kst, struct stat *st)
>    return 0;
>  }
>  
> -static inline int
> +static inline void
>  __cp_kstat_stat64_t64 (const struct kernel_stat *kst, struct __stat64_t64 *st)
>  {
>    st->st_dev = kst->st_dev;
> @@ -70,6 +70,4 @@ __cp_kstat_stat64_t64 (const struct kernel_stat *kst, struct __stat64_t64 *st)
>    st->st_mtim.tv_nsec = kst->st_mtime_nsec;
>    st->st_ctim.tv_sec = kst->st_ctime_sec;
>    st->st_ctim.tv_nsec = kst->st_ctime_nsec;
> -
> -  return 0;
>  }
> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/kstat_cp.h b/sysdeps/unix/sysv/linux/sparc/sparc64/kstat_cp.h
> index 0599b6a49e..d3f2841ade 100644
> --- a/sysdeps/unix/sysv/linux/sparc/sparc64/kstat_cp.h
> +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/kstat_cp.h
> @@ -18,7 +18,7 @@
>  
>  #include <errno.h>
>  
> -static inline int
> +static inline void
>  __cp_stat64_kstat64 (struct stat64 *st64, const struct kernel_stat64 *kst64)
>  {
>    st64->st_dev = kst64->st_dev;
> @@ -41,6 +41,4 @@ __cp_stat64_kstat64 (struct stat64 *st64, const struct kernel_stat64 *kst64)
>    st64->st_ctim.tv_nsec = kst64->st_ctime_nsec;
>    st64->__glibc_reserved4 = 0;
>    st64->__glibc_reserved5 = 0;
> -
> -  return 0;
>  }
> 

Re: [PATCH 1/3] linux: Use INTERNAL_SYSCALL on fstatat{64}

[Florian Weimer via Libc-alpha]

* Adhemerval Zanella via Libc-alpha:

> +  return INTERNAL_SYSCALL_ERROR_P (r)
> +	 ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r)
> +	 : 0;

I'm not really happy how the expression -r breaks the INTERNAL_*
abstraction.  Do you really have to change the calling convention for
__cp_kstat_stat?  I expect that if you don't do that (and make calls to
__cp_kstat_stat tail calls), you can preserve the abstraction.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 1/3] linux: Use INTERNAL_SYSCALL on fstatat{64}

[Adhemerval Zanella via Libc-alpha]

On 15/10/2020 12:45, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> +  return INTERNAL_SYSCALL_ERROR_P (r)
>> +	 ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r)
>> +	 : 0;
> 
> I'm not really happy how the expression -r breaks the INTERNAL_*
> abstraction.  Do you really have to change the calling convention for
> __cp_kstat_stat?  I expect that if you don't do that (and make calls to
> __cp_kstat_stat tail calls), you can preserve the abstraction.

The INTERNAL_* abstractions is something I would like to get rid of
since currently INTERNAL_SYSCALL follows the same convention for all
architectures (meaning that there is no extra auxiliary variable that
hold whether the syscall has failed, as previously for powerpc and
sparc) and Linux assumes that errno values are all in the range of
[-4096UL, -1UL].

We can now simplify the INTERNAL_SYSCALL tests to check for specific
errno value without the need to resorting the extra macros.  I really
see that we can move to something simpler and more readable internal
APIs as:

  #define INTERNAL_SYSCALL_CALL (syscall, ...) ...

  static inline _Bool internal_syscall_failed (unsigned long int val)
  {
    return val > -4096UL;
  }

  static inline long int __syscall_ret (unsigned long int val)
  {
    if (internal_syscall_failed (val))
      {
        errno = -val;
        return -1;
      }
    return 0;
  }

  #define INLINE_SYSCALL_CALL (...) \
   __syscall_ret (NTERNAL_SYSCALL_CALL (__VA_ARGS__))


And so we can write more specific syscall wrappers as:

  int r = INTERNAL_SYSCALL_CALL (syscall, ...);
  if (r == 0 || -r == ENOSYS)
    return __syscall_ret (r);
  r = INTERNAL_SYSCALL_CALL (syscall_fallback, ...);
  return __syscall_ret (r);

Or similar if there is no need to handle errno:

  return -INTERNAL_SYSCALL_CALL (syscall, ...);

Re: [PATCH 1/3] linux: Use INTERNAL_SYSCALL on fstatat{64}

[Adhemerval Zanella via Libc-alpha]

On 15/10/2020 14:09, Adhemerval Zanella wrote:
> 
> 
> On 15/10/2020 12:45, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> +  return INTERNAL_SYSCALL_ERROR_P (r)
>>> +	 ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r)
>>> +	 : 0;
>>
>> I'm not really happy how the expression -r breaks the INTERNAL_*
>> abstraction.  Do you really have to change the calling convention for
>> __cp_kstat_stat?  I expect that if you don't do that (and make calls to
>> __cp_kstat_stat tail calls), you can preserve the abstraction.
> 
> The INTERNAL_* abstractions is something I would like to get rid of
> since currently INTERNAL_SYSCALL follows the same convention for all
> architectures (meaning that there is no extra auxiliary variable that
> hold whether the syscall has failed, as previously for powerpc and
> sparc) and Linux assumes that errno values are all in the range of
> [-4096UL, -1UL].
> 
> We can now simplify the INTERNAL_SYSCALL tests to check for specific
> errno value without the need to resorting the extra macros.  I really
> see that we can move to something simpler and more readable internal
> APIs as:
> 
>   #define INTERNAL_SYSCALL_CALL (syscall, ...) ...
> 
>   static inline _Bool internal_syscall_failed (unsigned long int val)
>   {
>     return val > -4096UL;
>   }
> 
>   static inline long int __syscall_ret (unsigned long int val)
>   {
>     if (internal_syscall_failed (val))
>       {
>         errno = -val;
>         return -1;
>       }
>     return 0;
>   }
> 
>   #define INLINE_SYSCALL_CALL (...) \
>    __syscall_ret (NTERNAL_SYSCALL_CALL (__VA_ARGS__))
> 
> 
> And so we can write more specific syscall wrappers as:
> 
>   int r = INTERNAL_SYSCALL_CALL (syscall, ...);
>   if (r == 0 || -r == ENOSYS)
>     return __syscall_ret (r);
>   r = INTERNAL_SYSCALL_CALL (syscall_fallback, ...);
>   return __syscall_ret (r);
> 
> Or similar if there is no need to handle errno:
> 
>   return -INTERNAL_SYSCALL_CALL (syscall, ...);
> 

And in long term I would like to move the INTERNAL_SYSCALL_CALL to proper
inline function instead of macros.  It should result a more clean code
and avoid the pitfalls of using some construction with the macros (such
as BZ#25523). I have some patches done, but there are quite extensive...

Re: [PATCH 1/3] linux: Use INTERNAL_SYSCALL on fstatat{64}

[Florian Weimer via Libc-alpha]

* Adhemerval Zanella:

> On 15/10/2020 12:45, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> +  return INTERNAL_SYSCALL_ERROR_P (r)
>>> +	 ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r)
>>> +	 : 0;
>> 
>> I'm not really happy how the expression -r breaks the INTERNAL_*
>> abstraction.  Do you really have to change the calling convention for
>> __cp_kstat_stat?  I expect that if you don't do that (and make calls to
>> __cp_kstat_stat tail calls), you can preserve the abstraction.
>
> The INTERNAL_* abstractions is something I would like to get rid of
> since currently INTERNAL_SYSCALL follows the same convention for all
> architectures (meaning that there is no extra auxiliary variable that
> hold whether the syscall has failed, as previously for powerpc and
> sparc) and Linux assumes that errno values are all in the range of
> [-4096UL, -1UL].

We can certainly consider this, but doing this half-way in an unrelated
patch seems a bit odd.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 1/3] linux: Use INTERNAL_SYSCALL on fstatat{64}

[Adhemerval Zanella via Libc-alpha]

On 19/10/2020 09:56, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 15/10/2020 12:45, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> +  return INTERNAL_SYSCALL_ERROR_P (r)
>>>> +	 ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r)
>>>> +	 : 0;
>>>
>>> I'm not really happy how the expression -r breaks the INTERNAL_*
>>> abstraction.  Do you really have to change the calling convention for
>>> __cp_kstat_stat?  I expect that if you don't do that (and make calls to
>>> __cp_kstat_stat tail calls), you can preserve the abstraction.
>>
>> The INTERNAL_* abstractions is something I would like to get rid of
>> since currently INTERNAL_SYSCALL follows the same convention for all
>> architectures (meaning that there is no extra auxiliary variable that
>> hold whether the syscall has failed, as previously for powerpc and
>> sparc) and Linux assumes that errno values are all in the range of
>> [-4096UL, -1UL].
> 
> We can certainly consider this, but doing this half-way in an unrelated
> patch seems a bit odd.

We already have various cases I cases where INTERNAL_SYSCALL_ERRNO is not
use, I will revise and remove the INTERNAL_SYSCALL_ERRNO macro.  For
INTERNAL_SYSCALL_ERROR_P we can either keep as is or move to a proper
inline function.