hmm.. Re: talloc issues

[Simo Sorce]

On Tue, 2009-08-04 at 10:50 +0930, Rusty Russell wrote:
> On Fri, 31 Jul 2009 05:44:13 pm Sam Liddicott wrote:
> > Many libraries have their own free function that you have to use to free
> > data that the library allocates. None of this is ever obvious to a
> > casual user.
> 
> Yes, and this is one benefit of talloc; talloc_free is generic.
> 
> Note that this example *loses* that benefit.  Indirect free still works, but
> direct talloc_free doesn't (caller must use talloc_unlink).
> 
> (I think this is still true after your changes, no?).
> 
> > 2. reference counting (which you don't have to use)
> > #define malloc(s) talloc_zero(NULL, (s))
> > #define ref_inc(s) talloc_reference(NULL, (s))
> > #define ref_dec(s) talloc_unlink(NULL, (s))
> > #define free(s) talloc_free((s))
> > 
> > and suddenly you can reference count all your pointers - even char*,
> > with auto-freeing on zero refcount. Wow. Just like COM.
> 
> I think this is only a progression in your mind.  AFAICT references are
> not inherent to talloc at all.  A variant can be built as a separate layer
> on top of talloc (eg. http://ccan.ozlabs.org/info/talloc_link.html )
> 
> > When the problem gets more complicated than reference counting... then
> > it's simpler to start reference counting.
> > 
> > ..and talloc is the easiest way to reference count - you don't have to
> > change your structs for a start, and you don't have to port to C++ either.
> 
> I disagree.  To repeat myself, everyone needs to know you're using references,
> even independent code, so they don't simply talloc_free() something.
> 
> Back to code.  Imagine a previous version of upcache which didn't cache:
> 
> 	const char *get_upcase(const char *str)
> 	{
> 		char *ret = talloc_strdup(NULL, str);
> 
> 		if (ret) {
> 			unsigned int i;
> 			for (i = 0; ret[i]; i++)
> 				ret[i] = toupper(ret[i]);
> 		}
> 		return ret;
> 	}
> 
> When we added caching, we now need to make sure noone talloc_free()'s this.
> Tridge's "ban talloc_free" did this at runtime.  I think there's scope to
> argue that this code should be changed completely to break at compile time
> instead, (ie. return a "struct upcase" which is a real talloc object, and
> does the string refcounting internally in the destructor).

I am starting thinking that references should really be left out of main
talloc and simply added on top by whoever needs them. The reason is
simply that we seem to not get to an agreement about how talloc should
behave with references in the first place.
Until we all agree on what they are and how they should impact "basic"
talloc, most people will simply avoid them. This is bad because it means
most people will not understand/like talloc_reference() and it will
become one of those exotic things that tend to break when people that
use the feature are not around.
If we can catch all bugs with make test then it may still be ok, but if
things break at run-time then it becomes a real bad thing(TM), as that
will translate in bad experiences for users. We absolutely need to keep
improving stability, we've had our share of breakage but now that there
is some more competition we need to improve and can't afford to
introduce variables that makes the recipe more unstable.

Simo.