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!
