Re: [Qemu-block] [Qemu-devel] [PATCH v4 09/20] parallels: Switch to .bdrv_co_block_status()

[Eric Blake]

On 11/30/2017 03:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
> > We are gradually moving away from sector-based interfaces, towards
> > byte-based.  Update the parallels driver accordingly.  Note that
> > the internal function block_status() is still sector-based, because
> > it is still in use by other sector-based functions; but that's okay
> > because request_alignment is 512 as a result of those functions.
> > For now, no optimizations are added based on the mapping hint.
> >
> > Signed-off-by: Eric Blake <address@hidden>
> >
> > ---

> > +static int coroutine_fn parallels_co_block_status(BlockDriverState *bs,
> > +                                                  bool want_zero,
> > +                                                  int64_t offset,
> > +                                                  int64_t bytes,
> > +                                                  int64_t *pnum,
> > +                                                  int64_t *map,
> > +                                                  BlockDriverState 
> > **file)
> >   {
> >       BDRVParallelsState *s = bs->opaque;
> > -    int64_t offset;
> > +    int count;
> >
> > +    assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
> >       qemu_co_mutex_lock(&s->lock);
> > -    offset = block_status(s, sector_num, nb_sectors, pnum);
> > +    offset = block_status(s, offset >> BDRV_SECTOR_BITS,
> > +                          bytes >> BDRV_SECTOR_BITS, &count);
> >       qemu_co_mutex_unlock(&s->lock);
> >
> >       if (offset < 0) {
>
> pnum=count*BDRV_SECTOR_SIZE and map=0 setting missed here.
>
> >           return 0;
> >       }

Setting *map is only required when return value includes 
BDRV_BLOCK_OFFSET_VALID, so that one was not necessary.  But you do 
raise an interesting point about a pre-existing bug with pnum not being 
set.  Commit 298a1665a guarantees that *pnum is 0 (and not uninitialized 
garbage) - but that still violates the contract that we (want to) have 
that all drivers either make progress or return an error (setting pnum 
to 0 should only be done at EOF or on error).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org