From owner-cvs-all@FreeBSD.ORG Mon Jan 23 20:55:48 2006 Return-Path: X-Original-To: cvs-all@FreeBSD.org Delivered-To: cvs-all@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 7C96A16A41F; Mon, 23 Jan 2006 20:55:48 +0000 (GMT) (envelope-from trhodes@FreeBSD.org) Received: from pittgoth.com (ns1.pittgoth.com [216.38.206.188]) by mx1.FreeBSD.org (Postfix) with ESMTP id 12CE043D58; Mon, 23 Jan 2006 20:55:45 +0000 (GMT) (envelope-from trhodes@FreeBSD.org) Received: from localhost (ip68-105-180-11.dc.dc.cox.net [68.105.180.11]) (authenticated bits=0) by pittgoth.com (8.13.4/8.13.4) with ESMTP id k0NLX6wk016910 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Mon, 23 Jan 2006 16:33:07 -0500 (EST) (envelope-from trhodes@FreeBSD.org) Date: Mon, 23 Jan 2006 15:55:33 -0500 From: Tom Rhodes To: John Baldwin Message-Id: <20060123155533.22995959.trhodes@FreeBSD.org> In-Reply-To: <200601231535.59259.jhb@freebsd.org> References: <200601211210.k0LCAXYl069896@repoman.freebsd.org> <200601231459.25281.jhb@freebsd.org> <20060123150619.63e41bcd.trhodes@FreeBSD.org> <200601231535.59259.jhb@freebsd.org> X-Mailer: Sylpheed version 1.0.5 (GTK+ 1.2.10; i386-portbld-freebsd7.0) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: trhodes@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org, cvs-src@FreeBSD.org Subject: Re: cvs commit: src/sys/nfsserver nfs_serv.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Jan 2006 20:55:48 -0000 On Mon, 23 Jan 2006 15:35:57 -0500 John Baldwin wrote: > On Monday 23 January 2006 15:06, Tom Rhodes wrote: > > On Mon, 23 Jan 2006 14:59:22 -0500 > > > > John Baldwin wrote: > > > On Monday 23 January 2006 14:28, Tom Rhodes wrote: > > > > On Mon, 23 Jan 2006 13:56:16 -0500 > > > > > > > > John Baldwin wrote: > > > > > On Saturday 21 January 2006 07:10, Tom Rhodes wrote: > > > > > > trhodes 2006-01-21 12:10:33 UTC > > > > > > > > > > > > FreeBSD src repository > > > > > > > > > > > > Modified files: > > > > > > sys/nfsserver nfs_serv.c > > > > > > Log: > > > > > > Remove some dead code. > > > > > > > > > > > > Found with: Coverity Prevent(tm) > > > > > > > > > > Are you going to revert this change given the replies? > > > > > > > > Oh, I didn't interpret the comments as "this is wrong please > > > > back it out." I just seen replies, both public and private, > > > > complaining about the indentation. They went like: > > > > > > > > stefanf: "Are you sure this is correct?" > > > > > > When someone says this, you generally should be able to reply with either > > > "Yes, because of X, Y, and Z", or "oops, I guess not, I'll back it out" > > > > I did reply, but forgot to CC: the cvs lists. There was just > > no bothering to follow up since I figured the discussion died, > > since stefan never followed up. > > > > I'll find the reply and push it off. > > > > > > rwatson: "code is a mess in NFS" > > > > > > > > ru: quoting the code "bad indentation" > > > > njl quoting the code "bad indentation" > > > > > > > > rees (NFSv4 guy): "looks fine to me" > > > > > > > > If you, or anyone else for that matter actually wants it > > > > reverted, I'll do that. I'm not in the mood to argue > > > > with people today, or ever. :) > > > > > > > > > Hm, are you sure this change is correct? Apparently Coverity thinks > > > that dirp is always 0 at this point, yes? Looking at nfs_namei() I > > > don't believe that. > > > > > > > > > Note the "I don't believe that" part. > > > > > > > > > I'll put my $0.02 in and agree with Stefan Farfeleder. (Luckily, in > > > this case, the notorious NFS macros are not involved). The comments > > > on nfs_namei() state that dirp can be returned not-NULL even if an > > > error occurs and a check of the code paths in nfs_namei() indicates > > > that this is correct. Can you please re-evaluate your change. > > > > > > If (as I suspect), this is actually an incorrect report from Coverity, > > > we should probably report it back to them to investigate. > > > > > > > > > Please either offer explanations to address the concerns raised or back > > > it out. > > > > --------------------------------------------------------------- > > > > > Hm, are you sure this change is correct? Apparently Coverity thinks > > > that dirp is always 0 at this point, yes? Looking at nfs_namei() I > > > don't believe that. Also the comment above this is now stale and the > > > code inside 'if (error)' not indented properly. > > > > Yes, this change should be correct. dirp is always 0 except for > > one part (which you mention above) and is used/tested elsewhere > > for that reason. njl and ru have already got me on the stale > > comment and indention. Jim Reese (NFSv4 guy) also feels that > > this commit is "good." So I got post-commit review. ;) > > > > But I'll definitely agree with rwatson, the nfs code is really > > scary. :) > > -------------------------------------------------------------- > > Hmm, what do you mean by "one part"? For example, in nfs_namei() you can have > dirp != NULL in the following error cases (look for BEWM): > > int > nfs_namei(struct nameidata *ndp, fhandle_t *fhp, int len, > struct nfssvc_sock *slp, struct sockaddr *nam, struct mbuf **mdp, > caddr_t *dposp, struct vnode **retdirp, int v3, struct vattr *retdirattrp, > int *retdirattr_retp, struct thread *td, int pubflag) > { > ... > > *retdirp = NULL; > > ... > > /* > * Set return directory. Reference to dp is implicitly transfered > * to the returned pointer > */ > *retdirp = dp; > > ... > if (pubflag) { > ... > if ((unsigned char)*fromcp >= WEBNFS_SPECCHAR_START) { > switch ((unsigned char)*fromcp) { > ... > default: > error = EIO; > uma_zfree(namei_zone, cp); > goto out; <=== BEWM!!!! > } > } > ... > while (*fromcp != '\0') { > if (*fromcp == WEBNFS_ESC_CHAR) { > if (fromcp[1] != '\0' && fromcp[2] != '\0') { > ... > } else { > error = ENOENT; > uma_zfree(namei_zone, cp); > goto out; <=== BEWM!!!! > } > ... > } > ... > for (;;) { > ... > error = lookup(ndp); > if (error) > break; <=== BEWM!!! > ... > error = VOP_READLINK(ndp->ni_vp, &auio, cnp->cn_cred); > if (error) { > ... > vrele(ndp->ni_dvp); > vput(ndp->ni_vp); > break; <<< BEWM!!!! > } > ... > } > ... > out: > ... > } > > Won't your changes now be leaking a vnode reference in those cases? Ack, put me in my place. You're right, my bad, I'll back it out now. Sorry for the churn. -- Tom Rhodes