On 03/09/2016 02:31 PM, Sumit Bose wrote:
>On Tue, Mar 01, 2016 at 01:05:48PM +0100, Pavel Březina wrote:
>>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.
>>
>>Moved to mock_domain in tests.
>>
>>>>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.
>>
>>Renamed.
>>
>>>>+
>>>>+#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.
>>
>>I removed the macros. struct cache_req_data basically serves the same
>>purpose as cache_req_input did but this way new parameters can be easily
>>added without changing all calls and it's a bit clearer IMHO.
>>
>>See attached patches.
>>
>
>> From bd58ee38470026dc7d1a6a329038731a2f520e27 Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Tue, 1 Mar 2016 12:53:40 +0100
>>Subject: [PATCH 14/15] cache_req: rename cache_req_input to cache_req
>>
>>The input part has no longer meaning.
>>---
>> src/responder/common/responder_cache_req.c | 412 ++++++++++++++---------------
>> 1 file changed, 203 insertions(+), 209 deletions(-)
>>
>
>...
>
>>@@ -1175,25 +1171,25 @@ struct tevent_req *cache_req_send(TALLOC_CTX *mem_ctx,
>> return NULL;
>> }
>>
>>- CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, input, "New request\n");
>>+ CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr, "New request\n");
>
>cr is still NULL here as cache_req_create() is called below. Since
>CACHE_REQ_DEBUG tries to dereference cr this causes a segfault.
>
>bye,
>Sumit
>
>>
>> state->ev = ev;
>> state->rctx = rctx;
>> state->ncache = ncache;
>> state->neg_timeout = neg_timeout;
>> state->cache_refresh_percent = cache_refresh_percent;
>>- state->input = input = cache_req_input_create(state, rctx, data);
>>- if (state->input == NULL) {
>>+ state->cr = cr = cache_req_create(state, rctx, data);
>>+ if (state->cr == NULL) {
>> ret = ENOMEM;
>> goto immediately;
>> }
>>
>>- if (input->data->name.input != NULL && domain == NULL) {
>>+ if (cr->data->name.input != NULL && domain == NULL) {
>> /* Parse input name first, since it may contain domain name. */
>>- CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, input, "Parsing input name
[%s]\n",
>>- input->data->name.input);
>>+ CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr, "Parsing input name
[%s]\n",
>>+ cr->data->name.input);
>>
>>- subreq = sss_parse_inp_send(state, rctx,
input->data->name.input);
>>+ subreq = sss_parse_inp_send(state, rctx, cr->data->name.input);
>> if (subreq == NULL) {
>> ret = ENOMEM;
>> goto immediately;
Thanks, I didn't see it when running test since debugging was disabled
there. Those patches fix this and the debian issue.
http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/3880/
The CI now passes and Sumit was able to base his branch on these
patches, so ACK to all.