Samba 4 libnet_join and RPC-JOIN torture test

[Brad Henry]

Brad Henry wrote:

> Stefan Metzmacher wrote:
>
>> Am Montag, 12. September 2005 08:07 schrieb Andrew Bartlett:
>>  
>>
>>> On Mon, 2005-09-12 at 08:02 +0200, Stefan Metzmacher wrote:
>>>   
>>>
>>>> Am Montag, 12. September 2005 02:22 schrieb Brad Henry:
>>>>     
>>>>
>>>>> +       if (!lsa_open_policy.in.system_name) {
>>>>> +               r->out.error_string = NULL;
>>>>> +               talloc_free(tmp_ctx);
>>>>> +               return NT_STATUS_NO_MEMORY;
>>>>> +       }
>>>>>       
>>>>
>>>> please use if (!lsa_open_policy.in.system_name) goto no_mem;
>>>>
>>>> in all places, as you missed sometimes the talloc_free(tmp_ctx),
>>>>     
>>>
>>> Metze, I really don't like that style of code.  Yes, we should be
>>> consistent, and perhaps we should ensure that r->out.error_string = 
>>> NULL
>>>   
>>
>>
>> yep, that's what we should do
>>  
>>
>>> is done at the top, but I think the goto is ugly and overboard.
>>>   
>>
>>
>> but the problem is the talloc_free(tmp_ctx), which needs to be done,
>> and in client side functions (like this libnet_ functions) we can't 
>> just skip it.
>>
>>  
>>
>>> (The thing I most enjoy about the new talloc is that it killed most of
>>> that madness...)
>>>   
>>
>>
>> For server code where the whole in coming mem_ctx is destroyed, on 
>> failure,
>> we use just NT_STATUS_HAVE_NO_MEMORY(string);
>>
>> this gives us a nice way for a just one line of error handling.
>>
>> maybe we should add a bunch of macros, which take also a ppointer to 
>> the tmp_mem_ctx, and free's it before returning.
>>
>> some thing like this.
>>
>> NT_STATUS_HAVE_NO_MEMORY_TMPMEM(data, tmp_mem) do {
>>     if (!data) {
>>         if (tmp_mem) talloc_free(tmp_mem);
>>         return NT_STATUS_NO_MEMORY;
>>     }
>> } while (0)
>>
>> NT_STATUS_NOT_OK_RETURN_TMPMEM(status, tmp_mem) do {
>>     if (!NT_STATUS_IS_OK(status)) {
>>         if (tmp_mem) talloc_free(tmp_mem);
>>         return status;
>>     }
>> } while (0)
>>
>> then we would have a nice way of having just one line of error handling,
>> which makes the code much more readable, than having 4 or 5 lines of 
>> error handling for one line of real code.
>>
>>  
>>
> So if these macro's existed, I could do something like
>
> if (NT_STATUS_NO_MEMORY_TMPMEM(lsa_open_policy.in.system_name, tmp_ctx))
>        return NT_STATUS_NO_MEMORY;
>
> and
>
> if (NT_STATUS_NOT_OK_TMPMEM(status, tmp_ctx)) return status;
>
> right?
>
> I think that would be a lot nicer and more readable.  Should I add 
> these two macros to source/include/nt_status.h  then and change my 
> code to use them?
>
> Thanks,
> Brad
>
>
Here's the patch redone using NT_STATUS_HAVE_NO_MEMORY_TMPMEM(data, 
tmp_mem) and NT_STATUS_NOT_OK_RETURN_TMPMEM(status, tmp_mem).
I think it looks better.

What would you prefer to see me do about standardizing the rest of the 
error checking that I'm doing here? goto statements, different macros, 
something else?


Thanks,
Brad

-------------- next part --------------
A non-text attachment was scrubbed...
Name: SAMBA_4_0-join.diff
Type: text/x-patch
Size: 53985 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20050912/8a0c4f8c/SAMBA_4_0-join.bin