Hi Jeremy,

> - Log -----------------------------------------------------------------
> commit 6beba782f1bf951236813e0b46115b8102212c03
> Author: Jeremy Allison <j...@samba.org>
> Date:   Mon Apr 26 10:54:33 2010 -0700
> 
>     Fix crash when rescheduling oplock open.

> +     /*
> +      * This is subtle. We must null out the callback
> +      * before resheduling, else the first call to
> +      * tevent_req_nterror() causes the _receive()
> +      * function to be called, this causing tevent_req_post()
> +      * to crash.
> +      */
> +     tevent_req_set_callback(smb2req->subreq, NULL, NULL);
> +
>       im = tevent_create_immediate(smb2req);
>       if (!im) {
>               smbd_server_connection_terminate(smb2req->sconn,

I'm not sure this is correct.

I haven't looked in detail, but this looks like just hiding the real
problem.

The real problem is likely that we're abusing the tevent_req guidelines.

I think 8f67f873ace91964da066c421986e260aceba75b is maybe ok, for
getting stuff working, but I'd like to see the design changed.

smb2_deferred_open_timer() should not call smbd_smb2_request_dispatch().

The re-entrant should happen inside the smbd_smb2_create_* code,
the place were it decides to go async, instead of two layers above.

I think the smbd_smb2_create_* should setup a
smb2req->retry_callback(struct tevent_req *) function pointer.
smb2_deferred_open_timer() would then just call it should just call it.

I'd like to have something similar for smb1 (I know it would be a lot of
work), but the layer violation is really confusing.

The top level smb1/2 server code should not see any of this retry logic,
it should just do a foo_send() call set it's callback
on the returned tevent_req and get the final result with foo_recv().
All magic should be in one spot in the lower level.

metze

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to