On 06/10/2010 05:56 AM, Sumit Bose wrote:
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.
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
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.
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?
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.
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.