[RFC PATCH] __fxstat: replace if() with #if when checking version

[RFC PATCH] __fxstat: replace if() with #if when checking version

[Yury Norov]

__fxstat() is always passed with _STAT_VER as vers parameter and it's
in internal namespace.

It means we can replace runtime argument check with compile-time #ifdef.
It helps to make the body of _fxstat() smaller, and suppresses errors
if struct kernel_stat or __xstat_conv() is not declared, when they
are not needed.

If patch is found reasonable, I can check and fix other platforms and
stat syscalls. Could someone explain me, what for we need 'vers',
if we pass it with the only value everywhere. Maybe it's time to remove it
completely?

	* sysdeps/unix/sysv/linux/fxstat.c: Replace if() with #if
	when checking version

Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
---
 sysdeps/unix/sysv/linux/fxstat.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fxstat.c b/sysdeps/unix/sysv/linux/fxstat.c
index e33023b..2e79f8b 100644
--- a/sysdeps/unix/sysv/linux/fxstat.c
+++ b/sysdeps/unix/sysv/linux/fxstat.c
@@ -35,20 +35,22 @@
 int
 __fxstat (int vers, int fd, struct stat *buf)
 {
-  if (vers == _STAT_VER_KERNEL)
-    return INLINE_SYSCALL (fstat, 2, fd, buf);
+#if _STAT_VER == _STAT_VER_KERNEL
+  return INLINE_SYSCALL (fstat, 2, fd, buf);
+#else
 
-#ifdef STAT_IS_KERNEL_STAT
+# ifdef STAT_IS_KERNEL_STAT
   return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-#else
+# else
   struct kernel_stat kbuf;
   int result;
 
   result = INLINE_SYSCALL (fstat, 2, fd, &kbuf);
   if (result == 0)
-    result = __xstat_conv (vers, &kbuf, buf);
+    result = __xstat_conv (_STAT_VER, &kbuf, buf);
 
   return result;
+# endif
 #endif
 }
 
-- 
2.7.4

Re: [RFC PATCH] __fxstat: replace if() with #if when checking version

[Andreas Schwab]

On Sep 06 2016, Yury Norov <ynorov@caviumnetworks.com> wrote:

> __fxstat() is always passed with _STAT_VER as vers parameter and it's
> in internal namespace.

It is part of the glibc ABI.

> If patch is found reasonable, I can check and fix other platforms and
> stat syscalls. Could someone explain me, what for we need 'vers',
> if we pass it with the only value everywhere. Maybe it's time to remove it
> completely?

_STAT_VER has changed over time.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [RFC PATCH] __fxstat: replace if() with #if when checking version

[Yury Norov]

On Tue, Sep 06, 2016 at 03:08:58PM +0200, Andreas Schwab wrote:
> On Sep 06 2016, Yury Norov <ynorov@caviumnetworks.com> wrote:
> 
> > __fxstat() is always passed with _STAT_VER as vers parameter and it's
> > in internal namespace.
> 
> It is part of the glibc ABI.
> 
> > If patch is found reasonable, I can check and fix other platforms and
> > stat syscalls. Could someone explain me, what for we need 'vers',
> > if we pass it with the only value everywhere. Maybe it's time to remove it
> > completely?
> 
> _STAT_VER has changed over time.
> 
> Andreas.

OK, now I see. It seems like STAT_IS_KERNEL_STAT hint is used not
optimal way. In my understanding, it forces user pass vers == _STAT_VER_KERNEL.
But if so, glibc may avoid generating the text of __xstat_conv(), and
so no struct kernel_stat is needed. Take a look at the patch below. 
Does it make sense to you?

Yury.

diff --git a/sysdeps/unix/sysv/linux/fxstat.c b/sysdeps/unix/sysv/linux/fxstat.c
index e33023b..e9777df 100644
--- a/sysdeps/unix/sysv/linux/fxstat.c
+++ b/sysdeps/unix/sysv/linux/fxstat.c
@@ -35,20 +35,25 @@
 int
 __fxstat (int vers, int fd, struct stat *buf)
 {
+#ifdef STAT_IS_KERNEL_STAT
   if (vers == _STAT_VER_KERNEL)
     return INLINE_SYSCALL (fstat, 2, fd, buf);
-
-#ifdef STAT_IS_KERNEL_STAT
-  return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
+  else
+    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
 #else
-  struct kernel_stat kbuf;
-  int result;
+  if (vers == _STAT_VER_KERNEL)
+    return INLINE_SYSCALL (fstat, 2, fd, buf);
+  else
+    {
+      struct kernel_stat kbuf;
+      int result;
 
-  result = INLINE_SYSCALL (fstat, 2, fd, &kbuf);
-  if (result == 0)
-    result = __xstat_conv (vers, &kbuf, buf);
+      result = INLINE_SYSCALL (fstat, 2, fd, &kbuf);
+      if (result == 0)
+	result = __xstat_conv (vers, &kbuf, buf);
 
-  return result;
+      return result;
+    }
 #endif
 }
 

Re: [RFC PATCH] __fxstat: replace if() with #if when checking version

[Yury Norov]

On Tue, Sep 06, 2016 at 06:59:18PM +0300, Yury Norov wrote:
> On Tue, Sep 06, 2016 at 03:08:58PM +0200, Andreas Schwab wrote:
> > On Sep 06 2016, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > 
> > > __fxstat() is always passed with _STAT_VER as vers parameter and it's
> > > in internal namespace.
> > 
> > It is part of the glibc ABI.
> > 
> > > If patch is found reasonable, I can check and fix other platforms and
> > > stat syscalls. Could someone explain me, what for we need 'vers',
> > > if we pass it with the only value everywhere. Maybe it's time to remove it
> > > completely?
> > 
> > _STAT_VER has changed over time.
> > 
> > Andreas.
> 
> OK, now I see. It seems like STAT_IS_KERNEL_STAT hint is used not
> optimal way. In my understanding, it forces user pass vers == _STAT_VER_KERNEL.
> But if so, glibc may avoid generating the text of __xstat_conv(), and
> so no struct kernel_stat is needed. Take a look at the patch below. 
> Does it make sense to you?
> 
> Yury.

Sorry, I was wrong. Everything is right.

Re: [RFC PATCH] __fxstat: replace if() with #if when checking version

[Zack Weinberg]

On Tue, Sep 6, 2016 at 8:55 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> __fxstat() is always passed with _STAT_VER as vers parameter and it's
> in internal namespace.
>
> It means we can replace runtime argument check with compile-time #ifdef.
> It helps to make the body of _fxstat() smaller, and suppresses errors
> if struct kernel_stat or __xstat_conv() is not declared, when they
> are not needed.

We can't do this.  I think you may have already figured this out, but
let me write out a clear explanation for the benefit of future people
who may have the same idea.

_STAT_VER exists because the contents of struct stat have changed over
time.  When an old executable is used with a new C library, the 'vers'
argument will indicate that the executable expects the old layout, and
the library will translate from the kernel's (presumably newer) stat
layout to the executable's older expectations.  It is the same problem
that the kernel solves by changing the syscall numbers for
stat/fstat/lstat whenever they change the structure.  Therefore, the
'vers' argument *can* vary at runtime, even though all the code in the
*current* source tree always passes the same value.

To make this work, we have inline functions in <sys/stat.h>, backed up
by out-of-line copies in libc_nonshared.a, that present the documented
API (stat/fstat/lstat) and call __xstat/__fxstat/__lxstat with the
additional argument.  That is why these __-name functions are part of
the public ABI for libc.so.  Same principle as __printf_chk for
instance.

If we were designing this mechanism today from scratch we would
probably use symbol versions - unless there's some nonobvious reason
why that wouldn't work - but this mechanism *predates* symbol
versioning.  It has been around since the 1990s if not earlier.  Maybe
you could look into arranging to use symbol versions from now (or
better, the next time the kernel changes the stat structure) going
forward.  But the old __(|f|l)xstat symbols need to stick around for
binary compatibility, and their implementations need to test their
'vers' argument at runtime.

zw