On 04/14/2010 04:51 PM, Simo Sorce wrote:
On Wed, 14 Apr 2010 10:48:28 +0400
Eugene Indenbom<eindenbom(a)gmail.com> wrote:
> + /* private data is allocated as top-level talloc context as
> + * it may need to survive for a while after main handle
> destruction,
> + * see sdap_handle_data_lock/sdap_handle_data_release */
> + sh->private = talloc_zero(NULL, struct sdap_handle_data);
> + if (!sh->private) {
> + goto fail;
> + }
> +
> + sh->private->sh = sh;
> return sh;
> +
>
Sorry Eugene, but I *really* do not like things like this.
- Allocating on NULL: very high risk of memory leaks
We can allocate on sdap_handle_data on parent sdap_handle context and
later use talloc_steal to NULL context when we need to keep pointer to
sdap_handle_data until stack unwinds.
So normally sdap_handle_data would be owned, but when it orphan it will
become orphan in TALLOC_CTX sense too.
But I do not see any risk here: there is a destructor set on sdap_handle
that take cares of destruction of sdap_handle_data. There is no way to
get this out of control.
- Assigning sh to private->sh: very high risk of accessing free
memory
if sh is freed before private is.
private->sh is actually a boolean saying whether private data is orphan
or not. I see no reason not to have a pointer to owner at hand, but this
is not necessary.
And again there is no risk of having stale pointer here as there is a
destructor set on sdap_hanlde, that takes care of clearing the pointer
on time.
If preserving the handle is important I'd rather have a list of
handles, with old ones pushed down the list as soon as a connection is
marked bad so that they are not reused, but also not lost (memory wise).
So it looks like you do not really believe that talloc destructors are
always called and it's reliable to free memory in destructors. In my
humble opinion if memory deallocation is controlled from destructor
there is actually no actual need for talloc ownership.
You suggestion of putting sdap_handle objects on destroy queue and
garbage collecting them later does not sound as better alternative.
On 04/14/2010 05:00 PM, Simo Sorce wrote:
On Wed, 14 Apr 2010 10:48:28 +0400
Eugene Indenbom<eindenbom(a)gmail.com> wrote:
> 4. The suggested patch is a workaround. It relies on the agreement
> that all _send functions will be called from top level event context,
> not from subreq callback context. This is hard to guarantee.
>
It is easier to analyze and agree on for now.
It solves the problem, without changing the underlying logic.
This is a big plus for a patch to go into a stable tree as it will not
cause surprises.
That's wrong your patch is changing behaviour too:
1. It activates code paths that segfaulted before.
2. It changes time when sdap_handle is deallocated (and all its child
objects like LDAP handle). This way it substantially changes memory
distribution, object life time and other key parameters.
My patch behaves better in this sense. It at least does not change time
when LDAP handle is deallocated. Actually my patch affects only
sdap_handle destruction sequence and only in previously faulty case of
deletion recursion.
Regards, Eugene
PS Well, it's your project anyway and it's up to you and the project
team to decide on which way to take.