[QUICK] talloc bugs

[simo]

On Wed, 2009-07-01 at 21:08 +0200, Stefan (metze) Metzmacher wrote:
> Jeremy Allison schrieb:
> > On Wed, Jul 01, 2009 at 08:27:38AM -0400, simo wrote:
> >> On Wed, 2009-07-01 at 22:12 +1000, tridge at samba.org wrote:
> >>> Hi Simo,
> >>>
> >>>  > In any case I'd like to stress we need to maintain ABI compatibility, so
> >>>  > whatever the decision I think we need still quite some work on what was
> >>>  > currently committed (possibly with a revert first).
> >>>
> >>> To justify that please point me at a valid use of talloc that was in
> >>> our tree that the change broke. 
> >>>
> >>> The change affected code that was already ambiguous. Ambiguous code is
> >>> something we always want to avoid, and we should not be preserving
> >>> ambiguous APIs.
> >>>
> >>> Regarding the ABI, yes I broke that as talloc_free was a function and
> >>> is now a macro.
> > 
> > I don't think we can do this. We need to keep the ABI stable
> > for external users just like we do with libsmbclient.
> 
> I agree to this!
> 
> If all we need is to keep the external symbols intact, then maybe this
> is all we need:
> 
> diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
> index e68b653..0109123 100644
> --- a/lib/talloc/talloc.c
> +++ b/lib/talloc/talloc.c
> @@ -1865,3 +1865,17 @@ int talloc_is_parent(const void *context, const
> void *ptr)
>         }
>         return 0;
>  }
> +
> +#undef talloc_free
> +int talloc_free(void *ptr)
> +{
> +       return _talloc_free(ptr, __location__": compat talloc_free()");
> +}
> +
> +#undef talloc_reference
> +void *talloc_reference(const void *context, const void *ptr)
> +{
> +       return _talloc_reference(context, ptr,
> +                    __location__": compat talloc_reference()");
> +}
> +

No this does fix it only for talloc_free()

The symbols that changed are:
talloc_free() AWOL
_talloc_steal()
_talloc_reference()

what we can do is keep _talloc_steal() and _talloc_reference() with the
original prototypes and add _talloc_steal_loc() and
_talloc_reference_loc() with the new prototypes. 

For talloc_free the #undef trick may be ok, although it is the change in
behavior that worry me the most.

> But this would only allow application build against an old version
> to work with the new version at runtime, but not the other way arround
> because the old library is missing the new _talloc_free() and
> _talloc_reference() symbols.

What matters is that old apps running against a new libtalloc do not
break. The other way around is not important, as new apps can require a
new version of talloc and you should be able to safely install it
because older applications will not break.

Form an API point of view we still need to verify if apps will break at
runtime. The changes that have been done seem to imply old apps may
break or get memory leaks if they do not check the return from
talloc_free().
If this happens only when talloc_reference() is used that might be
acceptable I guess. After all we marked talloc_reference() as
deprecated, although if there's a way to avoid that it would be even
better.

We also need to remove the print to standard output and simply return an
error from talloc_free().

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>