qcow2: Use BDRV_SECTOR_BITS instead of its literal value

BDRV_SECTOR_BITS is defined to be 9 in block.h (and BDRV_SECTOR_SIZE
is calculated from that), but there are still a few placed where we
are using the literal value instead of the macro.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-cluster.c | 6 +++---
 block/qcow2.c         | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

On 2017-10-09 16:48, Alberto Garcia wrote:
> BDRV_SECTOR_BITS is defined to be 9 in block.h (and BDRV_SECTOR_SIZE
> is calculated from that), but there are still a few placed where we
> are using the literal value instead of the macro.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cluster.c | 6 +++---
>  block/qcow2.c         | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 0e5aec81cb..0a63604af6 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -748,8 +748,8 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
>          return 0;
>      }
>  
> -    nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
> -                  (cluster_offset >> 9);
> +    nb_csectors = ((cluster_offset + compressed_size - 1) >> BDRV_SECTOR_BITS) -
> +                  (cluster_offset >> BDRV_SECTOR_BITS);

I'm not sure about this one, because technically this is not the block
layer's sector size but actually 512 bytes ("Compressed size of the
images in sectors of 512 bytes" as per the spec).

>  
>      cluster_offset |= QCOW_OFLAG_COMPRESSED |
>                        ((uint64_t)nb_csectors << s->csize_shift);
> @@ -1582,7 +1582,7 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
>          }
>  
>          BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
> -        ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
> +        ret = bdrv_read(bs->file, coffset >> BDRV_SECTOR_BITS, s->cluster_data,
>                          nb_csectors);

Here it would probably be better to make it bdrv_pread() and then... Use
csize instead of nb_csectors, I guess...?

Max

>          if (ret < 0) {
>              return ret;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index f63d1831f8..dff903e05c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1139,7 +1139,7 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->cluster_bits = header.cluster_bits;
>      s->cluster_size = 1 << s->cluster_bits;
> -    s->cluster_sectors = 1 << (s->cluster_bits - 9);
> +    s->cluster_sectors = 1 << (s->cluster_bits - BDRV_SECTOR_BITS);
>  
>      /* Initialise version 3 header fields */
>      if (header.version == 2) {
> @@ -1636,7 +1636,7 @@ static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
>  
>      bytes = MIN(INT_MAX, nb_sectors * BDRV_SECTOR_SIZE);
>      qemu_co_mutex_lock(&s->lock);
> -    ret = qcow2_get_cluster_offset(bs, sector_num << 9, &bytes,
> +    ret = qcow2_get_cluster_offset(bs, sector_num << BDRV_SECTOR_BITS, &bytes,
>                                     &cluster_offset);
>      qemu_co_mutex_unlock(&s->lock);
>      if (ret < 0) {
>

On Mon 09 Oct 2017 05:07:56 PM CEST, Max Reitz wrote:
> On 2017-10-09 16:48, Alberto Garcia wrote:
>> BDRV_SECTOR_BITS is defined to be 9 in block.h (and BDRV_SECTOR_SIZE
>> is calculated from that), but there are still a few placed where we
>> are using the literal value instead of the macro.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/qcow2-cluster.c | 6 +++---
>>  block/qcow2.c         | 4 ++--
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 0e5aec81cb..0a63604af6 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -748,8 +748,8 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
>>          return 0;
>>      }
>>  
>> -    nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
>> -                  (cluster_offset >> 9);
>> +    nb_csectors = ((cluster_offset + compressed_size - 1) >> BDRV_SECTOR_BITS) -
>> +                  (cluster_offset >> BDRV_SECTOR_BITS);
>
> I'm not sure about this one, because technically this is not the block
> layer's sector size but actually 512 bytes ("Compressed size of the
> images in sectors of 512 bytes" as per the spec).

You're right, I'll resend the patch leaving this part out for now.

Berto