-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
[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.
[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.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
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.
If the missing group is not found in ldap_groups, that should throw an error.
- -- 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 SIGNED MESSAGE----- Hash: SHA1
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.
If the missing group is not found in ldap_groups, that should throw an error.
Thank you for the review, updated patches are attached.
-----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 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 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 SIGNED MESSAGE----- Hash: SHA1
On 10/14/2010 07:14 AM, Jakub Hrozek wrote:
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.
Patch 0001: Ack.
Patch 0002: Nack
I'd rather you used sysdb_attrs_get_string() to get the SYSDB_NAME in sdap_add_incomplete_groups().
strtol() is also not safe here, as gid_t is a uint32_t, which on 32-bit platforms may require unsigned long.
Also, since it would be a very useful thing to have, please add a function sysdb_attrs_get_ulong() and use that for getting the GID.
Otherwise this looks good.
- -- 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 SIGNED MESSAGE----- Hash: SHA1
On 10/14/2010 08:48 PM, Stephen Gallagher wrote:
Patch 0001: Ack.
Patch 0002: Nack
I'd rather you used sysdb_attrs_get_string() to get the SYSDB_NAME in sdap_add_incomplete_groups().
Done.
strtol() is also not safe here, as gid_t is a uint32_t, which on 32-bit platforms may require unsigned long.
Done. But IIRC I copied this bit from elsewhere in the code. Do we want to have a cleanup ticket to use the new sysdb_attrs_get_ulong() facility?
Also, since it would be a very useful thing to have, please add a function sysdb_attrs_get_ulong() and use that for getting the GID.
This is implemented in new patch 0001.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/15/2010 04:01 AM, Jakub Hrozek wrote:
On 10/14/2010 08:48 PM, Stephen Gallagher wrote:
Patch 0001: Ack.
Patch 0002: Nack
I'd rather you used sysdb_attrs_get_string() to get the SYSDB_NAME in sdap_add_incomplete_groups().
Done.
strtol() is also not safe here, as gid_t is a uint32_t, which on 32-bit platforms may require unsigned long.
Done. But IIRC I copied this bit from elsewhere in the code. Do we want to have a cleanup ticket to use the new sysdb_attrs_get_ulong() facility?
Please open a ticket. If possible, please try to fix this in time for 1.4.0. I have a feeling we have issues with handling high-value UID/GIDs.
I'm going to ask QE to test this.
Also, since it would be a very useful thing to have, please add a function sysdb_attrs_get_ulong() and use that for getting the GID.
This is implemented in new patch 0001.
Review coming a little later.
- -- 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 SIGNED MESSAGE----- Hash: SHA1
On 10/15/2010 07:12 AM, Stephen Gallagher wrote:
On 10/15/2010 04:01 AM, Jakub Hrozek wrote:
On 10/14/2010 08:48 PM, Stephen Gallagher wrote:
Patch 0001: Ack.
Patch 0002: Nack
I'd rather you used sysdb_attrs_get_string() to get the SYSDB_NAME in sdap_add_incomplete_groups().
Done.
strtol() is also not safe here, as gid_t is a uint32_t, which on 32-bit platforms may require unsigned long.
Done. But IIRC I copied this bit from elsewhere in the code. Do we want to have a cleanup ticket to use the new sysdb_attrs_get_ulong() facility?
Please open a ticket. If possible, please try to fix this in time for 1.4.0. I have a feeling we have issues with handling high-value UID/GIDs.
I'm going to ask QE to test this.
Also, since it would be a very useful thing to have, please add a function sysdb_attrs_get_ulong() and use that for getting the GID.
This is implemented in new patch 0001.
Review coming a little later.
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 SIGNED MESSAGE----- Hash: SHA1
On 10/15/2010 07:39 AM, Stephen Gallagher wrote:
On 10/15/2010 07:12 AM, Stephen Gallagher wrote:
On 10/15/2010 04:01 AM, Jakub Hrozek wrote:
On 10/14/2010 08:48 PM, Stephen Gallagher wrote:
Patch 0001: Ack.
Patch 0002: Nack
I'd rather you used sysdb_attrs_get_string() to get the SYSDB_NAME in sdap_add_incomplete_groups().
Done.
strtol() is also not safe here, as gid_t is a uint32_t, which on 32-bit platforms may require unsigned long.
Done. But IIRC I copied this bit from elsewhere in the code. Do we want to have a cleanup ticket to use the new sysdb_attrs_get_ulong() facility?
Please open a ticket. If possible, please try to fix this in time for 1.4.0. I have a feeling we have issues with handling high-value UID/GIDs.
I'm going to ask QE to test this.
Also, since it would be a very useful thing to have, please add a function sysdb_attrs_get_ulong() and use that for getting the GID.
This is implemented in new patch 0001.
Review coming a little later.
Ack to all three. Good work!
Pushed to master.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
sssd-devel@lists.fedorahosted.org