[SSSD] [PATCH] Fix faulty usn reinit

Jakub Hrozek jhrozek at redhat.com
Wed Jan 9 10:35:38 UTC 2013


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.



More information about the sssd-devel mailing list