[PATCH] Make loadparm more common

[Andrew Bartlett]

On Wed, 2014-04-02 at 07:03 +0200, Volker Lendecke wrote:
> On Wed, Apr 02, 2014 at 11:43:13AM +1300, Andrew Bartlett wrote:
> > On Tue, 2014-04-01 at 08:43 +0200, Volker Lendecke wrote:
> > > On Tue, Apr 01, 2014 at 05:21:02PM +1300, Andrew Bartlett wrote:
> > > > At this point, what I would like to see is these patches reviewed:
> > > > http://git.catalyst.net.nz/gitweb?p=samba.git;a=commitdiff;h=5fa1a3cd62e0dcefc5364f83046db025fc0e65b9
> > > 
> > > This has a talloc_zero(NULL,...). Would it be possible to
> > > use talloc_tos() here?
> > 
> > No, I'm not willing to add any more talloc_tos() to this area of the
> > code.  Almost all the odd unexpected failures caused by this patch set
> > were due to new talloc_tos() calls, because not all callers had a
> > talloc_stackframe().  
> 
> talloc_tos() should not fail if there is no stackframe
> around. It should just print a warning. Can you explain in
> more detail how this fails?

TALLOC_CTX *_talloc_tos(const char *location)
{
	struct talloc_stackframe *ts =
		(struct talloc_stackframe *)SMB_THREAD_GET_TLS(global_ts);

	if (ts == NULL || ts->talloc_stacksize == 0) {
		_talloc_stackframe(location);
		ts = (struct talloc_stackframe *)SMB_THREAD_GET_TLS(global_ts);
		DEBUG(0, ("no talloc stackframe at %s, leaking memory\n",
			  location));
#ifdef DEVELOPER
		smb_panic("No talloc stackframe");
#endif
	}

	return ts->talloc_stack[ts->talloc_stacksize-1];
}

> > Additionally, it would have to be un-done again before the code is made
> > common later in the series, and we don't use talloc_tos() in the top
> > level code. 
> 
> Then new code should take a talloc context, so that the
> caller can pass a talloc_tos(). talloc(NULL,...) is pretty
> much a no-go these days in new code.

This is not new code.  This is a long-over due attempt to make a small
dent on some of the oldest, and I would argue ugliest code in Samba.
The scope of what can be achieved is therefore limited, particularly as
each patch tries to do exactly one small thing.  Even the small steps
taken so far have required a massive amount of work (hence ending up
with a 90-patch backlog, plus more unpublished).  

If you would like to dive in to this area, I can ask Garming to publish
his WIP tree so you can work on top of that. 

> > > > http://git.catalyst.net.nz/gitweb?p=samba.git;a=commitdiff;h=746cbfcd93c38e5bb66e892fa2ef0d0d11888703
> > > 
> > > Just for simplicity, can we make it such that the equivalent
> > > piece of code in source3/param/loadparm.c looks exactly the
> > > same? If we touch the code, we should unify the code lines
> > > 1:1.
> > 
> > I'm a little confused, as that is what is being done here, but just
> > isn't all done in this commit.  Eventually they become identical, then
> > merge. 
> 
> Garmings patch has
> 
> for (data = pserviceSource->param_opt; data != NULL; data = data->next) {
> 
> whereas the source3 copy has
> 
> data = pserviceSource->param_opt;
>         while (data) {
> 
> The for loop is arguably better, but it is syntactically
> different, so it will cause someone to look at the code
> later again. I just thought it might be good to make this
> syntactically the same now that someone looks at the code
> anyway. True, this is a minor nit-pick that I possibly
> should not have brought up to not waste bandwidth, sorry for
> that.

Thanks.

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba