[SSSD] [PATCH] The new memberof plugin

Simo Sorce simo at redhat.com
Thu Sep 15 14:59:59 UTC 2011


On Thu, 2011-09-15 at 10:53 -0400, Stephen Gallagher wrote:
> On Thu, 2011-09-15 at 10:43 -0400, Simo Sorce wrote:
> > On Tue, 2011-08-30 at 17:54 +0200, Jan Zelený wrote:
> > > Attached patch rewrites almost entire memberof plugin. It heavily utilizes 
> > > hash tables instead of lists and arrays and it introduces concept of reference 
> > > counting which should heavily optimize all operations when no loops are 
> > > present in the user/group tree.
> > > 
> > > I tested the patch by running sysdb test suite, all tests passed. I also 
> > > attach a document where basic concepts of the plugin are explained.
> > > 
> > > Please note that the patch is functional, although I don't consider it ready. 
> > > I just want to get pre-ACK or some comments about the patch design. I have yet 
> > > to implement the recompute task, I will work on that later.
> > 
> > Jan,
> > yesterday I an Stephen got together to start a review of the patch. Due
> > to the complexity we tried to go thorugh the code together to make sure
> > we understand what is going on.
> > 
> > Also due to the complexity and need to refresh memory we were only be
> > able to go through most of the "add" code only, we were not able to
> > review the update or delete code paths yet.
> > 
> > Attached find a diff file with comments in C++ style (so that it is
> > clear they all need to be resolved before the patch can be considered
> > ok, as they show as a big red line in our editors) that you can
> > integrate in your tree and check one by one.
> > 
> > There are some questions in there, but in general we noticed a very
> > annoying lack of comments in the code itself. The accompaning PDF is
> > nice and all, but unless the logic is embedded and explained in the code
> > (like it was with the previous code) it will be ablocker and will make
> > review more difficult.
> > 
> > That said, so far so good, although we need to more carefully review the
> > refcounting stuff, we got to it late and only marginally reviewed it.
> > Need more time to fully grasp all details. 
> 
> 
> There was one other thing that we noticed that Simo forgot to mention.
> We realized that the original design was flawed in one major way: we
> should not handle "missing" members at all. If we encounter one, we
> should consider it a critical error and abort.
> 
> The original reasoning was that we were using the memberOf plugin to
> handle unknown members, but as the SSSD evolved, we're now handling this
> properly in the data providers and making sure the membership data is
> ready before committing it. We now have a three-pass mechanism of saving
> data in the SSSD:
> 1) Save users
> 2) Save all groups (empty)
> 3) Add members to groups
> 
> Doing it this way, we no longer need the checks for missing members in
> the memberOf plugin, which should simplify implementation a moderate
> amount.
> 

I think we added a comment about this in the diff, but yeah a proper
explanation was missing (pun :). Thanks for the writeup :)

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list