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

Message ID 1473166521-24827-1-git-send-email-ynorov@caviumnetworks.com
State New
Headers show

Commit Message

Yury Norov Sept. 6, 2016, 12:55 p.m. UTC
__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(-)

Comments

Andreas Schwab Sept. 6, 2016, 1:08 p.m. UTC | #1
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.
Zack Weinberg Sept. 6, 2016, 4:46 p.m. UTC | #2
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

Patch
diff mbox

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
 }