[SSSD] [PATCH] Save dummy groups to cache during initgroups (master)

Jakub Hrozek jhrozek at redhat.com
Mon Oct 11 09:43:43 UTC 2010


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

On 10/05/2010 10:01 PM, Jakub Hrozek wrote:
> On 10/04/2010 02:23 PM, Stephen Gallagher wrote:
>> On 09/30/2010 08:38 AM, Jakub Hrozek wrote:
>>> [PATCH 1/2] sysdb interface for adding fake groups
>>> Adds a sysdb_add_fake_group() call that adds an expired group entry.
> 
>>> Also tweaks the NSS code a little so the fake group entries that are
>>> invalid are skipped. This is not how we use the fake groups in patch #2
>>> as we also store the gid, but I think it's worth making sure we don't
>>> return fake groups now that we have a mechanism to add them.
> 
>> Nack.
>> I keep forgetting to add this to our formal coding specification, but
>> please do not pass a mem_ctx into any function that does not return an
>> allocated value.
> 
>> The tmp_ctx should be allocated on NULL so that it would turn up on a
>> valgrind report if it leaks. If it's attached to mem_ctx, then it will
>> still be available and undetectable until that parent context is freed.
> 
>> (In the 1.2 version, we'll probably need this memory context, since
>> we'll be dealing with sysdb as a tevent subrequest)
> 
> 
> 
>>> [PATCH 2/2] Save dummy groups to cache during initgroups
>>> When performing initgroups/getgrouplist on RFC2307, add fake group
>>> entries for those groups that are not cached already. That way, complete
>>> group membership is returned without saving complete group objects.
> 
>>> These two patches apply to master only but I have a 1.2 counterpart -
>>> currently only in my fedorapeople repo - I can resend them when/if these
>>> are approved as these read much easily, so I think it makes sense to
>>> review them first.
> 
>> Nack.
> 
>> Same comment as above with the memory context.
> 
>> We shouldn't return EINVAL when we get zero groups. We should just
>> short-circuit and return EOK, since there's nothing to be done.
> 
>> Personal preference: please separate missing[mi] and mi++ in the code.
>> It reads easier as separate lines.
> 
>> You need to initialize in_transaction to false, or error cases before
>> the transaction start will be wrong.
> 
>> If all groups are cached, you're leaking memory by not freeing tmp_ctx;
> 
>> I'm not sure why you're searching the sysdb for groups here. It's been
>> done by sdap_initgr_rfc2307_process(). We're only passing in the list of
>> entries that are missing from sysdb. If you're worried that the sysdb
>> has been changed since it was previously checked, then you probably want
>> to wrap the lookup in a transaction, not just the write.
> 

Actually, the way I read the code, we only compare the groups the user
is already a member of vs. the groups he should be a member of. We never
check if we should just add him to a group that is already present in
the sysdb or if we need to create a fake one. I think the check is needed.

> 
>> If the missing group is not found in ldap_groups, that should throw an
>> error.
> 
> 
> Thank you for the review, updated patches are attached.

Another round of patches attached, these contain the changes I did
during review of its 1.2 counterpart.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkyy3E8ACgkQHsardTLnvCUXFwCgj4FTZVYQK4rguMYnQsWKcu53
j8QAoOVUQ4a/oSKwO70gBdT8d5AjXWbL
=30Pv
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sysdb-interface-for-adding-incomplete-groups.patch
Type: text/x-patch
Size: 5295 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101011/3dbc0b7b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Save-dummy-groups-to-cache-during-initgroups.patch
Type: text/x-patch
Size: 5854 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101011/3dbc0b7b/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sysdb-interface-for-adding-incomplete-groups.patch.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101011/3dbc0b7b/attachment.sig>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Save-dummy-groups-to-cache-during-initgroups.patch.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101011/3dbc0b7b/attachment-0001.sig>


More information about the sssd-devel mailing list