[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: getgroups improvements
From: |
Jim Meyering |
Subject: |
Re: getgroups improvements |
Date: |
Fri, 13 Nov 2009 10:14:43 +0100 |
Eric Blake wrote:
> These days, I'm not sure how many systems still need the getgroups replacement
> because the system getgroups is buggy. However, there is the issue of mingw,
> which lacks getgroups, and for that matter, any notion of group management
> (well, windows does have groups, as evidenced by the fact that cygwin is able
> to manage them; but mingw does not have any way to access or manipulate
> groups:
> there is no get[e]gid, and stat.st_gid is always 0). At any rate, it's nicer
> to guarantee that mingw can at least link with getgroups, even if it does
> nothing.
>
> The getgroups replacement had a logic bug, which made it fail if you had more
> than 20 supplemental groups. To prove this, I added a unit test, then tested
> with ac_cv_func_getgroups_works=no and an account that belongs to 46 groups.
>
> Meanwhile, autoconf declares GETGROUPS_T as either int or gid_t, but that type
> is probably obsolete since the number of modern porting systems that either
> don't declare getgroups or declare it with the wrong type is probably 0. But
> since we have the ability to do replacement functions, I'd rather have the
> _only_ use of GETGROUPS_T be in getgroups.c, rather than making everyone else
> have to use it. This is an API change, but only affects the rare platform
> where getgroups has the wrong type.
>
> getgroups replaces a library function, so it should not exit(). On the other
> hand, the mgetgroups interface from coreutils is much nicer to use (one call,
> instead of 2); that would be a reasonable place to add an xalloc-die
> dependency, but this patch does not do that.
>
> I will be posting a followup patch to the coreutils list for using this
> series.
>
> Any problems with committing this series?
>
> Eric Blake (5):
> getgroups: fix logic error
> getgroups: avoid calling exit
> getgroups: provide stub for mingw
> getgroups: don't expose GETGROUPS_T to user
> mgetgroups: new module, taken from coreutils
Good catch on that bug fix.
And hiding GETGROUPS_T is definitely the way to go.
These changes look fine. I'll test via coreutils, after you push.
I guess no one who used the replacement and who had users with 20 or
more groups ever noticed.
Thanks!