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;