NamedPipe DCE/RPC struct split

[Andrew Bartlett]

Luke Kenneth Casson Leighton wrote:
> 
> okay.
> 
> i'm happy with this one.

Well it looks ok, but I have some basic problems:

* Firstly, it didn't compile.  Now really, a patch supplied for commit
to the tree should at least compile.  (read_from_pipe got an extra arg).

* Secondly, there are basic issues:  Like the pipes_struct that
contains:

	struct smb_np_struct *p; /* back-link to originating smb connection */

But this never gets filled in.

These functions are a good idea - but also illustrate another problem:

+
+uint16 pipe_internal_get_vuid(pipes_struct *p)
+{
+       return p->p->vuid;
+}
+
+struct connection_struct *pipe_internal_get_conn(pipes_struct *p)
+{
+       return p->p->conn;
+}
+

What the heck is p->p ?  Single letter names just make it harder to
follow.  (Try 
np_backlink).

* I have some design issues:

What is with:
 pipes_struct *make_internal_rpc_pipe_p(char *pipe_name, 
			      connection_struct *conn, uint16 vuid)

If that back-link (once filled in) already gives the conn and vuid? 
(There probably is a valid reason, but commenting on it make it easier.

* Similarly, there are some other issues of taste (which you can't have
been expected to know about):

Why malloc a structure, then place a talloc context on that structure? 
Sure, you can take care of the free at the same time but if you talloc()
it, and use 'talloc_init_named()' you can give the memory tacking system
a better idea whats using the memeory.  

* And other things that you should have known about:

If a malloc fails, try to complain to the logs.  Most mallocs/tallocs in
Samba are followed by 'DEBUG(0, ("malloc failed!\n"));' at least.

(Even better is to return an appropriate NT_STATUS, like
NT_STATUS_NO_MEMORY so the other end doesn't get something strange like
'ERRnofids').

On the smb_np_struct there is a void *np_state, designed to be operated
on only by its own particular functions, which know what kind of object
it really is.  These should be included as function pointers on the
structure - effectively re-typing the void* and removing its
complications.

If this isn't to be done at this stage then type it properly, and void*
it later (leave a comment if you want).

* Nitpicks that should have been cleaned up (even if in original code):

Don't use a variable for two purposes - ie once as a normal function
variable and once again as a temp in a loop, just declare a new one and
use it (srv_pipe_hnd.c:213)

If you can improve on some of these issues the patch may well make it
into the tree.

Finally, please don't take these as anything more than what they are: 
remarks on a patch that I have been asked to commit.  I have published
them publicly because I feel that others may find them useful if they
too want me to apply a patch to the tree.

Andrew Bartlett

-- 
Andrew Bartlett                                 abartlet at pcug.org.au
Manager, Authentication Subsystems, Samba Team  abartlet at samba.org
Student Network Administrator, Hawker College   abartlet at hawkerc.net
http://samba.org     http://build.samba.org     http://hawkerc.net