[SSSD] [PATCH] The new memberof plugin

Stephen Gallagher sgallagh at redhat.com
Wed Sep 21 11:42:43 UTC 2011


On Wed, 2011-09-21 at 13:24 +0200, Jan Zelený 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.
> > 
> > HTH,
> > Simo.
> 
> Simo,
> as promised before, I have a new version of the patch. This time it's 
> complete, including the rewritten recompute task. I also took almost all 
> comments you and Stephen gave me into account. The only thing I left 
> unresolved for the moment is the last comment in the diff you sent. The change 
> shouldn't be difficult, but it will need changes in many parts of the code, 
> therefore I'll rather send it in the next patch.


That's the change to suppress memberuid for non-posixGroup entries,
correct?

I'm going to say that this is an optimization that we don't need right
now. Please open an RFE for it and we'll schedule it for later
inclusion.

Right now the SSSD only ever uses the member/memberOf relationship for
groups, so the only advantage of this would be that we wouldn't add
memberUid attributes to non-POSIX groups in a nested chain, but I think
the gain there is not worth the effort at this time.

Actual review of the patch will be forthcoming, but I didn't want you
wasting time on this for now.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110921/99eeee97/attachment.sig>


More information about the sssd-devel mailing list