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 > _______________________________________________ busybox mailing list [email protected] http://busybox.net/cgi-bin/mailman/listinfo/busybox
