Re: [PATCH] hw/block/nvme: add compare command

[Minwoo Im]

Hello,

On 20-11-24 08:37:14, Klaus Jensen wrote:
> From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> 
> Add the Compare command.
> 
> This implementation uses a bounce buffer to read in the data from
> storage and then compare with the host supplied buffer.
> 
> Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> [k.jensen: rebased]
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c       | 100 +++++++++++++++++++++++++++++++++++++++++-
>  hw/block/trace-events |   2 +
>  2 files changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index f7f888402b06..f88710ca3948 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -999,6 +999,50 @@ static void nvme_aio_discard_cb(void *opaque, int ret)
>      nvme_enqueue_req_completion(nvme_cq(req), req);
>  }
>  
> +struct nvme_compare_ctx {
> +    QEMUIOVector iov;
> +    uint8_t *bounce;
> +    size_t len;
> +};
> +
> +static void nvme_compare_cb(void *opaque, int ret)
> +{
> +    NvmeRequest *req = opaque;
> +    NvmeNamespace *ns = req->ns;
> +    struct nvme_compare_ctx *ctx = req->opaque;
> +    g_autofree uint8_t *buf = NULL;

nit-picking here: unnecessary initialization to NULL.

> +    uint16_t status;
> +
> +    trace_pci_nvme_compare_cb(nvme_cid(req));
> +
> +    if (!ret) {
> +        block_acct_done(blk_get_stats(ns->blkconf.blk), &req->acct);
> +    } else {
> +        block_acct_failed(blk_get_stats(ns->blkconf.blk), &req->acct);
> +        nvme_aio_err(req, ret);
> +        goto out;
> +    }
> +
> +    buf = g_malloc(ctx->len);
> +
> +    status = nvme_dma(nvme_ctrl(req), buf, ctx->len, DMA_DIRECTION_TO_DEVICE,
> +                      req);
> +    if (status) {
> +        goto out;
> +    }

Don't we need to give status value to req->status in case of
(status != 0)?  If we don't give it to req->status, it will complete
with success status code even it fails during the nvme_dma().