composite rpc call

[Rafal Szczesniak]

On Tue, Feb 22, 2005 at 02:00:10PM +1100, Andrew Tridgell wrote:
> Mimir,
> 
>  > Could you please take a look at source/libnet/userinfo.c file and tell
>  > me what is coded incorrectly. I haven't tested it yet, so almost
>  > certainly something is broken, but you surely have some opinion on
>  > that code just by looking.
> 
> I hope you don't mind me replying publicly. I thought that others
> might find the answers interesting.

Of course. It has some educational valor for potential samba hackers.

> Comments in no particular order:
> 
> (1)
> The "struct rpc_composite_userinfo" needs to be in a public header
> file, as it will be used by callers

Yes, I know. I've forgotten to commit this bit. It's been fixed already.

> (2)
> In the _send() function you copy the io structure that the user passed
> in. Then in the _recv() function you fill in that copy. That means
> that with your current code the user never gets to see the results of
> the call.

Oh, you're right. I got inspired by loadfile code too much.

> There are two easy solutions you can choose from for this:
> 
>   1) don't copy the io structure, and assume that the caller keeps it
>      valid (ie. doesn't go out of scope) during the whole call. This
>      is how the pidl-generated RPC calls work.
> 
>   2) add a io parameter to the _recv() call, and fill it in, thus
>      allowing the caller to use a different io structure pointer in
>      the _send() and _recv() calls. This is how I structured the
>      libcli/nbt/ client library.

This one definately seems to be more apropriate for libnet.

> Which pattern you use is up to you. I haven't been consistent between
> subsystems in how I handle this, although within each subsystem I have
> chosen one method or the other. I suggest you do the same. 
> 
> 
> (3)
> Lines like this:
>   memcpy((void*)r.in.user_handle, (void*)rep->out.user_handle, sizeof(struct policy_handle));
> should be replaced with this:
>   r.in.user_handle = rep->out.user_handle;
> 
> using memcpy for a structure copy is ugly and unnecessary (this
> applies to all the memcpy() calls in your userinfo.c code)

I don't like it too. I just wasn't sure if ordinary pointer assign can
potentially be freed between the calls. But if you say I can just copy
it, not the structure itself, then it's fine to me.

> (4)
> You need to check for failure for calls like this:
> 	sid              = dom_sid_parse_talloc(c, s->io.in.sid);
> add something like this:
>        if (sid == NULL) goto failed;

Sure. Added.

> (5)
> The line 
>    s->pipe->conn->pending = dcerpc_samr_OpenUser_send(p, c, r);
> is completely wrong. That is operating on internal queues that
> application code should never touch (plus it won't work). Instead,
> follow the examples in other parts of the code and do something like
> this:

That was one of my primary concerns as digging through the structure
to do such ordinary thing seemed a bit suspicous to be right.

>        s->req = dcerpc_samr_OpenUser_send(p, c, r);
>        if (s->req == NULL) goto failed;
>        s->req->async.callback = userinfo_handler;
>        s->req->async.private  = c;
> 
> and add:
>        struct rpc_request *req;
> into struct userinfo_state
> 
> (6)
> The line
> 		c->state = RPC_REQUEST_DONE;
> uses the wrong type of enum. A composite_context structure contains
> this:
>  enum smbcli_request_state state
> so you probably meant this:
>        c->state = SMBCLI_REQUEST_DONE;
> or
>        c->state = SMBCLI_REQUEST_ERROR;
> The same goes for all other places you use RPC_REQUEST_* 

So it is SMBCLI states anyway, since that's the transport for RPC calls,
do I get it correctly ?

> (7)
> In userinfo_openuser() you do this:
> 	struct rpc_request *req = s->pipe->conn->pending;
> that should be this:
> 	struct rpc_request *req = s->req;
> 
> (8) 
> The line
> 	rep = (struct samr_OpenUser*)req->ndr.struct_ptr;
> is no good. What you probably wanted was to declare:
>    struct samr_OpenUser *openuser;
> in struct userinfo_state, then allocate it in the _send() call like
> this:
>    s->openuser = talloc(s, struct samr_OpenUser);

It looks like I need to put rpc calls' structures in userinfo_state,
doesn't it ? At least a few of them.

> That's enough for now. Several of the above problems apply to more
> than one place. When you've fixed up the above, show me the code again
> and I'll take another look.

OK, thank you.


cheers,
-- 
Rafal Szczesniak
Samba Team member  http://www.samba.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.samba.org/archive/samba-technical/attachments/20050222/7cb3e0ac/attachment.bin