NamedPipe DCE/RPC struct split
Luke Kenneth Casson Leighton
lkcl at samba-tng.org
Wed Jan 16 02:47:03 GMT 2002
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
which just really leaves the malloc comment and the
local-var one: the others are namedpipe-split related.
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
> 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
> * 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
> 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
> 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 --------------
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)
- 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));
+ DEBUG(0,("ERROR! no memory for pipes_struct!\n"));
@@ -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));
More information about the samba-technical