On Tue, Jan 08, 2013 at 05:54:59PM -0500, Simo Sorce wrote: > On Tue, 2013-01-08 at 21:52 +0100, Jakub Hrozek wrote: > > On Mon, Jan 07, 2013 at 11:13:03PM -0500, Simo Sorce wrote: > > > On Sun, 2013-01-06 at 00:37 -0500, Simo Sorce wrote: > > > > While looking at some code my eye fell on the fact that sdap_reinit.c > > > > was including sysdb_private.h > > > > > > > > That's a no-no on its own, you don't get to use private headers > > > > liberally, or I wouldn't have marked them "private" in the first place! > > > > > > > > However besides the abuse of the private headers I found also that the > > > > function was broken because it wasn't doing what it was trying to do > > > > (limit cleanups to users, groups and services). > > > > > > > > Instead it would search the whole tree (3 times) and later remove all > > > > entries w/o a USN. > > > > > > > > I think this could cause the code to remove *everything* not directly > > > > downloaded from the IPA tree (for example subdomain users) that lacks > > > > the SYSDB_USN attribute for example. > > > > > > > > I haven't tested the patch yet tbh, but I do not have the setup right > > > > now, if someone has a 2 servers setup ready and can force sssd to > > > > reconnect to the second and step through the cleanup to make sure it > > > > runs as it should I would be grateful. > > > > > > I am not sure how this function ever worked at all now, I found another > > > bug, state->sysdb where never assinged, so sysdb was NULl in some > > > calls ... > > > > > > Attached rebased patch that adds this fix. > > > > > > Simo. > > > > > > -- > > > Simo Sorce * Red Hat, Inc * New York > > > > > > I haven't tested the code yet, but can you split the patch into one that > > introduces the new sysdb function and one that uses it? > > Done > > > A unit test would be nice, too. > > Actually, after looking at the code I went a step further, I realized > all getsvc/enumsvc functions were doing the same thing so I change them > to just use the new sysdb_search_service functions instead of direct ldb > calls. This should be more robust and implicitly tests > sysdb_search_service in all current tests. > > The only annoyance is a slight mismatch in the interfaces so I had to > recreate a ldb_result structure in the getsvc calls. It's not the > prettiest solution but gets the job done w/o any side effect and w/o > requiring to change a lot of callers. > > > Also, can you use new DEBUG levels in the new code? > > Done. > > Attached new patchset. > > Simo.
Ack to patches #1 & #2 so far. They look OK, tests pass and service lookups still work (I only tested proxy provider proxying to files, but that should be enough I think). Patch #3 is more tricky to test. Feel free to push patches #1 and #2 to master if it would ease your rebasing if I'm not done testing the last one soon. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
