From 17443e0b2044377f8cc7b9f4bc23b7fe6cbb3e3d Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Sat, 4 Mar 2023 17:57:01 -0500 Subject: [PATCH] Cleanup: Remove constant comparisons reported by CodeQL CodeQL's cpp/constant-comparison query from its security-and-extended query set reported 4 instances where we have comparions that always evaluate the same way. In `draid_config_by_type()`, we have an early `if (nparity == 0)` check that returns `EINVAL`, making a later `if (nparity == 0 || nparity > VDEV_DRAID_MAXPARITY)` partially redundant. The later check prints an error message when parity is 0, but the early check does not. This is not useful feedback, so we move the later check to the place where the early check runs to replace the early check. In `perform_thread_merge()`, we return when `num_threads == 0`. After that block, we do `if (num_threads > 0) {`, which will always be true. We remove the `if` statement. In `sa_modify_attrs()`, we have a loop condition that is `k != 2`, but at the end of the loop, we have `if (k == 0 && hdl->sa_spill)` followed by an else that does a break. The result is that k != 2 will never be evaluated when it is false. We drop the comparison. In `zap_leaf_array_read()`, we have a for loop condition that is `i < ZAP_LEAF_ARRAY_BYTES && len > 0`. However, that loop itself is in a loop that is `while (len > 0)` and while the value of len is decremented inside the loop, when `len == 0`, it will return, such that `len > 0` inside the loop condition will always be true. We drop that part of the condition. Reviewed-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #14575 --- cmd/zpool/zpool_vdev.c | 15 ++++++--------- module/zfs/dmu_redact.c | 6 ++---- module/zfs/sa.c | 2 +- module/zfs/zap_leaf.c | 2 +- 4 files changed, 10 insertions(+), 15 deletions(-) diff --git a/cmd/zpool/zpool_vdev.c b/cmd/zpool/zpool_vdev.c index 8db1aef765..eb81dfe070 100644 --- a/cmd/zpool/zpool_vdev.c +++ b/cmd/zpool/zpool_vdev.c @@ -1329,8 +1329,13 @@ draid_config_by_type(nvlist_t *nv, const char *type, uint64_t children) return (EINVAL); nparity = (uint64_t)get_parity(type); - if (nparity == 0) + if (nparity == 0 || nparity > VDEV_DRAID_MAXPARITY) { + fprintf(stderr, + gettext("invalid dRAID parity level %llu; must be " + "between 1 and %d\n"), (u_longlong_t)nparity, + VDEV_DRAID_MAXPARITY); return (EINVAL); + } char *p = (char *)type; while ((p = strchr(p, ':')) != NULL) { @@ -1401,14 +1406,6 @@ draid_config_by_type(nvlist_t *nv, const char *type, uint64_t children) return (EINVAL); } - if (nparity == 0 || nparity > VDEV_DRAID_MAXPARITY) { - fprintf(stderr, - gettext("invalid dRAID parity level %llu; must be " - "between 1 and %d\n"), (u_longlong_t)nparity, - VDEV_DRAID_MAXPARITY); - return (EINVAL); - } - /* * Verify the requested number of spares can be satisfied. * An arbitrary limit of 100 distributed spares is applied. diff --git a/module/zfs/dmu_redact.c b/module/zfs/dmu_redact.c index 7afcc12313..6bd35713ff 100644 --- a/module/zfs/dmu_redact.c +++ b/module/zfs/dmu_redact.c @@ -746,10 +746,8 @@ perform_thread_merge(bqueue_t *q, uint32_t num_threads, bqueue_enqueue(q, record, sizeof (*record)); return (0); } - if (num_threads > 0) { - redact_nodes = kmem_zalloc(num_threads * - sizeof (*redact_nodes), KM_SLEEP); - } + redact_nodes = kmem_zalloc(num_threads * + sizeof (*redact_nodes), KM_SLEEP); avl_create(&start_tree, redact_node_compare_start, sizeof (struct redact_node), diff --git a/module/zfs/sa.c b/module/zfs/sa.c index 763b0c920f..f9daaabbed 100644 --- a/module/zfs/sa.c +++ b/module/zfs/sa.c @@ -1918,7 +1918,7 @@ sa_modify_attrs(sa_handle_t *hdl, sa_attr_type_t newattr, count = bonus_attr_count; hdr = SA_GET_HDR(hdl, SA_BONUS); idx_tab = SA_IDX_TAB_GET(hdl, SA_BONUS); - for (; k != 2; k++) { + for (; ; k++) { /* * Iterate over each attribute in layout. Fetch the * size of variable-length attributes needing rewrite diff --git a/module/zfs/zap_leaf.c b/module/zfs/zap_leaf.c index 2e8489c7df..e6afb1c58c 100644 --- a/module/zfs/zap_leaf.c +++ b/module/zfs/zap_leaf.c @@ -315,7 +315,7 @@ zap_leaf_array_read(zap_leaf_t *l, uint16_t chunk, struct zap_leaf_array *la = &ZAP_LEAF_CHUNK(l, chunk).l_array; ASSERT3U(chunk, <, ZAP_LEAF_NUMCHUNKS(l)); - for (int i = 0; i < ZAP_LEAF_ARRAY_BYTES && len > 0; i++) { + for (int i = 0; i < ZAP_LEAF_ARRAY_BYTES; i++) { value = (value << 8) | la->la_array[i]; byten++; if (byten == array_int_len) {