On Fri, Feb 19, 2016 at 02:16:15PM +0100, Pavel Březina wrote:
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? 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..
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
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..
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.