talloc issues

[Michael Adam]

Hi Tridge, Simo, Sam, Metze...

simo wrote:
> On Fri, 2009-07-24 at 14:41 +1000, tridge at samba.org wrote:
> > Hi Simo, Sam and Metze,
> > 
> > I've been looking over the talloc discussions to try to come to some
> > conclusions.
> > 
> > [...]
> >
> > Regarding Sam's patches, I still have some of the same concerns that I
> > posted last time. I think that Sam's changes make the situation
> > unambiguous only in the same sense that the old talloc code was
> > unambiguous. The old talloc behaviour was completely deterministic
> > from a strictly algorithmic sense, in that a talloc_free() on a
> > pointer with references would remove the most recent reference
> > first. Sam's patches are similarly unambiguous, in that it removes the
> > original parent first.
> 
> I think the essence of Sam's proposal, once corrected a bit from the
> original form, is that references do not influence "normal"/"expected"
> talloc behavior. IMHO this is extremely important.

I strongly agree to this point.

I think the "normal" or "basic" use of talloc is the use
of talloc_new()/talloc_free(). And the "advanced" or special
use is using references (for instance).

And code that sticks to the basic use should not need to know whether
the users of the talloc contexts it created are going to use references
on it or not. On the other hand, code that uses references is
aware of the fact and can act accordingly.

So my fundamental claim is that talloc_new()/talloc_free()
should always be safe to use.

> > I also think that the way that talloc_steal() can become a function
> > that effectively changes the reference count is also a big concern,
> > and not something I think we want.
> 
> I totally agree that making talloc_steal() "special", in this sense, is
> not useful at all, it is actually rather dangerous.
> I think this is something that the original Sam's proposal got wrong
> indeed.
> 
> Metze and I have discussed a bit what would be appropriate behavior for
> talloc_free() and talloc_steal() after you take a reference. I think the
> conclusion was (Metze correct me if I got something wrong) that 
> 
> A) once talloc_free() is called, any other talloc_free() or
> talloc_steal() on the same talloc context should fail with a double free
> error. Exactly like it would happen if there were no references.
> 
> B) the code using talloc_reference() must use talloc_reparent() and
> talloc_unreference(). You cannot use talloc_free() or talloc_steal(),
> they do not affect references. The code using the references is aware of
> the fact you used them so it shouldn't be difficult to stick with this
> rule.

I support this 100%. I think it is _the_ reasonable way to
deal with references in talloc.

If this change implies that callers using references need to be
changed, then this is perfectly ok! The reference-using callers
just need to be disciplined.

> Of course freeing a parent context frees also the references it may
> hold, nothing changes there.
> 
> (Btw I would rename talloc_reparent() to talloc_rereference() in this
> case and make it operate exclusively on references).
> 
> I think that this way you can safely use talloc the "normal" way in the
> original code and live issues, if any, only on the shoulders of who
> decides to use talloc_reference() without influencing code practices of
> the original context owner.
> 
> This is what I call the Sam's modified/corrected proposal.

Again, I strongly support this, as it seems to me that it makes
the simple things usable very simply and forces the users of the
more advanced features to be a little more concious about what
they are doing. This way, the code can become correct
(unambiguous) while still preserving the features.

I am deliberately not going into technical details
since I wanted to emphasize this superficial higher level
point of view of the callers.

Cheers - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20090727/d34e0e90/attachment.pgp>