Re: [PATCH] POSIX extended API set 2, round 2

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

   Date: Tue, 13 Nov 2012 09:52:21 +0000
   From: Emmanuel Dreyfus <manu%netbsd.org@localhost>

   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?