[Freeipa-devel] [PATCH] Improve performance of get_group_sids()
Sumit Bose
sbose at redhat.com
Thu Sep 27 20:21:31 UTC 2012
On Tue, Jul 10, 2012 at 11:04:20PM +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.
>
> bye,
> Sumit
> For some reasons I never got Simo's review mail and I just recently
> discovered it in the archive. His comments were:
>
> Nack, for minor issues.
>
> Patch1:
> - Please create a tests/ directory in daemons/ipa-kdb for the tests
done
>
> - Please fit the whole file within 79 columns, there are a number of
> lines towards the end of the tests that go beyond that.
>
done (I hope I haven't missed too many)
> - There is a spurious comment at the top:
> +/* talloc leak checks written by Martin Nagy for sssd */
removed
>
> - please add a dlinkslist.h file to /util instead of copying macros in
> the tests file
added together with ipa-kdb/tests/common_check.[ch]
>
> 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.
done
>
> - 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.
done, I took 8 (64bits) or did you meant to align on 64bytes to better align on
cache lines?
>
> - Please fit the whole file within 79 columns, there are a number of
> lines that go beyond that.
done
>
> - A Rid is a 32 bit unsigned, no need to cast to (unsigned long), just
> declare the format string as %u (unsigned)
I thought that int and unsigned are only guaranteed to have 16bit as minimal
size, but I would be happy to hear that newer Posix standards have increased
this to 32bits.
>
> - 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.
Added a new patch which shows the messages only if -DDEBUG is set.
bye,
Sumit
-------------- next part --------------
From c98b60d92c66718ea2eda8065c15130fc229bdcf Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Fri, 6 Jul 2012 12:24:01 +0200
Subject: [PATCH 36/37] 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 | 30 +++
daemons/ipa-kdb/ipa_kdb_mspac.c | 12 +-
daemons/ipa-kdb/tests/common_check.c | 110 +++++++++
daemons/ipa-kdb/tests/common_check.h | 48 ++++
daemons/ipa-kdb/tests/ipa_kdb_tests.c | 440 ++++++++++++++++++++++++++++++++++
util/dlinklist.h | 132 ++++++++++
6 Dateien ge?ndert, 768 Zeilen hinzugef?gt(+), 4 Zeilen entfernt(-)
create mode 100644 daemons/ipa-kdb/tests/common_check.c
create mode 100644 daemons/ipa-kdb/tests/common_check.h
create mode 100644 daemons/ipa-kdb/tests/ipa_kdb_tests.c
create mode 100644 util/dlinklist.h
diff --git a/daemons/ipa-kdb/Makefile.am b/daemons/ipa-kdb/Makefile.am
index 17c090418ec5a0e2a39d948dc385d509c5d05321..8cf719673a049d60111c67c3c99d807c952c34fc 100644
--- a/daemons/ipa-kdb/Makefile.am
+++ b/daemons/ipa-kdb/Makefile.am
@@ -50,6 +50,36 @@ ipadb_la_LIBADD = \
$(NDRPAC_LIBS) \
$(NULL)
+if HAVE_CHECK
+TESTS = ipa_kdb_tests
+check_PROGRAMS = ipa_kdb_tests
+endif
+
+ipa_kdb_tests_SOURCES = \
+ tests/ipa_kdb_tests.c \
+ tests/common_check.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 b5346fed1230d02a88c94ab913507112990a1651..e4c2a4d1b188b355605ada8f5f08fdb11d5adc02 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -659,9 +659,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;
@@ -669,6 +669,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");
@@ -730,7 +734,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/tests/common_check.c b/daemons/ipa-kdb/tests/common_check.c
new file mode 100644
index 0000000000000000000000000000000000000000..671c61655edeea635421fb6b7a3b304043dd8185
--- /dev/null
+++ b/daemons/ipa-kdb/tests/common_check.c
@@ -0,0 +1,110 @@
+/*
+ SSSD
+
+ Common utilities for check-based tests using talloc.
+
+ Authors:
+ Martin Nagy <mnagy at redhat.com>
+
+ Copyright (C) Red Hat, Inc 2009
+
+ 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/>.
+*/
+
+#include <stdio.h>
+#include <check.h>
+#include "tests/common_check.h"
+//#include "util/util.h"
+#include "dlinklist.h"
+
+TALLOC_CTX *global_talloc_context = NULL;
+
+
+struct size_snapshot {
+ struct size_snapshot *prev;
+ struct size_snapshot *next;
+
+ TALLOC_CTX *ctx;
+ size_t bytes_allocated;
+};
+
+static struct size_snapshot *snapshot_stack;
+
+void
+_check_leaks(TALLOC_CTX *ctx, size_t bytes, const char *location)
+{
+ size_t bytes_allocated;
+
+ bytes_allocated = talloc_total_size(ctx);
+ if (bytes_allocated != bytes) {
+ fprintf(stderr, "Leak report for %s:\n", location);
+ talloc_report_full(ctx, stderr);
+ fail("%s: memory leaks detected, %d bytes still allocated",
+ location, bytes_allocated - bytes);
+ }
+}
+
+void
+check_leaks_push(TALLOC_CTX *ctx)
+{
+ struct size_snapshot *snapshot;
+
+ snapshot = talloc(NULL, struct size_snapshot);
+ snapshot->ctx = ctx;
+ snapshot->bytes_allocated = talloc_total_size(ctx);
+ DLIST_ADD(snapshot_stack, snapshot);
+}
+
+void
+_check_leaks_pop(TALLOC_CTX *ctx, const char *location)
+{
+ struct size_snapshot *snapshot;
+ TALLOC_CTX *old_ctx;
+ size_t bytes_allocated;
+
+ if (snapshot_stack == NULL) {
+ fail("%s: trying to pop an empty stack");
+ }
+
+ snapshot = snapshot_stack;
+ DLIST_REMOVE(snapshot_stack, snapshot);
+
+ old_ctx = snapshot->ctx;
+ bytes_allocated = snapshot->bytes_allocated;
+
+ fail_if(old_ctx != ctx, "Bad push/pop order");
+
+ talloc_free(snapshot);
+ snapshot = NULL;
+ _check_leaks(old_ctx, bytes_allocated, location);
+}
+
+void
+leak_check_setup(void)
+{
+ talloc_enable_null_tracking();
+ global_talloc_context = talloc_new(NULL);
+ fail_unless(global_talloc_context != NULL, "talloc_new failed");
+ check_leaks_push(global_talloc_context);
+}
+
+void
+leak_check_teardown(void)
+{
+ check_leaks_pop(global_talloc_context);
+ if (snapshot_stack != NULL) {
+ fail("Exiting with a non-empty stack");
+ }
+ check_leaks(global_talloc_context, 0);
+}
diff --git a/daemons/ipa-kdb/tests/common_check.h b/daemons/ipa-kdb/tests/common_check.h
new file mode 100644
index 0000000000000000000000000000000000000000..a9b306e449149a37e68d6ee6cbe58f75db8a1bbc
--- /dev/null
+++ b/daemons/ipa-kdb/tests/common_check.h
@@ -0,0 +1,48 @@
+/*
+ SSSD
+
+ Common utilities for check-based tests using talloc.
+
+ Authors:
+ Martin Nagy <mnagy at redhat.com>
+
+ Copyright (C) Red Hat, Inc 2009
+
+ 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/>.
+*/
+
+#ifndef __TESTS_COMMON_H__
+#define __TESTS_COMMON_H__
+
+#include <talloc.h>
+#include "dlinklist.h"
+//#include "providers/data_provider.h"
+//#include "providers/ldap/sdap.h"
+
+extern TALLOC_CTX *global_talloc_context;
+
+#define check_leaks(ctx, bytes) _check_leaks((ctx), (bytes), __location__)
+void _check_leaks(TALLOC_CTX *ctx,
+ size_t bytes,
+ const char *location);
+
+void check_leaks_push(TALLOC_CTX *ctx);
+
+#define check_leaks_pop(ctx) _check_leaks_pop((ctx), __location__)
+void _check_leaks_pop(TALLOC_CTX *ctx, const char *location);
+
+void leak_check_setup(void);
+void leak_check_teardown(void);
+
+#endif /* !__TESTS_COMMON_H__ */
diff --git a/daemons/ipa-kdb/tests/ipa_kdb_tests.c b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
new file mode 100644
index 0000000000000000000000000000000000000000..e8941ae7c50f2d6d66b08329a73f5b5e99f08eb5
--- /dev/null
+++ b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
@@ -0,0 +1,440 @@
+/** 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 a license
+ * compatible with the GPL.
+ *
+ * You may link the code of this Program with Non-GPL Code and convey
+ * linked combinations including the two, provided that such Non-GPL
+ * Code only links to the code of this Program through those well
+ * defined interfaces identified in the file named EXCEPTION found in
+ * the source code files (the "Approved Interfaces"). The files of
+ * Non-GPL Code may instantiate templates or use macros or inline
+ * functions from the Approved Interfaces without causing the resulting
+ * work to be covered by the GPL. Only the copyright holders of this
+ * Program may make changes or additions to the list of Approved
+ * Interfaces.
+ *
+ * Authors:
+ * Sumit Bose <sbose at redhat.com>
+ *
+ * Copyright (C) 2012 Red Hat, Inc.
+ * All rights reserved.
+ * END COPYRIGHT BLOCK **/
+
+#include <check.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <talloc.h>
+#include <sss_idmap.h>
+
+#include "gen_ndr/ndr_krb5pac.h"
+
+#include "ipa_kdb.h"
+#include "tests/common_check.h"
+
+struct sss_idmap_ctx *global_idmap_ctx = NULL;
+
+#define DOM_SID "S-1-5-21-1234-5678-9012"
+#define OTHER_DOM_SID "S-1-5-21-1111-2222-3333"
+#define PRIMARY_GID 1234
+#define RID_BASE 10000
+
+int krb5_klog_syslog(int l, const char *format, ...)
+{
+ va_list ap;
+ char *s = NULL;
+ int ret;
+
+ va_start(ap, format);
+
+ ret = vasprintf(&s, format, ap);
+ va_end(ap);
+ if (ret < 0) {
+ /* ENOMEM */
+ return -1;
+ }
+
+ fprintf(stderr, "%s\n", s);
+ free(s);
+
+ return 0;
+}
+
+int get_group_sids(TALLOC_CTX *memctx,
+ struct PAC_LOGON_INFO_CTR *logon_info,
+ char ***_group_sids);
+
+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);
+
+static void *idmap_talloc(size_t size, void *pvt)
+{
+ return talloc_size(pvt, size);
+}
+
+static void idmap_free(void *ptr, void *pvt)
+{
+ talloc_free(ptr);
+}
+
+static int make_dom_sid_array(TALLOC_CTX *mem_ctx, size_t count,
+ struct dom_sid **_sids)
+{
+ size_t c;
+ char *sid_str;
+ enum idmap_error_code err;
+ struct dom_sid *sids;
+ struct dom_sid *sid;
+
+ sids = talloc_array(mem_ctx, struct dom_sid, count);
+ if (sids == NULL) {
+ return ENOMEM;
+ }
+
+ for (c = 0; c < count; c++) {
+ sid_str = talloc_asprintf(mem_ctx, "%s-%lu", OTHER_DOM_SID,
+ (unsigned long) RID_BASE + c);
+ if (sid_str == NULL) {
+ return ENOMEM;
+ }
+ err = sss_idmap_sid_to_smb_sid(global_idmap_ctx, sid_str, &sid);
+ talloc_free(sid_str);
+ if (err != IDMAP_SUCCESS) {
+ return EINVAL;
+ }
+ memcpy(&sids[c], sid, sizeof(struct dom_sid));
+ talloc_free(sid);
+ }
+
+ *_sids = sids;
+ return 0;
+}
+
+static int make_login_info(TALLOC_CTX *mem_ctx, size_t d_group_count,
+ size_t e_group_count,
+ struct PAC_LOGON_INFO_CTR **_logon_info)
+{
+ struct PAC_LOGON_INFO_CTR *logon_info;
+ enum idmap_error_code err;
+ size_t c;
+ char *sid_str;
+ struct dom_sid *sid;
+
+ logon_info = talloc_zero(mem_ctx, struct PAC_LOGON_INFO_CTR);
+ if (logon_info == NULL) {
+ return ENOMEM;
+ }
+
+ logon_info->info = talloc_zero(logon_info, struct PAC_LOGON_INFO);
+ if (logon_info->info == NULL) {
+ return ENOMEM;
+ }
+
+ err = sss_idmap_sid_to_smb_sid(global_idmap_ctx, DOM_SID, &sid);
+ if (err != IDMAP_SUCCESS) {
+ return EINVAL;
+ }
+ logon_info->info->info3.base.domain_sid = talloc_steal(logon_info->info,
+ sid);
+
+ logon_info->info->info3.base.primary_gid = PRIMARY_GID;
+
+ if (d_group_count > 0) {
+ logon_info->info->info3.base.groups.count = d_group_count;
+ logon_info->info->info3.base.groups.rids = talloc_array(logon_info,
+ struct samr_RidWithAttribute,
+ d_group_count);
+ if (logon_info->info->info3.base.groups.rids == NULL) {
+ return ENOMEM;
+ }
+
+ for (c = 0; c < d_group_count; c++) {
+ logon_info->info->info3.base.groups.rids[c].rid = RID_BASE + c;
+ logon_info->info->info3.base.groups.rids[c].attributes = 0;
+ }
+ }
+
+ if (e_group_count > 0) {
+ logon_info->info->info3.sidcount = e_group_count;
+ logon_info->info->info3.sids = talloc_array(logon_info,
+ struct netr_SidAttr,
+ e_group_count);
+ if (logon_info->info->info3.sids == NULL) {
+ return ENOMEM;
+ }
+
+ for (c = 0; c < e_group_count; c++) {
+ sid_str = talloc_asprintf(mem_ctx, "%s-%lu", OTHER_DOM_SID,
+ (unsigned long) RID_BASE + c);
+ if (sid_str == NULL) {
+ return ENOMEM;
+ }
+ err = sss_idmap_sid_to_smb_sid(global_idmap_ctx, sid_str, &sid);
+ talloc_free(sid_str);
+ if (err != IDMAP_SUCCESS) {
+ return EINVAL;
+ }
+ logon_info->info->info3.sids[c].sid = talloc_steal(logon_info->info,
+ sid);
+ logon_info->info->info3.sids[c].attributes = 0;
+ }
+ }
+
+ *_logon_info = logon_info;
+ return 0;
+}
+
+static int check_groups(size_t d_group_count, size_t e_group_count,
+ char **group_sids)
+{
+ int ret;
+ TALLOC_CTX *tmp_ctx;
+ char *tmp_str = NULL;
+ size_t c;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ tmp_str = talloc_asprintf(tmp_ctx, "%s-%d", DOM_SID, PRIMARY_GID);
+ if (tmp_str == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ if (strcmp(group_sids[0], tmp_str) != 0) {
+ ret = EINVAL;
+ goto done;
+ }
+
+ for (c = 0; c < d_group_count; c++) {
+ tmp_str = talloc_asprintf(tmp_ctx, "%s-%lu", DOM_SID,
+ (unsigned long) RID_BASE + c);
+ if (tmp_str == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ if (strcmp(group_sids[1 + c], tmp_str) != 0) {
+ fprintf(stderr, "[%s][%s]\n", group_sids[1 + c], tmp_str);
+ ret = EINVAL;
+ goto done;
+ }
+ }
+
+ for (c = 0; c < e_group_count; c++) {
+ tmp_str = talloc_asprintf(tmp_ctx, "%s-%lu", OTHER_DOM_SID,
+ (unsigned long) RID_BASE + c);
+ if (tmp_str == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ if (strcmp(group_sids[1 + d_group_count + c], tmp_str) != 0) {
+ fprintf(stderr, "[%s][%s]\n", group_sids[1 + d_group_count + c],
+ tmp_str);
+ ret = EINVAL;
+ goto done;
+ }
+ }
+
+ ret = 0;
+
+done:
+ talloc_free(tmp_ctx);
+ return ret;
+}
+
+struct get_groups_test_params {
+ size_t dom_group_count;
+ size_t external_group_count;
+};
+
+START_TEST(test_get_groups)
+{
+ struct PAC_LOGON_INFO_CTR *logon_info = NULL;
+ char **group_sids = NULL;
+ int ret;
+ size_t c;
+ struct timeval start_time;
+ struct timeval end_time;
+ struct timeval diff_time;
+ struct get_groups_test_params params[] = {
+ {0, 0},
+ {1000, 0},
+ {10000, 0},
+ {100000, 0},
+ {1000000, 0},
+ {0, 1000},
+ {0, 10000},
+ {0, 100000},
+ {0, 1000000},
+ {1000, 1000},
+ {10000, 10000},
+ {100000, 100000},
+ {1000000, 1000000},
+ {(size_t) -1, (size_t) -1}
+ };
+
+ for (c = 0; params[c].dom_group_count != (size_t) - 1; c++) {
+ ret = make_login_info(global_talloc_context, params[c].dom_group_count,
+ params[c].external_group_count, &logon_info);
+ fail_unless(ret == 0, "make_login_info failed.");
+
+ ret = gettimeofday(&start_time, NULL);
+ fail_unless(ret == 0, "gettimeofday failed.");
+
+ ret = get_group_sids(global_talloc_context, logon_info, &group_sids);
+ fail_unless(ret == 0, "get_group_sids failed [%d][%s].",
+ ret, strerror(ret));
+
+ ret = gettimeofday(&end_time, NULL);
+ fail_unless(ret == 0, "gettimeofday failed.");
+
+ talloc_free(logon_info);
+ logon_info = NULL;
+
+ fail_unless(check_groups(params[c].dom_group_count,
+ params[c].external_group_count,
+ group_sids) == 0,
+ "Unexpected SIDs returned.");
+
+ talloc_free(group_sids);
+ group_sids = NULL;
+
+ timersub(&end_time, &start_time, &diff_time);
+ fprintf(stderr, "get_groups: domain groups [%d] external group [%d] " \
+ "duration [%ds %dus]\n",
+ params[c].dom_group_count,
+ params[c].external_group_count,
+ (int) diff_time.tv_sec,
+ (int) diff_time.tv_usec);
+
+ }
+
+}
+END_TEST
+
+struct add_groups_test_params {
+ size_t dom_group_count;
+};
+
+START_TEST(test_add_groups)
+{
+ struct PAC_LOGON_INFO_CTR *logon_info = NULL;
+ struct dom_sid *group_sids = NULL;
+ int ret;
+ size_t c;
+ struct timeval start_time;
+ struct timeval end_time;
+ struct timeval diff_time;
+ struct add_groups_test_params params[] = {
+ {0},
+ {1000},
+ {10000},
+ {100000},
+ {1000000},
+ {2000000},
+ {(size_t) -1}
+ };
+
+ for (c = 0; params[c].dom_group_count != (size_t) - 1; c++) {
+ ret = make_login_info(global_talloc_context, 10, 10, &logon_info);
+ fail_unless(ret == 0, "make_login_info failed.");
+
+ ret = make_dom_sid_array(global_talloc_context,
+ params[c].dom_group_count, &group_sids);
+ fail_unless(ret == 0, "make_dom_sid_array failed.");
+
+ ret = gettimeofday(&start_time, NULL);
+ fail_unless(ret == 0, "gettimeofday failed.");
+
+ ret = add_groups(global_talloc_context, logon_info,
+ params[c].dom_group_count, group_sids);
+ fail_unless(ret == 0, "add_groups failed.");
+
+ ret = gettimeofday(&end_time, NULL);
+ fail_unless(ret == 0, "gettimeofday failed.");
+
+ talloc_free(group_sids);
+ group_sids = NULL;
+ talloc_free(logon_info);
+ logon_info = NULL;
+
+ timersub(&end_time, &start_time, &diff_time);
+ fprintf(stderr, "add_groups: domain groups [%d] duration [%ds %dus]\n",
+ params[c].dom_group_count,
+ (int) diff_time.tv_sec,
+ (int) diff_time.tv_usec);
+ }
+
+}
+END_TEST
+
+static void idmap_setup(void)
+{
+ enum idmap_error_code err;
+
+ err = sss_idmap_init(idmap_talloc, global_talloc_context, idmap_free,
+ &global_idmap_ctx);
+ fail_unless(err == IDMAP_SUCCESS, "sss_idmap_init failed.");
+}
+
+static void idmap_teardown(void)
+{
+ talloc_free(global_idmap_ctx);
+ global_idmap_ctx = NULL;
+}
+
+Suite * ipa_kdb_suite(void)
+{
+ Suite *s = suite_create("IPA kdb");
+
+ TCase *tc_group = tcase_create("Group filtering");
+ tcase_add_checked_fixture(tc_group,
+ leak_check_setup,
+ leak_check_teardown);
+ tcase_add_checked_fixture(tc_group,
+ idmap_setup,
+ idmap_teardown);
+ tcase_add_test(tc_group, test_get_groups);
+ tcase_add_test(tc_group, test_add_groups);
+ suite_add_tcase(s, tc_group);
+
+ tcase_set_timeout(tc_group, 300);
+ return s;
+}
+
+int main(void)
+{
+ int number_failed;
+
+ Suite *s = ipa_kdb_suite ();
+ SRunner *sr = srunner_create (s);
+ srunner_run_all (sr, CK_VERBOSE);
+ number_failed = srunner_ntests_failed (sr);
+ srunner_free (sr);
+
+ return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
+}
diff --git a/util/dlinklist.h b/util/dlinklist.h
new file mode 100644
index 0000000000000000000000000000000000000000..fa4b722559185ab2eb35c57cd84c9cb61a30fdb9
--- /dev/null
+++ b/util/dlinklist.h
@@ -0,0 +1,132 @@
+/*
+ Unix SMB/CIFS implementation.
+ some simple double linked list macros
+ Copyright (C) Andrew Tridgell 1998
+
+ 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/>.
+*/
+
+/* To use these macros you must have a structure containing a next and
+ prev pointer */
+
+#ifndef _DLINKLIST_H
+#define _DLINKLIST_H
+
+
+/* hook into the front of the list */
+#define DLIST_ADD(list, p) \
+do { \
+ if (!(list)) { \
+ (list) = (p); \
+ (p)->next = (p)->prev = NULL; \
+ } else { \
+ (list)->prev = (p); \
+ (p)->next = (list); \
+ (p)->prev = NULL; \
+ (list) = (p); \
+ }\
+} while (0)
+
+/* remove an element from a list - element doesn't have to be in list. */
+#define DLIST_REMOVE(list, p) \
+do { \
+ if ((p) == (list)) { \
+ (list) = (p)->next; \
+ if (list) (list)->prev = NULL; \
+ } else { \
+ if ((p)->prev) (p)->prev->next = (p)->next; \
+ if ((p)->next) (p)->next->prev = (p)->prev; \
+ } \
+ if ((p) != (list)) (p)->next = (p)->prev = NULL; \
+} while (0)
+
+/* promote an element to the top of the list */
+#define DLIST_PROMOTE(list, p) \
+do { \
+ DLIST_REMOVE(list, p); \
+ DLIST_ADD(list, p); \
+} while (0)
+
+/* hook into the end of the list - needs a tmp pointer */
+#define DLIST_ADD_END(list, p, type) \
+do { \
+ if (!(list)) { \
+ (list) = (p); \
+ (p)->next = (p)->prev = NULL; \
+ } else { \
+ type tmp; \
+ for (tmp = (list); tmp->next; tmp = tmp->next) ; \
+ tmp->next = (p); \
+ (p)->next = NULL; \
+ (p)->prev = tmp; \
+ } \
+} while (0)
+
+/* insert 'p' after the given element 'el' in a list. If el is NULL then
+ this is the same as a DLIST_ADD() */
+#define DLIST_ADD_AFTER(list, p, el) \
+do { \
+ if (!(list) || !(el)) { \
+ DLIST_ADD(list, p); \
+ } else { \
+ p->prev = el; \
+ p->next = el->next; \
+ el->next = p; \
+ if (p->next) p->next->prev = p; \
+ }\
+} while (0)
+
+/* demote an element to the end of the list, needs a tmp pointer */
+#define DLIST_DEMOTE(list, p, type) \
+do { \
+ DLIST_REMOVE(list, p); \
+ DLIST_ADD_END(list, p, type); \
+} while (0)
+
+/* concatenate two lists - putting all elements of the 2nd list at the
+ end of the first list */
+#define DLIST_CONCATENATE(list1, list2, type) \
+do { \
+ if (!(list1)) { \
+ (list1) = (list2); \
+ } else { \
+ type tmp; \
+ for (tmp = (list1); tmp->next; tmp = tmp->next) ; \
+ tmp->next = (list2); \
+ if (list2) { \
+ (list2)->prev = tmp; \
+ } \
+ } \
+} while (0)
+
+/* insert all elements from list2 after the given element 'el' in the
+ * first list */
+#define DLIST_ADD_LIST_AFTER(list1, el, list2, type) \
+do { \
+ if (!(list1) || !(el) || !(list2)) { \
+ DLIST_CONCATENATE(list1, list2, type); \
+ } else { \
+ type tmp; \
+ for (tmp = (list2); tmp->next; tmp = tmp->next) ; \
+ (list2)->prev = (el); \
+ tmp->next = (el)->next; \
+ (el)->next = (list2); \
+ if (tmp->next != NULL) tmp->next->prev = tmp; \
+ } \
+} while (0);
+
+#define DLIST_FOR_EACH(p, list) \
+ for ((p) = (list); (p) != NULL; (p) = (p)->next)
+
+#endif /* _DLINKLIST_H */
--
1.7.11.4
-------------- next part --------------
From b0ff2e6260747a04bf68a9b9175ab2a5cdd403b5 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Fri, 6 Jul 2012 21:40:35 +0200
Subject: [PATCH 37/37] Improve performance of get_group_sids()
This patch tries to improve the performance of get_group_sids() by
reducing the number of memory allocations.
---
daemons/ipa-kdb/ipa_kdb_mspac.c | 112 ++++++++++++++--------------------------
1 Datei ge?ndert, 38 Zeilen hinzugef?gt(+), 74 Zeilen entfernt(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index e4c2a4d1b188b355605ada8f5f08fdb11d5adc02..a8f56f3a6ff376b74f69a01e64a2d3de7c1a80d2 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -86,6 +86,7 @@ static char *memberof_pac_attrs[] = {
#define SID_ID_AUTHS 6
#define SID_SUB_AUTHS 15
#define MAX(a,b) (((a)>(b))?(a):(b))
+#define ALIGN_BASE 8
static int string_to_sid(char *str, struct dom_sid *sid)
{
@@ -185,43 +186,6 @@ static char *dom_sid_string(TALLOC_CTX *memctx, const struct dom_sid *dom_sid)
return buf;
}
-static struct dom_sid *dom_sid_dup(TALLOC_CTX *memctx,
- const struct dom_sid *dom_sid)
-{
- struct dom_sid *new_sid;
- size_t c;
-
- if (dom_sid == NULL) {
- return NULL;
- }
-
- new_sid = talloc(memctx, struct dom_sid);
- if (new_sid == NULL) {
- return NULL;
- }
-
- new_sid->sid_rev_num = dom_sid->sid_rev_num;
- for (c = 0; c < SID_ID_AUTHS; c++) {
- new_sid->id_auth[c] = dom_sid->id_auth[c];
- }
- new_sid->num_auths = dom_sid->num_auths;
- for (c = 0; c < SID_SUB_AUTHS; c++) {
- new_sid->sub_auths[c] = dom_sid->sub_auths[c];
- }
-
- return new_sid;
-}
-
-static int sid_append_rid(struct dom_sid *sid, uint32_t rid)
-{
- if (sid->num_auths >= SID_SUB_AUTHS) {
- return EINVAL;
- }
-
- sid->sub_auths[sid->num_auths++] = rid;
- return 0;
-}
-
/**
* @brief Takes a user sid and removes the rid.
* The sid is changed by this function,
@@ -636,29 +600,6 @@ static bool is_cross_realm_krbtgt(krb5_const_principal princ)
return true;
}
-static char *gen_sid_string(TALLOC_CTX *memctx, struct dom_sid *dom_sid,
- uint32_t rid)
-{
- char *str = NULL;
- int ret;
-
- ret = sid_append_rid(dom_sid, rid);
- if (ret != 0) {
- krb5_klog_syslog(LOG_ERR, "sid_append_rid failed");
- return NULL;
- }
-
- str = dom_sid_string(memctx, dom_sid);
- ret = sid_split_rid(dom_sid, NULL);
- if (ret != 0) {
- krb5_klog_syslog(LOG_ERR, "sid_split_rid failed");
- talloc_free(str);
- return NULL;
- }
-
- return str;
-}
-
int get_group_sids(TALLOC_CTX *memctx,
struct PAC_LOGON_INFO_CTR *logon_info,
char ***_group_sids)
@@ -666,19 +607,28 @@ int get_group_sids(TALLOC_CTX *memctx,
int ret;
size_t c;
size_t p = 0;
- struct dom_sid *domain_sid = NULL;
+ char *dom_sid_str = NULL;
char **group_sids = NULL;
+ char *group_sids_buf = NULL;
+ size_t dom_sid_str_len;
+ size_t sid_len;
+ char *idx;
+
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");
+ dom_sid_str = dom_sid_string(memctx,
+ logon_info->info->info3.base.domain_sid);
+ if (dom_sid_str == NULL) {
+ krb5_klog_syslog(LOG_ERR, "dom_sid_string failed.");
ret = ENOMEM;
goto done;
}
+ dom_sid_str_len = strlen(dom_sid_str);
+ sid_len = dom_sid_str_len + 12;
+ sid_len = sid_len + ( ALIGN_BASE - ( sid_len % ALIGN_BASE));
group_sids = talloc_array(memctx, char *,
2 +
@@ -690,27 +640,41 @@ int get_group_sids(TALLOC_CTX *memctx,
goto done;
}
- group_sids[p] = gen_sid_string(memctx, domain_sid,
- logon_info->info->info3.base.primary_gid);
- if (group_sids[p] == NULL) {
- krb5_klog_syslog(LOG_ERR, "gen_sid_string failed");
+ group_sids_buf = talloc_zero_size(group_sids,
+ sid_len * (1 + logon_info->info->info3.base.groups.count));
+ if (group_sids_buf == NULL) {
+ krb5_klog_syslog(LOG_ERR, "talloc_zero failed");
+ ret = ENOMEM;
+ goto done;
+ }
+
+ idx = group_sids_buf + (p * sid_len);
+ memcpy(idx, dom_sid_str, dom_sid_str_len);
+ ret = snprintf(idx + dom_sid_str_len, 12, "-%lu",
+ (unsigned long) logon_info->info->info3.base.primary_gid);
+ if (ret < 0 || ret >= 12) {
+ krb5_klog_syslog(LOG_ERR, "snprintf failed");
ret = EINVAL;
goto done;
}
+ group_sids[p] = idx;
p++;
for (c = 0; c < logon_info->info->info3.base.groups.count; c++) {
- group_sids[p] = gen_sid_string(memctx, domain_sid,
- logon_info->info->info3.base.groups.rids[c].rid);
- if (group_sids[p] == NULL) {
- krb5_klog_syslog(LOG_ERR, "gen_sid_string 2 failed");
+ idx = group_sids_buf + (p * sid_len);
+ memcpy(idx, dom_sid_str, dom_sid_str_len);
+ ret = snprintf(idx + dom_sid_str_len, 12, "-%lu",
+ (unsigned long) logon_info->info->info3.base.groups.rids[c].rid);
+ if (ret < 0 || ret >= 12) {
+ krb5_klog_syslog(LOG_ERR, "snprintf failed");
ret = EINVAL;
goto done;
}
+ group_sids[p] = idx;
p++;
}
for (c = 0; c < logon_info->info->info3.sidcount; c++) {
- group_sids[p] = dom_sid_string(memctx,
+ group_sids[p] = dom_sid_string(group_sids,
logon_info->info->info3.sids[c].sid);
if (group_sids[p] == NULL) {
krb5_klog_syslog(LOG_ERR, "dom_sid_string failed");
@@ -726,7 +690,7 @@ int get_group_sids(TALLOC_CTX *memctx,
ret = 0;
done:
- talloc_free(domain_sid);
+ talloc_free(dom_sid_str);
if (ret != 0) {
talloc_free(group_sids);
}
--
1.7.11.4
-------------- next part --------------
From 87ce1bc1d0d7508f3ed79da58972d0e1cc8b1f11 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Thu, 27 Sep 2012 15:33:55 +0200
Subject: [PATCH] ipadb: Show debug messages only in a debug build
---
daemons/ipa-kdb/ipa_kdb_mspac.c | 56 ++++++++++++++++++++++++++---------------
1 Datei ge?ndert, 36 Zeilen hinzugef?gt(+), 20 Zeilen entfernt(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index a8f56f3a6ff376b74f69a01e64a2d3de7c1a80d2..b3cef97db1bfe120d00b3bbfd30ed392dbb7b385 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -42,9 +42,25 @@ struct ipadb_mspac {
struct ipadb_adtrusts *trusts;
};
-
+#ifdef DEBUG
int krb5_klog_syslog(int, const char *, ...);
+void debug_fn(const char *format, ...)
+{
+ va_list ap;
+
+ va_start(ap, format);
+
+ krb5_klog_syslog(LOG_ERR, format, ap);
+
+ va_end(ap);
+}
+
+#define KDB_DEBUG(body) debug_fn body
+#else
+#define KDB_DEBUG(body)
+#endif
+
static char *user_pac_attrs[] = {
"objectClass",
"uid",
@@ -622,7 +638,7 @@ int get_group_sids(TALLOC_CTX *memctx,
dom_sid_str = dom_sid_string(memctx,
logon_info->info->info3.base.domain_sid);
if (dom_sid_str == NULL) {
- krb5_klog_syslog(LOG_ERR, "dom_sid_string failed.");
+ KDB_DEBUG(("dom_sid_string failed."));
ret = ENOMEM;
goto done;
}
@@ -635,7 +651,7 @@ int get_group_sids(TALLOC_CTX *memctx,
logon_info->info->info3.base.groups.count +
logon_info->info->info3.sidcount);
if (group_sids == NULL) {
- krb5_klog_syslog(LOG_ERR, "talloc_array failed");
+ KDB_DEBUG(("talloc_array failed"));
ret = ENOMEM;
goto done;
}
@@ -643,7 +659,7 @@ int get_group_sids(TALLOC_CTX *memctx,
group_sids_buf = talloc_zero_size(group_sids,
sid_len * (1 + logon_info->info->info3.base.groups.count));
if (group_sids_buf == NULL) {
- krb5_klog_syslog(LOG_ERR, "talloc_zero failed");
+ KDB_DEBUG(("talloc_zero failed"));
ret = ENOMEM;
goto done;
}
@@ -653,7 +669,7 @@ int get_group_sids(TALLOC_CTX *memctx,
ret = snprintf(idx + dom_sid_str_len, 12, "-%lu",
(unsigned long) logon_info->info->info3.base.primary_gid);
if (ret < 0 || ret >= 12) {
- krb5_klog_syslog(LOG_ERR, "snprintf failed");
+ KDB_DEBUG(("snprintf failed"));
ret = EINVAL;
goto done;
}
@@ -666,7 +682,7 @@ int get_group_sids(TALLOC_CTX *memctx,
ret = snprintf(idx + dom_sid_str_len, 12, "-%lu",
(unsigned long) logon_info->info->info3.base.groups.rids[c].rid);
if (ret < 0 || ret >= 12) {
- krb5_klog_syslog(LOG_ERR, "snprintf failed");
+ KDB_DEBUG(("snprintf failed"));
ret = EINVAL;
goto done;
}
@@ -677,7 +693,7 @@ int get_group_sids(TALLOC_CTX *memctx,
group_sids[p] = dom_sid_string(group_sids,
logon_info->info->info3.sids[c].sid);
if (group_sids[p] == NULL) {
- krb5_klog_syslog(LOG_ERR, "dom_sid_string failed");
+ KDB_DEBUG(("dom_sid_string failed"));
ret = EINVAL;
goto done;
}
@@ -763,7 +779,7 @@ static int map_groups(TALLOC_CTX *memctx, krb5_context kcontext,
basedn = talloc_asprintf(memctx, "cn=groups,cn=accounts,%s", ipactx->base);
if (basedn == NULL) {
- krb5_klog_syslog(LOG_ERR, "talloc_asprintf failed.");
+ KDB_DEBUG(("talloc_asprintf failed."));
kerr = ENOMEM;
goto done;
}
@@ -773,7 +789,7 @@ static int map_groups(TALLOC_CTX *memctx, krb5_context kcontext,
filter = talloc_asprintf(memctx, "(&(objectclass=ipaExternalGroup)(ipaExternalMember=%s))",
group_sids[c]);
if (filter == NULL) {
- krb5_klog_syslog(LOG_ERR, "talloc_asprintf failed.");
+ KDB_DEBUG(("talloc_asprintf failed."));
kerr = ENOMEM;
goto done;
}
@@ -782,7 +798,7 @@ static int map_groups(TALLOC_CTX *memctx, krb5_context kcontext,
entry_attrs, deref_search_attrs,
memberof_pac_attrs, &results);
if (kerr != 0) {
- krb5_klog_syslog(LOG_ERR, "ipadb_deref_search failed.");
+ KDB_DEBUG(("ipadb_deref_search failed."));
goto done;
}
@@ -799,7 +815,7 @@ static int map_groups(TALLOC_CTX *memctx, krb5_context kcontext,
break;
case 0:
if (deref_results == NULL) {
- krb5_klog_syslog(LOG_ERR, "No results.");
+ KDB_DEBUG(("No results."));
break;
}
@@ -809,7 +825,7 @@ static int map_groups(TALLOC_CTX *memctx, krb5_context kcontext,
sids = talloc_realloc(memctx, sids, struct dom_sid, count);
if (sids == NULL) {
- krb5_klog_syslog(LOG_ERR, "talloc_realloc failed.");
+ KDB_DEBUG(("talloc_realloc failed."));
kerr = ENOMEM;
goto done;
}
@@ -838,9 +854,9 @@ static int map_groups(TALLOC_CTX *memctx, krb5_context kcontext,
if (gid != 0 && sid.sid_rev_num != 0) {
/* TODO: check if gid maps to sid */
if (sid_index >= count) {
- krb5_klog_syslog(LOG_ERR, "Index larger than "
+ KDB_DEBUG(("Index larger than "
"array, this shoould "
- "never happen.");
+ "never happen."));
kerr = EFAULT;
goto done;
}
@@ -911,7 +927,7 @@ static krb5_error_code add_local_groups(krb5_context context,
ret = add_groups(memctx, info, ipa_group_sids_count, ipa_group_sids);
if (ret != 0) {
- krb5_klog_syslog(LOG_ERR, "add_groups failed");
+ KDB_DEBUG(("add_groups failed"));
return KRB5_KDB_INTERNAL_ERROR;
}
@@ -996,11 +1012,11 @@ static krb5_error_code filter_logon_info(krb5_context context,
/* check netbios/flat name */
if (strcasecmp(info->info->info3.base.logon_domain.string,
domain->flat_name) != 0) {
- krb5_klog_syslog(LOG_ERR, "PAC Info mismatch: domain = %s, "
+ KDB_DEBUG(("PAC Info mismatch: domain = %s, "
"expected flat name = %s, "
"found logon name = %s",
domain->domain_name, domain->flat_name,
- info->info->info3.base.logon_domain.string);
+ info->info->info3.base.logon_domain.string));
return EINVAL;
}
@@ -1011,11 +1027,11 @@ static krb5_error_code filter_logon_info(krb5_context context,
}
if (strcmp(domsid, domain->domain_sid) != 0) {
- krb5_klog_syslog(LOG_ERR, "PAC Info mismatch: domain = %s, "
+ KDB_DEBUG(("PAC Info mismatch: domain = %s, "
"expected domain SID = %s, "
"found domain SID = %s",
domain->domain_name, domain->domain_sid,
- domsid);
+ domsid));
talloc_free(domsid);
return EINVAL;
}
@@ -1500,7 +1516,7 @@ krb5_error_code ipadb_mspac_get_trusted_domains(struct ipadb_context *ipactx)
done:
if (ret != 0) {
- krb5_klog_syslog(LOG_ERR, "Failed to read list of trusted domains");
+ KDB_DEBUG(("Failed to read list of trusted domains"));
}
free(base);
return ret;
--
1.7.11.4
More information about the Freeipa-devel
mailing list