Message ID | 20180820144032.DC8D94028A147@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | __readlink_chk: Assume HAVE_INLINED_SYSCALLS | expand |
On 20/08/2018 11:40, Florian Weimer wrote: > HAVE_INLINED_SYSCALLS is always defined on Linux. > > 2018-08-20 Florian Weimer <fweimer@redhat.com> > > * sysdeps/unix/sysv/linux/generic/readlink_chk.c: Remove > HAVE_INLINED_SYSCALLS conditionals LGTM. > > diff --git a/sysdeps/unix/sysv/linux/generic/readlink_chk.c b/sysdeps/unix/sysv/linux/generic/readlink_chk.c > index 9240408a6b..5075e06dd1 100644 > --- a/sysdeps/unix/sysv/linux/generic/readlink_chk.c > +++ b/sysdeps/unix/sysv/linux/generic/readlink_chk.c > @@ -19,10 +19,8 @@ > #include <unistd.h> > #include <fcntl.h> > #include <sys/param.h> > -#ifdef HAVE_INLINED_SYSCALLS > # include <errno.h> > # include <sysdep.h> > -#endif > > > ssize_t > @@ -31,9 +29,5 @@ __readlink_chk (const char *path, void *buf, size_t len, size_t buflen) > if (len > buflen) > __chk_fail (); > > -#ifdef HAVE_INLINED_SYSCALLS > return INLINE_SYSCALL (readlinkat, 4, AT_FDCWD, path, buf, len); Maybe use INLINE_SYSCALL_CALL ? > -#else > - return __readlink (path, buf, len); > -#endif > } >
On 20/08/2018 13:18, Adhemerval Zanella wrote: > > > On 20/08/2018 11:40, Florian Weimer wrote: >> HAVE_INLINED_SYSCALLS is always defined on Linux. >> >> 2018-08-20 Florian Weimer <fweimer@redhat.com> >> >> * sysdeps/unix/sysv/linux/generic/readlink_chk.c: Remove >> HAVE_INLINED_SYSCALLS conditionals > > LGTM. In fact, do we really need to have a duplicated Linux implementation for this micro-optimization? Couldn't we just use default debug one and call '__readlink' instead? > >> >> diff --git a/sysdeps/unix/sysv/linux/generic/readlink_chk.c b/sysdeps/unix/sysv/linux/generic/readlink_chk.c >> index 9240408a6b..5075e06dd1 100644 >> --- a/sysdeps/unix/sysv/linux/generic/readlink_chk.c >> +++ b/sysdeps/unix/sysv/linux/generic/readlink_chk.c >> @@ -19,10 +19,8 @@ >> #include <unistd.h> >> #include <fcntl.h> >> #include <sys/param.h> >> -#ifdef HAVE_INLINED_SYSCALLS >> # include <errno.h> >> # include <sysdep.h> >> -#endif >> >> >> ssize_t >> @@ -31,9 +29,5 @@ __readlink_chk (const char *path, void *buf, size_t len, size_t buflen) >> if (len > buflen) >> __chk_fail (); >> >> -#ifdef HAVE_INLINED_SYSCALLS >> return INLINE_SYSCALL (readlinkat, 4, AT_FDCWD, path, buf, len); > > Maybe use INLINE_SYSCALL_CALL ? > >> -#else >> - return __readlink (path, buf, len); >> -#endif >> } >>
On 08/20/2018 06:21 PM, Adhemerval Zanella wrote: > > > On 20/08/2018 13:18, Adhemerval Zanella wrote: >> >> >> On 20/08/2018 11:40, Florian Weimer wrote: >>> HAVE_INLINED_SYSCALLS is always defined on Linux. >>> >>> 2018-08-20 Florian Weimer <fweimer@redhat.com> >>> >>> * sysdeps/unix/sysv/linux/generic/readlink_chk.c: Remove >>> HAVE_INLINED_SYSCALLS conditionals >> >> LGTM. > > In fact, do we really need to have a duplicated Linux implementation > for this micro-optimization? Couldn't we just use default debug > one and call '__readlink' instead? Like this? Thanks, Florian Subject: [PATCH] __readlink_chk: Remove micro-optimization To: libc-alpha@sourceware.org 2018-08-20 Florian Weimer <fweimer@redhat.com> * debug/readlink_chk.c (__readlink_chk): Always call __readlink. * sysdeps/unix/sysv/linux/generic/readlink_chk.c: Remove file. diff --git a/debug/readlink_chk.c b/debug/readlink_chk.c index c44d1d8c37..e3204004d2 100644 --- a/debug/readlink_chk.c +++ b/debug/readlink_chk.c @@ -29,9 +29,5 @@ __readlink_chk (const char *path, void *buf, size_t len, size_t buflen) if (len > buflen) __chk_fail (); -#ifdef HAVE_INLINED_SYSCALLS - return INLINE_SYSCALL (readlink, 3, path, buf, len); -#else return __readlink (path, buf, len); -#endif } diff --git a/sysdeps/unix/sysv/linux/generic/readlink_chk.c b/sysdeps/unix/sysv/linux/generic/readlink_chk.c deleted file mode 100644 index 52d1f5b522..0000000000 --- a/sysdeps/unix/sysv/linux/generic/readlink_chk.c +++ /dev/null @@ -1,33 +0,0 @@ -/* Copyright (C) 2011-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library. If not, see - <http://www.gnu.org/licenses/>. */ - -#include <unistd.h> -#include <fcntl.h> -#include <sys/param.h> -# include <errno.h> -# include <sysdep.h> - - -ssize_t -__readlink_chk (const char *path, void *buf, size_t len, size_t buflen) -{ - if (len > buflen) - __chk_fail (); - - return INLINE_SYSCALL_CALL (readlinkat, AT_FDCWD, path, buf, len); -}
On 20/08/2018 13:53, Florian Weimer wrote: > On 08/20/2018 06:21 PM, Adhemerval Zanella wrote: >> >> >> On 20/08/2018 13:18, Adhemerval Zanella wrote: >>> >>> >>> On 20/08/2018 11:40, Florian Weimer wrote: >>>> HAVE_INLINED_SYSCALLS is always defined on Linux. >>>> >>>> 2018-08-20 Florian Weimer <fweimer@redhat.com> >>>> >>>> * sysdeps/unix/sysv/linux/generic/readlink_chk.c: Remove >>>> HAVE_INLINED_SYSCALLS conditionals >>> >>> LGTM. >> >> In fact, do we really need to have a duplicated Linux implementation >> for this micro-optimization? Couldn't we just use default debug >> one and call '__readlink' instead? > > Like this? > LGTM, thanks. > Thanks, > Florian > > readlink_chk.patch > > > Subject: [PATCH] __readlink_chk: Remove micro-optimization > To: libc-alpha@sourceware.org > > 2018-08-20 Florian Weimer <fweimer@redhat.com> > > * debug/readlink_chk.c (__readlink_chk): Always call __readlink. > * sysdeps/unix/sysv/linux/generic/readlink_chk.c: Remove file. > > diff --git a/debug/readlink_chk.c b/debug/readlink_chk.c > index c44d1d8c37..e3204004d2 100644 > --- a/debug/readlink_chk.c > +++ b/debug/readlink_chk.c > @@ -29,9 +29,5 @@ __readlink_chk (const char *path, void *buf, size_t len, size_t buflen) > if (len > buflen) > __chk_fail (); > > -#ifdef HAVE_INLINED_SYSCALLS > - return INLINE_SYSCALL (readlink, 3, path, buf, len); > -#else > return __readlink (path, buf, len); > -#endif > } > diff --git a/sysdeps/unix/sysv/linux/generic/readlink_chk.c b/sysdeps/unix/sysv/linux/generic/readlink_chk.c > deleted file mode 100644 > index 52d1f5b522..0000000000 > --- a/sysdeps/unix/sysv/linux/generic/readlink_chk.c > +++ /dev/null > @@ -1,33 +0,0 @@ > -/* Copyright (C) 2011-2018 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011. > - > - The GNU C Library is free software; you can redistribute it and/or > - modify it under the terms of the GNU Lesser General Public > - License as published by the Free Software Foundation; either > - version 2.1 of the License, or (at your option) any later version. > - > - The GNU C Library is distributed in the hope that it will be useful, > - but WITHOUT ANY WARRANTY; without even the implied warranty of > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > - Lesser General Public License for more details. > - > - You should have received a copy of the GNU Lesser General Public > - License along with the GNU C Library. If not, see > - <http://www.gnu.org/licenses/>. */ > - > -#include <unistd.h> > -#include <fcntl.h> > -#include <sys/param.h> > -# include <errno.h> > -# include <sysdep.h> > - > - > -ssize_t > -__readlink_chk (const char *path, void *buf, size_t len, size_t buflen) > -{ > - if (len > buflen) > - __chk_fail (); > - > - return INLINE_SYSCALL_CALL (readlinkat, AT_FDCWD, path, buf, len); > -} >
On Mon, 20 Aug 2018, Florian Weimer wrote: > -#ifdef HAVE_INLINED_SYSCALLS > # include <errno.h> > # include <sysdep.h> > -#endif Need to update the preprocessor indentation here for the removed #ifdef.
On 08/20/2018 09:32 PM, Joseph Myers wrote: > On Mon, 20 Aug 2018, Florian Weimer wrote: > >> -#ifdef HAVE_INLINED_SYSCALLS >> # include <errno.h> >> # include <sysdep.h> >> -#endif > Need to update the preprocessor indentation here for the removed #ifdef. Oops, right. The file is gone in the follow-up patch. But I see I forgot to remove the #includes from debug/readlink_chk.c. I will fix that too. Florian
diff --git a/sysdeps/unix/sysv/linux/generic/readlink_chk.c b/sysdeps/unix/sysv/linux/generic/readlink_chk.c index 9240408a6b..5075e06dd1 100644 --- a/sysdeps/unix/sysv/linux/generic/readlink_chk.c +++ b/sysdeps/unix/sysv/linux/generic/readlink_chk.c @@ -19,10 +19,8 @@ #include <unistd.h> #include <fcntl.h> #include <sys/param.h> -#ifdef HAVE_INLINED_SYSCALLS # include <errno.h> # include <sysdep.h> -#endif ssize_t @@ -31,9 +29,5 @@ __readlink_chk (const char *path, void *buf, size_t len, size_t buflen) if (len > buflen) __chk_fail (); -#ifdef HAVE_INLINED_SYSCALLS return INLINE_SYSCALL (readlinkat, 4, AT_FDCWD, path, buf, len); -#else - return __readlink (path, buf, len); -#endif }