Re: [Qemu-devel] [PATCH 3/8] qcow: Switch qcow_co_readv to byte-based calls

[Eric Blake]

On 05/28/2018 06:04 AM, Kevin Wolf wrote:
> Am 25.04.2018 um 20:32 hat Eric Blake geschrieben:
> > We are gradually moving away from sector-based interfaces, towards
> > byte-based.  Make the change for the internals of the qcow
> > driver read function, by iterating over offset/bytes instead of
> > sector_num/nb_sectors, and repurposing index_in_cluster and n
> > to be bytes instead of sectors.
> >
> > A later patch will then switch the qcow driver as a whole over
> > to byte-based operation.
> >
> > Signed-off-by: Eric Blake <address@hidden>
> > ---
> >   block/qcow.c | 39 +++++++++++++++++++--------------------
> >   1 file changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/block/qcow.c b/block/qcow.c
> > index 32730a8dd91..bf9d80fd227 100644
> > --- a/block/qcow.c
> > +++ b/block/qcow.c
> > @@ -623,6 +623,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, 
> > int64_t sector_num,
> >       QEMUIOVector hd_qiov;
> >       uint8_t *buf;
> >       void *orig_buf;
> > +    int64_t offset = sector_num << BDRV_SECTOR_BITS;
> > +    int64_t bytes = nb_sectors << BDRV_SECTOR_BITS;
>
> This should be okay because nb_sectors is limited to INT_MAX / 512, but
> I wouldn't be surprised if Coverity complained about a possible
> truncation here. Multiplying with BDRV_SECTOR_SIZE instead wouldn't be
> any less readable and would avoid the problem.

Sure, will fix.

> >       if (qiov->niov > 1) {
> >           buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
> > @@ -636,36 +638,36 @@ static coroutine_fn int qcow_co_readv(BlockDriverState 
> > *bs, int64_t sector_num,
> >
> >       qemu_co_mutex_lock(&s->lock);
> >
> > -    while (nb_sectors != 0) {
> > +    while (bytes != 0) {
> >           /* prepare next request */
> > -        ret = get_cluster_offset(bs, sector_num << 9,
> > +        ret = get_cluster_offset(bs, offset,
> >                                    0, 0, 0, 0, &cluster_offset);
>
> This surely fits in a single line now?
>
> >           if (ret < 0) {
> >               break;
> >           }
> > -        index_in_cluster = sector_num & (s->cluster_sectors - 1);
> > -        n = s->cluster_sectors - index_in_cluster;
> > -        if (n > nb_sectors) {
> > -            n = nb_sectors;
> > +        index_in_cluster = offset & (s->cluster_size - 1);
> > +        n = s->cluster_size - index_in_cluster;
> > +        if (n > bytes) {
> > +            n = bytes;
> >           }
>
> "index" suggests that it refers to an object larger than a byte. qcow2
> renamed the variable to offset_in_cluster when the same change was made.
> A new name for a unit change would also make review a bit easier.

Will fix.

> The logic looks correct, though.
>
> Kevin

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