NamedPipe DCE/RPC struct split
abartlet at pcug.org.au
Tue Jan 15 00:34:05 GMT 2002
Luke Kenneth Casson Leighton wrote:
> 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
* 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 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
More information about the samba-technical