[PATCH] smb2 FSCTL_SRV_COPYCHUNK support

[Volker Lendecke]

On Tue, Mar 06, 2012 at 03:03:36PM +0100, David Disseldorp wrote:
> On Tue, 6 Mar 2012 09:28:03 +0100
> Volker Lendecke <Volker.Lendecke at SerNet.DE> wrote:
> 
> > Overall this looks very good! Two quick remarks:
> > 
> > Can you also augment the time_audit module? Next to
> > full_audit, it is another one where all functions should be
> > implemented.
> > 
> > The other one is in the implementation of the async engine.
> > You ordered the different _send, _done and _recv functions
> > so that you do not need prototypes. While this saves a few
> > lines, to me it is much easier to read if the functions are
> > lexically ordered according to their call sequence. First
> > the _send function, then the _done and after that the _recv
> > function. This makes the async flow almost as easy to read
> > as a non-async sequential flow. Can you change that? I know
> > there are examples in the code that do it differently, but I
> > think for new code we should adhere to this convention.
> 
> This round of changes addresses Volker's remarks above:
> - Add copy chunk hooks to vfs_time_audit
> - Reorder copy chunk _send, _done and _recv functions.
> One further change:

Functionally I think your patches are correct. All I have is
stylistic remarks:

660a15718a8fdd432abb0d66eca2de0ea01670d0 has
vfswrap_copy_chunk_send and _recv still in the wrong order.

Then this

+               if (cc_state->subreq == NULL) {
+                       cc_state->status = NT_STATUS_NO_MEMORY;
+               } else {
+                       /* wait for subreq completion */
+			 tevent_req_set_callback(cc_state->subreq,
+						 btrfs_copy_chunk_done,
+                                               req);
+                       return req;
+               }
+

is also kindof unusual. We have the convention to use

if (tevent_req_nomem(cc_state->subreq, req)) {
	return tevent_req_post(req, ev);
}
tevent_set_set_callback(...)

for this kind of error handling.

The following else can be eliminated, as all paths in the if
above return. You can also eliminate the

+       if (NT_STATUS_IS_OK(cc_state->status)) {
+               tevent_req_done(req);
+       } else {
+               tevent_req_nterror(req, cc_state->status);
+       }

in that routine further down, we don't have an error there.

In btrfs_copy_chunk_done, similarly I would do things a bit
differently:

+       if (NT_STATUS_IS_OK(cc_state->status)) {
+               tevent_req_done(req);
+       } else {
+               tevent_req_nterror(req, cc_state->status);
+       }

a bit differently:

if (tevent_req_nterror(req, cc_state->nt_status)) {
	return;
}
tevent_req_done(req);

Sorry to nit-pick, but I think this really makes a
difference for the reader. Async stuff is so complex that to
me every distraction from what's used elsewhere is a
major annoyance. Copying the list not to do finger-pointing,
this might also be instructive to others who want to touch
async programming using tevent_req.

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de