[SSSD] [PATCH] Save all data to sysdb in one transaction
Sumit Bose
sbose at redhat.com
Wed Sep 8 13:50:37 UTC 2010
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 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