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