Re: [Qemu-devel] [PATCH v1 02/13] qcow2: is_zero_sectors(): return true if area is outside of backing file

[Anton Nefedov]

On 05/22/2017 10:14 PM, Eric Blake wrote:
> On 05/22/2017 02:12 PM, Eric Blake wrote:
>
> > > +++ b/block/qcow2.c
> > > @@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t 
> > > start,
> > >      int64_t res;
> > >
> > >      if (start + count > bs->total_sectors) {
> > > -        count = bs->total_sectors - start;
> > > +        count = start < bs->total_sectors ? bs->total_sectors - start : 0;
> > >      }
> > >
> > >      if (!count) {
> > > @@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t 
> > > start,
> > >      }
> > >      res = bdrv_get_block_status_above(bs, NULL, start, count,
> > >                                        &nr, &file);
> > > -    return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
> > > +    return res >= 0
> > > +        && (((res & BDRV_BLOCK_ZERO) && nr == count)
> > > +            || nr == 0);
> >
> > The logic makes sense to me, although the formatting is unusual (we tend
> > to split && and || with the operator at the tail of the previous line,
> > not the head of the new line).
> >
> > This quick check may make me revisit whether it is worth my my RFC
> > series about adding BDRV_BLOCK_EOF for more quickly tracking reads
> > beyond EOF (my solution in that series was able to make the same change
> > to test 154, but by doing it at the block layer instead of the qcow2.c
> > code).  https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01127.html
>
> Actually, re-reading my RFC - I was able to change 6 instances in test
> 154, while your tweak only affected 2 instances (you still left four
> instances that were allocating).  So my approach may still be more
> optimal in the long run.

Yes, looks like your approach is more general; let's drop this one then

/Anton