[RFC PATCH] s3: libsmbclient: Add server-side copy support

[David Disseldorp]

Hi Ross,

A few comments inline...

On Mon,  6 Apr 2015 12:05:14 +0100, Ross Lagerwall wrote:
...
--- a/libcli/smb/smb2cli_ioctl.c
+++ b/libcli/smb/smb2cli_ioctl.c
@@ -195,12 +195,17 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq)
                .status = NT_STATUS_FILE_CLOSED,
                .body_size = 0x09,
        },
+       {
+               .status = NT_STATUS_INVALID_PARAMETER,
+               .body_size = 0x31
+       },
        };

        status = smb2cli_req_recv(subreq, state, &iov,
                                  expected, ARRAY_SIZE(expected));
        TALLOC_FREE(subreq);
-       if (tevent_req_nterror(req, status)) {
+       if (!NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER) &&
+                       tevent_req_nterror(req, status)) {
                return;
        }

The NT_STATUS_INVALID_PARAMETER response handling should be opcode
specific. See the MS-SMB2 3.3.4.4 Sending an Error Response.


--- a/source3/libsmb/cli_smb2_fnum.c
+++ b/source3/libsmb/cli_smb2_fnum.c
...
+static void cli_splice_copychunk_done(struct tevent_req *subreq)
+{
...
+       if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER)) {
+               state->chunk_len = cc_copy_rsp.chunk_bytes_written;
+               state->max_chunks = MIN(cc_copy_rsp.chunks_written,
+                                       cc_copy_rsp.total_bytes_written / state->chunk_len);

Copychunk request resizing should only occur once at most per splice
request, to avoid endless client loops. It might be worth storing the
server limits with the connection context.

+               TALLOC_FREE(state->cc_copy.chunks);
+               state->cc_copy.chunks = talloc_array(state,
+                                                    struct srv_copychunk,
+                                                    state->max_chunks);
+       } else {

Please add checks for allocation failures. I think the chunk array
allocation would be best done in cli_splice_copychunk_send() prior to
the request packing.

+       subreq = smb2cli_ioctl_send(state, state->ev, state->cli->conn,
+                              state->cli->timeout,
+                              state->cli->smb2.session,
+                              state->cli->smb2.tcon,
+                              state->dst_ph->fid_persistent, /* in_fid_persistent */
+                              state->dst_ph->fid_volatile, /* in_fid_volatile */
+                              FSCTL_SRV_COPYCHUNK,

FSCTL_SRV_COPYCHUNK requires that read access be granted on the
destination file handle. FSCTL_SRV_COPYCHUNK_WRITE does not require
this, so may be more suitable - the kernel client uses it for this
reason.

+static void cli_splice_key_done(struct tevent_req *subreq)
+{
+       struct tevent_req *req = tevent_req_callback_data(
+               subreq, struct tevent_req);
+       struct cli_smb2_splice_state *state =
+               tevent_req_data(req,
+               struct cli_smb2_splice_state);
+       TALLOC_CTX *frame = talloc_stackframe();

The stackframe ctx shouldn't be needed here. Convention is to use the
async request state as the temporary context for the duration of the
request.

Cheers, David