Bug 9173 - SMB2 compound request create/delete_on_close/close doesn't work as windows
SMB2 compound request create/delete_on_close/close doesn't work as windows
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.5
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 8622
  Show dependency treegraph
 
Reported: 2012-09-18 17:36 UTC by Stefan Metzmacher
Modified: 2012-09-28 08:22 UTC (History)
4 users (show)

See Also:


Attachments
Fix for master. (924 bytes, patch)
2012-09-19 18:43 UTC, Ira Cooper
jra: review+
metze: review-
Details
Patches for v4-0-test (33.85 KB, patch)
2012-09-22 23:48 UTC, Stefan Metzmacher
jra: review+
Details
Patchset for 3.6 (22.64 KB, patch)
2012-09-23 07:08 UTC, Michael Adam
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Metzmacher 2012-09-18 17:36:15 UTC
SMB2 compound request create/delete_on_close/close doesn't work as windows
Comment 1 Ira Cooper 2012-09-19 18:43:18 UTC
Created attachment 7908 [details]
Fix for master.
Comment 2 Jeremy Allison 2012-09-19 20:39:27 UTC
Comment on attachment 7908 [details]
Fix for master.

LGTM. Pushed to master. Patch also applies cleanly to 3.6.next.
Comment 3 Jeremy Allison 2012-09-19 20:39:50 UTC
Karolin please pick for 3.6.next.

Thanks !

Jeremy.
Comment 4 Jeremy Allison 2012-09-19 22:04:17 UTC
Oh hang on a minute... Things are a little more complicated. Working on this (sorry for the confusion Karolin :-).

Jeremy.
Comment 5 Stefan Metzmacher 2012-09-22 23:47:33 UTC
Comment on attachment 7908 [details]
Fix for master.

The full patchset will follow
Comment 6 Stefan Metzmacher 2012-09-22 23:48:22 UTC
Created attachment 7920 [details]
Patches for v4-0-test
Comment 7 Michael Adam 2012-09-23 07:08:18 UTC
Created attachment 7923 [details]
Patchset for 3.6

Patchset adapted for v3-6-test.
Comment 8 Michael Adam 2012-09-23 07:09:38 UTC
Metze and I have done these patchsets for v4-0-test and v3-6-test
based on Ira's initial patch.

Jeremy, would you review them?
Comment 9 Jeremy Allison 2012-09-25 00:50:32 UTC
Comment on attachment 7920 [details]
Patches for v4-0-test

Just wanted to let you know I'm still reviewing this. I think I need to understand tevent_req_defer_callback() better. I haven't found any good docs on that.

Jeremy.
Comment 10 Stefan Metzmacher 2012-09-25 07:13:29 UTC
(In reply to comment #9)
> Comment on attachment 7920 [details]
> Patches for v4-0-test
> 
> Just wanted to let you know I'm still reviewing this. I think I need to
> understand tevent_req_defer_callback() better. I haven't found any good docs on
> that.

With tevent_req_defer_callback() you setup an event context,
that will be used to schedule an immediate event from
within tevent_req_done(), tevent_req_error() and wrappers
and call tevent_req_notify_callback() from within the immediate event.

Normally you have tevent_req_done() and tevent_req_error() trigger
the callback inline, which means that after calling them the only
valid operation is to call "return", as all your state is likely to be
gone!

With tevent_req_defer_callback() before tevent_req_done/tevent_req_error
you're sure the callback will be triggered later and your state is still valid
after calling tevent_req_done/tevent_req_error.

From within a cancel function you should typically make use of
tevent_req_defer_callback(). And also in shutdown code like
smbXcli_conn_disconnect(), where we loop over a list of requests
we have to use it.

So related to this patchset it mainly replaces the custom immediate events
we had. And return the NT_STATUS_CANCELLED from the right layer.

Note the 3.6 patchset doesn't include this changes as we don't have
tevent_req_defer_callback() in 3.6.
Comment 11 Stefan Metzmacher 2012-09-25 07:29:37 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > Comment on attachment 7920 [details] [details]
> > Patches for v4-0-test
> > 
> > Just wanted to let you know I'm still reviewing this. I think I need to
> > understand tevent_req_defer_callback() better. I haven't found any good docs on
> > that.
> 
> With tevent_req_defer_callback() you setup an event context,
> that will be used to schedule an immediate event from
> within tevent_req_done(), tevent_req_error() and wrappers
> and call tevent_req_notify_callback() from within the immediate event.

There's also a doxygen comment for the function, if it's not enough for you
please propose a patch to add more once you understand it.

/**
 * @brief Finish multiple requests within one function
 *
 * Normally tevent_req_notify_callback() and all wrappers
 * (e.g. tevent_req_done() and tevent_req_error())
 * need to be the last thing an event handler should call.
 * This is because the callback is likely to destroy the
 * context of the current function.
 *
 * If a function wants to notify more than one caller,
 * it is dangerous if it just triggers multiple callbacks
 * in a row. With tevent_req_defer_callback() it is possible
 * to set an event context that will be used to defer the callback
 * via an immediate event (similar to tevent_req_post()).
 *
 * @code
 * struct complete_state {
 *       struct tevent_context *ev;
 *
 *       struct tevent_req **reqs;
 * };
 *
 * void complete(struct complete_state *state)
 * {
 *       size_t i, c = talloc_array_length(state->reqs);
 *
 *       for (i=0; i < c; i++) {
 *            tevent_req_defer_callback(state->reqs[i], state->ev);
 *            tevent_req_done(state->reqs[i]);
 *       }
 * }
 * @endcode
 *
 * @param[in]  req      The finished request.
 *
 * @param[in]  ev       The tevent_context for the immediate event.
 *
 * @return              The given request will be returned.
 */
Comment 12 Jeremy Allison 2012-09-25 16:39:04 UTC
Yes I read the doxygen comment, thanks. Once I fully understand it I'll try and update the docs. Thanks metze !

Jeremy.
Comment 13 Stefan Metzmacher 2012-09-25 18:10:04 UTC
(In reply to comment #7)
> Created attachment 7923 [details]
> Patchset for 3.6
> 
> Patchset adapted for v3-6-test.

Karolin, I think this also fixes the smb2.compound error
you always had in make test:-)
Comment 14 Jeremy Allison 2012-09-26 20:39:46 UTC
Comment on attachment 7920 [details]
Patches for v4-0-test

Ok, *NOW* I understand tevent_req_defer_callback() - it's using an immediate event internal to the req struct via tevent_req_post(). Phew, that is subtle - but really nice code !

Karolin please pick for 4.0.0rc.
Comment 15 Jeremy Allison 2012-09-26 21:58:06 UTC
Comment on attachment 7923 [details]
Patchset for 3.6

Looks good to me.
Comment 16 Jeremy Allison 2012-09-26 21:59:30 UTC
Re-assigning to Karolin for inclusion in 3.6.next and 4.0.0rc.next
Jeremy.
Comment 17 Karolin Seeger 2012-09-28 07:12:08 UTC
Pushed to autobuild-v4-0-test and v3-6-test.
Closing out bug report.

Thanks!
Comment 18 Karolin Seeger 2012-09-28 08:22:38 UTC
(In reply to comment #13)
> (In reply to comment #7)
> > Created attachment 7923 [details] [details]
> > Patchset for 3.6
> > 
> > Patchset adapted for v3-6-test.
> 
> Karolin, I think this also fixes the smb2.compound error
> you always had in make test:-)

It does indeed! :-)
Thanks a lot, Metze!