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

[Klaus Jensen <its@irrelevant.dk>]

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

On Nov 25 23:11, Minwoo Im wrote:
> 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.
> 

I don't think it is unnecessary when it using g_autofree?

> > +    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().
> 

Nice catch! nvme_aio_err normally takes care of this for blk/aio errors,
but this one slipped. Thanks!

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