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

Stephen Gallagher sgallagh at redhat.com
Thu Jun 10 11:53:44 UTC 2010


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/



More information about the sssd-devel mailing list