Date: Tue, 13 Nov 2012 09:52:21 +0000 From: Emmanuel Dreyfus <m...@netbsd.org>
New version of the patch for POSIX extended API set 2: http://ftp.espci.fr/shadow/manu/openat3.patch OK, thanks, this looks better. Some comments below, mostly very minor. First, though -- are there any changes to header files? I didn't see any in the patch. Also, in the future, can you pass `-p' to diff, to make it easier to follow where each hunk comes from? I tried to address the previous remarks, except for the non standard but more consistant system calls names. I beleive such stuff should be a symbol alias with a warning if you do not build with _NETBSD_SOURCE if we decide to do it. Sounds reasonable. What is still missing, but I think it should go in a separate commit: - open(2) flags: O_EXEC, O_SEARCH, O_NOCTTY, O_RSYNC, O_TTY_INIT, spec here: http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html - fexecve(2) system call Yes, those should go in a separate commit. (Although we already have O_NOCTTY and O_RSYNC.) --- sys/kern/vfs_syscalls.c 19 Oct 2012 02:07:22 -0000 1.459 +++ sys/kern/vfs_syscalls.c 13 Nov 2012 09:28:08 -0000 +static int fd_nameiat(int fdat, struct nameidata *ndp) +{ KNF? -do_sys_link(struct lwp *l, const char *path, const char *link, - int follow, register_t *retval) +do_sys_linkat(struct lwp *l, int fdpath, const char *path, int fdlink, + const char *link, int follow, register_t *retval) { ... - if (follow) + if (follow & AT_SYMLINK_FOLLOW) ... - return do_sys_link(l, path, link, 1, retval); + return do_sys_linkat(l, AT_FDCWD, path, AT_FDCWD, link, 1, retval); `1' does not look right here as the new `follow' argument, and perhaps the argument should be called `flags' instead. Is there an automatic test for this case? +static int +do_sys_accessat(struct lwp *l, int fdat, const char *path, + int mode, int flags) +{ ... /* Override default credentials */ cred = kauth_cred_dup(l->l_cred); - kauth_cred_seteuid(cred, kauth_cred_getuid(l->l_cred)); - kauth_cred_setegid(cred, kauth_cred_getgid(l->l_cred)); + if (nd_flag & AT_EACCESS) { + kauth_cred_seteuid(cred, kauth_cred_geteuid(l->l_cred)); + kauth_cred_setegid(cred, kauth_cred_getegid(l->l_cred)); Is this half of the branch necessary? +static int +do_sys_chmodat(struct lwp *l, int fdat, const char *path, int mode, int flags) +{ ... if (error != 0) - return (error); + goto out; ... +out: return (error); } Label necessary? (Same in do_sys_chownat.) --- tests/lib/libc/c063.orig/t_fstatat.c 1970-01-01 01:00:00.000000000 +0100 +++ tests/lib/libc/c063/t_fstatat.c 2012-11-12 15:05:30.000000000 +0100 +ATF_TC_BODY(fstatat_fd, tc) +{ ... + ATF_REQUIRE((dfd = open(DIR, O_RDONLY, 0)) != -1); + ATF_REQUIRE(fstatat(dfd, BASEFILE, &st, 0) == 0); + ATF_REQUIRE(close(dfd) == 0); +} We ought to have at least one test that memcmps the output of stat and the output of fstatat; perhaps this is the appropriate place to do it. --- tests/lib/libc/c063.orig/t_mkdirat.c 1970-01-01 01:00:00.000000000 +0100 +++ tests/lib/libc/c063/t_mkdirat.c 2012-11-10 03:00:26.000000000 +0100 +ATF_TC_BODY(mkdirat_fd, tc) +{ ... + ATF_REQUIRE((dfd = open(DIR, O_RDONLY, 0)) != -1); + ATF_REQUIRE((fd = mkdirat(dfd, BASESDIR, mode)) != -1); + ATF_REQUIRE(close(fd) == 0); Another mismatched close; there remain a few of these. --- tests/lib/libc/c063.orig/t_openat.c 1970-01-01 01:00:00.000000000 +0100 +++ tests/lib/libc/c063/t_openat.c 2012-11-09 20:13:43.000000000 +0100 +ATF_TC_BODY(openat_fd, tc) +{ ... + ATF_REQUIRE((dfd = open(DIR, O_RDONLY, 0)) != -1); + ATF_REQUIRE((fd = openat(dfd, BASEFILE, O_RDONLY, 0)) != -1); + ATF_REQUIRE(close(fd) == 0); +} And a missing close; there some more of these too (although perhaps for atf tests they probably don't really matter). --- tests/lib/libc/c063.orig/t_unlinkat.c 1970-01-01 01:00:00.000000000 +0100 +++ tests/lib/libc/c063/t_unlinkat.c 2012-11-13 03:43:04.000000000 +0100 Test for AT_REMOVEDIR's ENOTDIR case, perhaps?
