NamedPipe DCE/RPC struct split

[Luke Kenneth Casson Leighton]

okay, the ones that i _can_ deal with are attached.

the talloc context one is a "built-in" pipe design
issue that will have to be dealt with by jeremy / jf / gerry
etc.

which just really leaves the malloc comment and the
local-var one: the others are namedpipe-split related.

lukes

On Tue, Jan 15, 2002 at 07:30:02PM +1100, Andrew Bartlett wrote:
> 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
-------------- next part --------------
Index: rpc_server/srv_pipe_hnd.c
===================================================================
RCS file: /cvsroot/samba/source/rpc_server/srv_pipe_hnd.c,v
retrieving revision 1.72
diff -u -u -r1.72 srv_pipe_hnd.c
--- rpc_server/srv_pipe_hnd.c	5 Nov 2001 07:42:55 -0000	1.72
+++ rpc_server/srv_pipe_hnd.c	16 Jan 2002 10:44:23 -0000
@@ -122,7 +122,7 @@
 			      connection_struct *conn, uint16 vuid)
 {
 	int i;
-	pipes_struct *p;
+	pipes_struct *p, p_it;
 	static int next_pipe;
 
 	DEBUG(4,("Open pipe requested %s (pipes_open=%d)\n",
@@ -150,7 +150,10 @@
 	p = (pipes_struct *)malloc(sizeof(*p));
 
 	if (!p)
+	{
+		DEBUG(0,("ERROR! no memory for pipes_struct!\n"));
 		return NULL;
+	}
 
 	ZERO_STRUCTP(p);
 
@@ -238,8 +241,8 @@
 	chain_p = p;
 	
 	/* OVERWRITE p as a temp variable, to display all open pipes */ 
-	for (p = Pipes; p; p = p->next)
-		DEBUG(5,("open pipes: name %s pnum=%x\n", p->name, p->pnum));  
+	for (p_it = Pipes; p_it; p_it = p_it->next)
+		DEBUG(5,("open pipes: name %s pnum=%x\n", p_it->name, p_it->pnum));  
 
 	return chain_p;
 }