On 02/26/2016 01:47 PM, Jakub Hrozek wrote:
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?= <[email protected]>
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.
I'm sorry, I can't process those two negatives :-) Does it mean I should
move it to tests or to leave the patch as is?
From 7b86bf36da117f86c4390d3b56e2f6065754d16b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]>
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?= <[email protected]>
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?= <[email protected]>
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?= <[email protected]>
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?= <[email protected]>
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?= <[email protected]>
Date: Wed, 24 Feb 2016 12:37:51 +0100
Subject: [PATCH 15/15] cache_req: remove old comment
ACK
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
