Re: Fwd: Re: [Platform-testers] new snapshot available: coreutils-8.12.178-df9cd

[Jim Meyering]

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 <address@hidden>

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 <address@hidden>
> 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 <address@hidden>
> ---
>  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