master4-dcerpc-ok

[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()
> 
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=47b0885d5f3e9e3b46930c976078deb5fbf70da5
> 
> Makes use of it.
> 
> The point is that we had a lot of functions and layers to establish a
> dcerpc connection.
> Which was way to complex and very hard to rework when I'll switch to the
> new dcerpc infrastrature
> librpc/rpc/dcerpc_connection.c
> (https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-dcerpc-new
> has the work in progress for this.
> 
> I just want to have a single function to create a dcerpc connection,
> which will just
> take a dcerpc_binding. For now I've reduced it to two functions
> dcerpc_pipe_connect_b*
> and dcerpc_secondary_auth_connection*, while
> dcerpc_secondary_auth_connection() might be a wrapper
> around dcerpc_pipe_connect_b() later.
> 
> The idea is that the dcerpc_binding holds enough information to allow
> all needed ways
> to establish a connection.
> 
> The above example remembers the details of the ncacn_np transport layer
> (smbXcli_conn, smbXcli_session and smbXcli_tcon), which allow
> us to reuse an existing smb connection, so that the connection just
> starts with
> with smb1cli_ntcreate* or smb2cli_create* via
> tstream_smbXcli_np_open_send/recv
> and just opens a new handle to a named pipe, instead of opening a new
> smb connection.
> 
> So for now dcerpc_pipe->binding, will describe the current connection,
> including the transport "connection" details and the association group
> details,
> so that it has enough information to open a secondary connection.

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. 

The way I read the code, it keeps re-setting the same key-value pair,
where "connection" is the key.  I know you better than to assume that is
true, but we need the code to be clearer than that. 

Beyond that, I'm not a big fan of these generic functions (and the ones
already introduced) that change type-safe specific function pointers
that the compiler will check for run-time string based lookups.  I
didn't object earlier, but I still don't like it.  The things being
passed around here are well known in advance, and having extra functions
to set them (even if that means exposing specific types in the generic
interface) is still better in my eyes than what we loose in type safety
and compiler checking. 

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba