From 2a493a4c7127258b14c39e8c71a9d6f01167c5cd Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Fri, 23 Sep 2022 19:52:03 -0400 Subject: [PATCH] Fix unchecked return values and unused return values Coverity complained about unchecked return values and unused values that turned out to be unused return values. Different approaches were used to handle the different cases of unchecked return values: * cmd/zdb/zdb.c: VERIFY0 was used in one place since the existing code had no error handling. An error message was printed in another to match the rest of the code. * cmd/zed/agents/zfs_retire.c: We dismiss the return value with `(void)` because the value is expected to be potentially unset. * cmd/zpool_influxdb/zpool_influxdb.c: We dismiss the return value with `(void)` because the values are expected to be potentially unset. * cmd/ztest.c: VERIFY0 was used since we want failures if something goes wrong in ztest. * module/zfs/dsl_dir.c: We dismiss the return value with `(void)` because there is no guarantee that the zap entry will always be there. For example, old pools imported readonly would not have it and we do not want to fail here because of that. * module/zfs/zfs_fm.c: `fnvlist_add_*()` was used since the allocations sleep and thus can never fail. * module/zfs/zvol.c: We dismiss the return value with `(void)` because we do not need it. This matches what is already done in the analogous `zfs_replay_write2()`. * tests/zfs-tests/cmd/draid.c: We suppress one return value with `(void)` since the code handles errors already. The other return value is handled by switching to `fnvlist_lookup_uint8_array()`. * tests/zfs-tests/cmd/file/file_fadvise.c: We add error handling. * tests/zfs-tests/cmd/mmap_sync.c: We add error handling for munmap, but ignore failures on remove() with (void) since it is expected to be able to fail. * tests/zfs-tests/cmd/mmapwrite.c: We add error handling. As for unused return values, they were all in places where there was error handling, so logic was added to handle the return values. Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #13920 --- cmd/zdb/zdb.c | 15 +++++++++++---- cmd/zed/agents/zfs_retire.c | 3 ++- cmd/zpool_influxdb/zpool_influxdb.c | 8 ++++---- cmd/ztest.c | 2 +- lib/libzfsbootenv/lzbe_device.c | 5 +++-- module/zfs/dmu_recv.c | 2 ++ module/zfs/dsl_dataset.c | 5 ++++- module/zfs/dsl_dir.c | 2 +- module/zfs/zfs_fm.c | 12 ++++++------ module/zfs/zvol.c | 4 ++-- tests/zfs-tests/cmd/draid.c | 4 ++-- tests/zfs-tests/cmd/file/file_fadvise.c | 6 +++++- tests/zfs-tests/cmd/mmap_sync.c | 5 +++-- tests/zfs-tests/cmd/mmapwrite.c | 6 +++++- 14 files changed, 51 insertions(+), 28 deletions(-) diff --git a/cmd/zdb/zdb.c b/cmd/zdb/zdb.c index a3a363ca54f2..a17793c2d90e 100644 --- a/cmd/zdb/zdb.c +++ b/cmd/zdb/zdb.c @@ -4418,9 +4418,14 @@ dump_l2arc_log_blocks(int fd, l2arc_dev_hdr_phys_t l2dhdr, default: abd = abd_alloc_for_io(asize, B_TRUE); abd_copy_from_buf_off(abd, &this_lb, 0, asize); - zio_decompress_data(L2BLK_GET_COMPRESS( + if (zio_decompress_data(L2BLK_GET_COMPRESS( (&lbps[0])->lbp_prop), abd, &this_lb, - asize, sizeof (this_lb), NULL); + asize, sizeof (this_lb), NULL) != 0) { + (void) printf("L2ARC block decompression " + "failed\n"); + abd_free(abd); + goto out; + } abd_free(abd); break; } @@ -4455,7 +4460,7 @@ dump_l2arc_log_blocks(int fd, l2arc_dev_hdr_phys_t l2dhdr, lbps[0] = lbps[1]; lbps[1] = this_lb.lb_prev_lbp; } - +out: if (!dump_opt['q']) { (void) printf("log_blk_count:\t %llu with valid cksum\n", (u_longlong_t)rebuild->dh_lb_count); @@ -7586,7 +7591,7 @@ dump_mos_leaks(spa_t *spa) } else { dmu_object_info_t doi; const char *name; - dmu_object_info(mos, object, &doi); + VERIFY0(dmu_object_info(mos, object, &doi)); if (doi.doi_type & DMU_OT_NEWTYPE) { dmu_object_byteswap_t bswap = DMU_OT_BYTESWAP(doi.doi_type); @@ -8698,6 +8703,8 @@ main(int argc, char **argv) usage(); dump_opt['v'] = verbose; error = dump_path(argv[0], argv[1], &object); + if (error != 0) + fatal("internal error: %s", strerror(error)); } if (dump_opt['X'] || dump_opt['F']) diff --git a/cmd/zed/agents/zfs_retire.c b/cmd/zed/agents/zfs_retire.c index a9e8baaa2c54..5fe56fe81562 100644 --- a/cmd/zed/agents/zfs_retire.c +++ b/cmd/zed/agents/zfs_retire.c @@ -326,7 +326,8 @@ zfs_retire_recv(fmd_hdl_t *hdl, fmd_event_t *ep, nvlist_t *nvl, fmd_hdl_debug(hdl, "zfs_retire_recv: '%s'", class); - nvlist_lookup_uint64(nvl, FM_EREPORT_PAYLOAD_ZFS_VDEV_STATE, &state); + (void) nvlist_lookup_uint64(nvl, FM_EREPORT_PAYLOAD_ZFS_VDEV_STATE, + &state); /* * If this is a resource notifying us of device removal then simply diff --git a/cmd/zpool_influxdb/zpool_influxdb.c b/cmd/zpool_influxdb/zpool_influxdb.c index 251d588d832a..8440d8b572dc 100644 --- a/cmd/zpool_influxdb/zpool_influxdb.c +++ b/cmd/zpool_influxdb/zpool_influxdb.c @@ -265,7 +265,7 @@ get_vdev_name(nvlist_t *nvroot, const char *parent_name) uint64_t vdev_id = 0; char *vdev_type = (char *)"unknown"; - nvlist_lookup_string(nvroot, ZPOOL_CONFIG_TYPE, &vdev_type); + (void) nvlist_lookup_string(nvroot, ZPOOL_CONFIG_TYPE, &vdev_type); if (nvlist_lookup_uint64( nvroot, ZPOOL_CONFIG_ID, &vdev_id) != 0) @@ -302,9 +302,9 @@ get_vdev_desc(nvlist_t *nvroot, const char *parent_name) char *vdev_type = (char *)"unknown"; uint64_t vdev_id = UINT64_MAX; char *vdev_path = NULL; - nvlist_lookup_string(nvroot, ZPOOL_CONFIG_TYPE, &vdev_type); - nvlist_lookup_uint64(nvroot, ZPOOL_CONFIG_ID, &vdev_id); - nvlist_lookup_string(nvroot, ZPOOL_CONFIG_PATH, &vdev_path); + (void) nvlist_lookup_string(nvroot, ZPOOL_CONFIG_TYPE, &vdev_type); + (void) nvlist_lookup_uint64(nvroot, ZPOOL_CONFIG_ID, &vdev_id); + (void) nvlist_lookup_string(nvroot, ZPOOL_CONFIG_PATH, &vdev_path); if (parent_name == NULL) { s = escape_string(vdev_type); diff --git a/cmd/ztest.c b/cmd/ztest.c index 0712f286bf66..b5a3d1810a5a 100644 --- a/cmd/ztest.c +++ b/cmd/ztest.c @@ -2214,7 +2214,7 @@ ztest_replay_write(void *arg1, void *arg2, boolean_t byteswap) dmu_write(os, lr->lr_foid, offset, length, data, tx); } else { memcpy(abuf->b_data, data, length); - dmu_assign_arcbuf_by_dbuf(db, offset, abuf, tx); + VERIFY0(dmu_assign_arcbuf_by_dbuf(db, offset, abuf, tx)); } (void) ztest_log_write(zd, tx, lr); diff --git a/lib/libzfsbootenv/lzbe_device.c b/lib/libzfsbootenv/lzbe_device.c index 39e33324b315..21007fd9ef50 100644 --- a/lib/libzfsbootenv/lzbe_device.c +++ b/lib/libzfsbootenv/lzbe_device.c @@ -74,6 +74,7 @@ lzbe_set_boot_device(const char *pool, lzbe_flags_t flag, const char *device) /* version is mandatory */ fnvlist_add_uint64(nv, BOOTENV_VERSION, VB_NVLIST); + rv = 0; /* * If device name is empty, remove boot device configuration. */ @@ -95,8 +96,8 @@ lzbe_set_boot_device(const char *pool, lzbe_flags_t flag, const char *device) rv = ENOMEM; } } - - rv = zpool_set_bootenv(zphdl, nv); + if (rv == 0) + rv = zpool_set_bootenv(zphdl, nv); if (rv != 0) fprintf(stderr, "%s\n", libzfs_error_description(hdl)); diff --git a/module/zfs/dmu_recv.c b/module/zfs/dmu_recv.c index 55d03677feaa..0f3181f762d6 100644 --- a/module/zfs/dmu_recv.c +++ b/module/zfs/dmu_recv.c @@ -1874,6 +1874,8 @@ receive_object(struct receive_writer_arg *rwa, struct drr_object *drro, if (err == 0) { err = receive_handle_existing_object(rwa, drro, &doi, data, &object_to_hold, &new_blksz); + if (err != 0) + return (err); } else if (err == EEXIST) { /* * The object requested is currently an interior slot of a diff --git a/module/zfs/dsl_dataset.c b/module/zfs/dsl_dataset.c index 94b77aa1b743..a7aca48aac20 100644 --- a/module/zfs/dsl_dataset.c +++ b/module/zfs/dsl_dataset.c @@ -640,6 +640,8 @@ dsl_dataset_hold_obj(dsl_pool_t *dp, uint64_t dsobj, const void *tag, dsl_dataset_phys(ds)->ds_prev_snap_obj, ds, &ds->ds_prev); } + if (err != 0) + goto after_dsl_bookmark_fini; err = dsl_bookmark_init_ds(ds); } else { if (zfs_flags & ZFS_DEBUG_SNAPNAMES) @@ -688,11 +690,11 @@ dsl_dataset_hold_obj(dsl_pool_t *dp, uint64_t dsobj, const void *tag, winner = dmu_buf_set_user_ie(dbuf, &ds->ds_dbu); if (err != 0 || winner != NULL) { - bplist_destroy(&ds->ds_pending_deadlist); dsl_deadlist_close(&ds->ds_deadlist); if (dsl_deadlist_is_open(&ds->ds_remap_deadlist)) dsl_deadlist_close(&ds->ds_remap_deadlist); dsl_bookmark_fini_ds(ds); +after_dsl_bookmark_fini: if (ds->ds_prev) dsl_dataset_rele(ds->ds_prev, ds); dsl_dir_rele(ds->ds_dir, ds); @@ -703,6 +705,7 @@ dsl_dataset_hold_obj(dsl_pool_t *dp, uint64_t dsobj, const void *tag, list_destroy(&ds->ds_prop_cbs); list_destroy(&ds->ds_sendstreams); + bplist_destroy(&ds->ds_pending_deadlist); mutex_destroy(&ds->ds_lock); mutex_destroy(&ds->ds_opening_lock); mutex_destroy(&ds->ds_sendstream_lock); diff --git a/module/zfs/dsl_dir.c b/module/zfs/dsl_dir.c index a4db3ee2f306..b328959464b2 100644 --- a/module/zfs/dsl_dir.c +++ b/module/zfs/dsl_dir.c @@ -270,7 +270,7 @@ dsl_dir_hold_obj(dsl_pool_t *dp, uint64_t ddobj, if (dsl_dir_is_zapified(dd)) { inode_timespec_t t = {0}; - zap_lookup(dp->dp_meta_objset, ddobj, + (void) zap_lookup(dp->dp_meta_objset, ddobj, DD_FIELD_SNAPSHOTS_CHANGED, sizeof (uint64_t), sizeof (inode_timespec_t) / sizeof (uint64_t), diff --git a/module/zfs/zfs_fm.c b/module/zfs/zfs_fm.c index 47bb79a5d21a..a16c62675033 100644 --- a/module/zfs/zfs_fm.c +++ b/module/zfs/zfs_fm.c @@ -1389,17 +1389,17 @@ zfs_post_state_change(spa_t *spa, vdev_t *vd, uint64_t laststate) aux = fm_nvlist_create(NULL); if (vd && aux) { if (vd->vdev_physpath) { - (void) nvlist_add_string(aux, + fnvlist_add_string(aux, FM_EREPORT_PAYLOAD_ZFS_VDEV_PHYSPATH, vd->vdev_physpath); } if (vd->vdev_enc_sysfs_path) { - (void) nvlist_add_string(aux, + fnvlist_add_string(aux, FM_EREPORT_PAYLOAD_ZFS_VDEV_ENC_SYSFS_PATH, vd->vdev_enc_sysfs_path); } - (void) nvlist_add_uint64(aux, + fnvlist_add_uint64(aux, FM_EREPORT_PAYLOAD_ZFS_VDEV_LASTSTATE, laststate); } @@ -1496,12 +1496,12 @@ zfs_ereport_zvol_post(const char *subclass, const char *name, return; aux = fm_nvlist_create(NULL); - nvlist_add_string(aux, FM_EREPORT_PAYLOAD_ZFS_DEVICE_NAME, dev_name); - nvlist_add_string(aux, FM_EREPORT_PAYLOAD_ZFS_RAW_DEVICE_NAME, + fnvlist_add_string(aux, FM_EREPORT_PAYLOAD_ZFS_DEVICE_NAME, dev_name); + fnvlist_add_string(aux, FM_EREPORT_PAYLOAD_ZFS_RAW_DEVICE_NAME, raw_name); r = strchr(name, '/'); if (r && r[1]) - nvlist_add_string(aux, FM_EREPORT_PAYLOAD_ZFS_VOLUME, &r[1]); + fnvlist_add_string(aux, FM_EREPORT_PAYLOAD_ZFS_VOLUME, &r[1]); zfs_post_common(spa, NULL, FM_RSRC_CLASS, subclass, aux); fm_nvlist_destroy(aux, FM_NVA_FREE); diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c index 3a1870f568b7..2e2860ff0212 100644 --- a/module/zfs/zvol.c +++ b/module/zfs/zvol.c @@ -429,7 +429,7 @@ zvol_replay_truncate(void *arg1, void *arg2, boolean_t byteswap) if (error != 0) { dmu_tx_abort(tx); } else { - zil_replaying(zv->zv_zilog, tx); + (void) zil_replaying(zv->zv_zilog, tx); dmu_tx_commit(tx); error = dmu_free_long_range(zv->zv_objset, ZVOL_OBJ, offset, length); @@ -475,7 +475,7 @@ zvol_replay_write(void *arg1, void *arg2, boolean_t byteswap) dmu_tx_abort(tx); } else { dmu_write(os, ZVOL_OBJ, offset, length, data, tx); - zil_replaying(zv->zv_zilog, tx); + (void) zil_replaying(zv->zv_zilog, tx); dmu_tx_commit(tx); } diff --git a/tests/zfs-tests/cmd/draid.c b/tests/zfs-tests/cmd/draid.c index b995be46a373..831039288a55 100644 --- a/tests/zfs-tests/cmd/draid.c +++ b/tests/zfs-tests/cmd/draid.c @@ -139,7 +139,7 @@ read_map_key(const char *filename, const char *key, nvlist_t **cfg) if (error != 0) return (error); - nvlist_lookup_nvlist(allcfgs, key, &foundcfg); + (void) nvlist_lookup_nvlist(allcfgs, key, &foundcfg); if (foundcfg != NULL) { nvlist_dup(foundcfg, cfg, KM_SLEEP); error = 0; @@ -375,7 +375,7 @@ dump_map_nv(const char *key, nvlist_t *cfg, int verbose) map.dm_checksum = fnvlist_lookup_uint64(cfg, MAP_CHECKSUM); map.dm_children = fnvlist_lookup_uint64(cfg, MAP_CHILDREN); map.dm_nperms = fnvlist_lookup_uint64(cfg, MAP_NPERMS); - nvlist_lookup_uint8_array(cfg, MAP_PERMS, &map.dm_perms, &c); + map.dm_perms = fnvlist_lookup_uint8_array(cfg, MAP_PERMS, &c); dump_map(&map, key, (double)worst_ratio / 1000.0, avg_ratio / 1000.0, verbose); diff --git a/tests/zfs-tests/cmd/file/file_fadvise.c b/tests/zfs-tests/cmd/file/file_fadvise.c index e1afb6d0a11c..d64e2dea3696 100644 --- a/tests/zfs-tests/cmd/file/file_fadvise.c +++ b/tests/zfs-tests/cmd/file/file_fadvise.c @@ -89,7 +89,11 @@ main(int argc, char *argv[]) return (1); } - posix_fadvise(fd, 0, 0, advise); + if (posix_fadvise(fd, 0, 0, advise) != 0) { + perror("posix_fadvise"); + close(fd); + return (1); + } close(fd); diff --git a/tests/zfs-tests/cmd/mmap_sync.c b/tests/zfs-tests/cmd/mmap_sync.c index 0e4bba37d7be..618f1284aa73 100644 --- a/tests/zfs-tests/cmd/mmap_sync.c +++ b/tests/zfs-tests/cmd/mmap_sync.c @@ -32,7 +32,7 @@ static void cleanup(char *file) { - remove(file); + (void) remove(file); } int @@ -125,7 +125,8 @@ main(int argc, char *argv[]) elapsed += ((t2.tv_usec - t1.tv_usec) / 1000.0); if (elapsed > max_msync_time_ms) { fprintf(stderr, "slow msync: %f ms\n", elapsed); - munmap(ptr, LEN); + if (munmap(ptr, LEN) != 0) + perror("munmap"); cleanup(file); return (1); } diff --git a/tests/zfs-tests/cmd/mmapwrite.c b/tests/zfs-tests/cmd/mmapwrite.c index 0d277d6e3099..8534db339666 100644 --- a/tests/zfs-tests/cmd/mmapwrite.c +++ b/tests/zfs-tests/cmd/mmapwrite.c @@ -73,7 +73,11 @@ normal_writer(void *filename) err(1, "write failed!"); break; } - lseek(fd, page_size, SEEK_CUR); + if (lseek(fd, page_size, SEEK_CUR) == -1) { + err(1, "lseek failed on %s: %s", file_path, + strerror(errno)); + break; + } } }