On (02/03/16 18:22), Jakub Hrozek 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. >> > >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..) No, It just mean that test need to be explicitly linked with libldb.
Pavel would notice it if he used our CI :-) LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
