[PATCH] hw/nvme: remove unreachable default cases in nvme_directive_receive()

[PATCH] hw/nvme: remove unreachable default cases in nvme_directive_receive()

[Han Zhang]

Static analysis reported that the default cases in the switch
statements handling dtype and doper in nvme_directive_receive()
are unreachable.

The values of dtype and doper are already validated earlier in the
function, making the default branches redundant.

Remove the unreachable cases to simplify the control flow.

Reported-by: Ekaterina Zilotina
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2472
Signed-off-by: Han Zhang <ihanzh@outlook.com>
---
 hw/nvme/ctrl.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index cc4593cd42..36df6eeca2 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7550,25 +7550,13 @@ static uint16_t nvme_directive_receive(NvmeCtrl *n, NvmeRequest *req)
         return NVME_INVALID_FIELD | NVME_DNR;
     }
 
-    switch (dtype) {
-    case NVME_DIRECTIVE_IDENTIFY:
-        switch (doper) {
-        case NVME_DIRECTIVE_RETURN_PARAMS:
-            if (ns->endgrp && ns->endgrp->fdp.enabled) {
-                id.supported |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
-                id.enabled |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
-                id.persistent |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
-            }
-
-            return nvme_c2h(n, (uint8_t *)&id, trans_len, req);
-
-        default:
-            return NVME_INVALID_FIELD | NVME_DNR;
-        }
-
-    default:
-        return NVME_INVALID_FIELD;
+    if (ns->endgrp && ns->endgrp->fdp.enabled) {
+        id.supported |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
+        id.enabled |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
+        id.persistent |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
     }
+
+    return nvme_c2h(n, (uint8_t *)&id, trans_len, req);
 }
 
 static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
-- 
2.34.1

Re: [PATCH] hw/nvme: remove unreachable default cases in nvme_directive_receive()

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

On Mar 17 09:07, Han Zhang wrote:
> Static analysis reported that the default cases in the switch
> statements handling dtype and doper in nvme_directive_receive()
> are unreachable.
> 
> The values of dtype and doper are already validated earlier in the
> function, making the default branches redundant.
> 
> Remove the unreachable cases to simplify the control flow.
> 
> Reported-by: Ekaterina Zilotina
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2472
> Signed-off-by: Han Zhang <ihanzh@outlook.com>
> ---
>  hw/nvme/ctrl.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index cc4593cd42..36df6eeca2 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -7550,25 +7550,13 @@ static uint16_t nvme_directive_receive(NvmeCtrl *n, NvmeRequest *req)
>          return NVME_INVALID_FIELD | NVME_DNR;
>      }
>  
> -    switch (dtype) {
> -    case NVME_DIRECTIVE_IDENTIFY:
> -        switch (doper) {
> -        case NVME_DIRECTIVE_RETURN_PARAMS:
> -            if (ns->endgrp && ns->endgrp->fdp.enabled) {
> -                id.supported |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
> -                id.enabled |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
> -                id.persistent |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
> -            }
> -
> -            return nvme_c2h(n, (uint8_t *)&id, trans_len, req);
> -
> -        default:
> -            return NVME_INVALID_FIELD | NVME_DNR;
> -        }
> -
> -    default:
> -        return NVME_INVALID_FIELD;
> +    if (ns->endgrp && ns->endgrp->fdp.enabled) {
> +        id.supported |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
> +        id.enabled |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
> +        id.persistent |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
>      }
> +
> +    return nvme_c2h(n, (uint8_t *)&id, trans_len, req);
>  }
>  
>  static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
> -- 
> 2.34.1
> 
> 

I'd rather keep the switches there and drop the early checks. The
switches are good documentation for what's going on. Removing them makes
the code a little "what, is this correct?".

If you update that, I'll be happy to grab this :)


Thanks,
Klaus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2] hw/nvme: keep switch handling in nvme_directive_receive()

[ihanzhzh]

From: Han Zhang <ihanzh@outlook.com>

Static analysis reported that the default branches in
nvme_directive_receive() were unreachable because dtype and doper were
validated before the switch statements.

Keep the switch statements as explicit documentation of supported
directive type/operation combinations, and remove the redundant early
dtype/doper checks instead.

Also move namespace lookup into the RETURN_PARAMS path so invalid
dtype/doper values are rejected via switch defaults without an
unnecessary nvme_ns() lookup.

Reported-by: Ekaterina Zilotina
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2472
Signed-off-by: Han Zhang <ihanzhzh@gmail.com>
---
 hw/nvme/ctrl.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index cc4593cd42..479a4b8725 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7540,13 +7540,7 @@ static uint16_t nvme_directive_receive(NvmeCtrl *n, NvmeRequest *req)
 
     trans_len = MIN(sizeof(NvmeDirectiveIdentify), numd << 2);
 
-    if (nsid == NVME_NSID_BROADCAST || dtype != NVME_DIRECTIVE_IDENTIFY ||
-        doper != NVME_DIRECTIVE_RETURN_PARAMS) {
-        return NVME_INVALID_FIELD | NVME_DNR;
-    }
-
-    ns = nvme_ns(n, nsid);
-    if (!ns) {
+    if (nsid == NVME_NSID_BROADCAST) {
         return NVME_INVALID_FIELD | NVME_DNR;
     }
 
@@ -7554,6 +7548,11 @@ static uint16_t nvme_directive_receive(NvmeCtrl *n, NvmeRequest *req)
     case NVME_DIRECTIVE_IDENTIFY:
         switch (doper) {
         case NVME_DIRECTIVE_RETURN_PARAMS:
+            ns = nvme_ns(n, nsid);
+            if (!ns) {
+                return NVME_INVALID_FIELD | NVME_DNR;
+            }
+
             if (ns->endgrp && ns->endgrp->fdp.enabled) {
                 id.supported |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
                 id.enabled |= 1 << NVME_DIRECTIVE_DATA_PLACEMENT;
@@ -7567,7 +7566,7 @@ static uint16_t nvme_directive_receive(NvmeCtrl *n, NvmeRequest *req)
         }
 
     default:
-        return NVME_INVALID_FIELD;
+        return NVME_INVALID_FIELD | NVME_DNR;
     }
 }
 
-- 
2.34.1