We are going to use it in more places, calculating
"s->tracks << BDRV_SECTOR_BITS" doesn't look good.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/parallels.h | 1 +
 block/parallels.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/parallels.h b/block/parallels.h
index 5aa101cfc8..9a9209e320 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -79,6 +79,7 @@ typedef struct BDRVParallelsState {
     ParallelsPreallocMode prealloc_mode;
 
     unsigned int tracks;
+    unsigned int cluster_size;
 
     unsigned int off_multiplier;
     Error *migration_blocker;
diff --git a/block/parallels.c b/block/parallels.c
index 3c22dfdc9d..9594d84978 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -421,7 +421,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     int ret;
     uint32_t i;
     bool flush_bat = false;
-    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
 
     size = bdrv_getlength(bs->file->bs);
     if (size < 0) {
@@ -472,7 +471,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             high_off = off;
         }
 
-        if (prev_off != 0 && (prev_off + cluster_size) != off) {
+        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
             res->bfi.fragmented_clusters++;
         }
         prev_off = off;
@@ -487,10 +486,10 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         }
     }
 
-    res->image_end_offset = high_off + cluster_size;
+    res->image_end_offset = high_off + s->cluster_size;
     if (size > res->image_end_offset) {
         int64_t count;
-        count = DIV_ROUND_UP(size - res->image_end_offset, cluster_size);
+        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
         fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
                 fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
                 size - res->image_end_offset);
@@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EFBIG;
         goto fail;
     }
+    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
 
     s->bat_size = le32_to_cpu(ph.bat_entries);
     if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
-- 
2.29.2

On 2/24/21 1:47 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are going to use it in more places, calculating
> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/parallels.h | 1 +
>  block/parallels.c | 8 ++++----
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/parallels.h b/block/parallels.h
> index 5aa101cfc8..9a9209e320 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -79,6 +79,7 @@ typedef struct BDRVParallelsState {
>      ParallelsPreallocMode prealloc_mode;
>  
>      unsigned int tracks;
> +    unsigned int cluster_size;
>  
>      unsigned int off_multiplier;
>      Error *migration_blocker;
> diff --git a/block/parallels.c b/block/parallels.c
> index 3c22dfdc9d..9594d84978 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -421,7 +421,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>      int ret;
>      uint32_t i;
>      bool flush_bat = false;
> -    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
>  
>      size = bdrv_getlength(bs->file->bs);
>      if (size < 0) {
> @@ -472,7 +471,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>              high_off = off;
>          }
>  
> -        if (prev_off != 0 && (prev_off + cluster_size) != off) {
> +        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
>              res->bfi.fragmented_clusters++;
>          }
>          prev_off = off;
> @@ -487,10 +486,10 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>          }
>      }
>  
> -    res->image_end_offset = high_off + cluster_size;
> +    res->image_end_offset = high_off + s->cluster_size;
>      if (size > res->image_end_offset) {
>          int64_t count;
> -        count = DIV_ROUND_UP(size - res->image_end_offset, cluster_size);
> +        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
>          fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
>                  fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
>                  size - res->image_end_offset);
> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          ret = -EFBIG;
>          goto fail;
>      }
> +    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>  
>      s->bat_size = le32_to_cpu(ph.bat_entries);
>      if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
Reviewed-by: Denis V. Lunev <den@openvz.org>

Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We are going to use it in more places, calculating
> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          ret = -EFBIG;
>          goto fail;
>      }
> +    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>  
>      s->bat_size = le32_to_cpu(ph.bat_entries);
>      if (s->bat_size > INT_MAX / sizeof(uint32_t)) {

Checking the context, I saw this a few lines above:

    if (s->tracks > INT32_MAX/513) {

Is the 513 intentional?

Kevin

On 3/4/21 5:24 PM, Kevin Wolf wrote:
> Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> We are going to use it in more places, calculating
>> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>          ret = -EFBIG;
>>          goto fail;
>>      }
>> +    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>>  
>>      s->bat_size = le32_to_cpu(ph.bat_entries);
>>      if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
> Checking the context, I saw this a few lines above:
>
>     if (s->tracks > INT32_MAX/513) {
>
> Is the 513 intentional?
>
> Kevin
>
I can not remember why I have written this at that time,
but original comment for the commit was

commit d25d59802021a747812472780d80a0e792078f40
Author: Denis V. Lunev <den@openvz.org>
Date:   Mon Jul 28 20:23:55 2014 +0400

    parallels: 2TB+ parallels images support
   
    Parallels has released in the recent updates of Parallels Server 5/6
    new addition to his image format. Images with signature WithouFreSpacExt
    have offsets in the catalog coded not as offsets in sectors (multiple
    of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
   
    In this case all 64 bits of header->nb_sectors are used for image size.
   
    This patch implements support of this for qemu-img and also adds
specific
    check for an incorrect image. Images with block size greater than
    INT_MAX/513 are not supported. The biggest available Parallels image
    cluster size in the field is 1 Mb. Thus this limit will not hurt
    anyone.
   
    Signed-off-by: Denis V. Lunev <den@openvz.org>
    CC: Jeff Cody <jcody@redhat.com>
    CC: Kevin Wolf <kwolf@redhat.com>
    CC: Stefan Hajnoczi <stefanha@redhat.com>
    Reviewed-by: Jeff Cody <jcody@redhat.com>
    Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Thus I believe that this is intentional.

Den

Am 04.03.2021 um 15:57 hat Denis V. Lunev geschrieben:
> On 3/4/21 5:24 PM, Kevin Wolf wrote:
> > Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> We are going to use it in more places, calculating
> >> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> >>          ret = -EFBIG;
> >>          goto fail;
> >>      }
> >> +    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
> >>  
> >>      s->bat_size = le32_to_cpu(ph.bat_entries);
> >>      if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
> > Checking the context, I saw this a few lines above:
> >
> >     if (s->tracks > INT32_MAX/513) {
> >
> > Is the 513 intentional?
> >
> > Kevin
> >
> I can not remember why I have written this at that time,
> but original comment for the commit was
> 
> commit d25d59802021a747812472780d80a0e792078f40
> Author: Denis V. Lunev <den@openvz.org>
> Date:   Mon Jul 28 20:23:55 2014 +0400
> 
>     parallels: 2TB+ parallels images support
>    
>     Parallels has released in the recent updates of Parallels Server 5/6
>     new addition to his image format. Images with signature WithouFreSpacExt
>     have offsets in the catalog coded not as offsets in sectors (multiple
>     of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
>    
>     In this case all 64 bits of header->nb_sectors are used for image size.
>    
>     This patch implements support of this for qemu-img and also adds
> specific
>     check for an incorrect image. Images with block size greater than
>     INT_MAX/513 are not supported. The biggest available Parallels image
>     cluster size in the field is 1 Mb. Thus this limit will not hurt
>     anyone.
>    
>     Signed-off-by: Denis V. Lunev <den@openvz.org>
>     CC: Jeff Cody <jcody@redhat.com>
>     CC: Kevin Wolf <kwolf@redhat.com>
>     CC: Stefan Hajnoczi <stefanha@redhat.com>
>     Reviewed-by: Jeff Cody <jcody@redhat.com>
>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Thus I believe that this is intentional.

Hm, fair. It's a weird number. I would have guessed a typo, but if it's
in the commit message as well, it might be intentional. Or just a typo
combined with copy & paste.

If we ever remember or find a new reason why it has to be 513 rather
than 512, adding a comment would be nice.

Kevin