Hi,
this patch for master saves all IPA HBAC data in one transaction and proceeds if the sysdb transaction fails. It addresses a review comment from the "Use new schema for HBAC service checks" patch.
The first patch changes the handling of the host data to be compatible with the other data. The second removes the individual transactions and introduces a common sysdb transaction.
Do we want/need a similar patch for master, too?
bye, Sumit
On 06/10/2010 05:56 AM, Sumit Bose wrote:
Hi,
this patch for master saves all IPA HBAC data in one transaction and proceeds if the sysdb transaction fails. It addresses a review comment from the "Use new schema for HBAC service checks" patch.
The first patch changes the handling of the host data to be compatible with the other data.
Nack.
I think it would be better form in set_local_and_remote_host_info() to maintain a private memory context onto which to allocate the hbac_host_info objects until we're ready to return, at which time they should be stolen back onto mem_ctx.
This way if we end up in the failure condition, we don't exit the function with potentially host_count chunks of memory of sizeof(struct hbac_host_info) sitting around uselessly until the mem_ctx is eventually freed.
For most functions, I wouldn't recommend this, but this particular function has the potential of allocating a LOT of memory (if there are a lot of HBAC rules on the server) and I'd prefer that we not hang onto it if something goes wrong.
The second removes the individual transactions and introduces a common sysdb transaction.
Nack.
In hbac_save_data_to_sysdb() you start a transaction, then create a tmp_ctx, but if talloc_new() fails, you exit the function without cancelling the transaction.
You have a memory leak on the success case of hbac_save_data_to_sysdb(). You're not freeing tmp_ctx here. (Yeah, it's a short leak, as it will be cleared when mem_ctx is, but it's bad form).
Similar to my comment about set_local_and_remote_host_info() in the first patch, since it's possible for us to allocate a lot of memory (if count is large) in hbac_save_list(), I think it makes sense to do allocations in this function in a tmp_ctx. Furthermore, unless I'm reading this wrong, the allocation made here does not get returned to the calling function, so proper memory management policy would be to clean it up here.
We probably shouldn't be passing in a mem_ctx to hbac_save_list() at all. The function does not return anything up the stack, so we should contain its memory usage entirely within the function.
As a note for the future, I don't see a purpose for passing mem_ctx to sysdb_delete_recursive() either. It returns no values that we are supposed to be responsible for, so having it attach to our memory context is a bad idea. It should attach to a new toplevel context instead, so valgrind would notice if we ever forget to free it properly.
Do we want/need a similar patch for master, too?
I'm assuming you mean sssd-1-2 here. I don't think we're likely to hit situations where the sysdb can't be saved very often, so I'd suggest leaving it out of sssd-1-2 unless we get bugs reported on it.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Just a reminder that it's been about a month since there was any activity on this patch.
On 06/10/2010 07:53 AM, Stephen Gallagher wrote:
On 06/10/2010 05:56 AM, Sumit Bose wrote:
Hi,
this patch for master saves all IPA HBAC data in one transaction and proceeds if the sysdb transaction fails. It addresses a review comment from the "Use new schema for HBAC service checks" patch.
The first patch changes the handling of the host data to be compatible with the other data.
Nack.
I think it would be better form in set_local_and_remote_host_info() to maintain a private memory context onto which to allocate the hbac_host_info objects until we're ready to return, at which time they should be stolen back onto mem_ctx.
This way if we end up in the failure condition, we don't exit the function with potentially host_count chunks of memory of sizeof(struct hbac_host_info) sitting around uselessly until the mem_ctx is eventually freed.
For most functions, I wouldn't recommend this, but this particular function has the potential of allocating a LOT of memory (if there are a lot of HBAC rules on the server) and I'd prefer that we not hang onto it if something goes wrong.
The second removes the individual transactions and introduces a common sysdb transaction.
Nack.
In hbac_save_data_to_sysdb() you start a transaction, then create a tmp_ctx, but if talloc_new() fails, you exit the function without cancelling the transaction.
You have a memory leak on the success case of hbac_save_data_to_sysdb(). You're not freeing tmp_ctx here. (Yeah, it's a short leak, as it will be cleared when mem_ctx is, but it's bad form).
Similar to my comment about set_local_and_remote_host_info() in the first patch, since it's possible for us to allocate a lot of memory (if count is large) in hbac_save_list(), I think it makes sense to do allocations in this function in a tmp_ctx. Furthermore, unless I'm reading this wrong, the allocation made here does not get returned to the calling function, so proper memory management policy would be to clean it up here.
We probably shouldn't be passing in a mem_ctx to hbac_save_list() at all. The function does not return anything up the stack, so we should contain its memory usage entirely within the function.
As a note for the future, I don't see a purpose for passing mem_ctx to sysdb_delete_recursive() either. It returns no values that we are supposed to be responsible for, so having it attach to our memory context is a bad idea. It should attach to a new toplevel context instead, so valgrind would notice if we ever forget to free it properly.
Do we want/need a similar patch for master, too?
I'm assuming you mean sssd-1-2 here. I don't think we're likely to hit situations where the sysdb can't be saved very often, so I'd suggest leaving it out of sssd-1-2 unless we get bugs reported on it.
- -- 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 07/08/2010 12:57 PM, Stephen Gallagher wrote:
Just a reminder that it's been about a month since there was any activity on this patch.
*bump* Any word on this one, Sumit? Would you prefer that I have someone else look into this?
On 06/10/2010 07:53 AM, Stephen Gallagher wrote:
On 06/10/2010 05:56 AM, Sumit Bose wrote:
Hi,
this patch for master saves all IPA HBAC data in one transaction and proceeds if the sysdb transaction fails. It addresses a review comment from the "Use new schema for HBAC service checks" patch.
The first patch changes the handling of the host data to be compatible with the other data.
Nack.
I think it would be better form in set_local_and_remote_host_info() to maintain a private memory context onto which to allocate the hbac_host_info objects until we're ready to return, at which time they should be stolen back onto mem_ctx.
This way if we end up in the failure condition, we don't exit the function with potentially host_count chunks of memory of sizeof(struct hbac_host_info) sitting around uselessly until the mem_ctx is eventually freed.
For most functions, I wouldn't recommend this, but this particular function has the potential of allocating a LOT of memory (if there are a lot of HBAC rules on the server) and I'd prefer that we not hang onto it if something goes wrong.
The second removes the individual transactions and introduces a common sysdb transaction.
Nack.
In hbac_save_data_to_sysdb() you start a transaction, then create a tmp_ctx, but if talloc_new() fails, you exit the function without cancelling the transaction.
You have a memory leak on the success case of hbac_save_data_to_sysdb(). You're not freeing tmp_ctx here. (Yeah, it's a short leak, as it will be cleared when mem_ctx is, but it's bad form).
Similar to my comment about set_local_and_remote_host_info() in the first patch, since it's possible for us to allocate a lot of memory (if count is large) in hbac_save_list(), I think it makes sense to do allocations in this function in a tmp_ctx. Furthermore, unless I'm reading this wrong, the allocation made here does not get returned to the calling function, so proper memory management policy would be to clean it up here.
We probably shouldn't be passing in a mem_ctx to hbac_save_list() at all. The function does not return anything up the stack, so we should contain its memory usage entirely within the function.
As a note for the future, I don't see a purpose for passing mem_ctx to sysdb_delete_recursive() either. It returns no values that we are supposed to be responsible for, so having it attach to our memory context is a bad idea. It should attach to a new toplevel context instead, so valgrind would notice if we ever forget to free it properly.
Do we want/need a similar patch for master, too?
I'm assuming you mean sssd-1-2 here. I don't think we're likely to hit situations where the sysdb can't be saved very often, so I'd suggest leaving it out of sssd-1-2 unless we get bugs reported on it.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
- -- 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 09/08/2010 03:33 PM, Stephen Gallagher wrote:
*bump* Any word on this one, Sumit? Would you prefer that I have someone else look into this?
I think Sumit stated yesterday that he's currently swamped, as my task list for the upcoming release is shrinking, I can volunteer to finish the patch.
Jakub
On Wed, Sep 08, 2010 at 09:33:04AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 07/08/2010 12:57 PM, Stephen Gallagher wrote:
Just a reminder that it's been about a month since there was any activity on this patch.
*bump* Any word on this one, Sumit? Would you prefer that I have someone else look into this?
ah, sorry, I have a patch which addresses the comments in my tree, but I need to rebase it. I will send it tomorrow or Friday.
bye, Sumit
On 06/10/2010 07:53 AM, Stephen Gallagher wrote:
On 06/10/2010 05:56 AM, Sumit Bose wrote:
Hi,
this patch for master saves all IPA HBAC data in one transaction and proceeds if the sysdb transaction fails. It addresses a review comment from the "Use new schema for HBAC service checks" patch.
The first patch changes the handling of the host data to be compatible with the other data.
Nack.
I think it would be better form in set_local_and_remote_host_info() to maintain a private memory context onto which to allocate the hbac_host_info objects until we're ready to return, at which time they should be stolen back onto mem_ctx.
This way if we end up in the failure condition, we don't exit the function with potentially host_count chunks of memory of sizeof(struct hbac_host_info) sitting around uselessly until the mem_ctx is eventually freed.
For most functions, I wouldn't recommend this, but this particular function has the potential of allocating a LOT of memory (if there are a lot of HBAC rules on the server) and I'd prefer that we not hang onto it if something goes wrong.
The second removes the individual transactions and introduces a common sysdb transaction.
Nack.
In hbac_save_data_to_sysdb() you start a transaction, then create a tmp_ctx, but if talloc_new() fails, you exit the function without cancelling the transaction.
You have a memory leak on the success case of hbac_save_data_to_sysdb(). You're not freeing tmp_ctx here. (Yeah, it's a short leak, as it will be cleared when mem_ctx is, but it's bad form).
Similar to my comment about set_local_and_remote_host_info() in the first patch, since it's possible for us to allocate a lot of memory (if count is large) in hbac_save_list(), I think it makes sense to do allocations in this function in a tmp_ctx. Furthermore, unless I'm reading this wrong, the allocation made here does not get returned to the calling function, so proper memory management policy would be to clean it up here.
We probably shouldn't be passing in a mem_ctx to hbac_save_list() at all. The function does not return anything up the stack, so we should contain its memory usage entirely within the function.
As a note for the future, I don't see a purpose for passing mem_ctx to sysdb_delete_recursive() either. It returns no values that we are supposed to be responsible for, so having it attach to our memory context is a bad idea. It should attach to a new toplevel context instead, so valgrind would notice if we ever forget to free it properly.
Do we want/need a similar patch for master, too?
I'm assuming you mean sssd-1-2 here. I don't think we're likely to hit situations where the sysdb can't be saved very often, so I'd suggest leaving it out of sssd-1-2 unless we get bugs reported on it.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
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/
iEYEARECAAYFAkyHkJAACgkQeiVVYja6o6PURACgpC0W+4z+CG87NTIXndv6AFtj iasAn2/gdOdxzNhjzefPDXLDEQgsFFd3 =0qZe -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/08/2010 09:50 AM, Sumit Bose wrote:
On Wed, Sep 08, 2010 at 09:33:04AM -0400, Stephen Gallagher wrote: On 07/08/2010 12:57 PM, Stephen Gallagher wrote:
Just a reminder that it's been about a month since there was any activity on this patch.
*bump* Any word on this one, Sumit? Would you prefer that I have someone else look into this?
ah, sorry, I have a patch which addresses the comments in my tree, but I need to rebase it. I will send it tomorrow or Friday.
Okay, no problem. If there are any further review comments at that point, I'm going to turn it over to Jakub to finish up.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Wed, Sep 08, 2010 at 09:52:31AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/08/2010 09:50 AM, Sumit Bose wrote:
On Wed, Sep 08, 2010 at 09:33:04AM -0400, Stephen Gallagher wrote: On 07/08/2010 12:57 PM, Stephen Gallagher wrote:
Just a reminder that it's been about a month since there was any activity on this patch.
*bump* Any word on this one, Sumit? Would you prefer that I have someone else look into this?
ah, sorry, I have a patch which addresses the comments in my tree, but I need to rebase it. I will send it tomorrow or Friday.
Okay, no problem. If there are any further review comments at that point, I'm going to turn it over to Jakub to finish up.
rebased versions are attached. It was necessary to add some modifications during the rebase. I have done a couple of online and offline test but it would be nice if someone else can run additional tests.
bye, Sumit
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/
iEYEARECAAYFAkyHlR8ACgkQeiVVYja6o6MKkgCeNCEYKVfiOvZLl09I+zZJ7ZQ2 YAkAn2hu0LXBr3I7AIjJN9nOFWP8CTXT =yHsK -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/09/2010 08:29 AM, Sumit Bose wrote:
On Wed, Sep 08, 2010 at 09:52:31AM -0400, Stephen Gallagher wrote: On 09/08/2010 09:50 AM, Sumit Bose wrote:
On Wed, Sep 08, 2010 at 09:33:04AM -0400, Stephen Gallagher wrote: On 07/08/2010 12:57 PM, Stephen Gallagher wrote:
> Just a reminder that it's been about a month since there was any > activity on this patch. >
*bump* Any word on this one, Sumit? Would you prefer that I have someone else look into this?
ah, sorry, I have a patch which addresses the comments in my tree, but I need to rebase it. I will send it tomorrow or Friday.
Okay, no problem. If there are any further review comments at that point, I'm going to turn it over to Jakub to finish up.
rebased versions are attached. It was necessary to add some modifications during the rebase. I have done a couple of online and offline test but it would be nice if someone else can run additional tests.
Ack to both.
- -- 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 09/23/2010 07:51 AM, Stephen Gallagher wrote:
On 09/09/2010 08:29 AM, Sumit Bose wrote:
rebased versions are attached. It was necessary to add some modifications during the rebase. I have done a couple of online and offline test but it would be nice if someone else can run additional tests.
Ack to both.
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
-
Jakub Hrozek -
Stephen Gallagher -
Sumit Bose