Skip to content

Commit

Permalink
Cleanup: Address Clang's static analyzer's unused code complaints
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ryao authored Oct 14, 2022
1 parent 19516b6 commit 6a42939
Show file tree
Hide file tree
Showing 32 changed files with 45 additions and 75 deletions.
2 changes: 0 additions & 2 deletions cmd/raidz_test/raidz_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,6 @@ static void process_options(int argc, char **argv)
memcpy(o, &rto_opts_defaults, sizeof (*o));

while ((opt = getopt(argc, argv, "TDBSvha:er:o:d:s:t:")) != -1) {
value = 0;

switch (opt) {
case 'a':
value = strtoull(optarg, NULL, 0);
Expand Down
9 changes: 7 additions & 2 deletions cmd/zfs/zfs_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -1453,8 +1453,13 @@ destroy_callback(zfs_handle_t *zhp, void *data)
if (zfs_get_type(zhp) == ZFS_TYPE_SNAPSHOT) {
cb->cb_snap_count++;
fnvlist_add_boolean(cb->cb_batchedsnaps, name);
if (cb->cb_snap_count % 10 == 0 && cb->cb_defer_destroy)
if (cb->cb_snap_count % 10 == 0 && cb->cb_defer_destroy) {
error = destroy_batched(cb);
if (error != 0) {
zfs_close(zhp);
return (-1);
}
}
} else {
error = destroy_batched(cb);
if (error != 0 ||
Expand Down Expand Up @@ -2576,7 +2581,7 @@ zfs_do_upgrade(int argc, char **argv)
cb.cb_foundone = B_FALSE;
cb.cb_newer = B_TRUE;

ret = zfs_for_each(0, NULL, flags, ZFS_TYPE_FILESYSTEM,
ret |= zfs_for_each(0, NULL, flags, ZFS_TYPE_FILESYSTEM,
NULL, NULL, 0, upgrade_list_callback, &cb);

if (!cb.cb_foundone && !found) {
Expand Down
4 changes: 1 addition & 3 deletions lib/libefi/rdwr_efi.c
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,6 @@ efi_alloc_and_read(int fd, struct dk_gpt **vtoc)
void *tmp;
length = (int) sizeof (struct dk_gpt) +
(int) sizeof (struct dk_part) * (vptr->efi_nparts - 1);
nparts = vptr->efi_nparts;
if ((tmp = realloc(vptr, length)) == NULL) {
/* cppcheck-suppress doubleFree */
free(vptr);
Expand Down Expand Up @@ -565,10 +564,9 @@ int
efi_rescan(int fd)
{
int retry = 10;
int error;

/* Notify the kernel a devices partition table has been updated */
while ((error = ioctl(fd, BLKRRPART)) != 0) {
while (ioctl(fd, BLKRRPART) != 0) {
if ((--retry == 0) || (errno != EBUSY)) {
(void) fprintf(stderr, "the kernel failed to rescan "
"the partition table: %d\n", errno);
Expand Down
2 changes: 1 addition & 1 deletion lib/libzfs/libzfs_dataset.c
Original file line number Diff line number Diff line change
Expand Up @@ -2006,7 +2006,7 @@ zfs_prop_inherit(zfs_handle_t *zhp, const char *propname, boolean_t received)
if ((ret = changelist_prefix(cl)) != 0)
goto error;

if ((ret = zfs_ioctl(zhp->zfs_hdl, ZFS_IOC_INHERIT_PROP, &zc)) != 0) {
if (zfs_ioctl(zhp->zfs_hdl, ZFS_IOC_INHERIT_PROP, &zc) != 0) {
changelist_free(cl);
return (zfs_standard_error(hdl, errno, errbuf));
} else {
Expand Down
2 changes: 1 addition & 1 deletion lib/libzfs/libzfs_diff.c
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ write_free_diffs(FILE *fp, differ_info_t *di, dmu_diff_record_t *dr)
if (zc.zc_obj > dr->ddr_last) {
break;
}
err = describe_free(fp, di, zc.zc_obj, fobjname,
(void) describe_free(fp, di, zc.zc_obj, fobjname,
MAXPATHLEN);
} else if (errno == ESRCH) {
break;
Expand Down
1 change: 0 additions & 1 deletion lib/libzfs/libzfs_pool.c
Original file line number Diff line number Diff line change
Expand Up @@ -2214,7 +2214,6 @@ zpool_import_props(libzfs_handle_t *hdl, nvlist_t *config, const char *newname,
((policy.zlp_rewind & ZPOOL_TRY_REWIND) != 0), nv);
}
nvlist_free(nv);
return (0);
}

return (ret);
Expand Down
4 changes: 2 additions & 2 deletions lib/libzfs/libzfs_sendrecv.c
Original file line number Diff line number Diff line change
Expand Up @@ -2117,9 +2117,9 @@ send_prelim_records(zfs_handle_t *zhp, const char *from, int fd,
fnvlist_add_boolean(hdrnv, "raw");
}

if ((err = gather_nvlist(zhp->zfs_hdl, tofs,
if (gather_nvlist(zhp->zfs_hdl, tofs,
from, tosnap, recursive, raw, doall, replicate, skipmissing,
verbose, backup, holds, props, &fss, fsavlp)) != 0) {
verbose, backup, holds, props, &fss, fsavlp) != 0) {
return (zfs_error(zhp->zfs_hdl, EZFS_BADBACKUP,
errbuf));
}
Expand Down
2 changes: 1 addition & 1 deletion lib/libzfs/libzfs_util.c
Original file line number Diff line number Diff line change
Expand Up @@ -1249,7 +1249,7 @@ zcmd_read_dst_nvlist(libzfs_handle_t *hdl, zfs_cmd_t *zc, nvlist_t **nvlp)
static void
zprop_print_headers(zprop_get_cbdata_t *cbp, zfs_type_t type)
{
zprop_list_t *pl = cbp->cb_proplist;
zprop_list_t *pl;
int i;
char *title;
size_t len;
Expand Down
8 changes: 3 additions & 5 deletions lib/libzutil/os/linux/zutil_device_path_os.c
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,6 @@ dm_get_underlying_path(const char *dm_name)
char *tmp = NULL;
char *path = NULL;
char *dev_str;
int size;
char *first_path = NULL;
char *enclosure_path;

Expand All @@ -450,7 +449,7 @@ dm_get_underlying_path(const char *dm_name)
else
dev_str = tmp;

if ((size = asprintf(&tmp, "/sys/block/%s/slaves/", dev_str)) == -1) {
if (asprintf(&tmp, "/sys/block/%s/slaves/", dev_str) == -1) {
tmp = NULL;
goto end;
}
Expand Down Expand Up @@ -479,8 +478,7 @@ dm_get_underlying_path(const char *dm_name)
if (!enclosure_path)
continue;

if ((size = asprintf(
&path, "/dev/%s", ep->d_name)) == -1)
if (asprintf(&path, "/dev/%s", ep->d_name) == -1)
path = NULL;
free(enclosure_path);
break;
Expand All @@ -499,7 +497,7 @@ dm_get_underlying_path(const char *dm_name)
* enclosure devices. Throw up out hands and return the first
* underlying path.
*/
if ((size = asprintf(&path, "/dev/%s", first_path)) == -1)
if (asprintf(&path, "/dev/%s", first_path) == -1)
path = NULL;
}

Expand Down
4 changes: 1 addition & 3 deletions module/icp/algs/blake3/blake3.c
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,7 @@ static void chunk_state_update(const blake3_ops_t *ops,
input_len -= BLAKE3_BLOCK_LEN;
}

size_t take = chunk_state_fill_buf(ctx, input, input_len);
input += take;
input_len -= take;
chunk_state_fill_buf(ctx, input, input_len);
}

static output_t chunk_state_output(const blake3_chunk_state_t *ctx)
Expand Down
1 change: 0 additions & 1 deletion module/icp/algs/modes/ccm.c
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ ccm_mode_encrypt_contiguous_blocks(ccm_ctx_t *ctx, char *data, size_t length,
return (CRYPTO_SUCCESS);
}

lastp = (uint8_t *)ctx->ccm_cb;
crypto_init_ptrs(out, &iov_or_mp, &offset);

mac_buf = (uint8_t *)ctx->ccm_mac_buf;
Expand Down
1 change: 0 additions & 1 deletion module/icp/algs/modes/ctr.c
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ ctr_mode_contiguous_blocks(ctr_ctx_t *ctx, char *data, size_t length,
return (CRYPTO_SUCCESS);
}

lastp = (uint8_t *)ctx->ctr_cb;
crypto_init_ptrs(out, &iov_or_mp, &offset);

do {
Expand Down
1 change: 0 additions & 1 deletion module/icp/algs/modes/gcm.c
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ gcm_mode_encrypt_contiguous_blocks(gcm_ctx_t *ctx, char *data, size_t length,
return (CRYPTO_SUCCESS);
}

lastp = (uint8_t *)ctx->gcm_cb;
crypto_init_ptrs(out, &iov_or_mp, &offset);

gops = gcm_impl_get_ops();
Expand Down
2 changes: 1 addition & 1 deletion module/lua/ldo.c
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ int luaD_poscall (lua_State *L, StkId firstResult) {
}
res = ci->func; /* res == final position of 1st result */
wanted = ci->nresults;
L->ci = ci = ci->previous; /* back to caller */
L->ci = ci->previous; /* back to caller */
/* move results to correct place */
for (i = wanted; i != 0 && firstResult < L->top; i--)
setobjs2s(L, res++, firstResult++);
Expand Down
1 change: 0 additions & 1 deletion module/os/freebsd/zfs/zfs_znode.c
Original file line number Diff line number Diff line change
Expand Up @@ -1949,7 +1949,6 @@ zfs_obj_to_path_impl(objset_t *osp, uint64_t obj, sa_handle_t *hdl,
} else if (error != ENOENT) {
return (error);
}
error = 0;

for (;;) {
uint64_t pobj;
Expand Down
1 change: 0 additions & 1 deletion module/os/freebsd/zfs/zio_crypt.c
Original file line number Diff line number Diff line change
Expand Up @@ -1735,7 +1735,6 @@ zio_do_crypt_data(boolean_t encrypt, zio_crypt_key_t *key,
goto error;
if (locked) {
rw_exit(&key->zk_salt_lock);
locked = B_FALSE;
}

if (authbuf != NULL)
Expand Down
1 change: 0 additions & 1 deletion module/os/linux/zfs/zfs_znode.c
Original file line number Diff line number Diff line change
Expand Up @@ -2136,7 +2136,6 @@ zfs_obj_to_path_impl(objset_t *osp, uint64_t obj, sa_handle_t *hdl,
} else if (error != ENOENT) {
return (error);
}
error = 0;

for (;;) {
uint64_t pobj = 0;
Expand Down
1 change: 0 additions & 1 deletion module/os/linux/zfs/zio_crypt.c
Original file line number Diff line number Diff line change
Expand Up @@ -1968,7 +1968,6 @@ zio_do_crypt_data(boolean_t encrypt, zio_crypt_key_t *key,

if (locked) {
rw_exit(&key->zk_salt_lock);
locked = B_FALSE;
}

if (authbuf != NULL)
Expand Down
2 changes: 1 addition & 1 deletion module/zfs/arc.c
Original file line number Diff line number Diff line change
Expand Up @@ -3939,7 +3939,7 @@ arc_evict_hdr(arc_buf_hdr_t *hdr, kmutex_t *hash_lock, uint64_t *real_evicted)
* dropping from L1+L2 cached to L2-only,
* realloc to remove the L1 header.
*/
hdr = arc_hdr_realloc(hdr, hdr_full_cache,
(void) arc_hdr_realloc(hdr, hdr_full_cache,
hdr_l2only_cache);
*real_evicted += HDR_FULL_SIZE - HDR_L2ONLY_SIZE;
} else {
Expand Down
1 change: 0 additions & 1 deletion module/zfs/dbuf.c
Original file line number Diff line number Diff line change
Expand Up @@ -1549,7 +1549,6 @@ dbuf_read_impl(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags,
uint32_t aflags = ARC_FLAG_NOWAIT;
int err, zio_flags;

err = zio_flags = 0;
DB_DNODE_ENTER(db);
dn = DB_DNODE(db);
ASSERT(!zfs_refcount_is_zero(&db->db_holds));
Expand Down
1 change: 0 additions & 1 deletion module/zfs/dsl_bookmark.c
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,6 @@ dsl_bookmark_create_check_impl(dsl_pool_t *dp,
switch (error) {
case ESRCH:
/* happy path: new bmark doesn't exist, proceed after switch */
error = 0;
break;
case 0:
error = SET_ERROR(EEXIST);
Expand Down
3 changes: 2 additions & 1 deletion module/zfs/dsl_dataset.c
Original file line number Diff line number Diff line change
Expand Up @@ -3421,7 +3421,8 @@ dsl_dataset_promote_check(void *arg, dmu_tx_t *tx)
conflicting_snaps = B_TRUE;
} else if (err == ESRCH) {
err = 0;
} else if (err != 0) {
}
if (err != 0) {
goto out;
}
}
Expand Down
1 change: 0 additions & 1 deletion module/zfs/dsl_pool.c
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,6 @@ dsl_pool_open(dsl_pool_t *dp)
/*
* We might not have created the remap bpobj yet.
*/
err = 0;
} else {
goto out;
}
Expand Down
10 changes: 5 additions & 5 deletions module/zfs/mmp.c
Original file line number Diff line number Diff line change
Expand Up @@ -550,11 +550,11 @@ mmp_thread(void *arg)
uint32_t mmp_fail_intervals = MMP_FAIL_INTVS_OK(
zfs_multihost_fail_intervals);
hrtime_t mmp_fail_ns = mmp_fail_intervals * mmp_interval;
boolean_t last_spa_suspended = suspended;
boolean_t last_spa_multihost = multihost;
uint64_t last_mmp_interval = mmp_interval;
uint32_t last_mmp_fail_intervals = mmp_fail_intervals;
hrtime_t last_mmp_fail_ns = mmp_fail_ns;
boolean_t last_spa_suspended;
boolean_t last_spa_multihost;
uint64_t last_mmp_interval;
uint32_t last_mmp_fail_intervals;
hrtime_t last_mmp_fail_ns;
callb_cpr_t cpr;
int skip_wait = 0;

Expand Down
6 changes: 3 additions & 3 deletions module/zfs/spa.c
Original file line number Diff line number Diff line change
Expand Up @@ -6803,8 +6803,8 @@ spa_vdev_attach(spa_t *spa, uint64_t guid, nvlist_t *nvroot, int replacing,

pvd = oldvd->vdev_parent;

if ((error = spa_config_parse(spa, &newrootvd, nvroot, NULL, 0,
VDEV_ALLOC_ATTACH)) != 0)
if (spa_config_parse(spa, &newrootvd, nvroot, NULL, 0,
VDEV_ALLOC_ATTACH) != 0)
return (spa_vdev_exit(spa, NULL, txg, EINVAL));

if (newrootvd->vdev_children != 1)
Expand Down Expand Up @@ -7160,7 +7160,7 @@ spa_vdev_detach(spa_t *spa, uint64_t guid, uint64_t pguid, int replace_done)
* it may be that the unwritability of the disk is the reason
* it's being detached!
*/
error = vdev_label_init(vd, 0, VDEV_LABEL_REMOVE);
(void) vdev_label_init(vd, 0, VDEV_LABEL_REMOVE);

/*
* Remove vd from its parent and compact the parent's children.
Expand Down
1 change: 0 additions & 1 deletion module/zfs/vdev.c
Original file line number Diff line number Diff line change
Expand Up @@ -6078,7 +6078,6 @@ vdev_prop_get(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
strval = NULL;
zprop_source_t src = ZPROP_SRC_DEFAULT;
propname = za.za_name;
prop = vdev_name_to_prop(propname);

switch (za.za_integer_length) {
case 8:
Expand Down
4 changes: 3 additions & 1 deletion module/zfs/zcp_get.c
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,8 @@ get_zap_prop(lua_State *state, dsl_dataset_t *ds, zfs_prop_t zfs_prop)
} else {
error = dsl_prop_get_ds(ds, prop_name, sizeof (numval),
1, &numval, setpoint);

if (error != 0)
goto out;
#ifdef _KERNEL
/* Fill in temporary value for prop, if applicable */
(void) zfs_get_temporary_prop(ds, zfs_prop, &numval, setpoint);
Expand All @@ -489,6 +490,7 @@ get_zap_prop(lua_State *state, dsl_dataset_t *ds, zfs_prop_t zfs_prop)
(void) lua_pushnumber(state, numval);
}
}
out:
kmem_free(strval, ZAP_MAXVALUELEN);
if (error == 0)
get_prop_src(state, setpoint, zfs_prop);
Expand Down
Loading

0 comments on commit 6a42939

Please sign in to comment.