[PATCH 3/4] vfs: simplify error handling in namei

[Konstantin Belousov]

On Thu, Jul 09, 2015 at 12:07:10AM +0200, Mateusz Guzik wrote:
> From: Mateusz Guzik <mjg at 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);
?

I think that the two previous patches are self-contained ?
Please commit them, after that I think review of this patch can
be finished.

>  	if (!lookup_shared)
>  		cnp->cn_flags &= ~LOCKSHARED;
>  	fdp = p->p_fd;
> @@ -242,23 +245,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 +284,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);
> -- 
> 2.4.5