Date: Mon, 23 Jan 2006 15:06:19 -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: <20060123150619.63e41bcd.trhodes@FreeBSD.org> In-Reply-To: <200601231459.25281.jhb@freebsd.org> References: <200601211210.k0LCAXYl069896@repoman.freebsd.org> <200601231356.18292.jhb@freebsd.org> <20060123142822.1be78fcb.trhodes@FreeBSD.org> <200601231459.25281.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. :) -------------------------------------------------------------- -- Tom Rhodes
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060123150619.63e41bcd.trhodes>