Markus Duft wrote: > On 09/08/11 14:00, Jim Meyering wrote: >> Markus Duft wrote: >>> On 09/08/11 10:17, Jim Meyering wrote: >>>> Markus Duft wrote: >>>> > [snip] >> >> Thanks. I've pointed out a few issues, hoping you'll adjust >> accordingly and resubmit. > > sure :) this exercised my git-rebase-foo a lot though ... > >> >>> setgroups is not needed here, as it something different. i have 2 >> >> I added that conjunct quite deliberately. >> If we make the compromise of not detecting the getgr* performance >> problems, then we need some other way to ensure that we use the >> _nomembers functions only when needed. Appending " && ! HAVE_SETGROUPS" >> was my way of saying to use this kludge only on a system that also has >> the defect of a missing setgroups function. Yes, that deserves a >> comment. > > hm. ok. i added and commented this, although it feels quite wrong for
As well it should. As I mentioned initially, a better way would be to have a configure-time run-test that would detect the losing getgr* functions and set a macro like SLOW_GETGR__FUNCTIONS in that case. Then, when/if Interix fixes them, that work-around code will stop being used all by itself. But run-tests are best avoided, since they cause trouble when cross-compiling, and besides anything that attempts to detect a performance problem like that is doomed to fail some of the time. So we compromise. > me. is there any platform that has the _nomembers functions? if yes, > would it do any harm to use them? idk... If some other system has those _nomembers functions, do you know that using them won't have some unpleasant side-effect? What if some other system has working getgr* functions *and* the _nomembers variants. Would you still want to use the _nomembers ones? I wouldn't. > heh. while looking for a location to add the check, i realized there > is one already! :) so i ommitted this step. hope thats ok, my build > goes through, but for obvious reasons this is not significant (as i'll > never have it defined). a quick grep over the build tree yields this: > > mduft coreutils-8.12.193-d8dc8 $ find . -type f | xargs grep SETGROUPS > ./autom4te.cache/traces.0:m4trace:configure.ac:46: -1- > AH_OUTPUT([HAVE_SETGROUPS], [/* Define to 1 if you have the > setgroups\' function. */ > ./autom4te.cache/traces.0:@%:@undef HAVE_SETGROUPS]) > ./ChangeLog-2006: * src/setuidgid.c (main) [! HAVE_SETGROUPS]: Don't > call setgroups. > ./lib/config.h:/* #undef HAVE_SETGROUPS */ > ./lib/config.hin:#undef HAVE_SETGROUPS > ./lib/config.hin~:#undef HAVE_SETGROUPS > ./src/chroot.c:#if ! HAVE_SETGROUPS > ./src/setuidgid.c:#if HAVE_SETGROUPS > ./src/system.h: other than interix, check for HAVE_SETGROUPS, as interix is > ./src/system.h:#if ! HAVE_SETGROUPS > >> > [snip] > > hope the new versions are acceptable. otherwise, i'll be happy to > adjust once more, and resubmit tomorrow :) Thanks. I'll review one more time and apply these post-release.
