Re: [PATCH] POSIX extended API set 2

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

Some quick responses to the substance of the patch now that I have
skimmed it...

   Date: Sat, 10 Nov 2012 07:44:18 +0100
   From: manu%netbsd.org@localhost (Emmanuel Dreyfus)

   Index: sys/sys/namei.h

   +int nameiat_simple_kernel(int, const char *, namei_simple_flags_t,
   +    struct vnode **);
   +int nameiat_simple_user(int, const char *, namei_simple_flags_t, 
   +    struct vnode **);

   Index: sys/kern/vfs_lookup.c

   +#include <sys/fcntl.h> /* For AT_FCWD */
   +#include <sys/file.h> /* For file_t */

I don't think namei.h / vfs_lookup.c is the right place to be handling
file descriptors.  Can you make these take vnodes, rather than file
descriptors, or move them into vfs_syscalls.c?

   Index: sys/sys/syscall.h

   -/* $NetBSD: syscall.h,v 1.257 2012/10/02 01:46:20 christos Exp $ */
   +/* $NetBSD$ */

   Index: sys/kern/vfs_syscalls.c

   -/*  $NetBSD: vfs_syscalls.c,v 1.459 2012/10/19 02:07:22 riastradh Exp $     
*/
   +/*  $NetBSD: vfs_syscalls.c,v 1.458 2012/10/12 02:37:20 riastradh Exp $     
*/

The patch has some funny RCS ID updates.  Are you working against a
tree that is up-to-date in fact but which CVS thinks is not up-to-date
in the CVS metadata?

   +static inline int
   +do_sys_openat(lwp_t *l, int fdat, const char *path, int flags,
   +    int mode, int *fd)

What's the inline doing here?

   +{
   +        file_t *dfp;
   +    struct vnode *dvp;

There are some mixed tabs and spaces here.  Can you go through the
patch and make sure the indentation is consistent with KNF?

   +    /*
   +     * openat() falls back to open() behavior if
   +     * - path is absolute XXX check this.
   +     * - fd is AT_FDCWD
   +     */

Have you checked the XXX, and/or written automatic tests for it?

   +    error = pathbuf_copyin(path, &pb);
   +    if (error)
   +            return error;

   ...

   +    if (fdat != AT_FDCWD) {
   +            /* fd_getvnode() will use the descriptor for us */
   +            if ((error = fd_getvnode(fdat, &dfp)) != 0)
   +                    return (error);

   ...

   +    pathbuf_destroy(pb);

I think you'll leak pb here in the error branch above.  I haven't gone
through the other error branches in the patch, but the proliferation
of new resource acquisitions requiring clean-up makes me nervous.

   Index: tests/lib/libc/Makefile

It would be nice if we had some tests that use rump, perhaps in
tests/fs/vfs/t_vnops.c, and not just tests that use the libc stubs, so
that it would be possible to test the implementation of the system
calls in userland without requiring the host kernel to support them.

   +++ tests/lib/libc/c063/t_fchmodat.c 2012-11-10 07:01:09.000000000 +0100

   +ATF_TC_BODY(fchmodat_fd, tc)
   +{
   +    int dfd;
   +    int fd;
   +
   +    ATF_REQUIRE(mkdir(DIR, 0755) == 0);
   +    ATF_REQUIRE((fd = open(FILE, O_CREAT|O_RDWR, 0644)) != -1);
   +    ATF_REQUIRE(close(fd) == 0);
   +
   +    ATF_REQUIRE((dfd = open(DIR, O_RDONLY, 0)) != -1);
   +    ATF_REQUIRE((fd = fchmodat(dfd, BASEFILE, 0600, 0)) != -1);
   +    ATF_REQUIRE(close(fd) == 0);
   +}

Seems to me that this test, and similar ones below, also ought to
check to make sure that stat (and particularly not just statat) will
reflect the update, although that can be added later, I suppose.

Also, `fd = fchmodat(...); close(fd)'?  Looks like this test will
close stdin.  Same thing in a lot of the other tests below, although
some seem to omit the close altogether.  Perhaps this was copypasta
from the openat test, where `fd = openat(...); close(fd)' does make
sense?