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.
>
I'm still reading the patches (it's a lot of code..) but in the
meantime, Debian complains about the ldb_reset_err_string:
http://sssd-ci.duckdns.org/logs/job/38/42/debian_testing/ci-build-debug/ci-make-tests.log
(good call we only put it into tests, I think..)
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
