composite rpc call

[Andrew Tridgell]

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.

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


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

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)


(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;


(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:

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

(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);

   
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.

Cheers, Tridge