On Thu, Mar 10, 2016 at 11:42:53AM +0100, Pavel Březina wrote: > 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?= <[email protected]> > >>>>>>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?= <[email protected]> > >>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. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
