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

Jakub Hrozek jhrozek at redhat.com
Thu Oct 14 11:14:48 UTC 2010


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

On 10/13/2010 07:40 PM, Stephen Gallagher wrote:
> On 10/11/2010 05:43 AM, Jakub Hrozek wrote:
>> 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.
> 
> 
> Patch 0001: Ack.
> 
> Patch 0002: Nack.
> +    mi = 0;
> +
> +    /* All groups are cached, nothing to do */
> +    if (mi == 0) {
> 
> This is guaranteed to always be true, so we're always short-circuiting
> out of here. I'm not sure what purpose this check would have anyway.
> 
> The first for loop is incorrect. I assume you meant to loop through
> groupnames. Right now, you're looping through an uninitialized array.
> 
> Please change
> missing[mi++] = groupnames[i];
> to
> missing[mi] = groupnames[i];
> mi++
> 
> It reads cleaner.
> 
> 
> The second for loop is also incorrect. You're looping through the old
> list of every groupname, not the list you just created.
> 
> 
> Also, it's probably a good idea to start the transaction before the name
> check, just in case.
> 
> 

I must have sent wrong patches or goofed a rebase because these patches
were absolutely wrong (and there was a bug in the user processing
patches I'm almost certain I fixed).

Sorry about that, new patches attached.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAky25igACgkQHsardTLnvCXxnQCgm/pZh42g30lg2wO1RzMX477W
SucAnR7X95AtjnL8o2aY+PAHGG//YEJ7
=c3aZ
-----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/20101014/2cfce4c3/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: 5896 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101014/2cfce4c3/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/20101014/2cfce4c3/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/20101014/2cfce4c3/attachment-0001.sig>


More information about the sssd-devel mailing list