cvs commit: src/sys/nfsserver nfs_serv.c

[Tom Rhodes]

On Mon, 23 Jan 2006 14:59:22 -0500
John Baldwin <jhb at 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 at 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