Re: [Freeipa-devel] [PATCH] Improve performance of get_group_sids()

2012-07-13 Thread Simo Sorce
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
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] [PATCH] Improve performance of get_group_sids()

2012-07-10 Thread Sumit Bose
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.

bye,
Sumit
From a70dd5049943ae88aba46ef3e95b06a944efcf60 Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Fri, 6 Jul 2012 12:24:01 +0200
Subject: [PATCH] Add unit tests for ipa-kdb

To improve the performance of some functions used for group filtering we
need a way to see if changes really help or not. This patch adds unit
tests for some of these functions and measures the execution time.
---
 daemons/ipa-kdb/Makefile.am |   29 +++
 daemons/ipa-kdb/ipa_kdb_mspac.c |   12 +-
 daemons/ipa-kdb/ipa_kdb_tests.c |  548 +++
 util/ipa_pwd.c  |2 +
 4 Dateien geändert, 587 Zeilen hinzugefügt(+), 4 Zeilen entfernt(-)
 create mode 100644 daemons/ipa-kdb/ipa_kdb_tests.c

diff --git a/daemons/ipa-kdb/Makefile.am b/daemons/ipa-kdb/Makefile.am
index 
17c090418ec5a0e2a39d948dc385d509c5d05321..34b7d93c4912f988130ae7dd143e054574605071
 100644
--- a/daemons/ipa-kdb/Makefile.am
+++ b/daemons/ipa-kdb/Makefile.am
@@ -50,6 +50,35 @@ ipadb_la_LIBADD =\
$(NDRPAC_LIBS)  \
$(NULL)
 
+if HAVE_CHECK
+TESTS = ipa_kdb_tests
+check_PROGRAMS = ipa_kdb_tests
+endif
+
+ipa_kdb_tests_SOURCES =\
+   ipa_kdb_tests.c \
+   ipa_kdb.c   \
+   ipa_kdb_common.c\
+   ipa_kdb_mkey.c  \
+   ipa_kdb_passwords.c \
+   ipa_kdb_principals.c\
+   ipa_kdb_pwdpolicy.c \
+   ipa_kdb_mspac.c \
+   ipa_kdb_delegation.c\
+   ipa_kdb_audit_as.c  \
+   $(KRB5_UTIL_SRCS)   \
+   $(NULL)
+ipa_kdb_tests_CFLAGS = $(CHECK_CFLAGS)
+ipa_kdb_tests_LDADD =  \
+   $(CHECK_LIBS)   \
+   $(KRB5_LIBS)\
+   $(LDAP_LIBS)\
+   $(NDRPAC_LIBS)  \
+   -lnss3  \
+   -lkdb5  \
+   -lsss_idmap \
+   $(NULL)
+
 dist_noinst_DATA = ipa_kdb.exports
 
 EXTRA_DIST =   \
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 
1c7487c3c8f75d02466a2e0746fbef5d36e3d995..ef9aa96943a7e93c21c665e3292a9843fdd8e03d
 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -643,9 +643,9 @@ static char *gen_sid_string(TALLOC_CTX *memctx, struct 
dom_sid *dom_sid,
 return str;
 }
 
-static int get_group_sids(TALLOC_CTX *memctx,
-  struct PAC_LOGON_INFO_CTR *logon_info,
-  char ***_group_sids)
+int get_group_sids(TALLOC_CTX *memctx,
+   struct PAC_LOGON_INFO_CTR *logon_info,
+   char ***_group_sids)
 {
 int ret;
 size_t c;
@@ -653,6 +653,10 @@ static int get_group_sids(TALLOC_CTX *memctx,
 struct dom_sid *domain_sid = NULL;
 char **group_sids = NULL;
 
+if (logon_info == NULL || logon_info-info == NULL) {
+return EINVAL;
+}
+
 domain_sid = dom_sid_dup(memctx, logon_info-info-info3.base.domain_sid);
 if (domain_sid == NULL) {
 krb5_klog_syslog(LOG_ERR, dom_sid_dup failed);
@@ -714,7 +718,7 @@ done:
 return ret;
 }
 
-static int add_groups(TALLOC_CTX *memctx,
+int add_groups(TALLOC_CTX *memctx,
   struct PAC_LOGON_INFO_CTR *logon_info,
   size_t ipa_group_sids_count,
   struct dom_sid2 *ipa_group_sids)
diff --git a/daemons/ipa-kdb/ipa_kdb_tests.c b/daemons/ipa-kdb/ipa_kdb_tests.c
new file mode 100644
index 
..d3f6c2f38176e2fe3eb081b050c974f351f221ac
--- /dev/null
+++ b/daemons/ipa-kdb/ipa_kdb_tests.c
@@ -0,0 +1,548 @@
+/** BEGIN COPYRIGHT BLOCK
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/.
+ *
+ * Additional permission under GPLv3 section 7:
+ *
+ * In the following paragraph, GPL means the GNU General Public
+ * License, version 3 or any later version, and Non-GPL Code means
+ * code that is governed neither by the GPL nor