master4-dcerpc-ok

[Stefan (metze) Metzmacher]

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()
>>
>> 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. 

Yes, see the commit message of
https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=34e2c0762950210ac6ef5dd40a5b4211ca595b4e

> librpc/rpc: add dcerpc_binding_set_pointer_option()
>
> This allows the caller to set a talloc pointer
> as [<name>:pointer:<pid>:<type>=<address>]
> (e.g. [connection:pointer:12345:struct smbXcli_conn=0xf0123456789abcde])
> option of a dcerpc_binding.
>
> Callers have to be careful to keep the pointer valid!
>
> Signed-off-by: Stefan Metzmacher <metze at samba.org>

What about having a #define DCERPC_BINDING_CONNECTION_PREFIX "connection".
The idea behind this prefix is that, I'd like to have some kind of
dcerpc_binding_reset(binding, flags) function, which would get a
flag to reset all connection related options, it would scan the option
list and remove all options starting with "connection:".

In addition I may add a function that replaces dcerpc_binding_dup(),
instead it would take arguments, which specific which options should
be copied. As I made dcerpc_pipe->binding const lately, I'd also make
this private
and hide dcerpc_pipe completely behind the dcerpc_binding_handle
abstraction.
and just have a function (maybe dcerpc_binding_handle_get_binding())
that also takes the flags arguement and just ask for a copy of the binding
including the specified options.

What about having a wrapper function e.g.

NTSTATUS dcerpc_binding_set_smbXcli_pointers(struct dcerpc_binding *b,
					     struct smbXcli_conn *conn,
					     struct smbXcli_session *session,
					     struct smbXcli_tcon *tcon);

NTSTATUS dcerpc_binding_set_smbXcli_pointers(const struct dcerpc_binding *b,
					     struct smbXcli_conn **conn,
					     struct smbXcli_session **session,
					     struct smbXcli_tcon **tcon);

> 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. 

We could also introduce wrapper functions like

const char *dcerpc_binding_get_endpoint(const struct dcerpc_binding *b)
{
     return dcerpc_binding_get_string_option(b, "endpoint");
}

But the generic dcerpc_binding_[g|s]et_string_option() functions are needed
in order to make dcerpc_binding private.

metze