svn commit: r203990 - head/lib/libc/sys

[Bruce Evans]

On Wed, 17 Feb 2010, Poul-Henning Kamp wrote:

> In message <20100218044931.S95007 at delplex.bde.org>, Bruce Evans writes:
>> On Wed, 17 Feb 2010, Poul-Henning Kamp wrote:
>>
>>> Log:
>>>  Mention EISDIR as a possible errno.
>>
>> It's not a possible error.
>
> critter phk> cat > a.c
> #include <stdio.h>
> #include <err.h>
>
> int
> main(int argc, char **argv)
> {
>        if (unlink("/"))
>                err(1, "Told you so");
>        return (0);
> }
> critter phk> cc a.c
> critter phk> ./a.out
> a.out: Told you so: Is a directory
> critter phk>

Better fix the kernel bug than break the documentation then.

The bug is very old -- it happens in ~5.2, where the code is a bit easier
to understand:

% int
% kern_unlink(struct thread *td, char *path, enum uio_seg pathseg)
% {
% 	struct mount *mp;
% 	struct vnode *vp;
% 	int error;
% 	struct nameidata nd;
% 
% restart:
% 	bwillwrite();
% 	NDINIT(&nd, DELETE, NOFOLLOW | LOCKPARENT | LOCKLEAF, pathseg, path,
% 	    td);
% 	if ((error = namei(&nd)) != 0)
% 		return (error);

namei() returns EISDIR for "/" (due to DELETE and and the special handling
of the degenerate case which includes "/" and not much else, else the bug
would affect more cases).  Then we return the wrong errno before we test
VDIR.

% 	vp = nd.ni_vp;
% 	if (vp->v_type == VDIR)
% 		error = EPERM;
% 	else {

Untested fix for ~5.2.  Also fixes some style bugs.

% Index: vfs_syscalls.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/kern/vfs_syscalls.c,v
% retrieving revision 1.354
% diff -u -2 -r1.354 vfs_syscalls.c
% --- vfs_syscalls.c	24 Jun 2004 17:22:29 -0000	1.354
% +++ vfs_syscalls.c	17 Feb 2010 20:01:09 -0000
% @@ -1614,10 +1605,11 @@
%  restart:
%  	bwillwrite();
% -	NDINIT(&nd, DELETE, LOCKPARENT|LOCKLEAF, pathseg, path, td);
% +	NDINIT(&nd, DELETE, NOFOLLOW | LOCKPARENT | LOCKLEAF, pathseg, path,
% +	    td);

Style fixes:
- spell out NOFOLLOW (NOFOLLOW is 0, so omitting it is just confusing).
   This bug is in about 3 other vfs syscalls.
- there are spaces around binary operators in KNF.  These spaces are
   especially important for the "|" operator since this operator looks
   more like an alphanumeric character than most operator symbols, but
   they are most often omitted for this operator :-(.

%  	if ((error = namei(&nd)) != 0)
% -		return (error);
% +		return (error == EISDIR ? EPERM : error);

Fix the EISDIR bug.

%  	vp = nd.ni_vp;
%  	if (vp->v_type == VDIR)
% -		error = EPERM;		/* POSIX */
% +		error = EPERM;

Remove banal/misleading comment.  The new fixup needs a comment more than
this, but I don't want to add one.

%  	else {
%  		/*

ISTR trying to avoid the special handling for the degenerate case in
namei() and lookup() (2 almost identical copies of it).  The correct
fix may be there.  From lookup() in ~5.2:

% 	/*
% 	 * Check for degenerate name (e.g. / or "")
% 	 * which is a way of talking about a directory,
% 	 * e.g. like "/." or ".".
% 	 */
% 	if (cnp->cn_nameptr[0] == '\0') {
% 		if (dp->v_type != VDIR) {
% 			error = ENOTDIR;
% 			goto bad;
% 		}
% 		if (cnp->cn_nameiop != LOOKUP) {
% 			error = EISDIR;
% 			goto bad;
% 		}

Cases with trailing slashes are handled by removing the slash, but
this doesn't work for "/" since we don't want to end up with the fully
degenerate name of "".  Cases starting with this fully degnerate name
haven't been permitted since ~1988 when POSIX disallowed it, but the
comment in the code hasn't caught up with this.  The comment gives
these cases as examples only, but I think that is another bug and that
this is a complete list of degenerate names so the comment should say
"i.e.,".  After catching up with 1988 and removing "" from the list,
only "/" remains.  This is not really degenerate and can hopefully be
handled more directly and simply.  The != VDIR case in the above might
already be unreachable.

Bruce