[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.

One more comment here is that being able to return NTSTATUS from VFS
functions means that we have access to the full range of Windows error
codes, not that poor Unix substitute.

> 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
(何以解憂？唯有杜康。--曹操)