Bug 8462 - Move FSCTL processing into the VFS so that vendors can easily implement them
Move FSCTL processing into the VFS so that vendors can easily implement them
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: VFS Modules
unspecified
All All
: P5 enhancement
: ---
Assigned To: Samba Bugzilla Account
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-16 19:04 UTC by Richard Sharpe
Modified: 2011-10-07 14:06 UTC (History)
2 users (show)

See Also:


Attachments
A torture test that tests some aspects of the current implementation of FSCTLs (12.18 KB, patch)
2011-09-16 19:10 UTC, Richard Sharpe
no flags Details
A git format-patch patch against master that passes my torture test (24.98 KB, patch)
2011-09-17 03:49 UTC, Richard Sharpe
no flags Details
A git format-patch patch against master that passes my torture test (25.04 KB, application/octet-stream)
2011-09-17 10:42 UTC, Richard Sharpe
no flags Details
Reworked the patch to pass a talloc_context at Volker's suggestion (25.20 KB, patch)
2011-09-17 22:34 UTC, Richard Sharpe
no flags Details
Reworked the patch again to use talloc_array as suggested by Rusty (25.22 KB, patch)
2011-09-24 17:50 UTC, Richard Sharpe
no flags Details
Reworked it to make the changes suggested by Metze. (26.09 KB, patch)
2011-09-24 20:15 UTC, Richard Sharpe
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2011-09-16 19:04:53 UTC
Currently FSCTLs are implemented in the body of Samba itself, so if Vendors want to implement their own FSCTLs they have to modify Samba code.

It would be better if the processing of FSCTLs was handled in the VFS with the current code being the default in modules/vfs_default.c.
Comment 1 Richard Sharpe 2011-09-16 19:10:22 UTC
Created attachment 6898 [details]
A torture test that tests some aspects of the current implementation of FSCTLs

This test was written as an aide to getting the VFS default code correct and so that developers of vendor-specific FSCTLs have a template to work with.
Comment 2 Volker Lendecke 2011-09-16 22:30:52 UTC
While I do like the idea of pushing the FSCTL through the VFS I'm not sure we should do it within a 3.6.x release. I thought that our assumption was that we try to keep the VFS stable within a major release series.

Jeremy?
Comment 3 Jeremy Allison 2011-09-16 22:41:42 UTC
Yes, we need to keep the VFS stable for 3.6.x, this is more of a 4.0.x enhancement in terms of when we ship the code changes. No issues with keeping this bug up to date with private patches OEM's and vendors might need if they want to get this into their versions of 3.6.x however.

Jeremy.
Comment 4 Richard Sharpe 2011-09-17 03:49:30 UTC
Created attachment 6900 [details]
A git format-patch patch against master that passes my torture test

This patch builds and handles the torture test correctly as far as I can see. I checked it by looking at a Wireshark capture.

The patch is against Master. 

I will probably create one against 3.6.0 and 3.6.1 when it comes out.
Comment 5 Richard Sharpe 2011-09-17 10:40:08 UTC
Comment on attachment 6900 [details]
A git format-patch patch against master that passes my torture test

Superseded by latest patch
Comment 6 Richard Sharpe 2011-09-17 10:42:04 UTC
Created attachment 6906 [details]
A git format-patch patch against master that passes my torture test

I added a Signed-off-by:

This patch is against master. It passes my torture test and an inspection of a tcpdump capture suggests that it is working OK. I might write more tests.

Will probably merge it to 3.6.0 and 3.6.1 when it comes out and provide separate patches.
Comment 7 Richard Sharpe 2011-09-17 22:34:28 UTC
Created attachment 6908 [details]
Reworked the patch to pass a talloc_context at Volker's suggestion

This is essentially the same patch as the earlier one but now a talloc context is passed into the FSCTL VFS routine.

(I note that existing routines in vfs_default use talloc_tos().)
Comment 8 Richard Sharpe 2011-09-24 17:50:40 UTC
Created attachment 6933 [details]
Reworked the patch again to use talloc_array as suggested by Rusty

This is a slightly reworked patch to move FSCTL handling into the VFS.

It includes two changes from the original:

1. Pass a talloc context to the FSCTL VFS routine

2. Use talloc_array rather that talloc_size.

This has been tested using my torture test and passes.
Comment 9 Stefan Metzmacher 2011-09-24 19:20:56 UTC
Hi Richard,

As we're passing raw bytes here, could you change "char" into "uint8_t"?
In vfswrap_fsctl() we could have
const char *in_data = (const char *)_in_data;
char **out_data = (char **)_out_data;
on top to avoid warnings.
We may also need casts in the caller, but I think we should have the correct
types in the API.

Otherwise the patches look good to me.

If they pass the tests, we should run them in make test.

You need to add "NTTRANS-FSCTL" to the array, where "SMB2-BASIC" is already
listed in source3/selftest/tests.py.

metze
Comment 10 Richard Sharpe 2011-09-24 20:15:57 UTC
Created attachment 6935 [details]
Reworked it to make the changes suggested by Metze.

Reworked the patch one more time to:

1. Pass uint8_t pointers rather than char pointers and add casts where needed.
2. Enable the NTTRANS-FSCTL test in the self tests (have not tested this :-)
3. Pass a talloc context (requested by Volker)
4. Use talloc_array (requested by Rusty)

Again, it passes my NTTRANS-FSCTL test. However, my first patch with the new test will have to be applied before this patch, otherwise the self tests will fail.
Comment 11 Richard Sharpe 2011-10-07 14:05:48 UTC
This has been fixed with the following commits:

595cc42a46f5dfbac5c17a403fb1f82769f26ff2
Author: Richard Sharpe <realrichardsharpe@gmail.com>
Date:   Sat Oct 1 09:03:13 2011 -0700

    Add the new test_nttrans_fsctl.c to waf
    
    Autobuild-User: Richard Sharpe <sharpe@samba.org>
    Autobuild-Date: Sat Oct  1 19:36:53 CEST 2011 on sn-devel-104

commit c875ab8747d65cc6556228616f076b0928013c87
Author: Richard Sharpe <realrichardsharpe@gmail.com>
Date:   Fri Sep 16 11:52:22 2011 -0700

    Move FSCTL handling into the VFS. Initial code changes. Passes smbtorture NT

commit e8f143a45cceb8d2c01d45167a0c90d5c7f38289
Author: Richard Sharpe <realrichardsharpe@gmail.com>
Date:   Thu Sep 15 16:13:54 2011 -0700

    Add a torture test to test existing FSCTL responses