[PATCH 0/13] add SMB2 server-side copy support - V2

[Jeremy Allison]

On Tue, Nov 13, 2012 at 04:16:24PM +0100, David Disseldorp wrote:
> This change-set adds support for the FSCTL_SRV_COPYCHUNK and
> FSCTL_SRV_REQUEST_RESUME_KEY server-side copy SMB2 ioctls, allowing
> for network round-trip avoidance during a file copy, i.e.:
> http://www.samba.org/~ddiss/slides/snappery_samba/img17.html
> 
> Changes since V1:
> - Replace failure returns with torture_fail()/_skip()
> - Free tevent subreq after receive, rather than waiting for parent recv
> - Remove unneeded subreq from vfs_time_audit.c copychunk state
> - Alloc tevent reqs for copychunk tracking in skel VFS example code
> - Minor white space and debug output cleanups
> 
> Feedback appreciated.

Hi David,

Sorry it's taken a while to get to this review. But there
are a couple of changes I'd still like to see (sorry).

In this patch:

    smb2_ioctl: add support for FSCTL_SRV_COPYCHUNK
    
    SMB2 clients can issue FSCTL_SRV_COPYCHUNK requests in order to copy
    data between files on the server side only, rather than reading data
    then writing back from the client. FSCTL_SRV_COPYCHUNK is used by
    default for Explorer SMB2 file copies on Windows Server 2012.
    
    2.2.32.1 SRV_COPYCHUNK_RESPONSE in [MS-SMB2] describes the requirement
    for the server to provide maximum copychunk request size limits in ioctl
    responses carrying STATUS_INVALID_PARAMETER.

We're actually doing a write operation (via the COPYCHUNK request).
However, there is no CHECK_WRITE(fsp) call on the destination
fsp. This function checks that the fsp is open for writing and
writing is allowed on this share - without it we could be writing
to a file opened without FILE_WRITE access.

I think inside fsctl_srv_copychunk_send() you should have a check
similar to the one inside source3/smbd/smb2_write. Look for
the CHECK_WRITE macro inside that file for details.

Secondly in this patch:

    s3-vfs: add copy_chunk vfs hooks
    
    copy_chunk copies n bytes from a source file at a specific offset to a
    destination file at a given offset. This interface will be used in
    handling smb2 FSCTL_SRV_COPYCHUNK ioctl requests.
    
    Provide send and receive hooks for copy chunk VFS interface, allowing
    asynchronous behaviour.

There's a segment:

--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -2157,11 +2157,33 @@ NTSTATUS smb_vfs_call_fsctl(struct vfs_handle_struct *handle,
                            uint32_t *out_len)
 {
        VFS_FIND(fsctl);
-       return handle->fns->fsctl_fn(handle, fsp, ctx, function, req_flags, 
-                                    in_data, in_len, out_data, max_out_len, 
+       return handle->fns->fsctl_fn(handle, fsp, ctx, function, req_flags,
+                                    in_data, in_len, out_data, max_out_len,
                                     out_len);

Looks like just a whitespace fix to me. Can you make that a separate
change please.

Hope this is ok - shouldn't be too many more goes
around before we get this into master.

Cheers,

Jeremy.