Date: Mon, 23 Jan 2006 15:55:33 -0500 From: Tom Rhodes <trhodes@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> 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 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 23 Jan 2006 15:35:57 -0500
John Baldwin <jhb@freebsd.org> wrote:
> On Monday 23 January 2006 15:06, Tom Rhodes wrote:
> > On Mon, 23 Jan 2006 14:59:22 -0500
> >
> > John Baldwin <jhb@freebsd.org> wrote:
> > > On Monday 23 January 2006 14:28, Tom Rhodes wrote:
> > > > On Mon, 23 Jan 2006 13:56:16 -0500
> > > >
> > > > John Baldwin <jhb@FreeBSD.org> 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. :)
> > >
> > > <quote from="stefanf">
> > > 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.
> > > </quote>
> > >
> > > Note the "I don't believe that" part.
> > >
> > > <quote from="Peter Jeremy">
> > > 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.
> > > </quote>
> > >
> > > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060123155533.22995959.trhodes>