HomeFreeBSD

Cleanup: Address Clang's static analyzer's unused code complaints

Description

Cleanup: Address Clang's static analyzer's unused code complaints

These were categorized as the following:

  • Dead assignment 23
  • Dead increment 4
  • Dead initialization 6
  • Dead nested assignment 18

Most of these are harmless, but since actual issues can hide among them,
we correct them.

That said, there were a few return values that were being ignored that
appeared to merit some correction:

  • destroy_callback() in cmd/zfs/zfs_main.c ignored the error from destroy_batched(). We handle it by returning -1 if there is an error.
  • zfs_do_upgrade() in cmd/zfs/zfs_main.c ignored the error from zfs_for_each(). We handle it by doing a binary OR of the error value from the subsequent zfs_for_each() call to the existing value. This is how errors are mostly handled inside zfs_for_each(). The error value here is passed to exit from the zfs command, so doing a binary or on it is better than what we did previously.
  • get_zap_prop() in module/zfs/zcp_get.c ignored the error from dsl_prop_get_ds() when the property is not of type string. We return an error when it does. There is a small concern that the zfs_get_temporary_prop() call would handle things, but in the case that it does not, we would be pushing an uninitialized numval onto the lua stack. It is expected that dsl_prop_get_ds() will succeed anytime that zfs_get_temporary_prop() does, so that not giving it a chance to fix things is not a problem.
  • draid_merge_impl() in tests/zfs-tests/cmd/draid.c used nvlist_add_nvlist() twice in ways in which errors are expected to be impossible, so we switch to fnvlist_add_nvlist().

A few notable ones did not merit use of the return value, so we
suppressed it with (void):

  • write_free_diffs() in lib/libzfs/libzfs_diff.c ignored the error value from describe_free(). A look through the commit history revealed that this was intentional.
  • arc_evict_hdr() in module/zfs/arc.c did not need to use the returned handle from arc_hdr_realloc() because it is already referenced in lists.
  • spa_vdev_detach() in module/zfs/spa.c has a comment explicitly saying not to use the error from vdev_label_init() because whatever causes the error could be the reason why a detach is being done.

Unfortunately, I am not presently able to analyze the kernel modules
with Clang's static analyzer, so I could have missed some cases of this.
In cases where reports were present in code that is duplicated between
Linux and FreeBSD, I made a conscious effort to fix the FreeBSD version
too.

After this commit is merged, regressions like dee8934 should become
extremely obvious with Clang's static analyzer since a regression would
appear in the results as the only instance of unused code. That assumes
that Coverity does not catch the issue first.

My local branch with fixes from all of my outstanding non-draft pull
requests shows 118 reports from Clang's static anlayzer after this
patch. That is down by 51 from 169.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Cedric Berger <cedric@precidata.com>
Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Closes #13986

Details

Provenance
Richard Yao <richard.yao@alumni.stonybrook.edu>Authored on Oct 14 2022, 8:37 PM
GitHub <noreply@github.com>Committed on Oct 14 2022, 8:37 PM
Parents
rG19516b69ee41: Fix potential NULL pointer dereference in lzc_ioctl()
Branches
Unknown
Tags
Unknown

Event Timeline

GitHub <noreply@github.com> committed rG6a42939fcd62: Cleanup: Address Clang's static analyzer's unused code complaints (authored by Richard Yao <richard.yao@alumni.stonybrook.edu>).Oct 14 2022, 8:37 PM