On Tue, 2012-07-10 at 23:04 +0200, Sumit Bose wrote: > Hi, > > the following two patches are the first step to fix > https://fedorahosted.org/freeipa/ticket/2881. Unit tests with time > measurements are added and the performance of the get_group_sids() > function is improved by an order of magnitude. > > The caching of the LDAP results is still missing. I will finish this > after my PTO. But I haven't started to work on this. So if you think it > should be fixed earlier feel free to take the ticket.
Nack, for minor issues. Patch1: - Please create a tests/ directory in daemons/ipa-kdb for the tests - Please fit the whole file within 79 columns, there are a number of lines towards the end of the tests that go beyond that. - There is a spurious comment at the top: +/* talloc leak checks written by Martin Nagy for sssd */ - please add a dlinkslist.h file to /util instead of copying macros in the tests file Patch2: - Needs spacing in arithmetic operations. This line: + idx = group_sids_buf+(p*sid_len); should be: + idx = group_sids_buf + (p * sid_len); and other similar cases. - Should we align the sids in the big memory allocation you make ? + dom_sid_str_len = strlen(dom_sid_str); + sid_len = dom_sid_str_len + 12; this can result in any alignment, should we align32/64 sid_len ? It would waste a bit of space, but may be beneficial on some archs. - Please fit the whole file within 79 columns, there are a number of lines that go beyond that. - A Rid is a 32 bit unsigned, no need to cast to (unsigned long), just declare the format string as %u (unsigned) - Not introduced with this patch, but can you remove most of the debug stuff going to syslog ? I do not think we want to spam syslog with this low level stuff, we must not confuse debugging with logging like it happens in samba-land. If you really want to retain that stuff, wrap it around a macro that is enabled only with a debugging option passed to configure or make. Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ Freeipa-devel mailing list Freeipaemail@example.com https://www.redhat.com/mailman/listinfo/freeipa-devel