[PATCH] smb2 FSCTL_SRV_COPYCHUNK support
Stefan (metze) Metzmacher
metze at samba.org
Mon Mar 12 07:42:22 MDT 2012
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
+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.
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.
+ 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.
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.
+static void vfs_copy_chunk_done(struct tevent_req *subreq)
This should be named fsctl_srv_copychunk_done()
or fsctl_srv_copychunk_vfs_done().
+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.
+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.
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.
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.
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.
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.
Richard, you skipped SMB 2 in your work to add SMB_VFS_FSCTL()
(and I didn't notice :-( ). It's currently a big mess,
which will require a lot of work to cleanup properly.
metze
Am 09.03.2012 13:01, schrieb David Disseldorp:
> Hi,
>
> On Tue, 6 Mar 2012 16:50:41 +0100
> David Disseldorp <ddiss at suse.de> wrote:
>
>> On Tue, 6 Mar 2012 14:22:54 +0100
>> Volker Lendecke <Volker.Lendecke at SerNet.DE> wrote:
>>
>>> Functionally I think your patches are correct. All I have is
>>> stylistic remarks:
>> ...
>>> 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.
>>
>> No problem at all, I agree that your suggestions make the code easier
>> to read. Changes since last push:
>> - Fix vfs_default copy chunk async function order.
>> - Shorten tevent_req_nterror() and tevent_req_nomem() error paths. This
>> doesn't catch all status checks prior to tevent_req_nterror().
>>
>> Cheers, David
>>
>> http://git.samba.org/?p=ddiss/samba.git;a=shortlog;h=refs/heads/smb2_copychunk_async_rb7
>>
>> The following changes since commit 1f62df52aaafc4f777fed4541625a92f15c8e12c:
>>
>> s3: Move a talloc_strdup out of the main code path (2012-03-06 14:29:50 +0100)
>>
>> are available in the git repository at:
>> git://git.samba.org/ddiss/samba.git smb2_copychunk_async_rb7
>>
>> David Disseldorp (9):
>> s3-smb2: split ioctl handlers into separate functions
>> s3-smb2: split ioctl handler code on device type
>> s3-ioctl: fix smb2 named pipe ioctl handler
>> s3-server: add smb2 FSCTL_SRV_REQUEST_RESUME_KEY support
>> s3-vfs: add copy_chunk vfs hooks
>> s3-server: add support for smb2 FSCTL_SRV_COPYCHUNK
>> s3-vfs: add vfs_btrfs module
>> s3-server: remove smb2 ioctl error response assumption
>> s4-torture: skip FSCTL_SRV_ENUM_SNAPS test when not supported
>>
>> docs-xml/manpages-3/vfs_full_audit.8.xml | 2 +
>> examples/VFS/skel_opaque.c | 21 ++
>> examples/VFS/skel_transparent.c | 22 ++
>> libcli/smb/smb_constants.h | 2 +
>> selftest/skip | 1 -
>> source3/Makefile.in | 11 +
>> source3/configure.in | 13 +
>> source3/include/vfs.h | 25 ++-
>> source3/include/vfs_macros.h | 10 +
>> source3/modules/vfs_btrfs.c | 137 ++++++++++
>> source3/modules/vfs_default.c | 64 +++++
>> source3/modules/vfs_full_audit.c | 38 +++
>> source3/modules/vfs_time_audit.c | 80 ++++++
>> source3/modules/wscript_build | 9 +
>> source3/selftest/tests.py | 2 +-
>> source3/smbd/smb2_ioctl.c | 425 ++++++------------------------
>> source3/smbd/smb2_ioctl_dfs.c | 119 ++++++++
>> source3/smbd/smb2_ioctl_filesys.c | 49 ++++
>> source3/smbd/smb2_ioctl_named_pipe.c | 167 ++++++++++++
>> source3/smbd/smb2_ioctl_network_fs.c | 432 ++++++++++++++++++++++++++++++
>> source3/smbd/smb2_ioctl_private.h | 54 ++++
>> source3/smbd/vfs.c | 26 ++-
>> source3/wscript | 11 +
>> source3/wscript_build | 5 +
>> source4/libcli/smb2/ioctl.c | 29 ++-
>> source4/torture/smb2/ioctl.c | 68 +++++-
>> 26 files changed, 1465 insertions(+), 357 deletions(-)
>> create mode 100644 source3/modules/vfs_btrfs.c
>> create mode 100644 source3/smbd/smb2_ioctl_dfs.c
>> create mode 100644 source3/smbd/smb2_ioctl_filesys.c
>> create mode 100644 source3/smbd/smb2_ioctl_named_pipe.c
>> create mode 100644 source3/smbd/smb2_ioctl_network_fs.c
>> create mode 100644 source3/smbd/smb2_ioctl_private.h
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20120312/c06f0850/attachment.pgp>
More information about the samba-technical
mailing list