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.
Yes, I suspected that, but I left it there since it was in the original code.
Thanks for noticing this, I will certainly modify the behavior. Just to make
sure - I should test if the member object exists but if it doesn't, I should
just exit with an error, right?
Thanks
Jan