Cppcheck #11508

behlendorf · 2021-01-24T00:40:59Z

Motivation and Context

If we're going to use cppcheck as part of the CI we can do a better job of
integrating it so it can be easily run. This change also resolves errors reported
by newer versions of cppcheck. Since we expect the GitHub Action ubuntu-latest
image to switch from 18.04 to 20.04 fairly soon, and with it an updated version
of cppcheck, it would be best to address this now.

Description

In order for cppcheck to perform a proper analysis it needs to be
aware of how the sources are compiled (source files, include
paths/files, extra defines, etc). All the needed information is
available from the Makefiles and can be leveraged with a generic
cppcheck Makefile target. So let's add one.

Additional minor changes:

Removing the cppcheck-suppressions.txt file. With cppcheck 2.3
and these changes it appears to no longer be needed. Some inline
suppression's were also removed since they appear not to be
needed. We can add them back if it turns out they're needed
for older versions of cppcheck.
Added the ax_count_cpus m4 macro to detect at configure time how
many processors are available in order to run multiple cppcheck
jobs.
"PHONY =" line moved in to the Rules.am file which is included
at the top of all Makefile.am's. This is just convenient because
it allows us to use the += syntax to add phony targets.
One upside of this integration worth mentioning is it now allows
make cppcheck to be run in any directory to check that sub-tree.
For the moment, cppcheck is not run against the FreeBSD specific
kernel sources. The cppcheck-FreeBSD target will need to be
implemented and testing on FreeBSD to support this.

How Has This Been Tested?

On Fedora 33 with the distribution provided cppcheck 2.3, and
using the latest version of cppcheck (what will be 2.4).

./configure
make cppcheck

Resolved all errors reported and checked that cppcheck reported
that the relevant files were all checked.

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)
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:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
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.
All commit messages are properly formatted and contain Signed-off-by.

The `abd_get_offset_*()` routines create an abd_t that references another abd_t, and doesn't allocate any pages/buffers of its own. In some workloads, these routines may be called frequently, to create many abd_t's representing small pieces of a single large abd_t. In particular, the upcoming RAIDZ Expansion project makes heavy use of these routines. This commit adds the ability for the caller to allocate and provide the abd_t struct to a variant of `abd_get_offset_*()`. This eliminates the cost of allocating the abd_t and performing the accounting associated with it (`abdstat_struct_size`). The RAIDZ/DRAID code uses this for the `rc_abd`, which references the zio's abd. The upcoming RAIDZ Expansion project will leverage this infrastructure to increase performance of reads post-expansion by around 50%. Additionally, some of the interfaces around creating and destroying abd_t's are cleaned up. Most significantly, the distinction between `abd_put()` and `abd_free()` is eliminated; all types of abd_t's are now disposed of with `abd_free()`. Reviewed-by: Brian Atkinson <batkinson@lanl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Matthew Ahrens <mahrens@delphix.com> Issue #8853 Closes #11439

Explicitly check for NULL to satisfy cppcheck that "val" can never be NULL when passed to printf(). This looks like a false positive since is_blank_str() can never take the false conditional branch when passed a NULL. But there's no harm in adding the extra check. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

The double free reported for the realloc() failure branch is a false positive. It should be resolved in cppcheck 2.4 but for the benefit of older versions we supress the warning. https://trac.cppcheck.net/ticket/9292 Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

The ASSERT that the passed pointer isn't NULL appears after the pointer has already been dereferenced. Remove the redundant check. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

Identical condition and return expression 'rc', return value is always 0. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

In order for cppcheck to perform a proper analysis it needs to be aware of how the sources are compiled (source files, include paths/files, extra defines, etc). All the needed information is available from the Makefiles and can be leveraged with a generic cppcheck Makefile target. So let's add one. Additional minor changes: * Removing the cppcheck-suppressions.txt file. With cppcheck 2.3 and these changes it appears to no longer be needed. Some inline suppressions were also removed since they appear not to be needed. We can add them back if it turns out they're needed for older versions of cppcheck. * Added the ax_count_cpus m4 macro to detect at configure time how many processors are available in order to run multiple cppcheck jobs. This value is also now used as a replacement for nproc when executing the kernel interface checks. * "PHONY =" line moved in to the Rules.am file which is included at the top of all Makefile.am's. This is just convenient becase it allows us to use the += syntax to add phony targets. * One upside of this integration worth mentioning is it now allows `make cppcheck` to be run in any directory to check that subtree. * For the moment, cppcheck is not run against the FreeBSD specific kernel sources. The cppcheck-FreeBSD target will need to be implemented and testing on FreeBSD to support this. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

ghost

This works as-is on FreeBSD. Getting the checks right in the module code will take some tinkering on FreeBSD, but we can work on that separately.

The double free reported for the realloc() failure branch is a false positive. It should be resolved in cppcheck 2.4 but for the benefit of older versions we supress the warning. https://trac.cppcheck.net/ticket/9292 Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #11508

The ASSERT that the passed pointer isn't NULL appears after the pointer has already been dereferenced. Remove the redundant check. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #11508

Identical condition and return expression 'rc', return value is always 0. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #11508

In order for cppcheck to perform a proper analysis it needs to be aware of how the sources are compiled (source files, include paths/files, extra defines, etc). All the needed information is available from the Makefiles and can be leveraged with a generic cppcheck Makefile target. So let's add one. Additional minor changes: * Removing the cppcheck-suppressions.txt file. With cppcheck 2.3 and these changes it appears to no longer be needed. Some inline suppressions were also removed since they appear not to be needed. We can add them back if it turns out they're needed for older versions of cppcheck. * Added the ax_count_cpus m4 macro to detect at configure time how many processors are available in order to run multiple cppcheck jobs. This value is also now used as a replacement for nproc when executing the kernel interface checks. * "PHONY =" line moved in to the Rules.am file which is included at the top of all Makefile.am's. This is just convenient becase it allows us to use the += syntax to add phony targets. * One upside of this integration worth mentioning is it now allows `make cppcheck` to be run in any directory to check that subtree. * For the moment, cppcheck is not run against the FreeBSD specific kernel sources. The cppcheck-FreeBSD target will need to be implemented and testing on FreeBSD to support this. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #11508

Explicitly check for NULL to satisfy cppcheck that "val" can never be NULL when passed to printf(). This looks like a false positive since is_blank_str() can never take the false conditional branch when passed a NULL. But there's no harm in adding the extra check. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

The double free reported for the realloc() failure branch is a false positive. It should be resolved in cppcheck 2.4 but for the benefit of older versions we supress the warning. https://trac.cppcheck.net/ticket/9292 Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

The ASSERT that the passed pointer isn't NULL appears after the pointer has already been dereferenced. Remove the redundant check. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

Identical condition and return expression 'rc', return value is always 0. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

In order for cppcheck to perform a proper analysis it needs to be aware of how the sources are compiled (source files, include paths/files, extra defines, etc). All the needed information is available from the Makefiles and can be leveraged with a generic cppcheck Makefile target. So let's add one. Additional minor changes: * Removing the cppcheck-suppressions.txt file. With cppcheck 2.3 and these changes it appears to no longer be needed. Some inline suppressions were also removed since they appear not to be needed. We can add them back if it turns out they're needed for older versions of cppcheck. * Added the ax_count_cpus m4 macro to detect at configure time how many processors are available in order to run multiple cppcheck jobs. This value is also now used as a replacement for nproc when executing the kernel interface checks. * "PHONY =" line moved in to the Rules.am file which is included at the top of all Makefile.am's. This is just convenient becase it allows us to use the += syntax to add phony targets. * One upside of this integration worth mentioning is it now allows `make cppcheck` to be run in any directory to check that subtree. * For the moment, cppcheck is not run against the FreeBSD specific kernel sources. The cppcheck-FreeBSD target will need to be implemented and testing on FreeBSD to support this. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

Explicitly check for NULL to satisfy cppcheck that "val" can never be NULL when passed to printf(). This looks like a false positive since is_blank_str() can never take the false conditional branch when passed a NULL. But there's no harm in adding the extra check. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

The double free reported for the realloc() failure branch is a false positive. It should be resolved in cppcheck 2.4 but for the benefit of older versions we supress the warning. https://trac.cppcheck.net/ticket/9292 Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

The ASSERT that the passed pointer isn't NULL appears after the pointer has already been dereferenced. Remove the redundant check. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

Identical condition and return expression 'rc', return value is always 0. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

In order for cppcheck to perform a proper analysis it needs to be aware of how the sources are compiled (source files, include paths/files, extra defines, etc). All the needed information is available from the Makefiles and can be leveraged with a generic cppcheck Makefile target. So let's add one. Additional minor changes: * Removing the cppcheck-suppressions.txt file. With cppcheck 2.3 and these changes it appears to no longer be needed. Some inline suppressions were also removed since they appear not to be needed. We can add them back if it turns out they're needed for older versions of cppcheck. * Added the ax_count_cpus m4 macro to detect at configure time how many processors are available in order to run multiple cppcheck jobs. This value is also now used as a replacement for nproc when executing the kernel interface checks. * "PHONY =" line moved in to the Rules.am file which is included at the top of all Makefile.am's. This is just convenient becase it allows us to use the += syntax to add phony targets. * One upside of this integration worth mentioning is it now allows `make cppcheck` to be run in any directory to check that subtree. * For the moment, cppcheck is not run against the FreeBSD specific kernel sources. The cppcheck-FreeBSD target will need to be implemented and testing on FreeBSD to support this. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

In order for cppcheck to perform a proper analysis it needs to be aware of how the sources are compiled (source files, include paths/files, extra defines, etc). All the needed information is available from the Makefiles and can be leveraged with a generic cppcheck Makefile target. So let's add one. Additional minor changes: * Removing the cppcheck-suppressions.txt file. With cppcheck 2.3 and these changes it appears to no longer be needed. Some inline suppressions were also removed since they appear not to be needed. We can add them back if it turns out they're needed for older versions of cppcheck. * Added the ax_count_cpus m4 macro to detect at configure time how many processors are available in order to run multiple cppcheck jobs. This value is also now used as a replacement for nproc when executing the kernel interface checks. * "PHONY =" line moved in to the Rules.am file which is included at the top of all Makefile.am's. This is just convenient becase it allows us to use the += syntax to add phony targets. * One upside of this integration worth mentioning is it now allows `make cppcheck` to be run in any directory to check that subtree. * For the moment, cppcheck is not run against the FreeBSD specific kernel sources. The cppcheck-FreeBSD target will need to be implemented and testing on FreeBSD to support this. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #11508

The double free reported for the realloc() failure branch is a false positive. It should be resolved in cppcheck 2.4 but for the benefit of older versions we supress the warning. https://trac.cppcheck.net/ticket/9292 Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11508

behlendorf added Type: Building Indicates an issue related to building binaries Status: Code Review Needed Ready for review and testing labels Jan 24, 2021

behlendorf added 5 commits January 25, 2021 14:04

cppcheck: remove redundant ASSERTs

d4d009c

The ASSERT that the passed pointer isn't NULL appears after the pointer has already been dereferenced. Remove the redundant check. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

cppcheck: return value always 0

15fe6a6

Identical condition and return expression 'rc', return value is always 0. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

behlendorf force-pushed the cppcheck branch from 2b5b8f5 to c55285d Compare January 25, 2021 22:05

behlendorf requested a review from a user January 26, 2021 20:30

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 26, 2021

ghost approved these changes Jan 26, 2021

View reviewed changes

behlendorf closed this in 7454d2b Jan 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cppcheck #11508

Cppcheck #11508

behlendorf commented Jan 24, 2021

ghost left a comment

Cppcheck #11508

Cppcheck #11508

Conversation

behlendorf commented Jan 24, 2021

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ghost left a comment

Choose a reason for hiding this comment