Re: question about mgetgroups()

[Pádraig Brady]

On 05/26/2014 09:00 PM, Denis Excoffier wrote:
> Hello,
> 
> In mgetgroups.c, you can read the following piece of code:
> 
> --------------------------------------------------
> if (username)
>   {
>     enum { N_GROUPS_INIT = 10 };
>     max_n_groups = N_GROUPS_INIT;
> 
>     g = realloc_groupbuf (NULL, max_n_groups);
>     if (g == NULL)
>       return -1;
> 
>     while (1)
>       {
>         gid_t *h;
>         int last_n_groups = max_n_groups;
> 
>         /* getgrouplist updates max_n_groups to num required.  */
>         ng = getgrouplist (username, gid, g, &max_n_groups);
> 
>         /* Some systems (like Darwin) have a bug where they
>            never increase max_n_groups.  */
>         if (ng < 0 && last_n_groups == max_n_groups)
>           max_n_groups *= 2;
> 
>         if ((h = realloc_groupbuf (g, max_n_groups)) == NULL)
>           {
>             int saved_errno = errno;
>             free (g);
>             errno = saved_errno;
>             return -1;
>           }
>         g = h;
> 
>         if (0 <= ng)
>           {
>             *groups = g;
>             /* On success some systems just return 0 from getgrouplist,
>                so return max_n_groups rather than ng.  */
>             return max_n_groups;
>           }
>       }
>   }
> --------------------------------------------------
> 
> Is it really necessary to realloc g when ng >= 0? The answer
> could possibly be "Yes, in order to match with the exact number
> of groups found", in particular in the Darwin case, where for
> exemple 79 groups could remain unused when 81 are needed (here,
> N_GROUPS_INIT=10).
> 
> Please observe however that (except in the Darwin case) the
> realloc at the second (and last) iteration does not change the
> allocated size. Therefore, the realloc() + "g = h" assignment
> could possibly be surrounded by
>     if (last_n_groups != max_n_groups)
>       {
>         ...
>       }

When I wrote that code in coreutils I was simplifying it,
thus trying to avoid extra conditions, and noticed the fact that glibc will 
just ignore the realloc when
passed the same size (see last line of: 
http://www.gnu.org/software/libc/manual/html_node/Changing-Block-Size.html)
I.E. I was going for as simple as possible while not necessarily being as
fast as possible on non glibc.

But the question is fair and rather than adding a comment,
I'd add the more general condition you have above.

> or even by
>     if (ng < 0 && last_n_groups != max_n_groups)
>       {
>         ...
>       }
> if the answer to the first question is, in fact, "No".

This is done to reduce the allocation when the number of groups is less than 10.

> Or am i missing something? This has some (rather small) impact in 
> coreutils/id.

thanks,
Pádraig.