irpc changes

[Stefan (metze) Metzmacher]

Hi Andrew,

>>> In your recent irpc changes, why did you remove the convenience
>>> functions and macros? I can understand the underlying handle changes,
>>> but the resulting API that is now used has become a lot more verbose,
>>> for no obvious reason. 
>>
>> Because I wanted to get rid of 'struct irpc_request' being public.
>>
>> If you really want we could add an IRPC_CALL() macro for the sync case,
>> but I prefer that it's clear that the normal dcerpc bindings are used.
> 
> I've never really understood quite what the benefit these bindings are
> to the caller.  Can you fill me in on the details?  I know it's part of
> your plan to have the right place to keep state etc, but I don't really
> understand it more than that. 

RPC calls should work always the same, it doesn't matter if we use
the s4 dcerpc client, s4 irpc, s3 dcerpc client, s3 internal rpc
or s3 winbind parent/child.

We're now able to use the same pidl generated client stubs for all of them.

One important thing was that the binding handle is a private structure
which callers can't dereference. This will hopefully make the api
a bit more stable for external users like openchange in future.

It also matches the data model of the DCERPC specification,
it's similar to the opaque 'handle_t', which represents a explicit or
implicit
binding handle.

This change will make it possible to possibly share helper functions,
e.g. for setting a password on a samr binding handle between s3 and s4.
As the same function would work with a s3 or s4 dcerpc client connection.

This change will also makes it possible (/much easier) to unify the
s3 and s4 dcerpc client libraries.

I want to implement a simpler, but more powerful dcerpc client library,
which will allow us to implement "dcerpc pipes" (not named pipes),
see the OpenGroup DCERPC spec section "4.2.14 pipes" and MS-RPCE
2.2.5.3.4.5 Pipes.

The 'pipe' feature is needed in MS-FRS2 (the DFS-R Sysvol Replication),
see the BYTE_PIPE in RawGetFileDataAsync() and RdcGetFileDataAsync().

I also try to prepare our infrastructure to support dcerpc "callback"
functions,
too. I may not implement that initialy, as I haven't seen any existing users
of it, but we need to take this feature into account when designing the
new dcerpc layer.
It means the core dcerpc code needs to be generic and act as client and
server at the same
time.

>>> For example, where we previous had a irpc_call_send() we now have
>>> about 25 lines of code. 
>>
>> I guess you mean the changes in pymessaging...
>>
>> but irpc_call_send was removed and this place would be the only
>> caller of it, but that had implied that it's needed to keep irpc_request
>> public.
> 
> Is that a problem?  In a lot of the code I've been doing with tridge, we
> often added helper functions for just one use - but we soon found we had
> more callers.  In this case think keeping the ability to do a simple
> sync IRPC is still quite a valuable thing. 

Sure, but I don't want this style of helper functions, when only the generic
python code needs it.

The new methods for sync IRPC calls are still as simple as before.
See from commit c34cae81fee5e3b68746f9da97496bf056ff9d55

@@ -979,19 +979,25 @@ static int rootdse_become_master(struct ldb_module
*module,
                                 uint32_t role)
 {
        struct drepl_takeFSMORole r;
-       struct server_id *sid;
        struct messaging_context *msg;
        struct ldb_context *ldb = ldb_module_get_ctx(module);
        TALLOC_CTX *tmp_ctx = talloc_new(req);
        struct loadparm_context *lp_ctx = ldb_get_opaque(ldb, "loadparm");
        NTSTATUS status_call, status_fn;
+       struct dcerpc_binding_handle *irpc_handle;

        msg = messaging_client_init(tmp_ctx,
lpcfg_messaging_path(tmp_ctx, lp_ctx),
                                    ldb_get_event_context(ldb));

-       sid = irpc_servers_byname(msg, tmp_ctx, "dreplsrv");
+       irpc_handle = irpc_binding_handle_by_name(tmp_ctx, msg,
+                                                 "dreplsrv",
+                                                 &ndr_table_irpc);
+       if (irpc_handle == NULL) {
+               return ldb_oom(ldb);
+       }
        r.in.role = role;
-       status_call = IRPC_CALL(msg, sid[0], irpc, DREPL_TAKEFSMOROLE,
&r, NULL);
+
+       status_call = dcerpc_drepl_takeFSMORole_r(irpc_handle, tmp_ctx, &r);
        if (!NT_STATUS_IS_OK(status_call)) {
                return LDB_ERR_OPERATIONS_ERROR;
        }

The amount of code is exactly the same, but for me the new code is much more
easy to understand as it's a real function call instead of a magic macro.

metze


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100910/a06b2aa7/attachment.pgp>