[PATCH] smb2 FSCTL_SRV_COPYCHUNK support

[David Disseldorp]

Hi Metze,

Thanks for your very thorough feedback, comments in-line...

On Mon, 12 Mar 2012 14:42:22 +0100
"Stefan (metze) Metzmacher" <metze at samba.org> wrote:

> Hi David,
> 
> > Did anyone get a chance to look at this round of changes?  
> 
> https://git.samba.org/?p=ddiss/samba.git;a=commitdiff;h=27ffc797af59a1344f044ebd5fb06f1d3c460320
> 
> still doesn't follow what Volker had requested.
> 
> struct copy_chunk_state,
> fsctl_srv_copychunk_send()
> vfs_copy_chunk_done(),
> fsctl_srv_copychunk_recv()
> doesn't have the same prefix

Fixed.

> 
> +static struct tevent_req *fsctl_srv_copychunk_send(struct
> tevent_context *ev,
> +                                                  struct tevent_req *req,
> +                                       struct smbd_smb2_ioctl_state *state)
> 
> The first 2 arguments to a _send() function, are *always*
> TALLOC_CTX *mem_ctx, struct tevent_context *ev!
> 
> a _send() function should never get a 'req' as argument,
> 'req' is it's internal request handle that it gives to its caller.

Fixed.

> Also 'smbd_smb2_ioctl_state' is the *private* state of the
> smbd_smb2_ioctl_send/recv function, it should be only visiable.
> in smbd_smb2_ioctl_*() functions.

Discussed later...

> +       subreq = tevent_req_create(state, &cc_state,
> +                                  struct copy_chunk_state);
> +       if (subreq == NULL) {
> +               return NULL;
> +       }
> 
> This should be 'req' instead 'subreq', as from the point of view
> of the current function this is the current request.

Changed.

> Also the state structure needs to match the function name,
> so it should be 'struct fsctl_srv_copychunk_state'.
> The name of the variable is always 'state', which references
> the state of the current function.
> So it should be:
> 
>     req = tevent_req_create(mem_ctx, &state,
>                             struct fsctl_srv_copychunk_state);
>     if (req == NULL) {
>             return NULL;
>     }
> 
> +       if (ndr_ret != NDR_ERR_SUCCESS) {
> +               DEBUG(0, ("failed to unmarshall copy chunk req\n"));
> +               cc_state->status = NT_STATUS_INVALID_PARAMETER;
> +               goto err_post_status;
> +       }
> +
> 
> Please inline the code without goto here.
> 
> +       tevent_req_nterror(subreq, NT_STATUS_INVALID_PARAMETER);
> +       return tevent_req_post(subreq, ev);
> 
> This is important because tevent_req_nterror() records
> the current location using the '__location__' macro.
> 
> In gdb 'p req->internal.finish_location' will tell you the file,
> where the function failed.

Changed.

> 
> +static void vfs_copy_chunk_done(struct tevent_req *subreq)
> 
> This should be named fsctl_srv_copychunk_done()
> or fsctl_srv_copychunk_vfs_done().

Renamed.

> +static void fsctl_srv_copychunk_done(struct tevent_req *subreq)
> 
> This has nothing to do with the rest of fsctl_srv_copychunk_*,
> it belongs to the caller! It should have a name that belongs to
> the caller, maybe smb2_ioctl_network_fs_copychunk_done().
> And it should not appear between struct fsctl_srv_copychunk_state
> and fsctl_srv_copychunk_recv() in the sources,
> it should come after the smb2_ioctl_network_fs() function.

Moved below smb2_ioctl_network_fs().

> 
> +static NTSTATUS fsctl_srv_copychunk_recv(struct copy_chunk_state *cc_state,
> +                                        struct srv_copychunk_rsp *cc_rsp)
> 
> The first argument to the _recv() is *always* 'req' (the one the caller
> got from the _send() function). The caller should never see any reference
> to the internal 'state' structure.

Fixed.

> It's really important to follow the exact rules everywhere
> and implement very strict layering, even if it makes a code a bit more
> verbose and results in more lines of code.
> 
> https://git.samba.org/?p=ddiss/samba.git;a=commitdiff;h=abd2051818f1a3c5582b689ae5cf3fbef79053bc
> 
> Is an indication that the commit before removed to much.

Yes, it is.
I'm not quite sure what you mean here. I've added a "delegate async/sync
tracking to per ioctl device handlers" commit which removes the need for
state->req_outstanding and places tevent_req_post calls closer to
error detection. Hopefully this addresses these concerns.

> 
>                 subreq = np_write_send(state, ev,
> -                                      fsp->fake_file_handle,
> -                                      in_input.data,
> -                                      in_input.length);
> -               if (tevent_req_nomem(subreq, req)) {
> -                       return tevent_req_post(req, ev);
> +                                      state->fsp->fake_file_handle,
> +                                      state->in_input.data,
> +                                      state->in_input.length);
> +               if (subreq == NULL) {
> +                       status = NT_STATUS_FILE_CLOSED;
> +                       break;
>                 }
>                 tevent_req_set_callback(subreq,
>                                         smbd_smb2_ioctl_pipe_write_done,
>                                         req);
> -               return req;
> -
> 
> You should try to keep early returns as much as possible.

See above.

> Regarding the other commits, I think "struct smbd_smb2_ioctl_state" should
> state private in the .c file, pass explicit arguments if subfunctions
> need them.

My preference is to keep it exported for use in the main per dev type
handler, as well as smb2_ioctl_network_fs_copychunk_done() and
smbd_smb2_ioctl_pipe_*_done().

I agree that the per operation _send and _recv functions shouldn't be
peeking at struct smbd_smb2_ioctl_state, but I'm not so convinced that
doing so in smb2_ioctl_network_fs_copychunk_done or
smbd_smb2_ioctl_pipe_*_done() is such a layering violation. The _done
function is already responsible for marking the parent smb2 ioctl
request complete. Perhaps I've misunderstood here.

> As I said before on irc, we still need to resolve, the problem,
> the ioctl() code path of SMB 1 and SMB 2 are not in sync anymore.

Yes, further divergence here is not what I'd hoped for. As mentioned,
my preference is to start by plumbing through to the VFS at a per ioctl
level, to initially avoid the headaches of dealing with differing per
protocol version state types.

I've pushed a new branch attempting to address your concerns above to:

https://git.samba.org/?p=ddiss/samba.git;a=shortlog;h=refs/heads/smb2_copychunk_async_rb9_2squash

git://git.samba.org/ddiss/samba.git smb2_copychunk_async_rb9_2squash

The changes have not yet been squashed.

Cheers, David