Bernhard Voelker wrote: > On 02/17/2012 05:20 PM, Jim Meyering wrote: >> Good idea. >> And with that, we can also drop the eval "...". > > Great. > >> I have a slight preference for writing the file only from the destructor. >> WDYT? > > You convinced me ... but unfortunately not my PC. > > See the attached getxattr-speedup.log.gz - I added a "cat k.c" so that > you can check that I got your patch right. > It seems that GCC doesn't like our "__attribute__((destructor)) void p()". > It basically works in a standalone program, but not with LD_PRELOAD. > I didn't find much about this. Can this be a 64-bit issue?
Is that an open-suse system? I tried on an opensuse system with 2.6.37.6-0.11-desktop, and since there were no getxattr calls at all, no wrapper would run, and hence, your atexit call would never be reached. I've added this comment at the top: # This test is skipped on systems that lack LD_PRELOAD support; that's fine. + # Similarly, on a system that lacks getxattr altogether, skipping it is fine. and this new skip_ use below: test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?" Which may mean this test will simply be skipped on your system. But that is fine. > I worked on another alternative: using atexit(). I had to add -lc > to pull atexit from glibc, of course. And I made p() and count_1() > static to avoid potential name clashes with original ls symbols. See above. > Finally, I changed the comment "run the test, redirecting", as > we're obvisouly no longer redirecting. ;-) > What do you think? Good idea. I've updated that comment. > Ah yes, and another aspect came to my mind: the test does not > cover the other side of the changes in ls.c: it maybe should > prove that ls tries to get the attributes again when the cache > functions start having success for another file, i.e. that we > didn't break the normal XATTR, CAP and ACL queries. Is this > covered by other tests? No, but the dev-caching logic is very simple, so probably not worth a test for now. I expect to push this series shortly. >From b29db6767612cf70e9d4c7eb6ede9822f6173fca Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Mon, 13 Feb 2012 12:52:19 +0100 Subject: [PATCH] tests: test for ls speed-up * tests/ls/getxattr-speedup: New test. * tests/Makefile.am (TESTS): Add it. Improved-by: Bernhard Voelker --- tests/Makefile.am | 1 + tests/ls/getxattr-speedup | 64 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100755 tests/ls/getxattr-speedup diff --git a/tests/Makefile.am b/tests/Makefile.am index eed6ed6..2fee97d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -427,6 +427,7 @@ TESTS = \ ls/dired \ ls/file-type \ ls/follow-slink \ + ls/getxattr-speedup \ ls/infloop \ ls/inode \ ls/m-option \ diff --git a/tests/ls/getxattr-speedup b/tests/ls/getxattr-speedup new file mode 100755 index 0000000..2b7a1f3 --- /dev/null +++ b/tests/ls/getxattr-speedup @@ -0,0 +1,64 @@ +#!/bin/sh +# Show that we've eliminated most of ls' failing getxattr syscalls, +# regardless of how many files are in a directory we list. +# This test is skipped on systems that lack LD_PRELOAD support; that's fine. +# Similarly, on a system that lacks getxattr altogether, skipping it is fine. + +# Copyright (C) 2012 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program 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 General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. "${srcdir=.}/init.sh"; path_prepend_ ../src +print_ver_ ls + +# Replace each getxattr and lgetxattr call with a call to these stubs. +# Count those and write the total number of calls to the file "x" +# via a global destructor. +cat > k.c <<'EOF' || framework_failure_ +#include <errno.h> +#include <stdio.h> + +static unsigned long int n_calls; + +static void __attribute__ ((destructor)) +print_call_count (void) +{ + FILE *fp = fopen ("x", "w"); if (!fp) return; + fprintf (fp, "%lu\n", n_calls); fclose (fp); +} + +static ssize_t incr () { ++n_calls; errno = ENOTSUP; return -1; } +ssize_t getxattr (const char *path, const char *name, void *value, size_t size) +{ return incr (); } +ssize_t lgetxattr(const char *path, const char *name, void *value, size_t size) +{ return incr (); } +EOF + +# Then compile/link it: +$CC -fPIC -O2 -c k.c || framework_failure_ 'failed to compile with -fPIC' +ld -G k.o -lc -o k.so || framework_failure_ 'failed to invoke ld -G ...' + +# Create a few files: +seq 20 | xargs touch || framework_failure_ + +# Finally, run the test: +LD_PRELOAD=./k.so ls --color=always -l . || fail=1 + +test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?" + +# Ensure that there were no more than 3 *getxattr calls. +n_calls=$(cat x) +test "$n_calls" -le 3 || fail=1 + +Exit $fail -- 1.7.9.1.245.gbbe07
