[PATCH] Explain a bit of how loadparm works

[Simo]

On Wed, 2013-10-16 at 10:50 +0200, Volker Lendecke wrote:
> On Wed, Oct 16, 2013 at 10:45:58AM +0200, Michael Adam wrote:
> > On 2013-10-16 at 10:27 +0200, Michael Adam wrote:
> > > On 2013-10-16 at 09:00 +0200, Volker Lendecke wrote:
> > > >
> > > > One thing sticks out
> > > > immediately though is the requirement to use talloc_unlink.
> > > > This needs to go ASAP. This is not acceptable. Sorry to be
> > > > so strict, but talloc_reference/talloc_unlink is just a
> > > > complete no-go in central code. Please focus on removing
> > > > that very, very soon.
> > > 
> > > I absolutely agree generally, but I think talloc_unlink was
> > > mentioned here just by habit instead of talloc_free:
> > > Looking at the implementation of loadparm_init_s3():
> > > 
> > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > struct loadparm_context *loadparm_init_s3(TALLOC_CTX *mem_ctx,
> > > 					  const struct loadparm_s3_helpers *s3_fns)
> > > {
> > > 	struct loadparm_context *loadparm_context = talloc_zero(mem_ctx, struct loadparm_context);
> > > 	if (!loadparm_context) {
> > > 		return NULL;
> > > 	}
> > > 	loadparm_context->s3_fns = s3_fns;
> > > 	return loadparm_context;
> > > }
> > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > it seems that talloc_free() woule be safe on the return value, right?
> > 
> > Oops, apparently not right:
> > 
> > in lpcfg_gensec_settings(), a
> > settings->lp_ctx = talloc_reference(settings, lp_ctx);
> > is done...
> > 
> > We should really get rid of this!
> 
> And this is *EXACTLY* the reason why talloc_reference is so
> evil and needs to be die, die, die! You fell into the trap
> looking at the allocator, and somewhere completely distant
> your nice talloc hierarchy is destroyed.
> 
> Andrew, please remove the talloc_reference from the loadparm
> code as your next priority. Otherwise we will have to work
> on unmerging that again.

+1 the evilness of talloc_reference is that an arbitrary *user* of the
memory can change the semantics for *all* others, this is a recipe for
disaster and the reason this API must be discouraged and eventually
dumped.

Simo.