-----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 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----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel