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: m...@netbsd.org (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?