[SSSD] [PATCH] Save all data to sysdb in one transaction

Stephen Gallagher sgallagh at redhat.com
Wed Sep 8 13:33:04 UTC 2010


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



More information about the sssd-devel mailing list