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

[Bruce Evans]

On Thu, 25 Feb 2010, Jaakko Heinonen wrote:

> On 2010-02-18, Jaakko Heinonen wrote:
>>> 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).
>>
>> This causes a problem also for mkdir(2), rmdir(2) and rename(2). All of
>> them incorrectly return EISDIR for "/".
>
> Here is a patch which attempts to fix mkdir(2), rmdir(2), rename(2) and
> unlink(2) errno return value for "/".
>
> 	http://people.freebsd.org/~jh/patches/lookup-root.diff

Please enclose patches in mail so that they can be seen easily and quoted
normally.

% Index: sys/kern/vfs_lookup.c
% ===================================================================
% --- sys/kern/vfs_lookup.c	(revision 204273)
% +++ sys/kern/vfs_lookup.c	(working copy)

I'm still not sure if the problem should be fixed mainly in this file.
Translation of the errno is still needed in some cases, since the
correct errno is context-dependent and it would be ugly to pass the
context here and uglier to guess it.  The switches on on cnp->cn_nameiop
are little more than guessing.  On second thoughts, the context is
already passed unambigously for the RENAME op so setting the errno here
for RENAME is good.

% @@ -565,19 +565,22 @@ dirloop:
%  	}
% 
%  	/*
% -	 * Check for degenerate name (e.g. / or "")
% -	 * which is a way of talking about a directory,
% -	 * e.g. like "/." or ".".
% +	 * Check for "" which is a way of talking about the root directory.

This comment is still misleading.  "" is a pathname for a directory
entry that cannot exist, not a way of talking about the root directory.
However, we have replaced any trailing slashes by a NUL and have thus
corrupted the pathname if it was "/".  Perhaps we shouldn't have done
that, but reducing "/[/]*" to "" and then checking (cnp->cn_nameptr[0]
== '\0') seems to be the simplest way to check for the original pathname
being the root directory.

% +	 * We can't provide a parent node for CREATE, DELETE and RENAME
% +	 * operations.
%  	 */
%  	if (cnp->cn_nameptr[0] == '\0') {
% -		if (dp->v_type != VDIR) {
% -			error = ENOTDIR;
% +		KASSERT(dp->v_type == VDIR, ("dp is not a directory"));
% +		switch (cnp->cn_nameiop) {
% +		case CREATE:
% +			error = EEXIST;
%  			goto bad;
% -		}
% -		if (cnp->cn_nameiop != LOOKUP) {
% -			error = EISDIR;
% +		case DELETE:
% +		case RENAME:
% +			error = EBUSY;
%  			goto bad;
%  		}

EISDIR is also returned by ufs_lookup(), so it was not unique enough here.
EBUSY seems to be unique.  EEXIST seems to be unique too, but is less
generic.

% +		KASSERT(cnp->cn_nameiop == LOOKUP, ("nameiop must be LOOKUP"));

Clearer to put this in the switch.

%  		if (wantparent) {

Doesn't the comment about not being able to provide a parent belong here?

%  			ndp->ni_dvp = dp;
%  			VREF(dp);
% @@ -948,19 +951,22 @@ relookup(struct vnode *dvp, struct vnode
%  #endif
% 
%  	/*
% -	 * Check for degenerate name (e.g. / or "")
% -	 * which is a way of talking about a directory,
% -	 * e.g. like "/." or ".".
% +	 * Check for "" which is a way of talking about the root directory.
% +	 * We can't provide a parent node for CREATE, DELETE and RENAME
% +	 * operations.
%  	 */
%  	if (cnp->cn_nameptr[0] == '\0') {
% -		if (cnp->cn_nameiop != LOOKUP || wantparent) {
% -			error = EISDIR;
% +		KASSERT(dp->v_type == VDIR, ("dp is not a directory"));
% +		switch (cnp->cn_nameiop) {
% +		case CREATE:
% +			error = EEXIST;
%  			goto bad;
% -		}
% -		if (dp->v_type != VDIR) {
% -			error = ENOTDIR;
% +		case DELETE:
% +		case RENAME:
% +			error = EBUSY;
%  			goto bad;
%  		}
% +		KASSERT(cnp->cn_nameiop == LOOKUP, ("nameiop must be LOOKUP"));
%  		if (!(cnp->cn_flags & LOCKLEAF))
%  			VOP_UNLOCK(dp, 0);
%  		*vpp = dp;

This is in relookup().  I think relookup() is only called from rename(),
so the failing case is unreachable (since the root directory cannot
have moved, so any possible failure would already have occurred), and
lots more of the code for the non-failing case is dead too (since being
for rename() implies (cnp->cn_nameiop == RENAME && wantparent ...).
Also, if the op is somehow not RENAME and failure occurs, then its
errno should probably always be EBUSY and never EEXIST, since failure
of relookup() implies a topology change and EBUSY is closest to
describing that, while EEXIST is a stupid errno for failure to re-look-up
something which you hope still exists.

% Index: sys/kern/vfs_syscalls.c
% ===================================================================
% --- sys/kern/vfs_syscalls.c	(revision 204273)
% +++ sys/kern/vfs_syscalls.c	(working copy)
% @@ -1841,7 +1841,7 @@ restart:
%  	NDINIT_AT(&nd, DELETE, LOCKPARENT | LOCKLEAF | MPSAFE | AUDITVNODE1,
%  	    pathseg, path, fd, td);
%  	if ((error = namei(&nd)) != 0)
% -		return (error == EINVAL ? EPERM : error);
% +		return ((error == EINVAL || error == EBUSY) ? EPERM : error);

The parentheses are not needed.

%  	vfslocked = NDHASGIANT(&nd);
%  	vp = nd.ni_vp;
%  	if (vp->v_type == VDIR && oldinum == 0) {
% @@ -3618,9 +3618,6 @@ kern_renameat(struct thread *td, int old
%  	if (fromnd.ni_vp->v_type == VDIR)
%  		tond.ni_cnd.cn_flags |= WILLBEDIR;
%  	if ((error = namei(&tond)) != 0) {
% -		/* Translate error code for rename("dir1", "dir2/."). */
% -		if (error == EISDIR && fvp->v_type == VDIR)
% -			error = EINVAL;

I think this has nothing to do with the root directory (as its comment says),
but it is to translate the EISDIR returned by ufs_lookup(), etc., when `tond'
is for a (directory) pathname ending in ".".  So it should not be removed,
except possibly after changing ufs_lookup(), etc., to return EINVAL.  The
EISDIR in ufs_lookup() is only for RENAME, so it is strange that any
translation is needed.  I apparently put the translation here to avoid
looking at all leaf file systems.

%  		NDFREE(&fromnd, NDF_ONLY_PNBUF);
%  		vrele(fromnd.ni_dvp);
%  		vrele(fvp);

Bruce