Re: Patch: openat() family of system calls

[David Holland <dholland-tech%netbsd.org@localhost>]

On Sun, Jan 11, 2009 at 01:46:18PM +0000, Andrew Doran wrote:
 > This patch provides the necessary infrastructure for the openat() family of
 > system calls, and implements openat() and fstatat() which are used by the
 > ZFS userspace commands. It looks like these are on the way to becoming a
 > standard and are already present in Linux, Solaris and FreeBSD.

Surely "fstatat()" should be "statat()"? And shouldn't there be an
lstatat(), etc. etc.?

 >      http://www.netbsd.org/~ad/cleanout/openat.diff
 >
 > The code to extract and reference the root directory for the lookup is

YM "current directory", according to both your code and the linux
manpage... HTH. :-)

 > placed in namei(). This is because mixing vnode and file descriptor
 > references is tricky and can lead to code paths that deadlock. namei() is
 > a reasonably safe place to put it.

Please, please, don't put file descriptor code in namei. To the extent
we have any organizational structure at all above the VFS interface,
that violates it. There is no problem with looking up the filehandle
first in the syscall code.

Also *please* don't overload ndp->ni_loopcnt to hold a filehandle. The
contents of struct nameidata are already enough of a mess without
making it worse in such a fashion.

In general, I don't think the approach taken in this patch is a good
idea. namei works with vnodes and should sit firmly below the system
call layer that knows about file handles. This should not cause
locking or refcounting issues. Also, any changes to the namei calling
interface should be aimed at making it simpler. Wedging a new feature
in with the smallest quick and dirty patch that can be managed is not
the way to go, especially for a feature like this that if anything
makes the interface more ...symmetric rather than less.

I'm aware that my namei cleanup is way behind schedule. I'm getting
there, slowly.


also, some smaller points:

 > +#define     STARTATFD       0x0080  /* root directory given */

That should be "current directory given".

 > +#define AT_FDCWD            -100    /* use working directory */

Is that really what's intended? Why -100?

-- 
David A. Holland
dholland%netbsd.org@localhost