Bug 238167 - fs/nfsclient/nfs_clvnops.c: suspicious "if" statement
Summary: fs/nfsclient/nfs_clvnops.c: suspicious "if" statement
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Rick Macklem
Depends on:
Reported: 2019-05-27 08:59 UTC by Alexey Dokuchaev
Modified: 2019-06-02 00:26 UTC (History)
0 users

See Also:
rmacklem: mfc-stable12-
rmacklem: mfc-stable11-


Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Dokuchaev freebsd_committer 2019-05-27 08:59:22 UTC
PVS Studio reports: /usr/src/sys/fs/nfsclient/nfs_clvnops.c:2946:1: error: V523 The 'then' statement is equivalent to the 'else' statement.

The code indeed looks suspicious (shown with "svn blame"):

> 191783   rmacklem               BO_UNLOCK(bo);
> 191783   rmacklem               bremfree(bp);
> 191783   rmacklem               if (passone || !commit)
> 191783   rmacklem                   bp->b_flags |= B_ASYNC;
> 191783   rmacklem               else
> 191783   rmacklem                   bp->b_flags |= B_ASYNC;
> 191783   rmacklem               bwrite(bp);
Comment 1 Alexey Dokuchaev freebsd_committer 2019-05-28 08:31:13 UTC
Also, in the same file, there's another warning:

/usr/src/sys/fs/nfsclient/nfs_clvnops.c:1187:1: warning: V519 The 'error' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1184, 1187:

> 191783   rmacklem       error = 0;
> 191783   rmacklem       newvp = NULLVP;
> 304026   rmacklem       NFSINCRGLOBAL(nfsstatsv1.lookupcache_misses);
> 191783   rmacklem       error = nfsrpc_lookup(dvp, ...);
You might also reconsider needless local variable initialization on line 2496 (it is explicitly assigned three lines below):

> 191783   rmacklem       int error = 0, attrflag, dattrflag;
> 191783   rmacklem       u_int hash;
> 191783   rmacklem
> 191783   rmacklem       error = nfsrpc_lookup(dvp, ...);
Comment 2 Rick Macklem freebsd_committer 2019-05-28 22:01:57 UTC
None of these are bugs (at least by my definition of "bug").
I think you'd find that code has been like that for decades.
(I've actually spotted the "if/else" one and chuckled at it.)

The extraneous initialization of "error = 0" was probably needed
in some (possibly decades old) version of the code.

You can commit changes if you'd like, although I don't see any
reason to bother. (You do want to do the "bp->b_flags |= B_ASYNC;"
unconditionally. I think there was once an additional B_xxx flag that
needed to be set for either the if or else.)

Comment 3 Alexey Dokuchaev freebsd_committer 2019-05-29 07:57:28 UTC
While excessive initialization is technically not a bug (PVS Studio emits a warning, not error for it), if it's not needed these days (with modern compilers), it better be fixed.

The first issue (if/else) clearly looks like a bug to a reader, even if it actually does the right thing, and should be fixed.  If bp->b_flags |= B_ASYNC must be set unconditionally, then if/else should be dropped.

I'm neither familiar enough with this code neither an src committer, so I'd wait until someone comes by and decided to commit it. :-)
Comment 4 commit-hook freebsd_committer 2019-05-31 00:57:20 UTC
A commit references this bug:

Author: rmacklem
Date: Fri May 31 00:56:31 UTC 2019
New revision: 348451
URL: https://svnweb.freebsd.org/changeset/base/348451

  Clean up silly code case.

  This silly code segment has existed in the sources since it was brought
  into FreeBSD 10 years ago. I honestly have no idea why this was done.
  It was possible that I thought that it might have been better to not
  set B_ASYNC for the "else" case, but I can't remember.
  Anyhow, this patch gets rid of the if/else that does the same thing
  either way, since it looks silly and upsets a static analyser.
  This will have no semantic effect on the NFS client.

  PR:		238167

Comment 5 Rick Macklem freebsd_committer 2019-05-31 02:28:29 UTC
Have committed the one change to head and will do the other soon.
Since there is no broken semantics, I don't consider these as bugs,
but the code should be more readable with the commits and it will
keep this static analyser happy.

Since they aren't actual bugs, I don't plan on MFC'ng them and will
close this after the next commit.
Comment 6 commit-hook freebsd_committer 2019-05-31 03:14:10 UTC
A commit references this bug:

Author: rmacklem
Date: Fri May 31 03:13:09 UTC 2019
New revision: 348453
URL: https://svnweb.freebsd.org/changeset/base/348453

  Get rid of extraneous initialization.

  Get rid of an extraneous initialization, mainly to keep a static analyser
  happy. No semantic change.

  PR:		238167
  Submitted by:	Alexey Dokuchaev

Comment 7 Rick Macklem freebsd_committer 2019-05-31 03:17:03 UTC
Fixes committed to head. I left the third case, since I felt it was fine to
initialize error to 0 as a safety belt against future changes that might
result in it being not properly initialized.
Comment 8 Alexey Dokuchaev freebsd_committer 2019-05-31 08:35:39 UTC
Thanks for the changes, I agree, there's no need for MFC.

> I left the third case, since I felt it was fine to initialize error to 0
> as a safety belt against future changes that might result in it being not
> properly initialized.
Initialization of local variables actually can mask some real bugs (because if you accidentally omit the assignment, compiler won't be able to tell you that the variable is not properly initialized.  If I change the code to remove the assignment when calling nfsrpc_lookup(dvp, ...); the code would still compile fine.  But if I remove initization at declaration, the compiler will catch the error:

/usr/src/sys/fs/nfsclient/nfs_clvnops.c:2502:14: error: variable 'error' is
      uninitialized when used here [-Werror,-Wuninitialized]
        if (npp && !error) {

So, actually, you're removing the safety belts, not adding them. :-)  There are several bugs like this (int error = 0) in this file.

Our style(9) says: Be careful to not obfuscate the code by initializing variables in the declarations.  Use this feature only thoughtfully.
Comment 9 Rick Macklem freebsd_committer 2019-06-02 00:26:53 UTC
I choose not to depend on a compiler to find bugs for me (same applies to
static analysers). I am pretty careful to make sure that variables will
be initialized before they are used and I read my code multiple times
before it is committed. (Yes, I can still make mistakes.)

What I care about are stupid "might not be initialized" warnings generated
by some compiler I don't use (newer gcc for example), where there are
initialized before use, but the code structure is such that the static
analysis can't tell it has been.
--> These result in irritating (for others) build failures that I end up
    fixing by initializing the variable. This is when I usually choose to
    do so in the variable's declaration. When you are reading my code and
    see a variable initialized in a declaration, you can probably assume
    that an extraneous "warning, might not be initialized" has occurred
    for some previous version of the code.
    --> Leaving this is what I was referring to when I said "safety belt".

You are correct w.r.t. style(9). However, a lot of this code was written
by me and not specifically for FreeBSD, so it is in "my style".
And, since style(9) says:
     In general code can be considered ``new code'' when it makes up about 50%
     or more of the file(s) involved.  This is enough to break precedents in
     the existing code and use the current style guidelines.
I consider that style is retained unless a file gets largely re-written.
(I do "cheat" and tend to use style(9) in changes to existing code sometimes,
 but I resist any urge to change style in existing code, since it makes
 following what real changes have been applied more difficult.)

I don't plan on doing a commit for this, but I don't have a strong opinion
on it, so if someone else chooses to commit your change, I won't be upset.