[PATCH] Make loadparm more common

[Garming Sam]

Hi,

It's just been a week since posting these loadparm patches and I'm 
wondering if I can get any further comments and reviews.

The only patches which have really changed since Andrew posted them are 
the first two in the series. The first no longer has talloc
call on NULL and the second adds some extra panics and debugs, as well 
as removing another talloc call on NULL.


Cheers,

Garming Sam

On 11/04/14 11:32, Garming Sam wrote:
> On 02/04/14 11:43, 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?
> This talloc actually gets removed a few patches later. I've just 
> squashed these
> two patches together now.
>
> There's one other place,  get_parametrics_by_service, which now uses a 
> talloc_stackframe.
>
>> http://git.catalyst.net.nz/gitweb?p=samba.git;a=commitdiff;h=909fb5a6c7df2e2cc7e48d5814eb2cff53ee6664 
>>
>>> While in general a good direction, this one has the snippet
>>>
>>> -                               SAFE_FREE(f->subfname);
>>> -                               f->subfname = SMB_STRDUP(n2);
>>> +                               TALLOC_FREE(f->subfname);
>>> +                               f->subfname = talloc_strdup(f, n2);
>>>                                  TALLOC_FREE(n2);
>>>
>>> SMB_STRDUP panics on failure, talloc_strdup does not. So we
>>> need NULL checks in these scenarios. I haven't checked all
>>> places where this is done.
>
> In the places where there was an SMB_STRDUP or similar call, there
> should now be panics and debugs. Notably in the lib/param code, there's
> a number of places where it does neither in exactly the same places. In
> such places, I've just put a debug instead of a panic since many of
> those places get merged fairly soon after (and that would preserve the
> lib/param behaviour).
>
> Is there a preference in a panic or a debug?
>
>
> Branch here:
>
> git://git.catalyst.net.nz/samba.git loadparm-talloc-to-review5
>
> http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=refs/heads/loadparm-talloc-to-review5 
>
>
> I've run a make test and it appears to pass all the tests.
>
>
> I really appreciate that people do want to see this code changed. Just 
> working with it,
> I've managed to come across some real subtleties in the code and 
> between the two
> loadparms. It's taken lots of little changes to fix some these, many 
> of which get nullified a
> little while later. But without these minor changes, it really would 
> be difficult to not get it wrong.