On Mon, Mar 14, 2016 at 05:33:10PM +0100, Jakub Hrozek wrote: > 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.
master: * 097e65032c58b157ea886edba13e1fcb89e4feeb * 16f081b1336552132e5932a91941ea59620cd18a * d46005e0f4b01600ddf843a956c3e1329bb6f19c * 0d111f6975c7e132bf1069d6d7bd6d6afd2127df * f4d2ad64d7d4a991f93631b8a0b3a69ff9d241bf * 3a12f5cf2ee4a76c13b4d5ed9b0be87ad1d5cb2e * 8be4efb91f9af1bf49628c497a9de4b258bb893e * c19374b2a9b676ca534f52ef76d80f0945fe8fb2 * af820c9fc6aa1768e2e6b0df78fb489dbb1b28d0 * 9fc9365da3ff8e52814b7924d65c68eef72c484d * f6c337c6256879d47356cd099bb00aafba2650f0 * d64880d3cddc76132c5c1f0bb979cfa1097b8784 * 9bc1ba22fb14742c9c73aa57faaa9c725800616e * e42e8022f463fdb9136a9c3adb29d2d9d6b3780e * c23b0e16991af9a9877de029a3853afc46bde8bd _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
