Date: Thu, 9 Jul 2015 17:40:21 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: rwatson@FreeBSD.org, freebsd-fs@freebsd.org Subject: Re: [PATCH 3/4] vfs: simplify error handling in namei Message-ID: <20150709154021.GC1718@dft-labs.eu> In-Reply-To: <20150709102533.GO2080@kib.kiev.ua> References: <20150707085857.GZ2080@kib.kiev.ua> <1436393231-5831-1-git-send-email-mjguzik@gmail.com> <1436393231-5831-4-git-send-email-mjguzik@gmail.com> <20150709102533.GO2080@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jul 09, 2015 at 01:25:33PM +0300, Konstantin Belousov wrote:
> On Thu, Jul 09, 2015 at 12:07:10AM +0200, Mateusz Guzik wrote:
> > From: Mateusz Guzik <mjg@freebsd.org>
> >
> > The logic is reorganised so that there is one exit point prior to the
> > lookup loop. This is an intermediate step to making audit logging
> > functions use found vnode instead of translating ni_dirfd on their own.
> >
> > ni_startdir validation is removed. The only in-tree consumer is nfs
> > which already makes sure it is a directory.
> > ---
> > sys/kern/vfs_lookup.c | 50 +++++++++++++++++++++-----------------------------
> > 1 file changed, 21 insertions(+), 29 deletions(-)
> >
> > diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
> > index e434464..d48fcff 100644
> > --- a/sys/kern/vfs_lookup.c
> > +++ b/sys/kern/vfs_lookup.c
> > @@ -158,7 +158,7 @@ namei(struct nameidata *ndp)
> > struct vnode *dp; /* the directory we are searching */
> > struct iovec aiov; /* uio for reading symbolic links */
> > struct uio auio;
> > - int error, linklen;
> > + int error, linklen, startdir_used;
> > struct componentname *cnp = &ndp->ni_cnd;
> > struct thread *td = cnp->cn_thread;
> > struct proc *p = td->td_proc;
> > @@ -169,6 +169,9 @@ namei(struct nameidata *ndp)
> > ("namei: nameiop contaminated with flags"));
> > KASSERT((cnp->cn_flags & OPMASK) == 0,
> > ("namei: flags contaminated with nameiops"));
> > + if (ndp->ni_startdir != NULL)
> > + MPASS(ndp->ni_startdir->v_type == VDIR ||
> > + ndp->ni_startdir->v_type == VBAD);
> Write this as
> MPASS(ndp->ni_startdir == NULL || ... == VDIR || ... == VBAD);
> ?
>
Done.
> I think that the two previous patches are self-contained ?
> Please commit them, after that I think review of this patch can
> be finished.
>
Committed.
diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
index e434464..a93314e 100644
--- a/sys/kern/vfs_lookup.c
+++ b/sys/kern/vfs_lookup.c
@@ -158,7 +158,7 @@ namei(struct nameidata *ndp)
struct vnode *dp; /* the directory we are searching */
struct iovec aiov; /* uio for reading symbolic links */
struct uio auio;
- int error, linklen;
+ int error, linklen, startdir_used;
struct componentname *cnp = &ndp->ni_cnd;
struct thread *td = cnp->cn_thread;
struct proc *p = td->td_proc;
@@ -169,6 +169,8 @@ namei(struct nameidata *ndp)
("namei: nameiop contaminated with flags"));
KASSERT((cnp->cn_flags & OPMASK) == 0,
("namei: flags contaminated with nameiops"));
+ MPASS(ndp->ni_startdir == NULL || ndp->ni_startdir->v_type == VDIR ||
+ ndp->ni_startdir->v_type == VBAD);
if (!lookup_shared)
cnp->cn_flags &= ~LOCKSHARED;
fdp = p->p_fd;
@@ -242,23 +244,19 @@ namei(struct nameidata *ndp)
if (cnp->cn_flags & AUDITVNODE2)
AUDIT_ARG_UPATH2(td, ndp->ni_dirfd, cnp->cn_pnbuf);
+ startdir_used = 0;
dp = NULL;
cnp->cn_nameptr = cnp->cn_pnbuf;
if (cnp->cn_pnbuf[0] == '/') {
error = namei_handle_root(ndp, &dp);
- FILEDESC_SUNLOCK(fdp);
- if (error != 0) {
- vrele(ndp->ni_rootdir);
- if (ndp->ni_startdir != NULL)
- vrele(ndp->ni_startdir);
- namei_cleanup_cnp(cnp);
- return (error);
- }
} else {
if (ndp->ni_startdir != NULL) {
dp = ndp->ni_startdir;
- error = 0;
- } else if (ndp->ni_dirfd != AT_FDCWD) {
+ startdir_used = 1;
+ } else if (ndp->ni_dirfd == AT_FDCWD) {
+ dp = fdp->fd_cdir;
+ VREF(dp);
+ } else {
cap_rights_t rights;
rights = ndp->ni_rightsneeded;
@@ -285,25 +283,18 @@ namei(struct nameidata *ndp)
}
#endif
}
- if (error != 0 || dp != NULL) {
- FILEDESC_SUNLOCK(fdp);
- if (error == 0 && dp->v_type != VDIR) {
- vrele(dp);
- error = ENOTDIR;
- }
- }
- if (error) {
- vrele(ndp->ni_rootdir);
- namei_cleanup_cnp(cnp);
- return (error);
- }
+ if (error == 0 && dp->v_type != VDIR)
+ error = ENOTDIR;
}
- if (dp == NULL) {
- dp = fdp->fd_cdir;
- VREF(dp);
- FILEDESC_SUNLOCK(fdp);
- if (ndp->ni_startdir != NULL)
- vrele(ndp->ni_startdir);
+ FILEDESC_SUNLOCK(fdp);
+ if (ndp->ni_startdir != NULL && !startdir_used)
+ vrele(ndp->ni_startdir);
+ if (error != 0) {
+ if (dp != NULL)
+ vrele(dp);
+ vrele(ndp->ni_rootdir);
+ namei_cleanup_cnp(cnp);
+ return (error);
}
SDT_PROBE(vfs, namei, lookup, entry, dp, cnp->cn_pnbuf,
cnp->cn_flags, 0, 0);
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150709154021.GC1718>