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?= <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.
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?= <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
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org