[SSSD] [PATCH] Return all user members for RFC2307 groups (sssd-1.2)

Stephen Gallagher sgallagh at redhat.com
Fri Oct 8 20:48:01 UTC 2010


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/08/2010 04:45 PM, Jakub Hrozek wrote:
> On 10/08/2010 10:25 PM, Stephen Gallagher wrote:
>> On 10/08/2010 03:45 PM, Jakub Hrozek wrote:
>>> On 10/08/2010 04:57 PM, Stephen Gallagher wrote:
>>>> On 10/08/2010 05:09 AM, Jakub Hrozek wrote:
>>>>>>>> [PATCH 1/3] sysdb interface for adding expired user entries
>>>>>>>> These are added instead of looking up the members.
> 
>>>> Ack.
> 
>>>>>>>> [PATCH 2/3] Shortcut for save_group() to accept sysdb DNs as member
>>>>>>>> attributes
> 
>>>> Nack. I would rather we use an enum here instead of adding extra
>>>> booleans to the interface. Otherwise it's fine. (I would have made the
>>>> same comment to Ralf had I been the one to review this code)
> 
> 
>>> OK..I hope I got the combinations right, esp. for RFC2307bis, it seemed
>>> like store_members was never used in that case..
> 
> 
> 
>> I take it back. The first patch was much clearer. I didn't realize just
>> how much of an effect that was going to have. I was only talking about
>> replacing two booleans with one multi-valued variable, since the two
>> booleans appeared to be mutually-exclusive.
> 
>> This patch frightens me.
> 
> 
> I'm glad I can withdraw the change. Given the time-constraints we're
> under I was afraid of dragging in a regression. I'll keep this issue in
> mind for master, though.
> 
>>>>>>>> [PATCH 3/3] Add fake users during saving of RFC2307 group
>>>>>>>> Instead of looking up the user entries, we just add dummy expired
>>>>>>>> users that are refreshed when they are requested with
>>>>>>>> getpwnam/getpwuid
> 
>>>> Nack.
> 
>>>> sdap_process_group_send(): Please allocate attrs, filter and
>>>> grp_state->sysdb_dns atop grp_state and grp_state->sysdb_dns->values
>>>> atop grp_state->sysdb_dns. Allocating them on memctx means that they
>>>> will not go away until the parent memory context does. These values are
>>>> only needed for the lifetime of this request. If the parent memory
>>>> context lives a long time, this is basically a memory leak.
> 
>>>> Otherwise, I'm fine with this patch.
> 
> 
>>> OK but grp_state->sysdb_dns and grp_state->sysdb_dns->values are a
>>> little tricky. The thing is that they directly reference the "member:"
>>> sysdb attribute of the group being processed, so I think they should be
>>> allocated on the same context the group to allow for a safe saving of
>>> the group.
> 
>>> The cleanest solution would be to talloc_steal them on _recv, but we
>>> process all the groups at the some time so distinguishing between them
>>> in sdap_get_groups_processed() would not be trivial, I think. So I
>>> talloc_steal them onto the group in case the request is
>>> sdap_process_group_done()..hope this is OK.
> 
>>> We might want to keep both these issues in mind as both were copied from
>>> Ralf's patches that are under review for master.
> 
>> Sure, I'm fine with not complicating our lives. A little memory that
>> hangs around too long is not worth a redesign at this point.
> 
>> Ack.
> 
> 
> Attaching all three patches for clarity. Thank you for the review.


Ack to all three. Good work!


- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkyvg4EACgkQeiVVYja6o6NeoQCgpDZKos5kd+KJsUs/B8D8SKAu
IIIAn3+MBkfskLmwsGSFxHIMrDxUYXmE
=qugd
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list