[RFC]: VFS open return NTSTATUS (was:Add vfs_admin vfs module)

[Richard Sharpe]

On Wed, Oct 28, 2015 at 2:04 PM, Uri Simchoni <uri at samba.org> wrote:
> Hi,
>
> I've spent some time trying to change the interface of VFS POSIX open
> (open_fn). The change is that instead of returning a created file
> descriptor, it now sets the file descriptor in the FSP and returns an
> ntstatus.This can serve as a POC for a migration for all VFS methods to
> return NTSTATUS.

I really like this!

However, I do not think you need to go to such heroic efforts to not
break existing out-of-tree VFS modules that people have written.

We do not guarantee to maintain compatibility across major versions
and you have converted all the in-tree modules, which people can use
as a template for their own changes.


> What got me started was that I wanted VFS open_fn to have a slightly
> different interface, namely that it set the new file descriptor in the fsp
> instead of returning it. Richard indicated that I should use accessor
> functions to do it, and that he wants to drive a change where all VFS
> functions return an NTSTATUS. So I figured that while I'm at it, I might as
> well change the interface of open into one that returns NTSTATUS, and see
> how it looks.
>
> What I've learned:
> - Returning the error code (be it POSIX errno or NTSTATUS) instead or
> setting errno simplifies the code compared to setting errno and returning
> -1.
>
> - Returning an error also avoids the case of forgetting to set errno (unless
> one cheats and returns errno), and avoids the case of forgetting to save and
> restore errno in error handling.
>
> - about NTSTATUS instead of POSIX error code:
> a. It may seem unnatural as this is a "posix layer" with posix semantics
> (even if the VFS is not the kernel, we have to assume it has posix semantics
> if we are to mix and match vfs modules and sanely reason about it).
> b. There's a bug where fd_open() does some platform-specific errno fixes,
> thereby applying the fixes even where they do not belong (e.g. if the VFS is
> not calling the kernel's open). It's fixable with the current interface as
> well, but with an interface that returns NTSTATUS it would have been harder
> to make that mistake.
> c. Overall returning NTSTATUS simplifies the life of the VFS consumer, not
> affecting much a VFS "transparent" module, and makes life harder to VFS
> "opaque" modules. The latter ones now have to do the translation themselves,
> so if they encapsulate some POSIX-compliant library (e.g. ceph), they have
> extra work.
> d. The conversion I made was a "shallow" one - not drilling the
> errno->ntstatus into the call chain in vfs modules. Perhaps doing this would
> simplify the code further and avoid calling the unix->ntstatus error mapping
> function in some cases (see vfs_fruit for example).
>
> - Two bugs fixed while scanning the code (in vfs_fruit and vfs_commit), some
> more minor ones (not setting errno) spotted and not fixed.
>
> The patch set includes this interface change, as well as using accessor
> functions to set the file descriptor of the fsp.
>
> Comments/review appreciated.
> Thanks,
> Uri.
>



-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)