[RFC] Making talloc_reference() safer.

[ronnie sahlberg]

On Tue, Oct 25, 2011 at 8:57 PM, Volker Lendecke
<Volker.Lendecke at sernet.de> wrote:
> On Mon, Oct 24, 2011 at 04:46:27PM -0700, Jeremy Allison wrote:
>> On Mon, Oct 24, 2011 at 05:50:38PM +1100, ronnie sahlberg wrote:
>> >
>> > Even better I think would be if you had a new set of creator functions
>> >  where you could set the "no longer hierarical, now its multi-parent
>> > referenced" already from the initial creation. That would be even
>> > better imho  to force the choice of api at object creation time and
>> > not create it as ahierarical object and then later "upgrade" it to a
>> > different model.
>> >
>> >
>> > I.e. offer something like this
>> > talloc_new_ref(...)
>> > instead of using the pattern
>> > talloc_new(...)
>> > ...
>> > talloc_make_reference_ptr(...)
>>
>> Yes, this makes much more sense to me too.
>
> On further thought, I might be okay with calls like
>
> talloc_set_referencable()
> talloc_unset_referencable()
> talloc_is_referencable()
>
> talloc_reference() would return NULL for a non-referencable
> object, gensec could return EINVAL by first checking via
> talloc_is_referencable(). talloc_free() on a referencable
> object would just abort. Abort to me is preferable over just
> leaking because this makes sure we don't leak by accident.
>
> I still don't like talloc_reference() at all and I would
> much rather live without it, but with the set of calls it
> can become a very obvious part of an API. It complicates it,
> but if API developers see it as necessary, so be it. We can
> always work on alternatives.
>
> Why no talloc_new_referencable() as I first thought? This
> would trickle up all credential_init() style calls and
> unnecessarily clutter those APIs for non-referencing use.

The drawback is that without forcing the choice of API ar creation time you
have situation where an object is sometimes a hierarical object and at
other times it suddently switches to a different model and a different
api.
What if this is changed in an external module I call and that module
decides for me "your object is now using a completely different
model".

The implication then is that all codepaths will have to change for
these objects like this

from
talloc_free(object)

into

if (talloc_is_referenced(object)) {
     talloc_unref(parent, object)
} else {
    talloc_free(object)
}

Or similar for "talloc_steal()"  that would conditionally now become
talloc_ref() instead.
Teh horrors.

I personally think that the "hierarcy" vs "multi-parent" has to be
made at creation time and creation time only.


A bigger picture is that TALLOC is a hugely useful library which
should be encouraged to be used also externally in non-samba projects
as well.
So API is really important. It is even more important that just "what
API change is needed to solve a problem in samba", it is "what API
makes sense to the average OSS developer working on his/her pet
project".


regards
ronnie sahlberg


>
> With best regards,
>
> Volker Lendecke
>
> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
>