cmd/zpool cstyle issues

[tsoome]

add missing headers.
usage() is no-return, so anything after call to it is unreachable code.
use (void) cast where we do ignore return value.

Motivation and Context

fix errors and warnings in case the build system is checking the error cases listed above.

Description

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• [ x] Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• [ x] My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• [ x] I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• [ x] All commit messages are properly formatted and contain Signed-off-by.

[robn]

What's the rationale for the (void) casts? From what I've seen, they don't really do anything - they don't generally change code generations, and they don't silence "unused return" warnings when those would be appropriate. They do add a lot of visual clutter though, which is why I've been removing them when I change things. Is there a reason?

[tsoome]

What's the rationale for the (void) casts? From what I've seen, they don't really do anything - they don't generally change code generations, and they don't silence "unused return" warnings when those would be appropriate. They do add a lot of visual clutter though, which is why I've been removing them when I change things. Is there a reason?

The reason is that by using (void) cast we acknowledge that we do ignore the return value knowingly and they do silence 'unused return value' warnings. (Also they sometimes do make people to ask if it is really good idea to ignore the return value in some places where it should not be ignored;) we are getting those warnings with smatch.

[robn]

they do silence 'unused return value' warnings.

Which compiler & version are you using for this? If that were true we should have all hell breaking loose in CI, but I've never seen it making a difference there or locally.

Also they sometimes do make people to ask if it is really good idea to ignore the return value in some places where it should not be ignored

If a return outright must not be returned, it can be annotated with __attribute__((__warn_unused_result__)) (we do this for zll_commit().

To be clear, I'm not against more safety; I'm just don't know where it realistically does anything, and its a big readability hit. If we are going to do it, then I want to do it everywhere.

[tsoome]

they do silence 'unused return value' warnings.

Which compiler & version are you using for this? If that were true we should have all hell breaking loose in CI, but I've never seen it making a difference there or locally.

as I wrote, we do get them with smatch (static analyzer).

Also they sometimes do make people to ask if it is really good idea to ignore the return value in some places where it should not be ignored

If a return outright must not be returned, it can be annotated with __attribute__((__warn_unused_result__)) (we do this for zll_commit().

we can not annotate OS API;)

To be clear, I'm not against more safety; I'm just don't know where it realistically does anything, and its a big readability hit. If we are going to do it, then I want to do it everywhere.

readability hit is already there, the code is mixture of (void) casts used in inherited code and not used in updated code;)

I did create pull req for cmd/zpool because I havent got through next ones yet;)

[robn]

Sorry, I missed that you'd said smatch. Can you silence it? I haven't seen this regularly with other analyzers I've used (though perhaps they can be configured that way). This seems like a lot of clutter to add for one tool.

readability hit is already there, the code is mixture of (void) casts used in inherited code and not used in updated code;)

Yeah, I've been low-key removing them as I modify stuff.

Obviously, I'm not convinced, because they don't regularly silence anything in any of the compilers and tests we use. That's my opinion of course, and others might not agree. But I think if we are going to do this, I want a cstyle lint and/or compiler flags that will cause a compile error any time we miss one. Otherwise, its just going to fall out of sync again.

[tsoome]

Sorry, I missed that you'd said smatch. Can you silence it? I haven't seen this regularly with other analyzers I've used (though perhaps they can be configured that way). This seems like a lot of clutter to add for one tool.

yes we can silence it.

readability hit is already there, the code is mixture of (void) casts used in inherited code and not used in updated code;)

Yeah, I've been low-key removing them as I modify stuff.

Obviously, I'm not convinced, because they don't regularly silence anything in any of the compilers and tests we use. That's my opinion of course, and others might not agree. But I think if we are going to do this, I want a cstyle lint and/or compiler flags that will cause a compile error any time we miss one. Otherwise, its just going to fall out of sync again.

Well yes, essentially it is a question if we want to get notified about unhandled return values or not.

[behlendorf]

I've been mulling this over for a while now. Where I finally landed is I think it is reasonable to require a (void) cast for any intentionally ignored return values. It's true this isn't causing us any trouble with the current tool chains used by the CI, but the CI also doesn't cover every relevant build environment either. Plus I'm personally okay with the small amount of additional noise. I'd rather have clarity that a return value is intentionally being ignored, rather than it's been mistakenly ignored.

Where I think we all agree is we need to pick a style and enforce it. Otherwise we'll simply fall out of sync again. It sounds like if we were to add a smatch builder to the CI we could start selectively enforcing this as source files are updated. @tsoome how long does an analysis run take? Could you share some directions on how best to run smatch against the repo?