1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] qcow2: Use BDRV_SECTOR_BITS instead of its literal value

Alberto Garcia posted 1 patch 6 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171009144840.973-1-berto@igalia.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
block/qcow2-cluster.c | 6 +++---
block/qcow2.c         | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
[Qemu-devel] [PATCH] qcow2: Use BDRV_SECTOR_BITS instead of its literal value
Posted by Alberto Garcia 6 years, 6 months ago 
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);
 
     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);
         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) {
-- 
2.11.0
Re: [Qemu-devel] [Qemu-block] [PATCH] qcow2: Use BDRV_SECTOR_BITS instead of its literal value
Posted by Max Reitz 6 years, 6 months ago 
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) {
>
Re: [Qemu-devel] [Qemu-block] [PATCH] qcow2: Use BDRV_SECTOR_BITS instead of its literal value
Posted by Alberto Garcia 6 years, 6 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.