-----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-----