Remainder :-)
Tito
farmatito at tiscali.it
Sun Sep 28 07:41:08 UTC 2008
On Sunday 28 September 2008 01:11:45 Denys Vlasenko wrote:
> On Saturday 27 September 2008 17:06, Tito wrote:
> > On Saturday 27 September 2008 16:45:47 you wrote:
> > > On Saturday 27 September 2008 16:00, Tito wrote:
> > > > Hi Denys,
> > > > maybe this patches slipped through as my cc to you get
> > > > bounced by a spam filter that blacklisted my ip?
> > >
> > > I received them. sorry about this delay.
> > >
> > > > # [PATCH 1/3] add bb_getgrouplist_malloc to libbb Tito
> > > > http://www.busybox.net/lists/busybox/2008-September/033078.html
> > > > # [PATCH 2/3] port id to bb_getgrouplist_malloc + fixes Tito
> > > > http://www.busybox.net/lists/busybox/2008-September/033079.html
> > >
> > > Let's look at these two patches together.
> > >
> > > Here:
> > >
> > > if (username) {
> > > -#if HAVE_getgrouplist
> > > - int m;
> > > -#endif
> > > p = getpwnam(username);
> > > /* xuname2uid is needed because it exits on failure */
> > > uid = xuname2uid(username);
> > > gid = p->pw_gid; /* in this case PRINT_REAL is the same */
> > > -
> > > -#if HAVE_getgrouplist
> > > - n = 16;
> > > - groups = NULL;
> > > - do {
> > > - m = n;
> > > - groups = xrealloc(groups, sizeof(groups[0]) * m);
> > > - getgrouplist(username, gid, groups, &n); /* GNUism? */
> > > - } while (n > m);
> > > -#endif
> > > - } else {
> > > -#if HAVE_getgrouplist
> > > - n = getgroups(0, NULL);
> > > - groups = xmalloc(sizeof(groups[0]) * n);
> > > - getgroups(n, groups);
> > > -#endif
> > > }
> > > + group_list = bb_getgrouplist_malloc(uid, gid, NULL);
> > >
> > > you replace getgrouplist() call with bb_getgrouplist_malloc().
> > > bb_getgrouplist_malloc() iterates through the whole set of group records:
> > >
> > > + while((grp = getgrent())) {
> > > ...
> > > + while (*(grp->gr_mem)) {
> > > ...
> > > + }
> > > + }
> > >
> > > This may be very expensive. I know for the fact than in big organizations
> > > some groups are HUGE - I saw 500kbytes large "guests" group.
> > > (nscd died horribly trying to digest such a large group.)
> > >
> > > In these cases, user db is not in /etc/XXX files. It is in LDAP etc.
> > >
> > > getgrouplist() may have a more clever way of retrieving this data -
> > > instead of pulling megabytes from user datrabase by iterating through
> > > all groups it may just directly query uer db "give me group list
> > > of this user". Likely to be less than kilobyte of data.
> > >
> > > There is no way around it. If you want to make sure you have at least
> > > a fleeting chance of not performing horribly, you must use libc interfaces
> > > fro retrieving group lists, of which there is two known to me:
> > > initgroups (standard, but useless in many ceses) and getgrouplist (GNUism).
> > > Then, *if* your libc is well-written, it will hopefully do it efficiently.
> >
> > Hi,
> > getgrent in fact is used only if getgrouplist is not available as suggested by Bernard,
> > see #ifdef HAVE_getgrouplist
>
> I missed that.
>
> > > Secondly, make bloatcheck says:
> > >
> > > function old new delta
> > > bb_getgrouplist_malloc - 192 +192
> > > print_single - 106 +106
> > > print_group_list - 94 +94
> > > decode_format_string 824 839 +15
> > > ...(deleted gcc-induced random jitter +/-9 bytes)...
> > > printf_full 44 - -44
> > > id_main 539 331 -208
> > > ------------------------------------------------------------------------------
> > > (add/remove: 3/1 grow/shrink: 4/4 up/down: 418/-259) Total: 159 bytes
> > >
> > > This does not seem to be a progress.
>
> > I suppose +159 is with all 3 patches applied?
>
> This was with only two first patches. I retested it on glibc, where
> getgrouplist() is available:
>
> function old new delta
> bb_getgrouplist_malloc - 194 +194
> print_single - 106 +106
> print_group_list - 94 +94
> bbunpack 386 394 +8
> parse_command 1443 1440 -3
> .rodata 125866 125852 -14
> dpkg_main 2853 2836 -17
> printf_full 44 - -44
> id_main 541 331 -210
> ------------------------------------------------------------------------------
> (add/remove: 3/1 grow/shrink: 1/4 up/down: 402/-288) Total: 114 bytes
>
> > In this case bloat check is not so important as we are adding a function
> > to libbb that could be reused in other applets and the new function
> > has more features as the previous code: built in egid handling.
>
> It's difficult to see how much of the growth is because of fixes
> and how much because of bb_getgrouplist_malloc. Can you
> submit them separately, so that it is possible
> to measure them separately?
Please, take a look at http://www.busybox.net/lists/busybox/2008-September/033175.html
it has a reworked version of id with bb_getgrouplist_malloc (moved back to id.c) and all the fixes
and some more size reduction.
scripts/bloat-o-meter busybox_old busybox_unstripped
function old new delta
print_single - 129 +129
print_group_list - 84 +84
.rodata 119055 119046 -9
printf_full 44 - -44
id_main 539 393 -146
------------------------------------------------------------------------------
(add/remove: 2/1 grow/shrink: 0/2 up/down: 213/-199) Total: 14 bytes
> Meanwhile, I propose adding getgrouplist() to uclibc. See attached.
That's fine!!!
Ciao,
Tito
> --
> vda
>
More information about the busybox
mailing list