master4-dcerpc-ok

Andreas Schneider asn at samba.org
Wed Mar 19 02:53:08 MDT 2014


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.



	-- andreas

-- 
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             asn at samba.org
www.samba.org



More information about the samba-technical mailing list