[SSSD] [PATCH] The new memberof plugin

Jan Zelený jzeleny at redhat.com
Fri Sep 23 08:17:46 UTC 2011


> > Purge or expire the cache
> > getent group groupthatexistsandhasmembers
> > 
> > I tested on an RFC2307 server.
> > 
> > On Sep 22, 2011, at 5:10 AM, Jan Zelený <jzeleny at redhat.com> wrote:
> > >> 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).
> > > 
> > > Could you please send me steps how to reproduce?
> > > 
> > > Thanks
> > > Jan
> 
> Strange. I tested this exact scenario and it succeeded. I retested it a
> moment ago to be sure and it still goes fine. Could you send me some more
> info like a core dump or something?

After further consulation on IRC, I finally found the bug and fixed it. Sending 
patches in attachment.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Memberof-plugin-rewritten.patch.tar.bz2
Type: application/x-bzip-compressed-tar
Size: 20118 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110923/63569658/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Added-sysdb_memberof_rebuild-function.patch
Type: text/x-patch
Size: 1958 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110923/63569658/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-New-test-in-sysdb-test-suite-rebuild-memberof-tree.patch
Type: text/x-patch
Size: 1593 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110923/63569658/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110923/63569658/attachment.sig>


More information about the sssd-devel mailing list