On Wed, Feb 24, 2016 at 12:41:24PM +0100, Pavel Březina wrote:
>> From f61d0192b8254247802167ea385b52f65d4e175d Mon Sep 17
00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Thu, 18 Feb 2016 14:25:18 +0100
>>Subject: [PATCH 07/12] sysdb: reset ldb errors
>>
>>After ldb connect ldb context contains the following error:
>>"NULL Base DN invalid for a base search"
>>
>>This comes from internal ldb function ldb_set_default_dns() which
>>runs base search on NULL dn to discover records similar to what
>>rootDSE provides. However, tdb backend considers this an error
>>and sets the message above.
>>
>>This may break memory leak checks in tests when we do push/pop on
>>test_ctx which is a indirect parent of ldb_context. The error message
>>is allocated when push is called but it is freed by other ldb queries
>>and therefore not preset during the push phase and thus the leak check
>>fails.
>
>I know this fixed an error for you, but I wonder if it wouldn't be
>better to work around the issue or use this only in the test itself. The
>reason being that the function is only part of ldb_modules.h so it feels
>a bit hacky to use it outside an ldb module..
I am not aware of other simple way to reset the error. But maybe we can
run some simple ldb query that ought to be successful thus we can remove
ldb_module.h?
We can move it to the test (or to test domain setup) if you want.
It feels a bit cleaner to me to rely on a function that we're not
supposed to call only in tests and not in the deamon code.
>> From 7b86bf36da117f86c4390d3b56e2f6065754d16b Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Wed, 17 Feb 2016 10:07:18 +0100
>>Subject: [PATCH 08/12] cache_req tests: use leak check in test fixtures
>>
>>To ensure no memory is leak on long living context such as rctx.
>>
>>Resolves:
>>https://fedorahosted.org/sssd/ticket/2869
>
>ACK, but perhaps we'll have to amend this patch if we decide to drop the
>previous patch.
>
>> From 2375eb0a03c4d5fb094670fcd0a2e178f9a503c4 Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Fri, 19 Feb 2016 13:04:04 +0100
>>Subject: [PATCH 09/12] cache_req tests: improve user and group creation
>>
>>---
>> src/tests/cmocka/test_responder_cache_req.c | 257 +++++++++++++---------------
>
>There are some compilation warnings here:
> CC src/tests/cmocka/responder_cache_req_tests-test_responder_cache_req.o
>/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c: In
function ‘__wrap_sss_dp_get_account_send’:
>/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c:324:13:
warning: unused variable ‘ret’ [-Wunused-variable]
> errno_t ret;
> ^
>In file included from
/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c:21:0:
>/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c: In
function ‘test_user_by_name_multiple_domains_parse’:
>/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c:539:17:
warning: passing argument 1 of ‘_talloc_free’ discards ‘const’ qualifier from pointer
target type [-Wdiscarded-qualifiers]
> talloc_free(fqn);
> ^
>/usr/include/talloc.h:229:5: note: expected ‘void *’ but argument is of type ‘const
char *’
> int _talloc_free(void *ptr, const char *location);
> ^
>/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c: In
function ‘test_group_by_name_multiple_domains_parse’:
>/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c:1039:17:
warning: passing argument 1 of ‘_talloc_free’ discards ‘const’ qualifier from pointer
target type [-Wdiscarded-qualifiers]
> talloc_free(fqn);
> ^
>/usr/include/talloc.h:229:5: note: expected ‘void *’ but argument is of type ‘const
char *’
> int _talloc_free(void *ptr, const char *location);
> ^
> CCLD responder_cache_req-tests
>
>
>But the general intent looks good
It is interesting that I didn't get these warnings... fixed nevertheless.
I think it's because the Coverity checks already run GCC6. Anyway, the
latest Coverity run was clean.
>> From 4d4b87e40b25d1528aac2222d6a7444a32eae515 Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Fri, 19 Feb 2016 14:09:26 +0100
>>Subject: [PATCH 12/12] cache_req test: add lookup by sid
>
>LGTM, thanks for including the tests.
>
>Coverity returned clean.
Thank you for the review, see attached patches. I'm also inclined in
renaming 'cache_req_input' into 'cache_req' since the _input part has no
longer meaning. What do you think?
Fine by me.
From 4e66072760c5fa3b108fb19cbcc403ef100c944d Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 24 Feb 2016 12:15:51 +0100
Subject: [PATCH 13/15] cache_req: use struct instead of parameter list
---
src/responder/common/responder_cache_req.c | 91 ++++++++++++++++--------------
src/responder/common/responder_cache_req.h | 42 ++++++++++++--
src/responder/ifp/ifpsrv_cmd.c | 7 ++-
3 files changed, 92 insertions(+), 48 deletions(-)
[...]
index
a9388149170a486a509b1d92520d7fe8d21172d3..5b21d6d3e84469cbdb7223d467738ef156b94e5c 100644
--- a/src/responder/common/responder_cache_req.h
+++ b/src/responder/common/responder_cache_req.h
@@ -27,6 +27,42 @@
#include "responder/common/responder.h"
#include "responder/common/negcache.h"
+struct cache_req_data {
+ struct {
+ const char *input; /* Original input. */
+ const char *name; /* Parsed name or UPN. */
+ const char *lookup; /* Converted per domain rules. */
+ } name;
+ uint32_t id;
+ const char *cert;
+ const char *sid;
+ const char **attrs;
+};
+
+#define CACHE_REQ_DATA_INIT(data) \
+ memset((data), '\0', sizeof(struct cache_req_data))
What about using something like (untested):
#define CACHE_REQ_DATA_INIT { 0 }
so that we could use:
struct cache_req_data req_data = CACHE_REQ_DATA_INIT;
Then maybe we could not use the setter macros at all, but just
explicitly set the struct member.
+
+#define CACHE_REQ_DATA_SET_NAME(data, _name) do { \
+ CACHE_REQ_DATA_INIT(data); \
+ (data)->name.input = (_name); \
+} while (0)
In general I prefer functions over macros (there's little reason to use
macros in modern C IMO). At the very least, I would prefer to not use
underscrore, we normally use that for output parameters if you decide to
keep the macros.
From f3c749b25cff90dd851629c7d6424664e7ff52cb Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 24 Feb 2016 12:33:47 +0100
Subject: [PATCH 14/15] cache_req: hide cache_req_input
ACK
From e5770234954a05bdd0598687eb6cc1e061a530d9 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 24 Feb 2016 12:37:51 +0100
Subject: [PATCH 15/15] cache_req: remove old comment
ACK