proposition of further changes in dcerpc code
mimir at samba.org
Tue Dec 6 00:10:27 GMT 2005
On Tue, Dec 06, 2005 at 10:48:06AM +1100, tridge at samba.org wrote:
> > 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
Thank you for comments!
Samba Team member http://www.samba.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: Digital signature
Url : http://lists.samba.org/archive/samba-technical/attachments/20051206/bfce1ee0/attachment.bin
More information about the samba-technical