-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
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.
- --
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/
iEYEARECAAYFAky17vEACgkQeiVVYja6o6OKGACfe71kSNqc6Vq0dbGROo5aODl4
MG0AniuiVlMwo5KVoFL+qdob5TRwr/9h
=uL90
-----END PGP SIGNATURE-----