NamedPipe DCE/RPC struct split

[Luke Kenneth Casson Leighton]

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).
 
 ... *thinks*... there's a second call to read_from_pipe()
 isn't there?

> * 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).

*thinks*....

there's a better way to do this one.

- to get rid of the smb_np_struct *p.

- to store yet another copy of the vuid and conn in
  the pipes_struct

- to get rid of pipe_internal_get_vuid and
pipe_internal_get_conn.



> * 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.
 
 the pipes_struct needs to know about the conn and the vuid,
 for use in locations where p->vuid and p->conn are accessed.

> * 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.  

[you'll have to talk to andrew and jeremy about that: this patch
has no relevance to that question.]

> * 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.

[again, the cut/paste job i did doesn't _have_ a debug(0,"malloc
failed.  if you add one in the non-patched, i'll cut/paste it
to the patch!]

> (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.
 
 correct.  that's to be done as part of the "dso"ing.

> 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.

i am very sorry: i will not do any pre-cleanups, i'm sorry.

i do not want this patch to a) look more complicated b) do more than
one task c) get rejected on non-technical grounds not related
to the patch's purpose.

> Finally, please don't take these as anything more than what they are: 
> remarks on a patch that I have been asked to commit.  

yehyeh, of course not.

> 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.

well, could you, or someone, address the issues you raise that
aren't directly related to the purpose of the NamedPipe API patch?

remember that afaiac'd, this is a minimal effort here to let me
focus on other work.

let me know?

ta andrew.

luke