Re: [Qemu-devel] [PATCH 3/5] block: Use bdrv_nb_sectors() when sectors, not bytes are wanted

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 09.05.2014 um 18:27 hat Markus Armbruster geschrieben:
>> Markus Armbruster <address@hidden> writes:
>> 
>> > Instead of bdrv_nb_sectors().
>> >
>> > Aside: a few of these callers don't handle errors.  I didn't
>> > investigate whether they should.
>> >
>> > Signed-off-by: Markus Armbruster <address@hidden>
>> > ---
>> [...]
>> > diff --git a/block.c b/block.c
>> > index 44e1f57..1b99cb1 100644
>> > --- a/block.c
>> > +++ b/block.c
>> [...]
>> > @@ -3848,21 +3845,21 @@ static int64_t coroutine_fn 
>> > bdrv_co_get_block_status(BlockDriverState *bs,
>> >                                                       int64_t sector_num,
>> >                                                       int nb_sectors, int 
>> > *pnum)
>> >  {
>> > -    int64_t length;
>> > +    int64_t total_sectors;
>> >      int64_t n;
>> >      int64_t ret, ret2;
>> >  
>> > -    length = bdrv_getlength(bs);
>> > -    if (length < 0) {
>> > -        return length;
>> > +    total_sectors = bdrv_getlength(bs);
>> > +    if (total_sectors < 0) {
>> > +        return total_sectors;
>> >      }
>> >  
>> > -    if (sector_num >= (length >> BDRV_SECTOR_BITS)) {
>> > +    if (sector_num >= total_sectors) {
>> >          *pnum = 0;
>> >          return 0;
>> >      }
>> >  
>> > -    n = bs->total_sectors - sector_num;
>> > +    n = total_sectors - sector_num;
>> >      if (n < nb_sectors) {
>> >          nb_sectors = n;
>> >      }
>> > @@ -3893,8 +3890,8 @@ static int64_t coroutine_fn 
>> > bdrv_co_get_block_status(BlockDriverState *bs,
>> >              ret |= BDRV_BLOCK_ZERO;
>> >          } else if (bs->backing_hd) {
>> >              BlockDriverState *bs2 = bs->backing_hd;
>> > -            int64_t length2 = bdrv_getlength(bs2);
>> > -            if (length2 >= 0 && sector_num >= (length2 >> 
>> > BDRV_SECTOR_BITS)) {
>> > +            int64_t nb_sectors2 = bdrv_getlength(bs2);
>> > +            if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) {
>> >                  ret |= BDRV_BLOCK_ZERO;
>> >              }
>> >          }
>> [...]
>> 
>> I neglected to actually replace bdrv_getlength() by bdrv_nb_sectors()
>> here, breaking test 030 (I forgot that make check-block doesn't run all
>> the tests).  With that fixed, the tests pass.  Full respin wanted?
>
> Yes, please. I think you're aware of it, but in order to avoid
> misunderstandings let me mention that there are two places that need a
> fix here, for both total_sectors and nb_sectors2.

Correct.

> Also please consider splitting this patch so that the functions with
> major changes (bdrv_make_zero, bdrv_co_get_block_status) get separated
> at least.

Okay.