proposition of further changes in dcerpc code

[Rafal Szczesniak]

On Tue, Dec 06, 2005 at 10:48:06AM +1100, tridge at samba.org wrote:
> Mimir,
> 
>  > Attached patch is a change leading towards fully async
>  > dcerpc_pipe_connect function. It contains work on connection
>  > routine via named pipe. Note, that I have separated functions
>  > into dcerpc_connect.c, as dcerpc_util.c grows too big doing
>  > many different things. I believe dcerpc connection functions
>  > put in a separate file make a clear split.
> 
> yes, I agree on splitting out the connect code. The split you've made
> looks sensible.
> 
> In dcerpc_pipe_connect_ncacn_np_smb_send() you should probably put the
> smb_composite_connect_send() call outside the SCHANNEL if
> statement. It's the same call in both branches of the if() so no need
> to duplicate it.

You're right. Thanks for the point. I overlooked that

> I'd also like to see a few comments. I think that composite functions
> really do need a few comments at minimum stating what stage of the
> connection process is reached in each function. So for example,
> explain at the top of continue_smb_connect() that this is called when
> the SMB socket is established, and the IPC$ share open, and that the
> remaining task is to open the pipe.

Absolutely! I always put comments after initial coding. I wanted to hear
a few opinions and correct mistakes first. Comments will follow right away.

> I think you are also missing a couple of places where allocation
> failures should be checked for. For example, add a composite_nomem()

Yes, Metze also pointed lack of composite_nomem in places.

> check after copying the called_name. Apart from that the code looks
> good and I think you should commit. I think the structure interface is
> fine for this call, although if you'd prefer the function parameter
> approach then I won't object.

No, I like the structure interface here. The more because ncacn connect
functions use the same set of arguments.

> Have you thought about where the epm map calls will fit in?

If you mean the source file, I think they're going to stay in
dcerpc_util.c unless I (or someone else) think of something better.

> I know you're concentrating on ncacn_np for now, where epm isn't needed, but
> it might be worth thinking about what the API will look like when epm
> is taken into account.

Sure, I'll see how it looks when I start working on other ncacn connect
functions.

Thank you for comments!


cheers,
-- 
Rafal Szczesniak
Samba Team member  http://www.samba.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.samba.org/archive/samba-technical/attachments/20051206/bfce1ee0/attachment.bin