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

[Klaus Jensen <its@irrelevant.dk>]

[-- 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 --]