Date: Fri, 26 Feb 2010 10:33:27 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jaakko Heinonen <jh@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, Poul-Henning Kamp <phk@phk.freebsd.dk>, src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r203990 - head/lib/libc/sys Message-ID: <20100226091923.X2605@delplex.bde.org> In-Reply-To: <20100225195138.GA3323@a91-153-117-195.elisa-laajakaista.fi> References: <6413.1266433105@critter.freebsd.dk> <20100218064545.J2074@besplex.bde.org> <20100218095538.GA2318@a91-153-117-195.elisa-laajakaista.fi> <20100225195138.GA3323@a91-153-117-195.elisa-laajakaista.fi>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100226091923.X2605>