master4-dcerpc-ok

Stefan (metze) Metzmacher metze at samba.org
Wed Mar 19 04:11:05 MDT 2014


Am 19.03.2014 09:53, schrieb Andreas Schneider:
> On Wednesday 19 March 2014 09:45:26 Stefan  Metzmacher wrote:
>> Am 19.03.2014 08:43, schrieb Andrew Bartlett:
>>> On Wed, 2014-03-19 at 07:33 +0100, Stefan (metze) Metzmacher wrote:
>>>> Hi Andrew,
>>>>
>>>>>>>> here's new stuff in my
>>>>>>>> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/
>>>>>>>> master4-dcerpc-ok branch.
>>>>>>>>
>>>>>>>> Please review and push:-)
>>>>>>>
>>>>>>> There're some updates in this branch, which also fix the regressions
>>>>>>> Andrew found.
>>>>>>
>>>>>> Any idea why didn't our tests find that regression?
>>>>>>
>>>>>> Andrew Bartlett
>>>>>
>>>>> 305f97ceb5bc2c00d2e1725a3e45482d4e59bac2
>>>>>
>>>>>  s4:libcli/clilsa: make use of dcerpc_pipe_connect_b()
>>>>>
>>>>> Can you explain what calling this over and over with the same binding
>>>>> the "connection" does?
>>>>>
>>>>> 	status = dcerpc_binding_set_pointer_option(binding,
>>>>>
>>>>> +						   "connection",
>>>>> +						   struct smbXcli_conn,
>>>>> +						   cli->transport->conn);
>>>>> +	if (!NT_STATUS_IS_OK(status)) {
>>>>> +		talloc_free(lsa);
>>>>> +		return status;
>>>>> +	}
>>>>> +
>>>>> +	status = dcerpc_binding_set_pointer_option(binding,
>>>>> +						   "connection",
>>>>> +						   struct smbXcli_session,
>>>>> +						   cli->session->smbXcli);
>>>>> +	if (!NT_STATUS_IS_OK(status)) {
>>>>> +		talloc_free(lsa);
>>>>> +		return status;
>>>>> +	}
>>>>> +
>>>>> +	status = dcerpc_binding_set_pointer_option(binding,
>>>>> +						   "connection",
>>>>> +						   struct smbXcli_tcon,
>>>>> +						   lsa->ipc_tree->smbXcli);
>>>>>
>>>>> There is a similar pattern in 04061bb3bf6d63cfa76028a6c878df9d3fe7af2c
>>>>> s4:torture/samba3rpc: split out pipe_bind_smbXcli() and go via
>>>>> dcerpc_pipe_connect_b()
> 
> 
> I would argue that there are important comments in the code missing to 
> understand this ...
> 
> 
>>> What I don't get is why we have a string "connection" here, that doesn't
>>> change (so why isn't it a enum or at least a #define), and then a magic
>>> type, which you would think would be only a way to ensure the void*
>>> being passed in was correct, but I presume is used as part of a
>>> key-value list somewhere.
>>
>> Yes, see the commit message of
>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=34e2c0762950210a
>> c6ef5dd40a5b4211ca595b4e
> 
> This should be doxygen documentation of the function!! The last patchset I 
> reviewed I gave a NAK if a public function had no doxygen documentation.
> 
> I could review the patchset too or Andrew you give the NAK for missng 
> documentation.

I'll add some doxygen stuff...

metze


More information about the samba-technical mailing list