Markus Duft wrote: > On 09/08/11 10:17, Jim Meyering wrote: >> Markus Duft wrote: >> > [snip] >> >> A simpler approach might be ok: add a test for existence of the >> _nomembers functions (in m4/jm-macros.m4), and then add code like >> this for each in system.h, inserted after the declaration of getgrgid: >> >> /* Add a comment here, justifying this, based on what you >> said above. */ >> #if HAVE_GETGRGID_NOMEMBERS && ! HAVE_SETGROUPS >> # define getgrgid(n) getgrgid_nomembers(n) >> #endif >> >> #if HAVE_GETGRNAM_NOMEMBERS && ! HAVE_SETGROUPS >> ... >> >> Please include a complete patch that I can apply with >> "git am FILE", per instructions in HACKING: >> >> http://git.sv.gnu.org/cgit/coreutils.git/tree/HACKING > > ok.
Thanks. I've pointed out a few issues, hoping you'll adjust accordingly and resubmit. > 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. ... >>> diff -ru -x '*.o' coreutils-8.12.193-d8dc8.orig/src/chroot.c >>> coreutils-8.12.193-d8dc8/src/chroot.c >> >> I suggested to use #if ! HAVE_SETGROUPS, yet you used #ifdef __INTERIX. >> Why? ... > Date: Thu, 8 Sep 2011 11:18:17 +0200 > Subject: [PATCH 1/2] add check for setgroups(), which may be missing on some > platforms. Please adjust the one-line summary: build: accommodate missing setgroups on Interix > this adds the check plus a dummy, non-functional, always-successful > replacement function, to keep the original code untouched and simple. > > Signed-off-by: Markus Duft <[email protected]> Please omit the "Signed-off-by" line. You're the author, so that is redundant. I'll write the ChangeLog entries, if necessary, but I prefer that contributors give it a shot. * m4/jm-macros.m4: Check for setgroups. * src/chroot.c (setgroups) [! HAVE_SETGROUPS]: Define. > configure.ac | 2 +- > src/chroot.c | 11 +++++++++++ > 2 files changed, 12 insertions(+), 1 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 291b19e..c22f409 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -199,7 +199,7 @@ if test $utils_cv_localtime_cache = yes; then > fi > > # SCO-ODT-3.0 is reported to need -los to link programs using initgroups > -AC_CHECK_FUNCS([initgroups]) > +AC_CHECK_FUNCS([initgroups setgroups]) Please don't add it here, since it is unrelated to this ancient SCO hack. Add it on a separate line (in jm-macros.m4), with its own comment. > if test $ac_cv_func_initgroups = no; then > AC_CHECK_LIB([os], [initgroups]) > fi > diff --git a/src/chroot.c b/src/chroot.c > index 95c227b..a9b4b87 100644 > --- a/src/chroot.c > +++ b/src/chroot.c > @@ -52,6 +52,17 @@ static struct option const long_opts[] = > {NULL, 0, NULL, 0} > }; > > +#ifndef HAVE_SETGROUPS Please use the syntax I suggested: #if ! HAVE_SETGROUPS > +/* Some systems (like interix) lack supplemental group support. A dummy, > + * always successful replacement is added here to avoid the need to check > + * for setgroups availability everywhere, just to support broken platforms. > */ Adjusted for grammar and "voice" (avoid "passive". Prefer "active voice") I.e., instead of "___ is added", say "Define ___". Also for no-leading-"*"-on-each-continued-comment-line. /* At least Interix lacks supplemental group support. Define an always-successful replacement to avoid checking for setgroups availability everywhere, just to support broken platforms. */ > +static int setgroups(size_t size, gid_t const *list) > +{ > + (void)size; (void)list; > + return 0; > +} Formatting should match that used in the rest of the file: (and what I suggested initially) static int setgroups (size_t size, gid_t const *list) { (void) size; (void) list; return 0; } > +#endif > + > /* Call setgroups to set the supplementary groups to those listed in GROUPS. > GROUPS is a comma separated list of supplementary groups (names or > numbers). > Parse that list, converting any names to numbers, and call setgroups on > the > -- > 1.7.6.1 > > >>From 1ef299a66faa06becdd9bdb30ee6736a823e184b Mon Sep 17 00:00:00 2001 > From: Markus Duft <[email protected]> > Date: Thu, 8 Sep 2011 13:09:27 +0200 > Subject: [PATCH 2/2] add support for getgr*_nomembers functions on interix. > > interix provides faster replacements for getgr{gid,nam,ent} where > group member information is not fetched from domain controllers. > this makes 'id' usable on domain controlled interix boxes. > > Signed-off-by: Markus Duft <[email protected]> > --- > m4/jm-macros.m4 | 9 +++++++++ > src/system.h | 13 +++++++++++++ > 2 files changed, 22 insertions(+), 0 deletions(-) > > diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4 > index 58b000d..a6de8f1 100644 > --- a/m4/jm-macros.m4 > +++ b/m4/jm-macros.m4 > @@ -89,6 +89,15 @@ AC_DEFUN([coreutils_MACROS], > tcgetpgrp \ > ) > > + # those checks exists for interix, where there are blazingly fast > + # replacements for some functions, that don't query the domain > + # controller for user information where it is not needed. > + AC_CHECK_FUNCS_ONCE( \ > + getgrgid_nomembers \ > + getgrnam_nomembers \ > + getgrent_nomembers \ > + ) > + > dnl This can't use AC_REQUIRE; I'm not quite sure why. > cu_PREREQ_STAT_PROG > > diff --git a/src/system.h b/src/system.h > index 107dbd5..3e44f89 100644 > --- a/src/system.h > +++ b/src/system.h > @@ -213,6 +213,19 @@ struct passwd *getpwuid (); > struct group *getgrgid (); > #endif > > +/* Interix has replacements for getgr{gid,nam,ent}, that don't > + * query the domain controller for group members when not required. > + * This speeds up the calls tremendously (<1 ms vs. >3 s), so use them. */ > +#if HAVE_GETGRGID_NOMEMBERS > +# define getgrgid(gid) getgrgid_nomembers(gid) > +#endif > +#if HAVE_GETGRNAM_NOMEMBERS > +# define getgrnam(nam) getgrnam_nomembers(nam) > +#endif > +#if HAVE_GETGRENT_NOMEMBERS > +# define getgrent() getgrent_nomembers() > +#endif > + > #if !HAVE_DECL_GETUID > uid_t getuid (); > #endif
