[SSSD] [PATCH] The new memberof plugin

Stephen Gallagher sgallagh at redhat.com
Wed Sep 21 19:34:06 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.



Nack

First, the third patch in this set does not compile. You used
sysdb_memberof_recompute() in the test, but the function is
sysdb_memberof_rebuild().

With that typo fixed, I attempted to run with this patch. Any attempt to
run getgrnam() or getgrgid() results in a hang in the LDB until
eventually the monitor issues a SIGTERM to the backend (which doesn't
die, because it never gets back to the mainloop to process the signal).
-------------- 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/a518cf71/attachment.sig>


More information about the sssd-devel mailing list