On 02/23/2016 02:40 PM, Jakub Hrozek wrote:
On Fri, Feb 19, 2016 at 02:16:15PM +0100, Pavel Březina wrote:
> Fixes:
>
https://fedorahosted.org/sssd/ticket/2869
>
https://fedorahosted.org/sssd/ticket/2848
> From 681a8ff812f5af8adaed92bf1fd0248be7c2bda0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Fri, 12 Feb 2016 11:58:32 +0100
> Subject: [PATCH 01/12] cache_req: bring together search parameters
ACK
I think this would play nicer with the sysdb refactoring, too.
> From 0b5f044357a4f5d0b8d8b35c4ab2c2d0a7495bcf Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Fri, 12 Feb 2016 12:12:12 +0100
> Subject: [PATCH 02/12] cache_req: fix typo in debug message
ACK
> From 457757b2f7fd4c2dbd061aa779dc0f576eba0975 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Fri, 12 Feb 2016 12:20:04 +0100
> Subject: [PATCH 03/12] cache_req: break cache_req_input_create into more
> functions
ACK to the changes, but I wonder if it's correct to say "Bug" if the
input ID is 0, do you think an upper layer should check for that instead
and reject the zero ID with a more graceful message?
Let's keep it this way for the time being and deal with it when we
convert nss responder. From a quick look I can't see how it is handled
there but it will be clear when I dig into the code when changing it.
But we should definitely deal with this inside cache_req.
Also please
remember we'll support the root user one day..
> From 87972b1c7b7d74b052953e425cb532801bd51526 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Fri, 12 Feb 2016 12:26:16 +0100
> Subject: [PATCH 04/12] cache_req: rename debug_fqn to debugobj
ACK
> From 429023b7fe38b6a46d5ecaf4c345591e173eb87d Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Fri, 12 Feb 2016 13:12:34 +0100
> Subject: [PATCH 05/12] cache_req: improve debugging
ACK, I like this.
> From c38cae6eef560028c54ad135b4f3a4bead30263f Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Tue, 16 Feb 2016 13:14:00 +0100
> Subject: [PATCH 06/12] cache_req tests: remove unused users and groups
ACK
> 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.
> 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.
> From bd5957798c785ea0668fe3b74638e07dd0cfbe03 Mon Sep 17
00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Mon, 15 Feb 2016 11:57:02 +0100
> Subject: [PATCH 10/12] utils: return const char ** from dup_string_list
I think the reason we had char* and not const char * is that you don't have
to discard_const if you talloc_free() the result directly (as opposed to
freeing the parent).
> From b90e9bf4ec7b7377cbfd8e12801d5c186f3c292d Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Fri, 12 Feb 2016 14:28:33 +0100
> Subject: [PATCH 11/12] cache_req: add SID lookups
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2848
Just one question..
> input = cache_req_input_create(mem_ctx, rctx, CACHE_REQ_USER_BY_NAME,
> - name, 0, NULL);
> + name, 0, NULL, NULL, NULL);
Seeing the number of parametrs increase, what do you think about merging
them to an union and passing a pointer to a struct as an input instead? Each
CACHE_REQ_* maps to exactly one input type after all..
union is a no go, since the code depends that all possible attributes
are accessible on few places. I tried to use union first but it turned
out that it just complicates the code significantly so I sticked with
struct. And also using struct gives us the ability to actually check
that the input is valid. But passing a struct instead of parameters is a
good idea, I'm posting it as another patch.
> 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?