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. > > >> From 7b86bf36da117f86c4390d3b56e2f6065754d16b Mon Sep 17 00:00:00 2001 > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> > >>Date: Wed, 17 Feb 2016 10:07:18 +0100 > >>Subject: [PATCH 08/12] cache_req tests: use leak check in test fixtures > >> > >>To ensure no memory is leak on long living context such as rctx. > >> > >>Resolves: > >>https://fedorahosted.org/sssd/ticket/2869 > > > >ACK, but perhaps we'll have to amend this patch if we decide to drop the > >previous patch. > > > >> From 2375eb0a03c4d5fb094670fcd0a2e178f9a503c4 Mon Sep 17 00:00:00 2001 > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> > >>Date: Fri, 19 Feb 2016 13:04:04 +0100 > >>Subject: [PATCH 09/12] cache_req tests: improve user and group creation > >> > >>--- > >> src/tests/cmocka/test_responder_cache_req.c | 257 > >> +++++++++++++--------------- > > > >There are some compilation warnings here: > > CC > > src/tests/cmocka/responder_cache_req_tests-test_responder_cache_req.o > >/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c: > >In function ‘__wrap_sss_dp_get_account_send’: > >/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c:324:13: > > warning: unused variable ‘ret’ [-Wunused-variable] > > errno_t ret; > > ^ > >In file included from > >/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c:21:0: > >/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c: > >In function ‘test_user_by_name_multiple_domains_parse’: > >/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c:539:17: > > warning: passing argument 1 of ‘_talloc_free’ discards ‘const’ qualifier > >from pointer target type [-Wdiscarded-qualifiers] > > talloc_free(fqn); > > ^ > >/usr/include/talloc.h:229:5: note: expected ‘void *’ but argument is of type > >‘const char *’ > > int _talloc_free(void *ptr, const char *location); > > ^ > >/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c: > >In function ‘test_group_by_name_multiple_domains_parse’: > >/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_responder_cache_req.c:1039:17: > > warning: passing argument 1 of ‘_talloc_free’ discards ‘const’ qualifier > >from pointer target type [-Wdiscarded-qualifiers] > > talloc_free(fqn); > > ^ > >/usr/include/talloc.h:229:5: note: expected ‘void *’ but argument is of type > >‘const char *’ > > int _talloc_free(void *ptr, const char *location); > > ^ > > CCLD responder_cache_req-tests > > > > > >But the general intent looks good > > It is interesting that I didn't get these warnings... fixed nevertheless. I think it's because the Coverity checks already run GCC6. Anyway, the latest Coverity run was clean. > > >> From 4d4b87e40b25d1528aac2222d6a7444a32eae515 Mon Sep 17 00:00:00 2001 > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> > >>Date: Fri, 19 Feb 2016 14:09:26 +0100 > >>Subject: [PATCH 12/12] cache_req test: add lookup by sid > > > >LGTM, thanks for including the tests. > > > >Coverity returned clean. > > 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. > From 4e66072760c5fa3b108fb19cbcc403ef100c944d Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> > Date: Wed, 24 Feb 2016 12:15:51 +0100 > Subject: [PATCH 13/15] cache_req: use struct instead of parameter list > > --- > src/responder/common/responder_cache_req.c | 91 > ++++++++++++++++-------------- > src/responder/common/responder_cache_req.h | 42 ++++++++++++-- > src/responder/ifp/ifpsrv_cmd.c | 7 ++- > 3 files changed, 92 insertions(+), 48 deletions(-) [...] > index > a9388149170a486a509b1d92520d7fe8d21172d3..5b21d6d3e84469cbdb7223d467738ef156b94e5c > 100644 > --- a/src/responder/common/responder_cache_req.h > +++ b/src/responder/common/responder_cache_req.h > @@ -27,6 +27,42 @@ > #include "responder/common/responder.h" > #include "responder/common/negcache.h" > > +struct cache_req_data { > + struct { > + const char *input; /* Original input. */ > + const char *name; /* Parsed name or UPN. */ > + const char *lookup; /* Converted per domain rules. */ > + } name; > + uint32_t id; > + const char *cert; > + const char *sid; > + const char **attrs; > +}; > + > +#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. > From f3c749b25cff90dd851629c7d6424664e7ff52cb Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> > Date: Wed, 24 Feb 2016 12:33:47 +0100 > Subject: [PATCH 14/15] cache_req: hide cache_req_input ACK > From e5770234954a05bdd0598687eb6cc1e061a530d9 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> > Date: Wed, 24 Feb 2016 12:37:51 +0100 > Subject: [PATCH 15/15] cache_req: remove old comment ACK _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
