Re: [Qemu-block] [PATCH v3 2/6] block: Add VFIO based NVMe driver

[Stefan Hajnoczi]

On Wed, Jul 12, 2017 at 10:14:48AM +0800, Fam Zheng wrote:
> On Mon, 07/10 15:55, Stefan Hajnoczi wrote:
> > On Wed, Jul 05, 2017 at 09:36:31PM +0800, Fam Zheng wrote:
> > > +static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t 
> > > bytes,
> > > +                       QEMUIOVector *qiov, bool is_write, int flags)
> > > +{
> > > +    BDRVNVMeState *s = bs->opaque;
> > > +    int r;
> > > +    uint8_t *buf = NULL;
> > > +    QEMUIOVector local_qiov;
> > > +
> > > +    assert(QEMU_IS_ALIGNED(offset, s->page_size));
> > > +    assert(QEMU_IS_ALIGNED(bytes, s->page_size));
> > > +    assert(bytes <= s->max_transfer);
> > 
> > Who guarantees max_transfer?  I think request alignment is enforced by
> > block/io.c but there is no generic max_transfer handling code, so this
> > assertion can be triggered by the guest.  Please handle it as a genuine
> > request error instead of using an assertion.
> 
> There has been one since 04ed95f4843281e292d93018d56d4b14705f9f2c, see the 
> code
> around max_transfer in block/io.c:bdrv_aligned_*.

Thanks for pointing that out!

> > 
> > > +static int nvme_reopen_prepare(BDRVReopenState *reopen_state,
> > > +                               BlockReopenQueue *queue, Error **errp)
> > > +{
> > > +    return 0;
> > > +}
> > 
> > What is the purpose of this dummy .bdrv_reopen_prepare() implementation?
> 
> This is necessary for block jobs to work, other drivers provides dummy
> implementations as well.

Please include a comment similar to what the other drivers with dummy
implements do.

signature.asc
Description: PGP signature