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?=
<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.
master:
* 097e65032c58b157ea886edba13e1fcb89e4feeb
* 16f081b1336552132e5932a91941ea59620cd18a
* d46005e0f4b01600ddf843a956c3e1329bb6f19c
* 0d111f6975c7e132bf1069d6d7bd6d6afd2127df
* f4d2ad64d7d4a991f93631b8a0b3a69ff9d241bf
* 3a12f5cf2ee4a76c13b4d5ed9b0be87ad1d5cb2e
* 8be4efb91f9af1bf49628c497a9de4b258bb893e
* c19374b2a9b676ca534f52ef76d80f0945fe8fb2
* af820c9fc6aa1768e2e6b0df78fb489dbb1b28d0
* 9fc9365da3ff8e52814b7924d65c68eef72c484d
* f6c337c6256879d47356cd099bb00aafba2650f0
* d64880d3cddc76132c5c1f0bb979cfa1097b8784
* 9bc1ba22fb14742c9c73aa57faaa9c725800616e
* e42e8022f463fdb9136a9c3adb29d2d9d6b3780e
* c23b0e16991af9a9877de029a3853afc46bde8bd