Bug 238167
| Summary: | fs/nfsclient/nfs_clvnops.c: suspicious "if" statement | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Alexey Dokuchaev <danfe> |
| Component: | kern | Assignee: | Rick Macklem <rmacklem> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | Flags: | rmacklem:
mfc-stable12-
rmacklem: mfc-stable11- |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
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, ...); 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.) rick 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. :-) 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 Log: 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 Changes: head/sys/fs/nfsclient/nfs_clvnops.c 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. 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 Log: 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 Changes: head/sys/fs/nfsclient/nfs_clvnops.c 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. 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.
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.
|
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);